<AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline

classic Classic list List threaded Threaded
46 messages Options
123
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline

Ajit Ghaisas-2
This is the implementation of JEP 382 : New macOS Rendering Pipeline (Metal rendering pipeline)

JEP Link - https://bugs.openjdk.java.net/browse/JDK-8238361

-------------

Commit messages:
 - Project Lanai Patch

Changes: https://git.openjdk.java.net/jdk/pull/2403/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260931
  Stats: 17445 lines in 83 files changed: 17395 ins; 1 del; 49 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline

Magnus Ihse Bursie-3
On Thu, 4 Feb 2021 10:35:02 GMT, Ajit Ghaisas <[hidden email]> wrote:

> **Description :**
> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>
> **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)

Build changes look good. No opinion on the source changes.

-------------

Marked as reviewed by ihse (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline

Alexey Ushakov-3
In reply to this post by Ajit Ghaisas-2
On Thu, 4 Feb 2021 10:35:02 GMT, Ajit Ghaisas <[hidden email]> wrote:

> **Description :**
> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>
> **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)

Looks good except for one whitespace problem (src/java.desktop/share/native/libawt/java2d/SurfaceData.c)

src/java.desktop/share/native/libawt/java2d/SurfaceData.c line 238:

> 236: {
> 237:     SurfaceDataOps *ops = malloc(opsSize);
> 238:

The only whitespace added. Should be removed

-------------

Changes requested by avu (no project role).

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline

Kevin Rushforth-2
In reply to this post by Ajit Ghaisas-2
On Thu, 4 Feb 2021 10:35:02 GMT, Ajit Ghaisas <[hidden email]> wrote:

> **Description :**
> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>
> **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)

Here is my initial set of mostly minor comments and a couple questions.

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.h line 33:

> 31: #import "common.h"
> 32:
> 33: #import <JavaNativeFoundation/JavaNativeFoundation.h>

JNF has been removed from the jdk, so this must be removed or it will no longer compile. It has already been fixed in the lanai repo by [JDK-8261234](https://bugs.openjdk.java.net/browse/JDK-8261234).

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m line 38:

> 36: #import <string.h>
> 37: #import <ApplicationServices/ApplicationServices.h>
> 38: #import <JavaNativeFoundation/JavaNativeFoundation.h>

JNF has been removed from the jdk, so this must be removed or it will no longer compile. It has already been fixed in the lanai repo by [JDK-8261234](https://bugs.openjdk.java.net/browse/JDK-8261234).

make/modules/java.desktop/lib/Awt2dLibraries.gmk line 894:

> 892:   SHADERS_SUPPORT_DIR := $(SUPPORT_OUTPUTDIR)/native/java.desktop/libosxui
> 893:   SHADERS_AIR := $(SHADERS_SUPPORT_DIR)/shaders.air
> 894:   SHADERS_LIB := $(INSTALL_LIBRARIES_HERE)/shaders.metallib

Q: Should 2d (or awt) be in the name of this file, or is a generic name OK?

src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line 2:

> 1: /*
> 2:  * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.

Should be `2019, 2021,` (I missed this file when I fixed up the copyright notices and years).

src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line 81:

> 79:             (PrivilegedAction<String>) () ->
> 80:                     System.getProperty("java.home", "") + File.separator +
> 81:                             "lib" + File.separator + "shaders.metallib");

Same question as in the makefile about the name of the shader library file.

src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line 155:

> 153:             if (cfginfo != 0L) {
> 154:                 textureSize = nativeGetMaxTextureSize();
> 155:                 // 7160609: GL still fails to create a square texture of this

This refers to GL. Is this relevant to metal?

src/java.desktop/macosx/classes/sun/java2d/metal/MTLRenderQueue.java line 78:

> 76:      * of the MTL pipeline and related classes.
> 77:      */
> 78:     public static void sync() {

Should this be synchronized?

src/java.desktop/macosx/classes/sun/java2d/opengl/CGLGraphicsConfig.java line 142:

> 140:                 // TODO : This clamping code is same as in OpenGL.
> 141:                 // Whether we need such clamping or not in case of Metal
> 142:                 // will be pursued under 8260644

This change seems wrong. The comment suggests it belong in a metal class or method, not here where we are using OpenGL.

src/java.desktop/macosx/classes/sun/lwawt/macosx/CPlatformView.java line 193:

> 191:             responder.handleScrollEvent(x, y, absX, absY, event.getModifierFlags(),
> 192:                     event.getScrollDeltaX(), event.getScrollDeltaY(),
> 193:                     event.getScrollPhase());

Minor: This part of the file is otherwise unchanged. Maybe revert this whitespace-only change? Same applies to the last two changes in this file.

src/java.desktop/macosx/classes/sun/lwawt/macosx/CWarningWindow.java line 227:

> 225:                             CWrapper.NSWindow.orderWindow(ptr,
> 226:                                     CWrapper.NSWindow.NSWindowAbove,
> 227:                                     ownerPtr);

Minor: This part of the file is otherwise unchanged. Maybe revert this whitespace-only change? Same applies futher down in a couple places.

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/EncoderManager.h line 47:

> 45:  *  2. Updates 'mutable' properties encoder: pipelineState (with corresponding buffers), clip, transform, e.t.c. To avoid
> 46:  *  unnecessary calls of [encoder setXXX] this manager compares requested state with cached one.
> 47:  * */

Minor: `* */` --> `*/`

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/EncoderManager.m line 204:

> 202: {
> 203:     if (clip.stencilMaskGenerationInProgress == JNI_TRUE) {
> 204:         // don't set setScissorOrStencil when generateion in progress

Minor: typo in comment, should be `generation`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline

Kevin Rushforth-2
In reply to this post by Ajit Ghaisas-2
On Thu, 4 Feb 2021 10:35:02 GMT, Ajit Ghaisas <[hidden email]> wrote:

> **Description :**
> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>
> **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)

I left a few additional comments. Overall this looks good to me.

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m line 82:

> 80:     (JNIEnv *env, jclass mtlgc)
> 81: {
> 82:     FILE *f = popen("/usr/sbin/system_profiler SPDisplaysDataType", "r");

How robust is this? It seems like the contents of this could be an implementation detail and subject to change. Is it documented by Apple?

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m line 83:

> 81: {
> 82:     FILE *f = popen("/usr/sbin/system_profiler SPDisplaysDataType", "r");
> 83:     bool metalSupported = JNI_FALSE;

This code is mixing types; it should be `jboolean metalSupported`

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLMaskFill.m line 26:

> 24:  */
> 25:
> 26: #ifndef HEADLESS

I see a few occurrences of `#ifndef HEADLESS` in the metal pipeline. Is this needed? I don't see any of the other native macos files in Java2D (e.g., the OpenGL pipeline) doing the same. Will this file ever be compiled with HEADLESS being undefined?

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLPaints.h line 41:

> 39: /**
> 40:  * The MTLPaint class represents paint mode (color, gradient, e.t.c.)
> 41:  * */

Minor: `* */` --> `*/`; also typo: `e.t.c.` should be `etc.`

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceDataBase.h line 37:

> 35:
> 36: /**
> 37:  * The MTLSDOps structure describes a native OpenGL surface and contains all

Should that be "Metal" and not "OpenGL" ?

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLTextRenderer.m line 77:

> 75:  * be safe to use this one glyph cache for all screens in a multimon
> 76:  * environment, since the glyph cache texture is shared between all contexts,
> 77:  * and (in theory) OpenGL drivers should be smart enough to manage that

`Metal` drivers?

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLTexurePool.m line 29:

> 27: #import "Trace.h"
> 28:
> 29: #define SCREEN_MEMORY_SIZE_4K (4096*2160*4) //~33,7 mb

This means that a 4k display with a narrower aspect ratio wouldn't fit (assuming there ever were to be such a thing). What would happen if you encountered a screen that was, say, 4k * 2.5K?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline

Kevin Rushforth-2
In reply to this post by Ajit Ghaisas-2
On Thu, 4 Feb 2021 10:35:02 GMT, Ajit Ghaisas <[hidden email]> wrote:

> **Description :**
> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>
> **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)

The file in `RenderPerfTest` should have a GPLv2 license header (no Classpath). I filed [JDK-8261273](https://bugs.openjdk.java.net/browse/JDK-8261273) and also highlighted a couple examples below.

test/jdk/performance/client/RenderPerfTest/Makefile line 10:

> 8: #   - Redistributions of source code must retain the above copyright
> 9: #     notice, this list of conditions and the following disclaimer.
> 10: #

As mentioned in the preliminary review, this file and `build.xml` have a BSD copyright. I think that should be GPLv2 (without classpath exception since this is a test).

test/jdk/performance/client/RenderPerfTest/src/renderperf/RenderPerfTest.java line 9:

> 7:  * published by the Free Software Foundation.  Oracle designates this
> 8:  * particular file as subject to the "Classpath" exception as provided
> 9:  * by Oracle in the LICENSE file that accompanied this code.

Since this a test, this should not have the Classpath exception.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v2]

Ajit Ghaisas-2
In reply to this post by Ajit Ghaisas-2
> **Description :**
> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>
> **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)

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 three additional commits since the last revision:

 - Lanai PR#174 - 8261234 - kcr
 - Merge branch 'master' into 8260931_lanai_JEP_branch
   Merge from openjdk/jdk
 - Project Lanai Patch

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/0b8d96b2..8ed7b5f5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=00-01

  Stats: 12194 lines in 245 files changed: 7103 ins; 4454 del; 637 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Ajit Ghaisas-2
In reply to this post by Ajit Ghaisas-2
> **Description :**
> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>
> **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)

Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:

  Lanai PR#175 - 8261304 - aghaisas

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/8ed7b5f5..6044adc0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=01-02

  Stats: 32 lines in 10 files changed: 0 ins; 12 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Ajit Ghaisas-2
In reply to this post by Kevin Rushforth-2
On Fri, 5 Feb 2021 18:42:02 GMT, Kevin Rushforth <[hidden email]> wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Lanai PR#175 - 8261304 - aghaisas
>
> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 894:
>
>> 892:   SHADERS_SUPPORT_DIR := $(SUPPORT_OUTPUTDIR)/native/java.desktop/libosxui
>> 893:   SHADERS_AIR := $(SHADERS_SUPPORT_DIR)/shaders.air
>> 894:   SHADERS_LIB := $(INSTALL_LIBRARIES_HERE)/shaders.metallib
>
> Q: Should 2d (or awt) be in the name of this file, or is a generic name OK?

I think, a generic name is OK as the path of shader file already has both awt (libawt_lwawt) and java2d in it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Kevin Rushforth-2
On Mon, 8 Feb 2021 13:40:22 GMT, Ajit Ghaisas <[hidden email]> wrote:

>> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 894:
>>
>>> 892:   SHADERS_SUPPORT_DIR := $(SUPPORT_OUTPUTDIR)/native/java.desktop/libosxui
>>> 893:   SHADERS_AIR := $(SHADERS_SUPPORT_DIR)/shaders.air
>>> 894:   SHADERS_LIB := $(INSTALL_LIBRARIES_HERE)/shaders.metallib
>>
>> Q: Should 2d (or awt) be in the name of this file, or is a generic name OK?
>
> I think, a generic name is OK as the path of shader file already has both awt (libawt_lwawt) and java2d in it.

In the source tree, yes, but not in the jdk image where it ends up in `$JAVA_HOME/lib/shaders.metallib`. I don't have a problem with this, as long as it is a deliberate decision.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Gerard Ziemski-2
In reply to this post by Kevin Rushforth-2
On Sat, 6 Feb 2021 00:53:08 GMT, Kevin Rushforth <[hidden email]> wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Lanai PR#175 - 8261304 - aghaisas
>
> The file in `RenderPerfTest` should have a GPLv2 license header (no Classpath). I filed [JDK-8261273](https://bugs.openjdk.java.net/browse/JDK-8261273) and also highlighted a couple examples below.

General comment - I am not sure I like the `MTL` prefix acronym in names (ex. `sun.java2d.metal.MTLVolatileSurfaceManager`).

I think you tried to match the `CGL`, but that is a real acronym that stands for "Core Graphics Layer" (I think).

`MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I suppose, but also just `Metal` would work just fine.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Gerard Ziemski-2
On Mon, 8 Feb 2021 17:15:25 GMT, Gerard Ziemski <[hidden email]> wrote:

>> The file in `RenderPerfTest` should have a GPLv2 license header (no Classpath). I filed [JDK-8261273](https://bugs.openjdk.java.net/browse/JDK-8261273) and also highlighted a couple examples below.
>
> General comment - I am not sure I like the `MTL` prefix acronym in names (ex. `sun.java2d.metal.MTLVolatileSurfaceManager`).
>
> I think you tried to match the `CGL`, but that is a real acronym that stands for "Core Graphics Layer" (I think).
>
> `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I suppose, but also just `Metal` would work just fine.

I'm in the process of reviewing this feature, but there is lots of code to go through - please wait for my review.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Kevin Rushforth-2
In reply to this post by Gerard Ziemski-2
On Mon, 8 Feb 2021 17:15:25 GMT, Gerard Ziemski <[hidden email]> wrote:

> General comment - I am not sure I like the `MTL` prefix acronym in names (ex. `sun.java2d.metal.MTLVolatileSurfaceManager`).
>
> I think you tried to match the `CGL`, but that is a real acronym that stands for "Core Graphics Layer" (I think).
>
> `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I suppose, but also just `Metal` would work just fine.

`MTL` is the abbreviation that Apple uses for Metal in all of their APIs. The only potential issue I might see with this prefix is in the native code where there could be name collisions between Java2D's names and Apple's names. Since we haven't run into such a collision, I don't think this needs to change, and wouldn't necessarily affect the Java class names anyway. If we were to consider it, `METAL` seems better than `ML` (which is too short to be descriptive and might suggest machine learning).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Gerard Ziemski-2
In reply to this post by Ajit Ghaisas-2
On Mon, 8 Feb 2021 12:28:07 GMT, Ajit Ghaisas <[hidden email]> wrote:

>> **Description :**
>> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
>> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>>
>> **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)
>
> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
>
>   Lanai PR#175 - 8261304 - aghaisas

> > General comment - I am not sure I like the `MTL` prefix acronym in names (ex. `sun.java2d.metal.MTLVolatileSurfaceManager`).
> > I think you tried to match the `CGL`, but that is a real acronym that stands for "Core Graphics Layer" (I think).
> > `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I suppose, but also just `Metal` would work just fine.
>
> `MTL` is the abbreviation that Apple uses for Metal in all of their APIs. The only potential issue I might see with this prefix is in the native code where there could be name collisions between Java2D's names and Apple's names. Since we haven't run into such a collision, I don't think this needs to change, and wouldn't necessarily affect the Java class names anyway. If we were to consider it, `METAL` seems better than `ML` (which is too short to be descriptive and might suggest machine learning).

If Apple itself uses `MTL` then we are good.

src/java.desktop/macosx/classes/sun/awt/CGraphicsConfig.java line 35:

> 33:
> 34: import sun.java2d.SurfaceData;
> 35: import sun.java2d.opengl.CGLLayer;

Not needed import anymore?

src/java.desktop/macosx/classes/sun/awt/CGraphicsDevice.java line 113:

> 111:             // This indicates fallback to other rendering pipeline also failed.
> 112:             // Should never reach here
> 113:             throw new InternalError("Error - unable to initialize any rendering pipeline.");

There is no software based renderer to fall back here?

src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 66:

> 64:                        propString.equals("f") ||
> 65:                        propString.equals("False") ||
> 66:                        propString.equals("F"))

Shouldn't `1` and `0` be also allowed here?

src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 100:

> 98:                         oglState = PropertyState.ENABLED; // Enable default pipeline
> 99:                         metalState = PropertyState.DISABLED; // Disable non-default pipeline
> 100:                     }

This matches JEP 382 specification, but even when both GL is `false` and Metal is `false` we still get GL? There is no software based pipeline anymore?

src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 131:

> 129:         @Native
> 130:         static final int CAPS_EXT_GRAD_SHADER  = (FIRST_PRIVATE_CAP << 3);
> 131:         /** Indicates the presence of the GL_ARB_texture_rectangle extension. */

Reference to `GL_ARB_texture_rectangle` extension in Metal pipeline?

src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 134:

> 132:         @Native
> 133:         static final int CAPS_EXT_TEXRECT      = (FIRST_PRIVATE_CAP << 4);
> 134:         /** Indicates the presence of the GL_NV_texture_barrier extension. */

Reference to `GL_NV_texture_barrier` extension in Metal pipeline?

src/java.desktop/macosx/classes/sun/java2d/metal/MTLRenderQueue.java line 97:

> 95:     public static void disposeGraphicsConfig(long pConfigInfo) {
> 96:         MTLRenderQueue rq = getInstance();
> 97:         rq.lock();

Is it allowed to have multiple `MTLRenderQueue` instances?

If not, then I see this pattern everywhere:

        MTLRenderQueue rq = getInstance();
        rq.lock();
        {
          ...
        }
        rq.unlock();
why not just do:

        MTLRenderQueue.lock();
        {
          ...
        }
        MTLRenderQueue.unlock();

and have `MTLRenderQueue.lock()` and `MTLRenderQueue.unlock()` implement getting the actual lock instance and locking/unlocking it?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Ajit Ghaisas-2
In reply to this post by Kevin Rushforth-2
On Mon, 8 Feb 2021 14:22:27 GMT, Kevin Rushforth <[hidden email]> wrote:

>> I think, a generic name is OK as the path of shader file already has both awt (libawt_lwawt) and java2d in it.
>
> In the source tree, yes, but not in the jdk image where it ends up in `$JAVA_HOME/lib/shaders.metallib`. I don't have a problem with this, as long as it is a deliberate decision.

OK. I get your point. Keeping the name unchanged for now as there won't be another .metallib in JDK. The name can be changed in the future if need arises.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]

Ajit Ghaisas-2
In reply to this post by Ajit Ghaisas-2
> **Description :**
> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>
> **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)

Ajit Ghaisas has updated the pull request incrementally with two additional commits since the last revision:

 - Lanai PR#177 - 8261430 - aghaisas
 - Lanai PR#176 - 8261399 - jdv

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/6044adc0..64173289

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=02-03

  Stats: 49 lines in 14 files changed: 0 ins; 43 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Ajit Ghaisas-2
In reply to this post by Gerard Ziemski-2
On Mon, 8 Feb 2021 16:53:16 GMT, Gerard Ziemski <[hidden email]> wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Lanai PR#175 - 8261304 - aghaisas
>
> src/java.desktop/macosx/classes/sun/awt/CGraphicsDevice.java line 113:
>
>> 111:             // This indicates fallback to other rendering pipeline also failed.
>> 112:             // Should never reach here
>> 113:             throw new InternalError("Error - unable to initialize any rendering pipeline.");
>
> There is no software based renderer to fall back here?

OpenGL was the sole rendering pipeline on macOS. Now we are adding Metal rendering pipeline. There is no software based fall back renderer.

> src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 66:
>
>> 64:                        propString.equals("f") ||
>> 65:                        propString.equals("False") ||
>> 66:                        propString.equals("F"))
>
> Shouldn't `1` and `0` be also allowed here?

No.
0 and 1 are not supported for existing VM options for rendering pipelines. We won't be adding it for metal pipeline either.

> src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 100:
>
>> 98:                         oglState = PropertyState.ENABLED; // Enable default pipeline
>> 99:                         metalState = PropertyState.DISABLED; // Disable non-default pipeline
>> 100:                     }
>
> This matches JEP 382 specification, but even when both GL is `false` and Metal is `false` we still get GL? There is no software based pipeline anymore?

OpenGL was the sole rendering pipeline on macOS. Now we are adding Metal rendering pipeline. There is no software based fall back renderer.

If user requests both - GL and Metal - as false - we shall use 'the default' pipeline.
Current default render pipeline on macOS is OpenGL.
Changing default render pipeline on macOS is not in the scope of this JEP.

> src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 61:
>
>> 59:
>> 60: public abstract class MTLSurfaceData extends SurfaceData
>> 61:         implements AccelSurface {
>
> `MTLSurfaceData` and `OGLSurfaceData` seem to share lots of the same code, isn't there a way to refactor the common code out?
>
> There are other files that structually look identical, except for the names of classes they use, ex `MTLMaskBlit.java` and `OGLMaskBlit.java`.

Although it is a good suggestion, I do not recommend this refactoring -
- We do not wish to destabilize proven OpenGL pipeline by refactoring - additional testing on OpenGL pipeline also would be needed.
- OpenGL pipeline on macOS might be removed once Apple removes OpenGL support from macOS - this will render this refactoring pointless.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Jayathirth D V-2
In reply to this post by Gerard Ziemski-2
On Mon, 8 Feb 2021 18:05:02 GMT, Gerard Ziemski <[hidden email]> wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Lanai PR#175 - 8261304 - aghaisas
>
> src/java.desktop/macosx/classes/sun/java2d/metal/MTLRenderQueue.java line 97:
>
>> 95:     public static void disposeGraphicsConfig(long pConfigInfo) {
>> 96:         MTLRenderQueue rq = getInstance();
>> 97:         rq.lock();
>
> Is it allowed to have multiple `MTLRenderQueue` instances?
>
> If not, then I see this pattern everywhere:
>
>         MTLRenderQueue rq = getInstance();
>         rq.lock();
>         {
>           ...
>         }
>         rq.unlock();
> why not just do:
>
>         MTLRenderQueue.lock();
>         {
>           ...
>         }
>         MTLRenderQueue.unlock();
>
> and have `MTLRenderQueue.lock()` and `MTLRenderQueue.unlock()` implement getting the actual lock instance and locking/unlocking it?

Thanks for the recommendation. This is a common behavior among different pipelines :  D3D, OpenGL and Metal. Mentioned lock/unlock should be implemented in parent RenderQueue class after refactoring and it will hit other pipelines. Its better to not touch other pipelines as part of this JEP.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]

Prasanta Sadhukhan-2
In reply to this post by Ajit Ghaisas-2
On Tue, 9 Feb 2021 12:29:52 GMT, Ajit Ghaisas <[hidden email]> wrote:

>> **Description :**
>> This is the implementation of [JEP 382 : New macOS Rendering Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
>> 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](http://openjdk.java.net/projects/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](https://bugs.openjdk.java.net/browse/JDK-8251396)
>>
>> **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
>
> Ajit Ghaisas has updated the pull request incrementally with two additional commits since the last revision:
>
>  - Lanai PR#177 - 8261430 - aghaisas
>  - Lanai PR#176 - 8261399 - jdv

src/java.desktop/macosx/classes/sun/java2d/metal/MTLBlitLoops.java line 110:

> 108: //                         MTLSurfaceData.PF_BYTE_GRAY),
> 109: //                 new MTLSwToSurfaceBlit(SurfaceType.UshortGray,
> 110: //                         MTLSurfaceData.PF_USHORT_GRAY),

Probably we could remove this commented lines

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

Gerard Ziemski-2
In reply to this post by Ajit Ghaisas-2
On Mon, 8 Feb 2021 23:07:39 GMT, Gerard Ziemski <[hidden email]> wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Lanai PR#175 - 8261304 - aghaisas
>
> Changes requested by gziemski (Committer).

I tried to code review the native implementation file, but Metal APIs is brand new to me and it's been a long while since I worked with graphics API, so I can't be of much help really.

The code I've looked at looked clean and nothing caught my eye. But it's a partial code review at best.

Good job!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2403
123