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

[Phil Race]

On Fri, 24 Feb 2023 18:58:37 GMT, Kevin Rushforth <kcr 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
>
> 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?

Nope. I'll put it back.

> 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.

OK .. but I did not add that as such .. it was there already.

> 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.

ok

> 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?

No, this is correct.
See https://learn.microsoft.com/en-us/typography/opentype/spec/sbix

"The glyphDataOffset array includes offsets for every glyph ID, plus one extra."

> 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?

My guess is that the info it gathered was not used in the glyph image path
and it was an attempt at optimisation.
As to why it needed to be removed, it is because  I DO need that information and there'll be a NPE (IIRC) if I don't remove it.

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

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