RFR: 8290866: Apple Color Emoji turns gray after JavaFX version 18

[Kevin Rushforth]

On Thu, 23 Feb 2023 20:43:29 GMT, Phil Race <prr at openjdk.org> wrote:

> This fix properly supports colour rendering of Emoji on macOS
> 
> 
> On other platforms the Emoji will be rendered as ordinary greyscale glyphs - if there is font
> support for the requested code point.
> 
> A simple manual test is provided which uses a Text node, Label control
> and editable TextField control.
> 
> Some highlights of the code
> - To determine if it is a color emoji glyph probe the 'sbix' font table which is what is used by Apple
> - Text runs now break at an Emoji glyph
> - The Emoji is retrieved as an BGRA image - ie 4 channel including alpha
> - It was necessary to retrieve the Emoji glyph bounds via a different CoreText API since
>    the bounds that were being retrieved were wrong for the Emoji image - causing clipping
> - It was necessary to retrieve the Emoji code point advance via a CoreText API since
>    the HMTX metrics were very wrong - causing overlapping glyphs
> - drawString checks if it is an Emoji run and redirects to a new drawColorGlyph method
>    which draws the image as a texture
> - All 3 rendering pipelines have this support and have been verified on all 3 desktop platforms

My testing all looks good. Since all of the changes for the shader-based pipelines are in BaseShaderGraphics, I decided it might be interesting to try this with the metal pipeline (from the `jfx-sandbox:metal` branch). And... it just worked!

I did a partial code review (I'll finish this afternoon) and left a few comments and questions.

modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java line 1182:

> 1180: 
> 1181: 
> 1182:     public float getAdvance(int glyphCode, float ptSize) {

Did you mean to remove the `@Override` for this method?

modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java line 1184:

> 1182:     public float getAdvance(int glyphCode, float ptSize) {
> 1183:         if (glyphCode == CharToGlyphMapper.INVISIBLE_GLYPH_ID)
> 1184:             return 0f;

Please surround with curly braces.

modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java line 1430:

> 1428: 
> 1429:    private static int USHORT_MASK = 0xffff;
> 1430:    private static int UINT_MASK   = 0xffffffff;

Minor: can be final.

modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java line 1448:

> 1446:               return false;
> 1447:            }
> 1448:            return dataOffsets[gid] < dataOffsets[gid+1];

If `gid` is equal to `dataOffsets.length - 1`, won't this throw AIOOBE?

modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java line 1456:

> 1454:    private boolean isSbixGlyph(int glyphID) {
> 1455:        if (sbixStrikes == null) {
> 1456:            synchronized (this) {

This looks like a double-checked locking pattern, which is not guaranteed to be thread-safe.

Suggestion: move the `synchronized` before the `if`.

modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTGlyph.java line 90:

> 88: 
> 89:         if (drawShapes) return;
> 90: 

Just curious: do you know why this test was there in the first place? And why it needs to be removed?

-------------

PR: https://git.openjdk.org/jfx/pull/1047