8290866: Apple Color Emoji turns gray after JavaFX version 18 #1047

prrace · 2023-02-23T20:43:29Z

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8290866: Apple Color Emoji turns gray after JavaFX version 18

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/1047/head:pull/1047
$ git checkout pull/1047

Update a local copy of the PR:
$ git checkout pull/1047
$ git pull https://git.openjdk.org/jfx pull/1047/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1047

View PR using the GUI difftool:
$ git pr show -t 1047

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1047.diff

bridgekeeper · 2023-02-23T20:44:49Z

👋 Welcome back prr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

kevinrushforth · 2023-02-23T20:49:48Z

/reviewers 2

mlbridge · 2023-02-23T20:50:09Z

Webrevs

openjdk · 2023-02-23T20:51:20Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle · 2023-02-24T16:44:35Z

I see color emojis and no clipping of color emojis, so this is a major improvement.

I see some clipping of the emoji flag sequence characters, for example \ud83c\uddfa\ud83c\udde6 when moving from the primary retina (scale=2) to secondary external (scale=1) monitor:

retina:

secondary:

andy-goryachev-oracle · 2023-02-24T16:46:23Z

On a side note, it seems FX does not currently support rendering of flag emoji sequences such as 🇺🇦
\ud83c\uddfa\ud83c\udde6

is this something we ought to be supporting?

andy-goryachev-oracle · 2023-02-24T16:50:05Z

Another question: on non-retina monitor, emojis appear blurry when text size is set to 12. I vaguely recall that when resizing down, the resizing operation might be followed by a sharpening filter (or perhaps some other op can be used?).
I don't think this is a deal breaker though.
What do you think?

andy-goryachev-oracle · 2023-02-24T00:22:55Z

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

+        if (isColorGlyph()) {
+            return (w * 4); // has alpha
+        } else {
+          return isLCDGlyph() ? w * 3 : w;


indentation?

prrace · 2023-02-24T18:48:20Z

I see some clipping of the emoji flag sequence characters, for example \ud83c\uddfa\ud83c\udde6 when moving from the primary retina (scale=2) to secondary external (scale=1) monitor:

I tried the same size and sequence and in the provided test and did the same move from retina to external and there is NO clipping in any of Text/Label/TextField.
So I suspect something specific to what you are doing in your test .. not a general emoji problem.

prrace · 2023-02-24T18:49:27Z

On a side note, it seems FX does not currently support rendering of flag emoji sequences such as 🇺🇦 \ud83c\uddfa\ud83c\udde6

is this something we ought to be supporting?

Perhaps .. but that is beyond the scope of this fix.

prrace · 2023-02-24T18:53:03Z

Another question: on non-retina monitor, emojis appear blurry when text size is set to 12. I vaguely recall that when resizing down, the resizing operation might be followed by a sharpening filter (or perhaps some other op can be used?).
I don't think this is a deal breaker though.
What do you think?

We just get the image from the OS.
The Mac Emoji font is basically a bunch of PNG images at different sizes and I expect they
deliberately blur it when downscaling.

andy-goryachev-oracle · 2023-02-24T18:57:54Z

So I suspect something specific to what you are doing in your test .. not a general emoji problem.

or specific to my Mac Ventura 13.1. In my test, i am setting the font explicitly. A simpler test clips the flag symbols on both retina and the external monitor:

public class TextAreaApp extends Application {
    public static void main(String[] args) {
        Application.launch(TextAreaApp.class, args);
    }

    @Override
    public void start(Stage stage) throws Exception {
        String text =
            "[🇺🇦❤️🏁🇺🇸🔥🦋\n😀 😃 😄 😁 😆 😅 🤣 😂\n🙂 🙃 😉 😊 😇]";
            //"A regular Arabic verb, كَتَبَ‎ kataba (to write).");
        //t.setFont(new Font("Noto Sans Arabic Regular", 12));
        TextArea control = new TextArea(text);
        
        stage.setScene(new Scene(control));
        stage.setTitle(getClass().getSimpleName() + " " + System.getProperty("java.version"));
        stage.setWidth(800);
        stage.setHeight(500);
        stage.show();
    }
}

andy-goryachev-oracle · 2023-02-24T18:58:46Z

"[\ud83c\uddfa\ud83c\udde6\u2764\ufe0f\ud83c\udfc1\ud83c\uddfa\ud83c\uddf8\ud83d\udd25\ud83e\udd8b\n\ud83d\ude00 \ud83d\ude03 \ud83d\ude04 \ud83d\ude01 \ud83d\ude06 \ud83d\ude05 \ud83e\udd23 \ud83d\ude02\n\ud83d\ude42 \ud83d\ude43 \ud83d\ude09 \ud83d\ude0a \ud83d\ude07]";

prrace · 2023-02-24T19:10:23Z

So I suspect something specific to what you are doing in your test .. not a general emoji problem.

or specific to my Mac Ventura 13.1. In my test, i am setting the font explicitly. A simpler test clips the flag symbols on both retina and the external monitor:

I have no idea why Ventura would make a difference but your "simpler" app works with no clipping on macOS 12.6.1 which is what I have ...

andy-goryachev-oracle · 2023-02-24T19:17:08Z

app works with no clipping on macOS 12.6.1

clipping occurs on retina at any resolution, from "Larger Text" to "More Space". It might be specific to Ventura, and possibly to the flag emojis. I tried to paste random code blocks, see no obvious clipping:

kevinrushforth

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

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

prrace · 2023-02-24T19:36:13Z

app works with no clipping on macOS 12.6.1

clipping occurs on retina at any resolution, from "Larger Text" to "More Space". It might be specific to Ventura, and possibly to the flag emojis. I tried to paste random code blocks, see no obvious clipping:

My best guess is they tweaked something in the font and anyway this is NOT Emoji rendering.
ie those letters in dashed boxes are just mapping to the ordinary glyph rendering.
As such it doesn't go down the path I added for getting full bounds of Emoji and all it really
means is that it is a separate bug for ordinary glyph bounds/metrics. ie if you separated those
it would make no difference - they'd still be clipped.

andy-goryachev-oracle

tested w/MonkeyTester on Mac Ventura 13.1
LGTM

kevinrushforth

Looks good. I'll reapprove if you want to fix the typo in the instructions.

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

tests/manual/text/EmojiTest.java

openjdk · 2023-02-24T22:10:36Z

@prrace This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290866: Apple Color Emoji turns gray after JavaFX version 18

Reviewed-by: kcr, angorya

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

a916629: Merge
1dd0a59: 8303019: cssref.html incorrect internal link in Path
7bf2372: 8293587: Fix mistakes in FX API docs

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

prrace · 2023-02-24T22:43:32Z

/integrate

openjdk · 2023-02-24T22:45:06Z

Going to push as commit 0de0837.
Since your change was applied there have been 3 commits pushed to the master branch:

a916629: Merge
1dd0a59: 8303019: cssref.html incorrect internal link in Path
7bf2372: 8293587: Fix mistakes in FX API docs

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-24T22:45:19Z

@prrace Pushed as commit 0de0837.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

prrace added 2 commits February 23, 2023 12:28

8290866

e5072c9

8290866

2416aeb

openjdk bot changed the title ~~8290866~~ 8290866: Apple Color Emoji turns gray after JavaFX version 18 Feb 23, 2023

openjdk bot added the rfr Ready for review label Feb 23, 2023

kevinrushforth requested review from arapte and kevinrushforth February 23, 2023 20:49

andy-goryachev-oracle reviewed Feb 24, 2023

View reviewed changes

kevinrushforth reviewed Feb 24, 2023

View reviewed changes

8290866

acff6da

andy-goryachev-oracle approved these changes Feb 24, 2023

View reviewed changes

8290866

cae5274

kevinrushforth approved these changes Feb 24, 2023

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/javafx/font/PrismFontFile.java Show resolved Hide resolved

tests/manual/text/EmojiTest.java Outdated Show resolved Hide resolved

andy-goryachev-oracle approved these changes Feb 24, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Feb 24, 2023

8290866

82bf8d3

andy-goryachev-oracle approved these changes Feb 24, 2023

View reviewed changes

openjdk bot removed the ready Ready to be integrated label Feb 24, 2023

kevinrushforth approved these changes Feb 24, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Feb 24, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 24, 2023

openjdk bot closed this Feb 24, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 24, 2023

8290866: Apple Color Emoji turns gray after JavaFX version 18 #1047

8290866: Apple Color Emoji turns gray after JavaFX version 18 #1047

Uh oh!

Conversation

prrace commented Feb 23, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 23, 2023

Uh oh!

kevinrushforth commented Feb 23, 2023

Uh oh!

mlbridge bot commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Feb 23, 2023

Uh oh!

andy-goryachev-oracle commented Feb 24, 2023

Uh oh!

andy-goryachev-oracle commented Feb 24, 2023

Uh oh!

andy-goryachev-oracle commented Feb 24, 2023

Uh oh!

andy-goryachev-oracle Feb 24, 2023

Choose a reason for hiding this comment

Uh oh!

prrace commented Feb 24, 2023

Uh oh!

prrace commented Feb 24, 2023

Uh oh!

prrace commented Feb 24, 2023

Uh oh!

andy-goryachev-oracle commented Feb 24, 2023

Uh oh!

andy-goryachev-oracle commented Feb 24, 2023

Uh oh!

prrace commented Feb 24, 2023

Uh oh!

andy-goryachev-oracle commented Feb 24, 2023

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prrace commented Feb 24, 2023

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prrace commented Feb 24, 2023

Uh oh!

openjdk bot commented Feb 24, 2023

Uh oh!

openjdk bot commented Feb 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

prrace commented Feb 23, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 23, 2023 •

edited

Loading

openjdk bot commented Feb 24, 2023 •

edited

Loading