Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Wed, 10 Feb 2021 21:40:46 GMT, Gerard Ziemski wrote: >> Changes requested by gziemski (Committer). > > I tried to code review the native implementation files, 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! A small thing caught my eye when I was comparing OpenGL and Metal pipelines running J2Demo.jar It looks to me like OpenGL rendering pipeline uses brighter colors. I prefer the more subdued colors that Metal is rendering, but not sure if this is something you want to investigate further. I tried to capture what I see below: OpenGL: https://user-images.githubusercontent.com/63425797/107575744-dc0ca180-6bb5-11eb-9eb3-5cff415eb8a3.png";> Metal (the green color has a yellowish tint): https://user-images.githubusercontent.com/63425797/107575716-cf884900-6bb5-11eb-8ae8-14212ec94e87.png";> Performance wise I did not see much difference either way. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 23:07:39 GMT, Gerard Ziemski 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 18:05:02 GMT, Gerard Ziemski 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 16:53:16 GMT, Gerard Ziemski 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 14:22:27 GMT, Kevin Rushforth 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 12:28:07 GMT, Ajit Ghaisas 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
Changes requested by gziemski (Committer).
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`.
-
PR: https://git.openjdk.java.net/jdk/pull/2403
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 12:28:07 GMT, Ajit Ghaisas 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.
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 17:15:25 GMT, Gerard Ziemski 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 17:15:25 GMT, Gerard Ziemski 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Sat, 6 Feb 2021 00:53:08 GMT, Kevin Rushforth 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 13:40:22 GMT, Ajit Ghaisas 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Fri, 5 Feb 2021 18:42:02 GMT, Kevin Rushforth 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
Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
> **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
