jfx
jfx copied to clipboard
openjdk
8271024: Implement macOS Metal Rendering Pipeline
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=mtloption is a must for testing the Metal pipeline - It would be a good idea to test both OpenGL and Metal pipelines
- Known issues and tasks can be found here: https://bugs.openjdk.org/issues/?filter=46763
/skara contributor add @kevinrushforth /skara contributor add @aghaisas /skara contributor add @jayathirthrao
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)
Issue
- JDK-8271024: Implement macOS Metal Rendering Pipeline (Enhancement - P3)
Contributors
- Kevin Rushforth
<[email protected]> - Ajit Ghaisas
<[email protected]> - Jayathirth D V
<[email protected]> - Ambarish Rapte
<[email protected]>
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1824/head:pull/1824
$ git checkout pull/1824
Update a local copy of the PR:
$ git checkout pull/1824
$ git pull https://git.openjdk.org/jfx.git pull/1824/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1824
View PR using the GUI difftool:
$ git pr show -t 1824
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1824.diff
Using Webrev
:wave: Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@arapte This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8271024: Implement macOS Metal Rendering Pipeline
Co-authored-by: Kevin Rushforth <[email protected]>
Co-authored-by: Ajit Ghaisas <[email protected]>
Co-authored-by: Jayathirth D V <[email protected]>
Co-authored-by: Ambarish Rapte <[email protected]>
Reviewed-by: angorya, nlisker, kcr, lkostyra
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
Webrevs
- 21: Full - Incremental (b6ce51e6)
- 20: Full - Incremental (cc3dffc4)
- 19: Full - Incremental (eb63c223)
- 18: Full - Incremental (4602bc3b)
- 17: Full - Incremental (78c90b5a)
- 16: Full - Incremental (95739d6f)
- 15: Full - Incremental (705d7ec7)
- 14: Full - Incremental (3b30062e)
- 13: Full - Incremental (5f24f1de)
- 12: Full - Incremental (db244a02)
- 11: Full - Incremental (bf7141a7)
- 10: Full - Incremental (1a9a0a41)
- 09: Full - Incremental (774890e4)
- 08: Full - Incremental (b9b35608)
- 07: Full - Incremental (fe40d04d)
- 06: Full - Incremental (b8be34a5)
- 05: Full - Incremental (e8ac354a)
- 04: Full - Incremental (67e06ae8)
- 03: Full - Incremental (6a26e459)
- 02: Full - Incremental (0d0aa42c)
- 01: Full - Incremental (ca8b6af0)
- 00: Full (9634aafe)
@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).
As this is a large PR I don't expect any one reviewer to review the whole thing, but I do want to make sure that each piece is reviewed by someone. So we'd like as many eyes on this as possible.
The parts we are most concerned with getting reviewed are changes in existing classes that could impact other pipelines, particularly the default ES2 pipeline on macOS
For example, there are some minor changes in the Prism base classes, which necessitated changes in the pipeline-specific classes.
The largest change in shared code is the glass refactoring to move the rendering-pipeline-specific code (OpenGL or Metal) into pipeline-specific classes. For some classes, the existing code is split between the base class and the CGL subclass (with the MTL class being new implementation for Metal).
Here are the main classes to look at:
-
GlassCGLFrameBufferObject -- renamed from GlassFrameBufferObject with minor changes
-
GlassMTLFrameBufferObject -- sibling class
-
GlassOffscreen -- pipeline-specific pieces moved to subclasses
- GlassCGLOffscreen
- GlassMTLOffscreen
-
GlassLayer3D -- pipeline-specific pieces moved to sub-layers (not using inheritance)
- GlassLayerCGL3D
- GlassLayerMTL3D
-
GlassView -- minor changes to remove OpenGL-isms and fix a few other interface issues
-
GlassView3D -- pipeline-specific pieces moved to subclasses
- GlassViewCGL3D
- GlassViewMTL3D
@jayathirthrao can give more details on the above glass refacatoring changes if you are interested
@beldenfox If you are willing to look at it, we would be quite appreciative of your comments or suggestions on the glass refactoring.
@tiainen Would you be able to look at the build changes and also do a test build?
@beldenfox Thanks for your inputs.
- Regarding adding comments in GlassView/Layer classes about how they are not subclasses but subViews/subLayers : Sure i will go ahead and add comments in the class. Also as you mentioned we can name them better:
- GlassView3D -> GlassViewEventHandler(Since it is common class which handles events for both OpenGL and Metal pipeline) Or GlassSubView(Since it is added as subView in GlassHostView in GlassView.m). Please suggest if you have better names that we can use here.
- GlassViewCGL3D -> GlassViewCGL(With comment mentioning how it is not subclass but a subView of GlassView3D)
- GlassViewMTL3D -> GlassViewMTL(With comment mentioning how it is not subclass but a subView of GlassView3D)
- At other places also i will remove 3D in names and appropriate comment in CGL/MTL layer class.
-
Regarding NSOpenGLView : Since both CGL and MTL views are not subclass of same NSView we have current structure with Event handling in common class GlassView3D and CGL & MTL Views as subViews of GlassView3D. I also tried removing NSOpenGLView reference and then running Ensemble8 in ES2 pipeline and things work fine. But we want to minimize changes related to already present default ES2 pipeline in this PR as part of refactoring effort. We can work on it in as a follow-up issue.
-
Regarding change in MacOSXWindowSystemInterface.m : Same here we want to minimize changes related to ES2 in this PR. We can work on it in as a follow-up issue.
-
Regarding setOpaque() in GlassWindow.m : While refactoring Glass for Metal pipeline under JDK-8325379 this was identified as follow up issue : JDK-8356758. We were aware of how this code related to JDK-8095004 but thanks for pointing to JDK-8095040 which was not verified. There are no regression seen with this refactoring, PickTest3D works properly in both ES2 and MTL pipeline of this PR and matches output seen with mainline code. I verified that test mentioned in JDK-8095040 on latest mainline code and i still see that black filling is drawn. Same behaviour is seen with ES2 and MTL pipeline with this PR. But it works properly in 8u431, looks like the issue has resurfaced so i have created new bug : JDK-8359904
-
Regarding removal of JNI method getNativeLayer() from GlassView.m : Yes this is not used anywhere and we can remove it. I removed this code, built and tested Ensemble8, started CI headful test run. If everything is green will go ahead and remove it and its corresponding code in MacView.java
@jayathirthrao Thanks for the thorough response. And, again, it's great to see Metal happening.
- GlassView3D -> GlassViewEventHandler(Since it is common class which handles events for both OpenGL and Metal pipeline) Or GlassSubView(Since it is added as subView in GlassHostView in GlassView.m). Please suggest if you have better names that we can use here.
I want to keep the GlassView prefix since these classes implement the platform side of the View class in the toolkit. If we were to rename anything it would be GlassViewCGL and GlassViewMTL since these are more closely related to Prism than the toolkit View class. But I'm overthinking things. For now I think we should keep GlassView and GlassView3D along with GlassViewCGL and GlassViewMTL and just clarify what's going on in the comments. At some point I want to combine GlassView and GlassView3D into one class but that can wait for a follow-up issue.
- Regarding setOpaque() in GlassWindow.m
In my testing the setOpaque call isn't happening in this PR since the layer hasn't been created yet. But it doesn't seem to matter (?)
I'm having a hard time reproducing the original PickTest3D bug in the master branch. The screenshots in the bug report show translucency but the original bug report doesn't mention it and the PickTest3D code doesn't call for it. I tweaked PickTest3D to try a few combinations of setOpacity and DECORATED/TRANSPARENT and that setOpaque call didn't seem to make any difference even in the master branch. I must be missing something.
As for the UNIFIED issue I don't think there's a bug. The implementation relied on the OS to draw a brushed metal background but Apple removed that years ago and now just draws black. I will take a look but only because I want to understand how this works; we might need similar logic if we want to support desktop translucency effects. UNIFIED is deprecated and doesn't work on Windows either so we should probably close the new bug as Never Fix.
I'm having a hard time reproducing the original PickTest3D bug in the master branch.
Spoke too soon. PickTest3D has been tweaked since the original Mac bug was posted and resolved.
@beldenfox Found out the root cause for why we are seeing layer as nil GlassWindow.m.
We have additional level of abstraction for views and we set layer for GlassViewCGL/MTL and not for GlassView3D. We already have change in GlassView.m->getGlassView() to get GlassView3D object and then call getLayer() on it. We need to make similar change in GlassWindow.m->getMacView(), with this change we are able to get appropriate layer by calling getLayer(). This change is made and testing is in progress.
Also what i am also noticing is setOpaque() in GlassWindow.m is never called on OpenGL pipeline when i run Ensemble8 demos, PickTest3D or test present in JDK-8095040. I will keep this OpenGL specific setOpaque call since we don't want to make changes related to OpenGL in this PR. But i will add comment about refactoring and moving it to CGL specific class in future.
Regarding classname change, i will keep GlassView3D name as it is but remove 3D references in other classes.
Glass changes are updated based on review inputs.
Also @arapte noticed commented out GLASS_POOL_PUSH/POP logic in GlassView.m which is updated to match mainline code. Since this enables common autoreleasepool, explicit autorelease calls for Vertex & Index buffer in MetalMesh.m are removed. Latest code is tested again and headful CI runs are green with both OpenGL and Metal.
@jayathirthrao Thanks for renaming the classes. Much appreciated.
- Regarding setOpaque() in GlassWindow.m :
That setOpaque: call still isn't being executed. The code is looking at the GlassLayer, not the GlassLayerCGL, so the isKindOfClass: is failing and setOpaque: is never called.
But there's no point in changing that code until we can reproduce the original PickTest3D bug and I haven't been able to do that. Have you? The bug (JDK-8095004) was entered in 2013 but the version of PickTest3D in apps/toys was added in early 2015. I don't know how closely it matches the code that caused the original bug report.
The original bug report doesn't add up. The comments state that the problem stems from using a Phong material that has a diffuse color with an alpha of 0.5. That should cause the individual objects to become translucent which is what happens when I tweak the PickTest3D code. But the screen shot in the bug report shows solid objects; the entire scene is washed out but the individual objects are opaque.
I can't test this on my Windows box probably because I'm running in a VM. Prism reports that the D3D renderer is running without error but I still get blank screens except for the ColorCube toy.
I did a build on our infrastructure and everything worked fine. We successfully tested the resulting runtime with both Metal and OpenGL and on Intel and Apple Silicon hardwares.
@beldenfox Thanks for pointing to improper isKindOfClass: usage. That is updated now and setOpaque is getting called properly as in mainline.
Regarding reproducing the issue originally seen in PickTest3D : PickTest3D is updated in 2015 under JDK-8130532 to use alpha value as 1.0 instead of 0.5 for diffuse color. I reverted this change to use 0.5 and disabled calling setOpaque in GlassWindow.m. Unfortunately, i see proper output with translucent objects and there is no difference in output of PickTest3D with/without setOpaque call even with 0.5 alpha value. As you have also observed in current code setOpaque call has no effect in output even when we use 0.5 alpha value. Currently i am trying get information related to PickTest3D code pre 2015.
Regarding running demos in VM : Is Prism falling back to SW pipeline? (You can check for this info using -Dprism.verbose=true). If it is falling back to SW pipeline in VM, we can try to force it to use hardware pipeline using -Dprism.forceGPU=true.
Regarding reproducing the issue originally seen in PickTest3D : PickTest3D is updated in 2015 under JDK-8130532 to use alpha value as 1.0 instead of 0.5 for diffuse color.
Thanks for tracking that down. The description in JDK-8130532 references JDK-8095058. From what I can tell that was the original bug that was causing incorrect results on macOS and covering up the incorrect alpha value in the diffuse color. I think using setOpaque: to rip out the alpha channel was a work-around for JDK-8095058 and is probably no longer necessary.
But removing the alpha channel made macOS match the output of Windows and Linux. Is JavaFX designed to work without an alpha channel when the window is not TRANSPARENT?
Regarding running demos in VM : Is Prism falling back to SW pipeline? (You can check for this info using -Dprism.verbose=true).
In my Windows VM Prism reports that it's using the D3D pipeline. And Prism isn't throwing any errors or issuing any warnings, it just doesn't draw except for the ColorCube toy.
@beldenfox JDK-8095004 talks about default opacity of CALayer being NO. But current documentation mentions that by default CALayer is opaque. And there are no pointers whether Apple has changed this behaviour in-between. Also as you mentioned there is update in shader also to ignore alpha channel when its value is 0.0 in JDK-8095058. So there are lot of factors at different levels which will make PickTest3D to behave differently now compared to pre 2015.
So on current code i have verified that PickTest3D works properly with different combinations of alpha value in diffusecolor and opacity for the stage. Regarding whether we need to remove setOpaque/not : As part for glass refactoring in this PR its better to keep the default ES2 pipeline code as it is.
see a few warnings:
Description Resource Type Path Location
The method createStockShader(String) of type DummyResourceFactory should be tagged with @Override since it actually overrides a superinterface method DummyResourceFactory.java Java Problem /graphics/src/main/java/com/sun/prism/null3d line 132
The method getResourceFactory() of type MTLContext should be tagged with @Override since it actually overrides a superclass method MTLContext.java Java Problem /graphics/src/main/java/com/sun/prism/mtl line 127
The method getNativeHandle() of type MTLRTTexture should be tagged with @Override since it actually overrides a superclass method MTLRTTexture.java Java Problem /graphics/src/main/java/com/sun/prism/mtl line 110
The method createRTTexture(int, int, Texture.WrapMode, boolean) of type MTLResourceFactory should be tagged with @Override since it actually overrides a superinterface method MTLResourceFactory.java Java Problem /graphics/src/main/java/com/sun/prism/mtl line 342
The method getAccelType() of type MTLShaderSource should be tagged with @Override since it actually overrides a superinterface method MTLShaderSource.java Java Problem /graphics/src/main/java/com/sun/scenario/effect/impl/hw/mtl line 41
The method loadSource(String) of type MTLShaderSource should be tagged with @Override since it actually overrides a superinterface method MTLShaderSource.java Java Problem /graphics/src/main/java/com/sun/scenario/effect/impl/hw/mtl line 34
Hmm, I must be doing something wrong. Launching the monkey tester with the following args:
-Dprism.order=mtl
-Dprism.verbose=true
and getting this exception (macOS 15.5 on M1 silicon):
Prism pipeline init order: mtl
Using Double Precision Marlin Rasterizer
Using dirty region optimizations
Not using texture mask for primitives
Not forcing power of 2 sizes for textures
Using hardware CLAMP_TO_ZERO mode
Opting in for HiDPI pixel scaling
Not forcing UploadingPainter
Prism pipeline name = com.sun.prism.mtl.MTLPipeline
Loading native metal library, named: prism_mtl
Succeeded: Loading native metal library.
(X) Got class = class com.sun.prism.mtl.MTLPipeline
Initialized prism pipeline: com.sun.prism.mtl.MTLPipeline
RenderJob.run: internal exception
java.lang.ExceptionInInitializerError
at javafx.graphics/com.sun.prism.mtl.MTLResourceFactory.<init>(MTLResourceFactory.java:60)
at javafx.graphics/com.sun.prism.mtl.MTLPipeline.getResourceFactory(MTLPipeline.java:87)
at javafx.graphics/com.sun.prism.mtl.MTLPipeline.getDefaultResourceFactory(MTLPipeline.java:78)
at javafx.graphics/com.sun.prism.GraphicsPipeline.getDefaultResourceFactory(GraphicsPipeline.java:155)
at javafx.graphics/com.sun.javafx.tk.quantum.QuantumRenderer.lambda$1(QuantumRenderer.java:155)
at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:545)
at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:369)
at javafx.graphics/com.sun.javafx.tk.RenderJob.run(RenderJob.java:58)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1095)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:619)
at javafx.graphics/com.sun.javafx.tk.quantum.QuantumRenderer$PipelineRunnable.run(QuantumRenderer.java:122)
at java.base/java.lang.Thread.run(Thread.java:1447)
Caused by: java.lang.RuntimeException: java.io.IOException: Stream closed
at javafx.graphics/com.sun.prism.mtl.MTLContext.<clinit>(MTLContext.java:109)
... 12 more
Caused by: java.io.IOException: Stream closed
at java.base/java.io.BufferedInputStream.getInIfOpen(BufferedInputStream.java:170)
at java.base/java.io.BufferedInputStream.read1(BufferedInputStream.java:328)
at java.base/java.io.BufferedInputStream.read(BufferedInputStream.java:388)
at java.base/java.io.InputStream.readNBytes(InputStream.java:411)
at java.base/java.io.InputStream.readAllBytes(InputStream.java:348)
at javafx.graphics/com.sun.prism.mtl.MTLContext.<clinit>(MTLContext.java:104)
... 12 more
The method createStockShader(String) of type DummyResourceFactory should be tagged with
@Overridesince it actually overrides a superinterface method DummyResourceFactory.java Java Problem /graphics/src/main/java/com/sun/prism/null3d line 132
Thank you, added @Override annotation to mentioned methods.
Still getting the exception loading "msl/jfxshaders.metallib". I am running in eclipse, so perhaps the eclipse project files need to include some added paths? I've got a few of them in the workspace:
Is mtl a good name for the prism.order property? Should it be metal?
Is
mtla good name for theprism.orderproperty? Should it bemetal?
"mtl" is sometimes used a short for "material", which has overlapping context here. I'd prefer the explicit "metal".
Is
mtla good name for theprism.orderproperty? Should it bemetal?"mtl" is sometimes used a short for "material", which has overlapping context here. I'd prefer the explicit "metal".
mtl was chose because
- It aligns with our existing abbreviated names for pipelines like d3d, es2, sw
- The metal library also uses 'MTL' as prefix all metal library types like MTLDevice, MTLCommandQueue, MTLCommandBuffer and more..
I think It would feel relevant in the context of -Dprism.order
Still getting the exception loading "msl/jfxshaders.metallib". I am running in eclipse, so perhaps the eclipse project files need to include some added paths? I've got a few of them in the workspace:
It should use the file from build directory:
build/modular-sdk/modules/javafx.graphics/com/sun/prism/mtl/msl/jfxshaders.metallib