On Mon, 4 Aug 2025 10:46:37 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-andy: review comments
modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
line 78:
> 76: private boolean hasTextureVar;
> 77:
> 78: private final List<String> helperFunctions = new ArrayList<>();;
Double `;`
modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
line 170:
> 168: "ddx", "dfdx",
> 169: "ddy", "dfdy",
> 170: "intcast", "int");
Not that it matters probably, but why are these immutable maps and not
switches? If we know all the strings at compile time and the structure is used
only for key-based retrievals, we're not gaining anything with a map.
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPipeline.java line
90:
> 88:
> 89: // This enables sharing of MTLCommandQueue between PRISM and
> GLASS
> 90: Map<String, Long> devDetails =
> MTLPipeline.getInstance().getDeviceDetails();
`getDeviceDetails()` returns a raw `Map`, but here it's cast to `Map<String,
Long>`. Is it safe? We probably want to fix it in `GraphicsPipelin`.
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
line 193:
> 191:
> 192: MTLTextureData textData = new MTLTextureData(context, pResource,
> size);
> 193: MTLTextureResource<MTLTextureData> resource = new
> MTLTextureResource(textData, true);
`MTLTextureResource` is raw -> `MTLTextureResource<>`
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
line 197:
> 195: // contentX and contentY is set as 0 unlike D3D/ES2.
> 196: // The wrap mode are addressed, can be mapped to D3D/ES2 only if
> necessary.
> 197: return new MTLTexture(getContext(), resource, formatHint,
> wrapMode, allocw, alloch, 0, 0, allocw, alloch, useMipmap);
Same raw type.
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line
149:
> 147: for (int colIndex = 0; colIndex < rowStride;
> colIndex += 3) {
> 148: index = rowIndex + colIndex;
> 149: arr32Bit[dstIndex++] = arr[index + 2];
`arr` might be `null` here if `buf.hasArray() == false`, resulting in an NPE.
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line
174:
> 172: for (int colIndex = 0; colIndex < srcw; colIndex++) {
> 173: index = rowIndex + colIndex;
> 174: arr32Bit[dstIndex++] = arr[index];
Same here.
modules/javafx.graphics/src/main/jsl-decora/GenAllDecoraShaders.java line 50:
> 48: for (int i = 0; i < compileShaders.length; i++) {
> 49: args[7] = compileShaders[i][1];
> 50: args[8] = compileShaders[i][2];
I'd add a comment to `args[7]` and 8 because it's not clear what these are and
where `args` is coming from since a `main` method can be invoked from
everywhere.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2252690093
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2252699423
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2252715145
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2252718882
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2252720009
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2253497317
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2253498081
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2253617601
