On Thu, 23 Feb 2023 20:43:29 GMT, Phil Race <p...@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
