-
Notifications
You must be signed in to change notification settings - Fork 521
8271024: Implement macOS Metal Rendering Pipeline #1824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back arapte! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
@arapte |
@arapte |
@arapte |
/skara contributor add @arapte |
@arapte |
Webrevs
|
/reviewers 3 |
@kevinrushforth |
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:
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metal is here! Yes!
It took me a while to sort out how the GlassView and GlassLayer classes work in this PR. It’s not clear based on the naming that the classes which handle drawing (GlassViewCGL3D and GlassViewMTL3D ) are not subclasses of the class that handles events (GlassView3D). The naming conventions could be clearer or at the very least there should be some comments in the code.
I think some of this structure is being dictated by the way GlassViewCGL3D is derived from NSOpenGLView since that prevents us from making it a subclass of, say, GlassView3D. But I’m almost certain the NSOpenGLView reference is just cruft. All of the OpenGL processing happens in a CAOpenGLLayer which has nothing to do with the (unused) NSOpenGLView machinery. In the current master and this PR I can derive from NSView instead of NSOpenGLView and everything works fine.
There is some code in Prism (MacOSXWindowSystemInterface.m) that can associate an NSOpenGLContext with an NSView but I don’t think a view is ever passed in. It would be nice to clean that code up since the view-related calls are deprecated and generating compiler warnings.
I think the NSOpenGLView reference should be removed. Beyond that you could keep the current class structure and view layout since there’s something to be said for separating drawing and event processing into different NSViews.
I would rather not see the 3D terminology carried over into the new classes. It's there to make a distinction that's no longer relevant (according to the comments there used to be a GlassView2D). But that is a personal pet peeve so feel free to ignore it.
Look like it’s not possible to add comments to unchanged code in GitHub (?!) so I guess I have to write the following issues up here.
In GlassWindow.m line 807 there’s this bit of mysterious code:
CALayer *layer = [window->view layer];
if (([layer isKindOfClass:[CAOpenGLLayer class]] == YES) &&
(([window->nsWindow styleMask] & NSWindowStyleMaskTexturedBackground) == NO))
{
[((CAOpenGLLayer*)layer) setOpaque:[window->nsWindow isOpaque]];
}
It was put there to resolve JDK-8095004 and JDK-8095040. In this PR the layer is nil at this point so this code isn’t doing anything. I haven’t verified whether this is causing a regression. In the master branch that setOpaque: call will always happen since the NSWindowStyleMaskTexturedBackground bit is never set (it’s associated with the obsolete UNIFIED stage style).
Over in GlassView.m there’s a JNI method called _getNativeLayer that retrieves a CALayer. There’s no need to update this logic since the Java side is never called (it’s in MacView.java). This should all just be removed.
@beldenfox Thanks for your inputs.
|
@jayathirthrao Thanks for the thorough response. And, again, it's great to see Metal happening.
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.
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. |
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.
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. |
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?
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:
|
Hmm, I must be doing something wrong. Launching the monkey tester with the following args:
and getting this exception (macOS 15.5 on M1 silicon):
|
Thank you, added |
Is |
"mtl" is sometimes used a short for "material", which has overlapping context here. I'd prefer the explicit "metal". |
mtl was chose because
I think It would feel relevant in the context of |
Should the new library be copied to I've added the directory you mentioned to the
Passing
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
Reviewed the Metal-native-side of this commit (native-prism-mtl
folder) and left some comments to talk through, nothing extremely major though. I'll verify the code on my side once we get closer to review completion.
modules/javafx.graphics/src/main/native-prism-mtl/MetalContext.m
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-mtl/MetalTexture.m
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-mtl/MetalTexture.m
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-mtl/MetalTexture.m
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-mtl/MetalTexture.m
Outdated
Show resolved
Hide resolved
Did a bit more digging in Eclipse:
Even after I add the dir with the resource to the
The reason is that eclipse is looking for the resources for the graphics module in |
Got it running in Eclipse (thank you @kevinrushforth !), please pick up changes from this branch: will test and report. |
Went through all the controls in the monkey tester (inc. animated), saw no problems or graphic artifacts. Encountered a weird issue with the WebView - not sure if related. Had to go off VPN to load https://yahoo.com, then logged back into VPN and the application froze completely with the beach ball icon. Can drag the window to another monitor (looks fuzzy bc resolution), but otherwise locked up. I don't know when it locked up exactly, but you may want to investigate (I don't think network going up or down should have any impact on the application responsiveness). Here is where it is stuck in the FX application thread: stdout/stderr before the app locked up (with the
|
Thank you @andy-goryachev-oracle @kevinrushforth , I have included the change from your branch, tested that regular build and lunching apps from terminal works same as before. |
I am unable to reproduce this issue, neither with stand alone HelloWebView nor with monkey tester. WebView does require the proxy to be set when using VPN. Following command would load the when using VPN.
Metal change should not cause such issue, this seems WebView specific, I shall try to reproduce and may be file a WebView issue. |
you might want to sync up with the latest master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first batch of comments (java), mostly questions and minor suggestions. will review the native code next.
...es/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
Outdated
Show resolved
Hide resolved
...es/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
Outdated
Show resolved
Hide resolved
...es/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
Outdated
Show resolved
Hide resolved
...es/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
Outdated
Show resolved
Hide resolved
...es/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/hw/mtl/MTLShaderSource.java
Outdated
Show resolved
Hide resolved
I agree. |
modules/javafx.graphics/src/main/native-glass/mac/GlassCGLOffscreen.m
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-glass/mac/GlassMTLOffscreen.m
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-mtl/MetalPipelineManager.m
Show resolved
Hide resolved
Agreed, although you might wait until after the jfx25 fork at this point. |
Thanks for the review Andy... |
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
Build changes - There are new tasks added to build.gradle for
Prism - Prism is the rendering engine of JavaFX.
Glass - Glass is the windowing toolkit of JavaFX
Testing
Note to reviewers :
-Dprism.order=mtl
option is a must for testing the Metal pipeline/skara contributor add @kevinrushforth
/skara contributor add @aghaisas
/skara contributor add @jayathirthrao
Progress
Issue
Contributors
<[email protected]>
<[email protected]>
<[email protected]>
<[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
Link to Webrev Comment