On Tue, 22 Jul 2025 17:00:25 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 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 12 additional > commits since the last revision: > > - Merge branch 'master' into impl-metal > - add comment for ES2SwapChain.getFboID > - remove MTLLog > - andy review comments 1 > - changes for running apps in eclipse > - review-update: jni method refactoring > - add @Override > - minor cleanup changes in glass > - Use appropriate layer for setting opacity > - Glass changes after Metal PR inputs > - ... and 2 more: https://git.openjdk.org/jfx/compare/4a48e6e5...1a9a0a41 I focused on reviewing native code and skimmed over the rest. Looks good overall. I only found one more issue in `build.gradle` which we should address, as it could pose some problems when merging D3D12 and Metal together. build.gradle line 2680: > 2678: // Prism JSL > 2679: outPkg = IS_WINDOWS ? "com/sun/prism/d3d/hlsl" : (IS_MAC ? > "com/sun/prism/mtl/msl" : "") > 2680: addJSL(project, "Prism", outPkg, null) { sourceDir, destinationDir > -> I'm not quite sure if this is the best way to do it when counting in other platforms. While this used to work well on Windows with ES2 and D3D being the only backends, and it works well on Mac with ES2-MTL transition, it would unfortunately fall apart when mixing in D3D12. On D3D12 branch JSLC generates two types of shaders at once - HLSL and HLSL6. With this change, on Windows both shaders would end up in `com/sun/prism/d3d/hlsl` when they should be separated. I think we should look through this and unify the way `addJSL` is called so that it functions better for multiple shader types. This would also be helpful when eventually merging direct3d12 to this code. I think a better approach (you can source this from `direct3d12` branch [right here - added L1710 and L2808](https://github.com/openjdk/jfx-sandbox/commit/380fc8ea06a0a3187460013b254f93df4d683b32)) would be to have a generic package prefix `com/sun/prism` here and add shader-specific suffixes inside `addJSL` compile targets. Those targets would also be enabled-disabled based on current build platform (ex. HLSL/HLSL6 are both enabled by IS_WINDOWS property, whereas MSL would be enabled by IS_MAC property). Let me know what you think of this approach and would it work with your backend. ------------- PR Review: https://git.openjdk.org/jfx/pull/1824#pullrequestreview-3055099478 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2230880830
