On Fri, 25 Jul 2025 15:27:47 GMT, Nir Lisker <nlis...@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
