On Mon, 8 Feb 2021 12:28:07 GMT, Ajit Ghaisas <aghai...@openjdk.org> 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