RFR: 8271024: Implement macOS Metal Rendering Pipeline [v11]
Ambarish Rapte
arapte at openjdk.org
Wed Jul 30 10:04:32 UTC 2025
On Fri, 25 Jul 2025 15:27:47 GMT, Nir Lisker <nlisker at openjdk.org> wrote:
>> 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/df483ca3...1a9a0a41
>
> build.gradle line 2705:
>
>> 2703: exec {
>> 2704: commandLine("${metalCompiler}")
>> 2705: args += [ "-std=macos-metal2.4" ]
>
> Should this version number be moved to the properties file with the other versions? Is it expected to be updated? Metal 4 was released recently, and Metal 3 was released a few years ago. Are we not going to end up with a version that's going to be "old" by the time it's released?
Yes, moving to build.properties seems good idea. done.
macos-metal2.4 is required to support macos 12, so changing this version should be done only in unavoidable circumstances or may be when support for macos 12 is over.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2021, 2025, Oracle and/or its affiliates. All rights reserved.
>
> Is the copyright of 2021 needed if the file was introduced in 2025? Other files are also like that, but not all.
Yes, 2021 is needed. As the files were available in public sandbox repo since then.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 52:
>
>> 50: import static java.util.Map.entry;
>> 51:
>> 52: public class MSLBackend extends SLBackend {
>
> Since this is a new class, can we have a bit of class docs to explain what it is and what it relates to? Same for the other classes where you deem it not trivial.
Yes, there are several internal classes that are missing doc. added a line for this class, but shall leave other internal classes as is, we would need a separate task to update such classes across all pipelines.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 64:
>
>> 62:
>> 63: private String shaderFunctionName;
>> 64: private String textureSamplerName;
>
> This variable is assigned but never read (unused). Is it needed for anything?
Removed it. must be residue from a trial.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 182:
>
>> 180: protected String getQualifier(Qualifier q) {
>> 181: String qual = QUAL_MAP.get(q.toString());
>> 182: return (qual != null) ? qual : q.toString();
>
> All these "get or else" methods can use `Map::getOrDefault`:
>
> return QUAL_MAP.getOrDefault(q.toString(), q.toString());
>
> And if `toString` is expensive, it can be stored in a local variable first.
Changed to :
@Override
protected String getQualifier(Qualifier q) {
String qualifier = q.toString();
return QUAL_MAP.getOrDefault(qualifier, qualifier);
}
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 256:
>
>> 254: Variable var = d.getVariable();
>> 255: Qualifier qual = var.getQualifier();
>> 256: if (qual == Qualifier.CONST) { // example. const int i = 10;
>
> Using a switch expression here on the enum will be cleaner. First, the `else` at the end can't be reached unless `qual == null`. If it's at all possible for it to be `null`, it should be explicitly expressed (via `case null -> ...`). Second, if other enum constants are added later (is it possible even?), we *should* get a compiler error and not go to the `else` blindly.
>
> I would also extract the content of each branch to its own private method, especially when there're a lot of comments that could go on it:
>
> case null -> visitVarDecl(d) // if can be null
> case CONST -> handleConst()
> case PARAM -> handleParam()
Yes, null is possible for regular file local, function local variables.
new methods seem for case block seems overkill. This class is used at build time.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 317:
>
>> 315: String shaderType = isPrismShader ? "PRISM" : "DECORA";
>> 316:
>> 317: if (fragmentShaderHeader == null) {
>
> Looks like `fragmentShaderHeader` is only used once and then retained in memory just to check later on if it was initialized and written. Perhaps a boolean flag would make it clearer? In that case the builder is used and discarded with the appropriately-named flag conveying a meaning better than "string builder == null", such as `wasHeaderWritten`.
>
> I would also extract the content of the `if` to its own method, but it's just my style to break up long methods.
Yes, fragmentShaderHeader is used only once. I extracted the if block to another method `writeFragmentShaderHeader()` with using multiline string.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 359:
>
>> 357:
>> 358: if (objCHeader.length() == 0) {
>> 359: objCHeader.append("#ifndef " + shaderType + "_SHADER_COMMON_H\n" +
>
> Is `objHeader` supposed to have this content initialized only once? Doesn't `shaderType` change in later calls?
> It's also never cleared, is it supposed to retain all its content?
shaderType changes in following calls.
The objHeader is initialized once with the content above, and then it gets appended for every shader and written to the file after parsing every shader. It is supposed to retain all the content till all decora or prism shaders are parsed.
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 511:
>
>> 509: texSamplerMap = new HashMap<>();
>> 510: helperFunctions = new ArrayList<>();
>> 511: uniformNames = new ArrayList<>();
>
> You should be able to initialize these at the declaration site and make them final. Here you can call `clear` to reset the content. Perhaps it's better than recreating the data structure?
>
> Alternatively, you can encapsulate all the variables and methods that are used per-shader in a single inner class and create a new instance per shader for the duration of handling that shader. This approach provides better separation and encapsulation and tends to be less error prone.
Changed the variable to initialize with declaration and use clear().
> modules/javafx.graphics/src/main/java/com/sun/prism/GraphicsPipeline.java line 52:
>
>> 50: */
>> 51: GLSL,
>> 52: /**
>
> An empty line between the constants can help with readability.
We tried to keep minimal changes to common files. The enum did not use new line before, so would like to avoid it.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 50:
>
>> 48: import java.nio.ByteBuffer;
>> 49:
>> 50: public class MTLContext extends BaseShaderContext {
>
> Looks like all the fields here can be private. I don't see them used outside of this class.
We have similar fields declared as public in other internal classes. Even D3DContext has similar properties declared as public. It would be good to keep these similar. We can make such changes as a cleanup on a wider level.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 97:
>
>> 95: static {
>> 96: final String shaderLibName = "msl/jfxshaders.metallib";
>> 97: final Class clazz = MTLContext.class;
>
> Raw type -> `Class<MTLContext>`
Changed.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 232:
>
>> 230: }
>> 231: MTLShader.setTexture(texUnit, tex, linear, wrapMode);
>> 232: }
>
> Maybe this is cleaner, and the removal of the default branch is safer.
>
> if (tex == null) return;
> boolean linear = tex.getLinearFiltering();
> int wrapMode = switch (tex.getWrapMode()) {
> case CLAMP_NOT_NEEDED -> MTL_SAMPLER_ADDR_MODE_NOP;
> case CLAMP_TO_EDGE, CLAMP_TO_EDGE_SIMULATED, CLAMP_TO_ZERO_SIMULATED -> MTL_SAMPLER_ADDR_MODE_CLAMP_TO_EDGE;
> case CLAMP_TO_ZERO -> MTL_SAMPLER_ADDR_MODE_CLAMP_TO_ZERO;
> case REPEAT, REPEAT_SIMULATED -> MTL_SAMPLER_ADDR_MODE_REPEAT;
> };
> MTLShader.setTexture(texUnit, tex, linear, wrapMode);
>
> Can the texture legally be `null` and this method still called with it? Looks odd to call `updateTexture` with a `null` texture.
In case of es2, d3d, calling with null texture is valid: Calling updateTexture with null texture and with a valid texUnit means to reset that texUnit texture. However it is not required to in case of metal hence else block is no-op
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 247:
>
>> 245: } else {
>> 246: scratchTx = scratchTx.mul(xform).mul(perspectiveTransform);
>> 247: }
>
> Looks like `mul` already does the short-circuiting identity check, so no need for the if-else:
> `scratchTx.set(projViewTx).mul(xform).mul(getPerspectiveTransformNoClone());`
The current code seems easy to read, and offers understanding what happens if identity. If changed it would get lost
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 258:
>
>> 256: protected void updateWorldTransform(BaseTransform xform) {
>> 257: worldTx.setIdentity();
>> 258: if ((xform != null) && (!xform.isIdentity())) {
>
> Redundant identity check.
The function is exactly similar to that of ES2Context. It would be good to keep code similar for ease of debugging in case of issues.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 485:
>
>> 483:
>> 484: private void updateRawMatrix(GeneralTransform3D src) {
>> 485: rawMatrix[0] = (float)src.get(0); // Scale X
>
> Is there a meaningful loss of precision here?
In all pipelines, the native side requires a float value. So, this is just more of the same and safe.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLGraphics.java line 32:
>
>> 30: import com.sun.prism.paint.Color;
>> 31:
>> 32: public class MTLGraphics extends BaseShaderGraphics {
>
> Doesn't look like the class needs to be public. Same for all other classes in this package. I suggest minimizing visibility to avoid leaking the class outside of its scope and also for better readability context.
Changed this and all other applicable classes.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLMesh.java line 32:
>
>> 30:
>> 31: class MTLMesh extends BaseMesh {
>> 32: static int count = 0;
>
> Do we use a new line after a class declaration?
changed.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLMesh.java line 83:
>
>> 81: }
>> 82:
>> 83: static class MTLMeshDisposerRecord implements Disposer.Record {
>
> Can be private.
changed this and other internal classes.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLMeshView.java line 40:
>
>> 38: private final long nativeHandle;
>> 39:
>> 40: final private MTLMesh mesh;
>
> `mesh` is unused. Is it awaiting a dereference in `dispose()`?
Yes, MTLMesh can be shared by multiple MTLMeshView.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPhongMaterial.java line 37:
>
>> 35: import com.sun.prism.impl.Disposer;
>> 36:
>> 37: class MTLPhongMaterial extends BasePhongMaterial implements PhongMaterial {
>
> The superclass already implements the interface, so it's redundant here.
yes, corrected the signature
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPipeline.java line 139:
>
>> 137: default:
>> 138: return false;
>> 139: }
>
> Can be like above.
>
> Also, can Metal really support SM3? Isn't it a D3D-only model?
Yes, SM3 is specific to D3D, but it is used by ES2 as well.
There is no SM3 like concept for es2 or mtl. So same SM3 is reused.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRTTexture.java line 39:
>
>> 37: import com.sun.prism.mtl.MTLTexture;
>> 38: import com.sun.prism.mtl.MTLTextureData;
>> 39: import com.sun.prism.mtl.MTLTextureResource;
>
> Unused
MTLTextureResource is used in this class
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRenderTarget.java line 1:
>
>> 1: /*
>
> Why is this interface needed if it's implemented only in 1 class and the return value from the method is always 0?
The interface is used in D3D and ES2 but not in case of Metal. Would like to keep it for symmetry.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 44:
>
>> 42: import com.sun.prism.impl.TextureResourcePool;
>> 43: import com.sun.prism.impl.ps.BaseShaderFactory;
>> 44: import com.sun.prism.mtl.MTLContext;
>
> Unused import
yes, un-required.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 170:
>
>> 168: allocw = w;
>> 169: alloch = h;
>> 170: }
>
> If you prefer:
>
> int allocw = PrismSettings.forcePow2 ? nextPowerOfTwo(w, Integer.MAX_VALUE) : w;
> int alloch = PrismSettings.forcePow2 ? nextPowerOfTwo(h, Integer.MAX_VALUE) : h;
if-else seems good for multiple statements.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 176:
>
>> 174: int bpp = formatHint.getBytesPerPixelUnit();
>> 175: if (allocw >= (Integer.MAX_VALUE / alloch / bpp)) {
>> 176: throw new RuntimeException("Illegal texture dimensions (" + allocw + "x" + alloch + ")");
>
> Is a `RuntimeException` necessary here? Can we not recover?
Yes, the size is not supported and same exception is thrown from other pipelines.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 64:
>
>> 62: public static Shader createShader(MTLContext ctx, String fragFuncName, Map<String, Integer> samplers,
>> 63: Map<String, Integer> params, int maxTexCoordIndex,
>> 64: boolean isPixcoordUsed, boolean isPerVertexColorUsed) {
>
> All 4 are unused.
Yes, but we have similar method signature in D3D as well. Let's keep these for maintaining similarity.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 67:
>
>> 65: if (shaderMap.containsKey(fragFuncName)) {
>> 66: return shaderMap.get(fragFuncName);
>> 67: } else {
>
> `else` is not required because `if` returns. Up to you.
>
> Can also use `Map::getOrDefault` if you want to extract the shader creation into a method.
Shall keep if-else.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 79:
>
>> 77: } else {
>> 78: return new MTLShader(ctx, fragFuncName);
>> 79: }
>
> Can use `Map::getOrDefault`.
if else block seems easy on eyes.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLVramPool.java line 54:
>
>> 52: @Override
>> 53: public long estimateRTTextureSize(int width, int height, boolean hasDepth) {
>> 54: // REMIND: need to deal with size of depth buffer, etc.
>
> Is this comment supposed to go in?
Yes, It is carried from other pipeline.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2238918320
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2238963008
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2239028225
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2239034503
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2239521920
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2241975027
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2239576859
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2239836593
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2239852923
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2239860795
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2239961492
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240049769
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240067008
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240074327
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240091921
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240110474
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240267443
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240270064
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240296571
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240386496
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240402281
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240459275
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2240619553
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2242141227
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2241505629
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2241526419
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2241528735
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2241593787
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2241603494
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2241611456
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2241935257
More information about the openjfx-dev
mailing list