Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8269806: Emoji rendering on Linux #4798

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

YaaZ
Copy link
Contributor

@YaaZ YaaZ commented Jul 15, 2021

It was implemented in JetBrains Runtime a year ago and was ported & refactored for this PR
It includes:

  • Bitmap glyph loading via Freetype
  • Manual scaling & transformation of bitmap glyphs with nearest-neighbor or bilinear-mipmap style algorithms depending on the text antialiasing hint
  • Storing BGRA glyphs in glyph cache & rendering them as plain images, as currently used XRender text drawing functions doesn't support colored glyphs
  • Small fixes in related code like null-checks which could cause NPE & comment typos

Progress

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

Issue

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4798

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/4798.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 15, 2021

👋 Welcome back YaaZ! 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.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 15, 2021
@openjdk
Copy link

openjdk bot commented Jul 15, 2021

@YaaZ The following labels will be automatically applied to this pull request:

  • 2d
  • awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added 2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org labels Jul 15, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 15, 2021

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 12, 2021

@YaaZ This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@YaaZ
Copy link
Contributor Author

YaaZ commented Aug 22, 2021

@prrace could you take a look, please?

@prrace
Copy link
Contributor

prrace commented Aug 22, 2021

I'm in the early stages but

  1. It doesn't build on the OL6.4 "official" build environment (note SFAIK the latest info
    is still here : https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms)

Maybe there'll be an update for JDK 18 but it hasn't happened yet

src/java.desktop/share/native/libfontmanager/freetypeScaler.c:1193:24: error: 'FT_LOAD_COLOR' undeclared (first use in this function)
renderFlags |= FT_LOAD_COLOR;
^~~~~~~~~~~~~
src/java.desktop/share/native/libfontmanager/freetypeScaler.c:1193:24: note: each undeclared identifier is reported only once for each function it appears in
src/java.desktop/share/native/libfontmanager/freetypeScaler.c:1259:50: error: 'FT_PIXEL_MODE_BGRA' undeclared (first use in this function); did you mean 'FT_PIXEL_MODE_GRAY'?
} else if (ftglyph->bitmap.pixel_mode == FT_PIXEL_MODE_BGRA) {
^~~~~~~~~~~~~~~~~~
FT_PIXEL_MODE_GRAY

At the very least this means you will need to include defines for these if they aren't defined and make sure
that symbols that may be similarly from a later freetype than available on older linuxes are loaded dynamically
and the support skipped where it doesn't exist.

  1. I see Xrender code changes but am wondering what happens with the X11 and OpenGL pipelines ?
    Errors of some code ? Silently behaves like this new code doesn't exist ?

OK I can partially answer that one - garbage rendering with X11 on Ubuntu 20.04

Looks like you are supplying the BRGA glyphs but no loops to handle it.

This needs to be implemented for X11 and OpenGL

@prrace
Copy link
Contributor

prrace commented Aug 23, 2021

In addition I think the test is a bit flaky
"Noto Color Emoji" may not be available on all Linuxes and for whatever reason your update makes the test fail on Mac.
test result: Error. can't find Emoji in test directory or libraries

So lots to do here before I look again.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Sep 10, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 20, 2021

@YaaZ This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@miurahr
Copy link

miurahr commented Oct 12, 2021

"Noto Color Emoji" may not be available on all Linuxes and for whatever reason your update makes the test fail on Mac.

@YaaZ Any progress do you have? As Linux user I'd like to help if I can.

@prrace
Copy link
Contributor

prrace commented Oct 30, 2021

@YaaZ nothing has really happened here in > 2 months. When can we expect an update ?

@YaaZ
Copy link
Contributor Author

YaaZ commented Nov 8, 2021

Hi! Sorry, just was busy with other work, but I'm going to get back to this in 1-2 weeks. BTW in the meantime I implemented emoji for JetBrains Runtime on Windows and will create a PR here after I deal with this one.

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 6, 2021

@YaaZ This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@YaaZ
Copy link
Contributor Author

YaaZ commented Dec 29, 2021

Hi @prrace! By X11 pipeline, do you mean -Dsun.java2d.xrender=false? Because I wasn't able to reproduce this on Ubuntu 20.04

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 26, 2022

@YaaZ This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@prrace
Copy link
Contributor

prrace commented Feb 7, 2022

Hi @prrace! By X11 pipeline, do you mean -Dsun.java2d.xrender=false? Because I wasn't able to reproduce this on Ubuntu 20.04

Yes and -Dsun.java2d.opengl=True

Can't remember if I tried the latter but the former was garbage for me.

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 7, 2022

@YaaZ This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@prrace
Copy link
Contributor

prrace commented Mar 24, 2022

It didn't build for me and it didn't work for me - on X11. So is there an update or should it be returned to draft or handed off to someone else ?

@YaaZ
Copy link
Contributor Author

YaaZ commented Mar 24, 2022

Builds with old Freetype should work now, however I cannot reproduce rendering issues you mentioned, everything renders OK for me with -Dsun.java2d.xrender=false and -Dsun.java2d.opengl=True on Ubuntu 20.04 some 21 one, OL 8.5 and Fedora 35. Probably I'm missing something

@YaaZ
Copy link
Contributor Author

YaaZ commented Mar 24, 2022

@prrace do you have any ideas how can I reproduce these rendering issues?

@prrace
Copy link
Contributor

prrace commented Mar 24, 2022

It should just be -Dsun.java2d.xrender=false to use X11 instead of Xrender.
I am not able to say how that worked for you .. and not me ...
I can re-apply your patches and create a new build and test the current version.
You could trace through to see what loops are actually used for rendering the text

java -Dsun.java2d.trace=log ....

@YaaZ
Copy link
Contributor Author

YaaZ commented Mar 25, 2022

For -Dsun.java2d.xrender=false:

sun.java2d.loops.FillRect::FillRect(AnyColor, SrcNoEa, AnyInt)
sun.java2d.loops.FillParallelogram::FillParallelogram(AnyColor, SrcNoEa, AnyInt)
sun.java2d.loops.DrawGlyphListColor$General::DrawGlyphListColor(OpaqueColor, SrcNoEa, IntRgb)
sun.java2d.loops.Blit$GeneralMaskBlit::Blit(IntArgbPre, SrcOverNoEa, IntRgb)
sun.java2d.loops.MaskBlit::MaskBlit(IntArgbPre, SrcOver, IntRgb)
sun.java2d.loops.Blit::Blit(IntRgb, SrcNoEa, IntRgb)

For -Dsun.java2d.opengl=True:

OGLDrawGlyphs
sun.java2d.opengl.OGLRTTSurfaceToSurfaceBlit::Blit("OpenGL Surface (render-to-texture)", AnyAlpha, "OpenGL Surface")

And everything renders OK.
Actually it's exactly this version, I didn't rebase it over fresh master.

@prrace
Copy link
Contributor

prrace commented Mar 25, 2022

font2dtest

This screenshot is Font2DTest with your current patch and -Dsun.java2d.xrender=false on Ubuntu 20.04
I don't understand how you aren't seeing the garbage emojis, since your patch has no X11 support.
Where the garbage glyphs are, they appear as correct colour emojis with xrender.

@prrace
Copy link
Contributor

prrace commented Mar 25, 2022

Ah - I bet you haven't tested AA=OFF ?

@YaaZ
Copy link
Contributor Author

YaaZ commented Mar 25, 2022

Nope, AA=OFF does work for me as well with XR, X11 and OGL. Maybe the problem is in selecting the appropriate fallback font, in my test I select the specific emoji font which does work, but in your Font2DTest it's more complicated, like it must require a fallback emoji font. I'll investigate this.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 15, 2022
@YaaZ
Copy link
Contributor Author

YaaZ commented Jul 15, 2022

🎉 Celebrating 1 year of this PR by adding bunch of changes that include:

  • Emoji rendering for Windows (colored outline glyphs) - based on Linux version via Freetype
  • Emoji rendering via OutlineTextRenderer for big font sizes - both colored outline & bitmap glyphs

As well as many changes related to text layout & font fallback on all platforms:

  • Detect emoji sequences which need shaping
  • Make variation selectors work with the font fallback mechanism
  • Properly handle ZWJ sequences in our layout code

@prrace can you take a look, please?

YaaZ added 2 commits Jul 15, 2022
Get rid of unneeded abstract methods

Use SSIM for OutlineTextRendererEmoji test
@openjdk
Copy link

openjdk bot commented Jul 21, 2022

@YaaZ this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8269806
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed rfr Pull request is ready for review labels Jul 21, 2022
@openjdk openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Jul 21, 2022
@@ -1815,6 +1830,19 @@ void appendGlyphOutline(int glyphID, GeneralPath result, float x, float y) {
PathIterator iterator = gp.getPathIterator(null);
result.append(iterator, false);
}

void appendGlyphRenderData(int glyphID, GlyphRenderData result, float x, float y) {
// !!! fontStrike needs a method for this. For that matter, GeneralPath does.
Copy link
Contributor

@avu avu Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use well known "TODO" here

Copy link
Contributor Author

@YaaZ YaaZ Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was just copied from appendGlyphOutline above :)

@YaaZ YaaZ requested a review from prrace Aug 29, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 2, 2022

@YaaZ This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@YaaZ
Copy link
Contributor Author

YaaZ commented Sep 27, 2022

💅

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 23, 2022

@YaaZ This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bourgesl
Copy link
Contributor

bourgesl commented Nov 28, 2022

@YaaZ Could you merge with latest openjdk20 master to let CI build again ?

@bourgesl
Copy link
Contributor

bourgesl commented Dec 2, 2022

Still failing on x86? What are the 2 test failures?

@prrace
Copy link
Contributor

prrace commented Dec 2, 2022

I imported the changes and submitted to our CI test system. There seem to be a number of failures of the provided tests.
java/awt/font/MacEmoji.java : fails on x64 and aarch64 with Error: can't find Emoji in test directory or libraries

java/awt/font/EmojiVariation.java :
failed on linux-aarch64 with Exception: java.lang.Error: Required font not found: Noto Color Emoji

I don't think a test should fail just because there are no Emoji fonts installed.
Lots of system configs will run into this relatively bleeding edge reqt.

and windows-64 fails this test with a REALLY long message repeating lots of times a message like
0023-COLOR: Expected color but rendered mono

java/awt/font/ComplexEmoji.java failed on linux-aarch64 with Exception: java.lang.Error: Required font not found: Noto Color Emoji

and failed on windows-x64 with Exception: 3: Empty image

java/awt/font/OutlineTextRendererEmoji.java: failed on aarch64 with Exception: java.lang.Error: Required font not found: Noto Color Emoji

I haven't yet looked at how this behaves when manually testing on current platforms. These are just automated tests.

@YaaZ
Copy link
Contributor Author

YaaZ commented Dec 2, 2022

Gosh these tests were OK last time ran them, maybe broke with master merging or something. Unfortunately I'll only have time to look at this next year, will let you know what I find.
If you gonna run the tests manually (and assuming there's no regression), it should work without any special configuration on any sane Windows & macOS. As for Linux, I tested this on a desktop Ubuntu 20.04-22.04, but note that minimal installation doesn't include Noto Color Emoji.

@YaaZ
Copy link
Contributor Author

YaaZ commented Dec 3, 2022

@prrace just a few comments regarding your CI results:

  1. I don't quite understand the error message for MacEmoji, but note that this test was renamed to Emoji
  2. All Linux fails come from absent emoji font, it would probably work with installed font
  3. Windows renders only monochrome emoji - may it be an old Windows where there's no color emoji font? (Win8 or older?)

So macOS seems to work and others may be just configuration problems, definitely worth trying to run manually on modern Windows & Linux.

And about failing from not found font: I will fix this. AFAIK currently if we gonna skip the test, we just pretend like it succeeded. There's no way to explicitly mark test as skipped, right?

@prrace
Copy link
Contributor

prrace commented Dec 3, 2022

@prrace just a few comments regarding your CI results:

  1. I don't quite understand the error message for MacEmoji, but note that this test was renamed to Emoji

Indeed the contents are a class called Emoji
I don't know why the patch still called it MacEmoji

Hmm I applied the patch skara generated and it looks like macOS's patch didn't do what it was asked

This is what was in the patch (diff) file -
diff --git a/test/jdk/java/awt/font/MacEmoji.java b/test/jdk/java/awt/font/Emoji.java
similarity index 91%
rename from test/jdk/java/awt/font/MacEmoji.java
rename to test/jdk/java/awt/font/Emoji.java
index 198db5c107fb..b44c68db7caa 100644
--- a/test/jdk/java/awt/font/MacEmoji.java
+++ b/test/jdk/java/awt/font/Emoji.java

  1. All Linux fails come from absent emoji font, it would probably work with installed font

Yes, I expect that too, but the problem is we don't control all the systems used to test.

  1. Windows renders only monochrome emoji - may it be an old Windows where there's no color emoji font? (Win8 or older?)

Not exactly its a server version of Windows 10. Today, all of the "headless" tests run on Windows Server.
Either the font isn't available for that OS, or quite possibly installing all the fonts isn't something that is done.
The test does need to be hardened against an absence of a font. Marking it headful isn't a guarantee either because they could still run on Windows Server.

So macOS seems to work and others may be just configuration problems, definitely worth trying to run manually on modern Windows & Linux.

This is largely outside of our control. They need to pass on supported configs.

And about failing from not found font: I will fix this. AFAIK currently if we gonna skip the test, we just pretend like it succeeded. There's no way to explicitly mark test as skipped, right?

Usually yes.

@prrace
Copy link
Contributor

prrace commented Dec 3, 2022

I submitted a skara bug about the rename https://bugs.openjdk.org/browse/SKARA-1711

@YaaZ
Copy link
Contributor Author

YaaZ commented Dec 3, 2022

That's a real pain how skipped tests just silently pass. Regression tests should catch regressions, not pretend that everything is OK when they in fact didn't test anything. I believe skipping a test should be explicit, it's not a "Passed" nor "Failed" state, it's more like "Unknown". Would be very unfortunate to see green tests in CI and then find a regression which happened million years ago just because this test was skipped all the time but you never knew. Even agreeing on some special exit code for skipped tests would already be a good step forward. Just a point for a discussion.

@avu
Copy link
Contributor

avu commented Dec 5, 2022

There are several options to resolve this problem without failing the tests:

  • you can provide a diagnostic message, so passing for configurations missing necessary fonts won’t be silent
  • create your own font with the necessary properties and submit it within the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org client client-libs-dev@openjdk.org rfr Pull request is ready for review
5 participants