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

8260931: Implement JEP 382: New macOS Rendering Pipeline #2403

Closed
wants to merge 47 commits into from

Conversation

aghaisas
Copy link
Contributor

@aghaisas aghaisas commented Feb 4, 2021

Description :
This is the implementation of JEP 382 : New macOS Rendering Pipeline
It implements a Java 2D internal rendering pipeline for macOS using the Apple Metal API.
The entire work on this was done under OpenJDK Project - Lanai

We iterated through several Early Access (EA) builds and have reached a stage where it is ready to be integrated to openjdk/jdk. The latest EA build is available at - https://jdk.java.net/lanai/

A new option -Dsun.java2d.metal=true | True needs to be used to use this pipeline.

Testing :
This implementation has been tested with the tests present at - Test Plan for JEP 382: New macOS Rendering Pipeline

Note to reviewers :

  1. Default rendering pipeline on macOS has not been changed by this PR. OpenGL still stays as the default rendering pipeline and Metal rendering pipeline is optional to choose.

  2. To apply and test this PR -
    To enable the metal pipeline you must specify on command line -Dsun.java2d.metal=true (No message will be printed in this case) or -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is enabled gets printed)

  3. Review comments (including some preliminary informal review comments) are tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598


Progress

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

Issue

  • JDK-8260931: Implement JEP 382: New macOS Rendering Pipeline

Reviewers

Contributors

  • Jayathirth D V <jdv@openjdk.org>
  • Alexey Ushakov <avu@openjdk.org>
  • Artem Bochkarev <abochkarev@openjdk.org>
  • Prasanta Sadhukhan <psadhukhan@openjdk.org>
  • Denis Konoplev <dkonoplev@openjdk.org>
  • Phil Race <prr@openjdk.org>
  • Kevin Rushforth <kcr@openjdk.org>
  • Magnus Ihse Bursie <ihse@openjdk.org>
  • Ajit Ghaisas <aghaisas@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403
$ git checkout pull/2403

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 4, 2021

👋 Welcome back aghaisas! 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
Copy link

openjdk bot commented Feb 4, 2021

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

  • 2d
  • awt
  • build

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 build build-dev@openjdk.org awt client-libs-dev@openjdk.org labels Feb 4, 2021
@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/reviewers 3

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

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

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add jdv

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Jayathirth D V <jdv@openjdk.org> successfully added.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add avu

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Alexey Ushakov <avu@openjdk.org> successfully added.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add abochkarev

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Artem Bochkarev <abochkarev@openjdk.org> successfully added.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add psadhukhan

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Prasanta Sadhukhan <psadhukhan@openjdk.org> successfully added.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add dkonoplev

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Denis Konoplev <dkonoplev@openjdk.org> successfully added.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add prr

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Phil Race <prr@openjdk.org> successfully added.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add kcr

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Kevin Rushforth <kcr@openjdk.org> successfully added.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add ihse

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Magnus Ihse Bursie <ihse@openjdk.org> successfully added.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Feb 4, 2021
@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@aghaisas this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please use the /issue command in a comment in this pull request.

@aghaisas
Copy link
Contributor Author

aghaisas commented Feb 4, 2021

/contributor add aghaisas

@openjdk
Copy link

openjdk bot commented Feb 4, 2021

@aghaisas
Contributor Ajit Ghaisas <aghaisas@openjdk.org> successfully added.

@aghaisas aghaisas changed the title [Draft] 8260931: Implement JEP 382: New macOS Rendering Pipeline 8260931: Implement JEP 382: New macOS Rendering Pipeline Feb 4, 2021
@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Mar 10, 2021
@openjdk
Copy link

openjdk bot commented Mar 10, 2021

@aghaisas 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:

8260931: Implement JEP 382: New macOS Rendering Pipeline

Co-authored-by: Jayathirth D V <jdv@openjdk.org>
Co-authored-by: Alexey Ushakov <avu@openjdk.org>
Co-authored-by: Artem Bochkarev <abochkarev@openjdk.org>
Co-authored-by: Prasanta Sadhukhan <psadhukhan@openjdk.org>
Co-authored-by: Denis Konoplev <dkonoplev@openjdk.org>
Co-authored-by: Phil Race <prr@openjdk.org>
Co-authored-by: Kevin Rushforth <kcr@openjdk.org>
Co-authored-by: Magnus Ihse Bursie <ihse@openjdk.org>
Co-authored-by: Ajit Ghaisas <aghaisas@openjdk.org>
Reviewed-by: ihse, avu, kcr, gziemski, prr, kizune, jdv, psadhukhan, serb

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

  • f7e0a09: 8263425: AArch64: two potential bugs in C1 LIRGenerator::generate_address()
  • 554dd29: 8263564: Consolidate POSIX code for runtime exit support: os::shutdown, os::abort and os::die
  • da9ead5: 8263399: CDS should archive only classes allowed by module system
  • 9c84899: 8263555: use driver-mode to run ClassFileInstaller
  • 8e562d2: 8263477: serviceability/sa/ClhsdbDumpheap.java timed out
  • a7aba2b: 8263549: 8263412 can cause jtreg testlibrary split
  • d339320: 8263136: C4530 was reported from VS 2019 at access bridge
  • a528771: 8262491: AArch64: CPU description should contain compatible board list
  • 86e4c75: 8256156: JFR: Allow 'jfr' tool to show metadata without a recording
  • 0b68ced: 8263548: runtime/cds/appcds/SharedRegionAlignmentTest.java fails to compile after JDK-8263412
  • ... and 25 more: https://git.openjdk.java.net/jdk/compare/273f8bdf5fe14e65e40d3d908f58dd682bb472ed...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 Pull request is ready to be integrated label Mar 10, 2021
@aghaisas
Copy link
Contributor Author

We have addressed the review comments (except for a few minor follow-on issues)
This version of the PR is the one which we are planning to integrate into the JDK. Please review and approve.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Latest changes look good. I confirmed that this PR has all of the content from the lanai repo.

cachedSrc = srcTmp.get();
}

// We can convert argb_pre data from MTL surface in two places:
Copy link
Member

Choose a reason for hiding this comment

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

OK, just to confirm. I wrote that text for OGL because it was the fastest way to transfer+convert the data from the video card to the memory. And as far as I understand the metal has the same limitation? It is not possible to convert it to some non-ARGB/Pre format on the fly while transferring the pixles?

@mlbridge
Copy link

mlbridge bot commented Mar 12, 2021

Mailing list message from Scott Palmer on awt-dev:

On Mar 11, 2021, at 9:53 PM, Sergey Bylokhov <serb at openjdk.java.net> wrote:

?On Fri, 12 Mar 2021 02:29:04 GMT, Jayathirth D V <jdv at openjdk.org> wrote:

src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 323:

321: * more code just to support a few uncommon cases.
322: */
323: public boolean canRenderLCDText(SunGraphics2D sg2d) {

Just curious, can we render LCD on 10.14+ via metal? Does it work fine?

Yes Sergey it works fine in 10.14+ systems via metal. Most of the JCK manual tests use LCD text on UI Components and it is recently verified in 10.14+ systems for EA10.

Ok, for some reason I thought that the new macOS stopped providing LCD glyphs.

Are the glyphs different or just the way they are rasterized?

Newer versions of macOS don?t do LCD text and have gone back to plain grey-scale anti-aliasing.
Java 2D should default to NOT doing LCD anti-aliasing for text on macOS if it wants to fit in with the look of native applications.
(I?m not sure if that applies to non-retina displays.)

Scott

@mlbridge
Copy link

mlbridge bot commented Mar 12, 2021

Mailing list message from Jayathirth D V on awt-dev:

On 12-Mar-2021, at 9:29 AM, Scott Palmer <swpalmer at gmail.com> wrote:

On Mar 11, 2021, at 9:53 PM, Sergey Bylokhov <serb at openjdk.java.net> wrote:

?On Fri, 12 Mar 2021 02:29:04 GMT, Jayathirth D V <jdv at openjdk.org> wrote:

src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 323:

321: * more code just to support a few uncommon cases.
322: */
323: public boolean canRenderLCDText(SunGraphics2D sg2d) {

Just curious, can we render LCD on 10.14+ via metal? Does it work fine?

Yes Sergey it works fine in 10.14+ systems via metal. Most of the JCK manual tests use LCD text on UI Components and it is recently verified in 10.14+ systems for EA10.

Ok, for some reason I thought that the new macOS stopped providing LCD glyphs.

Are the glyphs different or just the way they are rasterized?

Newer versions of macOS don?t do LCD text and have gone back to plain grey-scale anti-aliasing.
Java 2D should default to NOT doing LCD anti-aliasing for text on macOS if it wants to fit in with the look of native applications.
(I?m not sure if that applies to non-retina displays.)

By default we don?t do LCD anti-aliasing for text, only when we set Text Rendering Hint to use LCD antialiasing we take sub-pixel rendering path.

Scott

@mrserb
Copy link
Member

mrserb commented Mar 12, 2021

By default we don?t do LCD anti-aliasing for text, only when we set Text Rendering Hint to use LCD antialiasing we take sub-pixel rendering path.

And it actually produces LCD glyphs? not the grayscale?

@mlbridge
Copy link

mlbridge bot commented Mar 12, 2021

Mailing list message from Jayathirth D V on awt-dev:

On 12-Mar-2021, at 10:30 AM, Sergey Bylokhov <serb at openjdk.java.net> wrote:

On Fri, 12 Mar 2021 00:09:54 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

Ajit Ghaisas has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 47 additional commits since the last revision:

- Lanai PR#214 - 8263324 - avu
- Merge branch 'master' into 8260931_lanai_JEP_branch
- Lanai PR#213 - 8263325 - avu
- Lanai PR#212 - 8259825 - aghaisas
- Lanai PR#211 - 8262882 - aghaisas
- Merge branch 'master' into 8260931_lanai_JEP_branch
- Lanai PR#210 - 8263159 - jdv
- Lanai PR#209 - 8262936 - jdv
- Lanai PR#208 - 8262928 - jdv
- Lanai PR#207 - 8262750 - jdv
- ... and 37 more: https://git.openjdk.java.net/jdk/compare/9d82be31...369c3d2

Latest changes look good. I confirmed that this PR has all of the content from the lanai repo.

By default we don?t do LCD anti-aliasing for text, only when we set Text Rendering Hint to use LCD antialiasing we take sub-pixel rendering path.

And it actually produces LCD glyphs? not the grayscale?

We get glyphs which take more than a byte to represent each pixel.
Based on which we take LCD sub-pixel rendering path.

@aghaisas
Copy link
Contributor Author

/integrate

@openjdk openjdk bot closed this Mar 15, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 15, 2021
@openjdk
Copy link

openjdk bot commented Mar 15, 2021

@aghaisas Since your change was applied there have been 38 commits pushed to the master branch:

  • 0638303: 8263497: Clean up sun.security.krb5.PrincipalName::toByteArray
  • ba22e6f: 8263446: Avoid unary minus over unsigned type in ObjectSynchronizer::dec_in_use_list_ceiling
  • b371f90: 8263504: Some OutputMachOpcodes fields are uninitialized
  • f7e0a09: 8263425: AArch64: two potential bugs in C1 LIRGenerator::generate_address()
  • 554dd29: 8263564: Consolidate POSIX code for runtime exit support: os::shutdown, os::abort and os::die
  • da9ead5: 8263399: CDS should archive only classes allowed by module system
  • 9c84899: 8263555: use driver-mode to run ClassFileInstaller
  • 8e562d2: 8263477: serviceability/sa/ClhsdbDumpheap.java timed out
  • a7aba2b: 8263549: 8263412 can cause jtreg testlibrary split
  • d339320: 8263136: C4530 was reported from VS 2019 at access bridge
  • ... and 28 more: https://git.openjdk.java.net/jdk/compare/273f8bdf5fe14e65e40d3d908f58dd682bb472ed...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8afec70.

💡 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
Labels
2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org build build-dev@openjdk.org integrated Pull request has been integrated
10 participants