On Fri, 1 Aug 2025 17:09:58 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> ### Description
>> This is the implementation of new graphics rendering pipeline for JavaFX
>> using Metal APIs on MacOS.
>> We released two Early Access (EA) builds and have reached a stage where it
>> is ready to be integrated.
>> 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 (by providing `-Dprism.order=mtl`)
>> The `-Dprism.verbose=true` option can be used to verify the rendering
>> pipeline in use.
>>
>> ### Details about the changes
>>
>> **Shader changes**
>> - MSLBackend class: This is the primary class that parses (Prism and Decora)
>> jsl shaders into Metal shaders(msl)
>> - There are a few additional Metal shader files added under directory :
>> modules/javafx.graphics/src/main/native-prism-mtl/msl
>>
>> **Build changes** - There are new tasks added to build.gradle for
>> - Generation/ Compilation/ linking of Metal shaders
>> - Compilation of Prism Java and Objective C files
>>
>> **Prism** - Prism is the rendering engine of JavaFX.
>> - Added Metal specific Java classes and respective native implementation
>> which use Metal APIs
>> - Java side changes:
>> - New Metal specific classes: Classes prefixed with MTL, are added here :
>> modules/javafx.graphics/src/main/java/com/sun/prism/mtl
>> - Modification to Prism common classes: A few limited changes were
>> required in Prism common classes to support Metal classes.
>> - Native side changes:
>> - New Metal specific Objective C implementation is added here:
>> modules/javafx.graphics/src/main/native-prism-mtl
>>
>> **Glass** - Glass is the windowing toolkit of JavaFX
>> - Existing Glass classes are refactored to support both OpenGL and Metal
>> pipelines
>> - Added Metal specific Glass implementation.
>>
>> ### Testing
>> - Testing performed on different hardware and macOS versions.
>> - HW - macBooks with Intel, Intel with discrete graphics card, Apple
>> Silicon (M1, M2 and M4)
>> - macOS versions - macOS 13, macOS 14 and macOS 15
>> - Functional Testing:
>> - All headful tests pass with Metal pipeline.
>> - Verified samples applications: Ensemble and toys
>> - Performance Testing:
>> - Tested with RenderPerfTest: Almost all tests match/exceed es2
>> performance except a few that fall a little short on different hardwares.
>> These will be addressed separately (there are open bugs already filed)
>>
>> ### Note to reviewers :
>> - Providing `-Dprism.order=mtl` option is a must for testing the Metal
>> pipeline
>> - It woul...
>
> Ambarish Rapte has updated the pull request incrementally with one additional
> commit since the last revision:
>
> kcr: review: crash regression fix
I left a few inline comments, but this looks good overall, and is almost ready
to go in.
I'll approve once you've addressed Andy's and my questions and have done the
merge from master and a full test.
build.gradle line 1699:
> 1697: doFirst {
> 1698: mkdir "$project.buildDir/gensrc/mtl-headers"
> 1699: }
This is only needed for macOS builds. Should it be conditional?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 98:
> 96: try {
> 97: final String shaderLibName = "msl/jfxshaders.metallib";
> 98: final Class<?> clazz =
> Class.forName("com.sun.prism.mtl.MTLContext");
What is the purpose of this change? It's almost always better to reference a
known class directly rather than `Class.forName(...)`, especially in this case
where we're referring to ourself. I recommend reverting this change and
assigning it to `MTLContext.class` directly.
modules/javafx.graphics/src/main/native-glass/mac/GlassView.m line 496:
> 494: [view release];
> 495: }
> 496: GLASS_POOL_POP; // it was pushed by "_begin"*/
Minor: looks like you have a leftover `*/`
modules/javafx.graphics/src/main/native-prism-mtl/MetalContext.m line 1550:
> 1548: */
> 1549: JNIEXPORT void JNICALL
> Java_com_sun_prism_mtl_MTLResourceFactory_nReleaseTexture
> 1550: (JNIEnv *env, jclass class, jlong pTexture)
I missed the discussion on this. I presume that the native context pointer was
removed because it was unused? The other pipelines pass it (probably because
they need it), so I was surprised that MTL doesn't. I guess there is no harm in
removing it, if it really is unneeded.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1824#pullrequestreview-3080640437
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2248947770
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2248918721
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2248956901
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2248968967
