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

8263583: Emoji rendering on macOS #3007

Closed
wants to merge 4 commits into from
Closed

Conversation

@JB-Dmitry
Copy link
Contributor

@JB-Dmitry JB-Dmitry commented Mar 15, 2021

This is the implementation used by JetBrains Runtime for the last 4 years, after some cleanup, and with one problem,
found while preparing the pull request, fixed.
Even though typical scenarios for a UI application should be covered, it's not a complete solution. In particular, emoji-s
still won't be rendered for large font sizes (more than 100pt), and for non-trivial composite/painting modes.
Notable implementation details are listed below.

Glyph image generation

Deprecated CGContextShowGlyphsAtPoint function, used by JDK on macOS to render text, cannot render emojis,
CTFontDrawGlyphs is used instead. It ignores the scale component of text transformation matrix, so a 'real-sized'
CTFont object should be passed to it. The same sizing procedure is done when calculating glyph metrics, because they
are not scaled proportionally with font size (as they do for vector fonts).

Glyph image storage

Existing GlyphInfo structure is used to store color glyph image. Color glyph can be distinguished by having 4 bytes
of storage per pixel. Color components are stored in pre-multiplied alpha format.

Glyph rendering

Previously, GlyphList instance always contained glyphs in the same format (solid, grayscale or LCD), determined by the
effective rendering hint. Now the renderers must be prepared to GlyphList having 'normal' glyphs interspersed with
color glyphs (they can appear due to font fallback). This isn't a problem for OpenGL renderer (used for on-screen painting),
but GlyphListLoopPipe-based renderers (used for off-screen painting) needed an adjustment to be able to operate on
specific segments of GlyphList.
As an incidental optimization, calculation of GlyphList bounds ('getBounds' method) is performed now only when needed
(most text renderers don't need this information).
Speaking of the actual rendering of the glyph image, it's done by the straightforward glDrawPixels call in OpenGL renderer,
and by re-using existing Blit primitive in off-screen renderers.

Testing

There's no good way to test the new functionality automatically, but I've added a test verifying that 'something' is
rendered for the emoji character, when painting to BufferedImage.

Existing tests pass after the change.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3007/head:pull/3007
$ git checkout pull/3007

Update a local copy of the PR:
$ git checkout pull/3007
$ git pull https://git.openjdk.java.net/jdk pull/3007/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3007

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3007.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 15, 2021

👋 Welcome back dbatrak! 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 label Mar 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@JB-Dmitry The following label will be automatically applied to this pull request:

  • 2d

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

@openjdk openjdk bot added the 2d label Mar 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 15, 2021

Webrevs

@avu
Copy link

@avu avu commented Mar 15, 2021

Looks good

@prrace
Copy link
Contributor

@prrace prrace commented Mar 15, 2021

Initial testing it looks OK with OpenGL but it renders as garbage with the metal pipeline.
I'd guess metal isn't expecting 4bpp and the stride is wrong.
I see updates in OGLTextRenderer so something similar likely needed for metal.
Since metal is now integrated this should be fixed before this change can be pushed.

@prrace
Copy link
Contributor

@prrace prrace commented Mar 15, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@prrace
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

// This instance is used only for lookup.
static {
GraphicsPrimitiveMgr.registerGeneral(
new DrawGlyphListColor(null, null, null));
Copy link
Member

@mrserb mrserb Mar 16, 2021

Choose a reason for hiding this comment

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

If it is a complete "General" loop which supports all possible combination of incoming src/comp/dst/transform/etc I suggest to confirm that in the test.

@JB-Dmitry
Copy link
Contributor Author

@JB-Dmitry JB-Dmitry commented Mar 16, 2021

Initial testing it looks OK with OpenGL but it renders as garbage with the metal pipeline.
I'd guess metal isn't expecting 4bpp and the stride is wrong.
I see updates in OGLTextRenderer so something similar likely needed for metal.
Since metal is now integrated this should be fixed before this change can be pushed.

I'm going to add Metal implementation soon.

@JB-Dmitry
Copy link
Contributor Author

@JB-Dmitry JB-Dmitry commented Mar 16, 2021

If it is a complete "General" loop which supports all possible combination of incoming src/comp/dst/transform/etc I suggest to confirm that in the test.

@mrserb This code is currently only used by GlyphListLoopPipe-based text renderers (SolidTextRenderer, AATextRenderer, LCDTextRenderer), so it won't be invoked for arbitrary composites and paint types. But different destination types (image formats) and transforms are supported.

What kind of test are you having in mind? I can extend the added MacEmoji.java test, that will check that 'something' is painted to different types of BufferedImage-s and/or with transforms applied. Or you want me to add a manual test case?

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 16, 2021

What kind of test are you having in mind? I can extend the added MacEmoji.java test, that will check that 'something' is painted to different types of BufferedImage-s and/or with transforms applied. Or you want me to add a manual test case?

The automated tests will be enough, not sure about 'something is painted' however, it is better to check that it works.

@JB-Dmitry
Copy link
Contributor Author

@JB-Dmitry JB-Dmitry commented Mar 17, 2021

@mrserb I don't know how to check automatically that glyph painting works correctly. Could you please suggest a way to do it? In JetBrains Runtime we have a test that checks that rendered emoji glyph matches one of stored 'golden images', but I don't think that's suitable for OpenJDK, unless someone will volunteer to update that list when the need arises (e.g. when newer version of macOS changes the font, or rendering details). At the moment, we have already 7 golden images for a single glyph in our repository.

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 18, 2021

You the current image from the MacEmoji test as a golden image for other formats/transforms/extraalpha/etc. For example, if DST type is changed then it is unlikely the shape of the emoji will be changed as well, or if it is too big/ too small.

BTW It will be good if the test will fail on the current metal implementation.

JB-Dmitry added 2 commits Mar 19, 2021
implementation for Metal rendering pipeline
extended test case to cover different types of target surfaces
@JB-Dmitry
Copy link
Contributor Author

@JB-Dmitry JB-Dmitry commented Mar 19, 2021

Implementation for Metal pipeline has been added.

I've also updated the test case to check rendering into different types of images, including VolatileImage. The test will fail now, if corresponding OpenGL (or Metal, if it's explicitly enabled) implementation part would be rolled back.

@avu
Copy link

@avu avu commented Mar 19, 2021

Looks good!

@mrserb
Copy link
Member

@mrserb mrserb commented Apr 7, 2021

It looks fine to me. I will run the tests in CI just in case.

mrserb
mrserb approved these changes Apr 13, 2021
@JB-Dmitry
Copy link
Contributor Author

@JB-Dmitry JB-Dmitry commented Apr 19, 2021

Any chance for a second review?

prrace
prrace approved these changes May 6, 2021
@@ -324,6 +324,19 @@ void MTLTR_FreeGlyphCaches() {
}
}

MTLPaint* storedPaint = nil;
Copy link
Contributor

@prrace prrace May 6, 2021

Choose a reason for hiding this comment

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

should this be static ?

Copy link
Contributor Author

@JB-Dmitry JB-Dmitry May 7, 2021

Choose a reason for hiding this comment

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

Added 'static', thanks.

@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2021

@JB-Dmitry 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:

8263583: Emoji rendering on macOS

Reviewed-by: serb, prr

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 1005 new commits pushed to the master branch:

  • cd1c17c: 8266404: Fatal error report generated with -XX:+CrashOnOutOfMemoryError should not contain suggestion to submit a bug report
  • 2effdd1: 8267112: JVMCI compiler modules should be kept upgradable
  • da4dfde: 8264777: Overload optimized FileInputStream::readAllBytes
  • 3b11d81: 8266742: Check W^X state on possible safepoint
  • 79b3944: 8266520: Revert to OpenGL as the default 2D rendering pipeline for macOS
  • 3c010a7: 8265705: aarch64: KlassDecodeMovk mode broken
  • cf97252: 8264561: javap get NegativeArraySizeException on bad instruction
  • b8856b1: 8263614: javac allows local variables to be accessed from a static context
  • ea36836: 8267236: Versioned platform link in TestMemberSummary.java
  • d5a15f7: 8263438: Unused method AbstractMemberWriter.isInherited
  • ... and 995 more: https://git.openjdk.java.net/jdk/compare/32c7fcc67010e44411918cc73681422fd8b7a67a...master

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.

@openjdk openjdk bot added the ready label May 6, 2021
add 'static' modifier to a variable
prrace
prrace approved these changes May 7, 2021
@JB-Dmitry
Copy link
Contributor Author

@JB-Dmitry JB-Dmitry commented May 12, 2021

@mrserb Could you please re-approve the request? There's been only a tiny change since you've approved it earlier.

Do I understand correctly that this re-approval is required to integrate the change? Github shows the request as having two approvals, and the 'ready' tag. Or is it just openjdk bot not implementing the workflow correctly?

mrserb
mrserb approved these changes May 17, 2021
@mrserb
Copy link
Member

@mrserb mrserb commented May 20, 2021

It is not necessary to get reapprove for push, it is just a good style to do.

@JB-Dmitry
Copy link
Contributor Author

@JB-Dmitry JB-Dmitry commented May 31, 2021

/integrate

@openjdk openjdk bot closed this May 31, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 31, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 31, 2021

@JB-Dmitry Since your change was applied there have been 1175 commits pushed to the master branch:

  • 1ab2776: 8247608: Javadoc: CSS margin is not applied consistently
  • 9031477: 8267945: ZGC: Revert NUMA changes (JDK-8266217 and JDK-8241354) after JDK-8241423
  • 6627432: 8267953: restore 'volatile' to ObjectMonitor::_owner field
  • 964bac9: 8267706: bin/idea.sh tries to use cygpath on WSL
  • 591b0c3: 8264624: change the guarantee() calls added by JDK-8264123 to assert() calls
  • 0c0ff7f: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException
  • 24bf35f: 8265367: [macos-aarch64] 3 java/net/httpclient/websocket tests fail with "IOException: No buffer space available"
  • 1413f9e: 8241423: NUMA APIs fail to work in dockers due to dependent syscalls are disabled by default
  • 1d2c7ac: 8267555: Fix class file version during redefinition after 8238048
  • 97ec5ad: 8265753: Remove manual JavaThread transitions to blocked
  • ... and 1165 more: https://git.openjdk.java.net/jdk/compare/32c7fcc67010e44411918cc73681422fd8b7a67a...master

Your commit was automatically rebased without conflicts.

Pushed as commit 236bd89.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants