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/bc5879c6...1a9a0a41 I did a sanity review of some of the Java section as I don't have a Mac to test on. 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? 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. 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. 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? 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. modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 216: > 214: // For every user defined function, pass reference to 4 > samplers and > 215: // reference to the uniforms struct. > 216: if > (!CoreSymbols.getFunctions().contains(getFuncName(e.getFunction().getName())) > && Eclipse kindly warns that `CoreSymbols.getFunctions()` is a collection of `Function`, but it's searched of a `String`. Perhaps `.getName()` shouldn't be called here? modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 238: > 236: if (first) { > 237: // Add 4 sampler variables and "device <Uniforms>& > uniforms" as the parameter to all user defined functions. > 238: if > (!CoreSymbols.getFunctions().contains(getFuncName(d.getFunction().getName())) > && Same "unlikely argument". 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() 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. 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? 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. modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 529: > 527: } else { > 528: objCHeaderFileName = DECORA_SHADER_HEADER_FILE_NAME; > 529: } If you prefer, this can be `objCHeaderFileName = isPrismShader ? PRISM_SHADER_HEADER_FILE_NAME : DECORA_SHADER_HEADER_FILE_NAME;` modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/model/CoreSymbols.java line 57: > 55: > 56: public static Set<Function> getFunctions() { > 57: return Collections.unmodifiableSet(getAllFunctions()); Could be `Set::copyOf` if you prefer. 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. 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. 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>` 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. 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());` 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. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 302: > 300: default: > 301: throw new InternalError("Unrecognized composite mode: " > + mode); > 302: } This can be an expression switch: int mtlCompMode = switch (mode) { case CLEAR -> MTL_COMPMODE_CLEAR; case SRC -> MTL_COMPMODE_SRC; case SRC_OVER -> MTL_COMPMODE_SRCOVER; case DST_OUT -> MTL_COMPMODE_DSTOUT; case ADD -> MTL_COMPMODE_ADD; }; No need for a default branch with enum. You want an error when a new constant is added. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 315: > 313: // implement or change in future if necessary > 314: long dstNativeHandle = dstRTT == null ? 0L : > ((MTLTexture)dstRTT).getNativeHandle(); > 315: long srcNativeHandle = ((MTLTexture)srcRTT).getNativeHandle(); Raw types -> `MTLTexture<?>` modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line 407: > 405: } else { > 406: throw new IllegalArgumentException("illegal value for > CullMode: " + cullMode); > 407: } Can be an expression switch. 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? 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 other classes in this package, like `MTLContext`. I suggest minimizing visibility to avoid leaking the class outside of its scope and also for better readability context. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLGraphics.java line 45: > 43: return null; > 44: } > 45: return new MTLGraphics(context, target); Can be a ternary if you prefer. 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? 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. 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()`? modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLMeshView.java line 113: > 111: } > 112: > 113: static class MTLMeshViewDisposerRecord implements Disposer.Record { Can be private. Same in other places. 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. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPhongMaterial.java line 43: > 41: private final MTLContext context; > 42: private final long nativeHandle; > 43: private TextureMap maps[] = new TextureMap[MAX_MAP_TYPE]; Can be final. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPhongMaterial.java line 81: > 79: Texture texture = (image == null) ? null > 80: : context.getResourceFactory().getCachedTexture(image, > Texture.WrapMode.REPEAT, useMipmap); > 81: long hTexture = (texture != null) ? ((MTLTexture) > texture).getNativeHandle() : 0; Raw type modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPipeline.java line 65: > 63: @Override > 64: public boolean init() { > 65: Map devDetails = new HashMap(); Raw types (though the problem is upstream and really should be fixed). modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPipeline.java line 93: > 91: Map devDetails = MTLPipeline.getInstance().getDeviceDetails(); > 92: devDetails.put("mtlCommandQueue", > 93: > mtlResourceFactory.getContext().getMetalCommandQueue()); Raw type again, causing a warning because the compiler doesn't know if `put` is given valid types. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPipeline.java line 129: > 127: default: > 128: return false; > 129: } Can be a switch expression. Also no need for the comment because the constant has docs 💯 return switch (type) { case MSL -> true; default -> false; }; 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? 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 modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRTTexture.java line 53: > 51: > 52: private boolean opaque; > 53: private boolean MSAA; Can be final except for `opaque`. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRTTexture.java line 89: > 87: wrapMode, msaa); > 88: MTLTextureData textData = new MTLRTTextureData(context, nPtr, > size); > 89: MTLTextureResource resource = new MTLTextureResource(textData, > true); Raw type modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRTTexture.java line 101: > 99: > 100: MTLTextureData textData = new MTLFBOTextureData(context, nPtr, > size); > 101: MTLTextureResource resource = new MTLTextureResource(textData, > false); Raw type modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRTTexture.java line 133: > 131: // In future, if needed, need to implement pix as ByteBuffer > 132: if (pix instanceof IntBuffer) { > 133: nReadPixelsFromRTT(nTexPtr, (IntBuffer)pix); Can pattern match instead of cast. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRTTextureData.java line 29: > 27: > 28: public class MTLRTTextureData extends MTLTextureData { > 29: MTLRTTextureData(MTLContext context, long texPtr, long size) { New line, also in other places? 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? 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 modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 57: > 55: > 56: public class MTLResourceFactory extends BaseShaderFactory { > 57: private final MTLContext context; New line? modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 103: > 101: return createShader(pixelShaderName, samplers, params, > maxTexCoordIndex, > 102: isPixcoordUsed, isPerVertexColorUsed); > 103: } catch (Exception e) { `e` is unused and can be `_` if this is correct (e.g., no `e.printStackTrace()`). Same in other places. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 114: > 112: Shader shader = MTLShader.createShader(getContext(), shaderName, > samplers, > 113: params, maxTexCoordIndex, isPixcoordUsed, > isPerVertexColorUsed); > 114: return shader; Can return the method result directly if you prefer. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 121: > 119: if (shaderName == null) { > 120: throw new IllegalArgumentException("Shader name must be > non-null"); > 121: } Can be `Objects.requireNonNull(shaderName, "Shader name must be non-null")` if an NPE acceptable instead of an IAE (I think it's better even). modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 126: > 124: System.err.println("MTLResourceFactory: Prism - > createStockShader: " + shaderName); > 125: } > 126: Class klass = Class.forName("com.sun.prism.shader." + > shaderName + "_Loader"); Raw type modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 137: > 135: > 136: @Override > 137: public TextureResourcePool getTextureResourcePool() { `TextureResourcePool` is a raw type. 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; 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? modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 194: > 192: > 193: MTLTextureData textData = new MTLTextureData(context, pResource, > size); > 194: MTLTextureResource resource = new MTLTextureResource(textData, > true); Raw type modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 281: > 279: @Override > 280: public boolean isFormatSupported(PixelFormat format) { > 281: switch (format) { I suggest an exhaustive no-default switch for enums. return switch (format) { case BYTE_RGB, BYTE_GRAY, BYTE_ALPHA, BYTE_BGRA_PRE, INT_ARGB_PRE, FLOAT_XYZW, BYTE_APPLE_422 -> true; case MULTI_YCbCr_420 -> false; }; You can keep the 1 constant per line formatting if you prefer. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 348: > 346: int createh = height; > 347: int cx = 0; > 348: int cy = 0; Unused variables modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java line 353: > 351: createw = nextPowerOfTwo(createw, Integer.MAX_VALUE); > 352: createh = nextPowerOfTwo(createh, Integer.MAX_VALUE); > 353: } Can be folded into a ternary in the initial assignment if you prefer. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 46: > 44: private final Map<Integer, WeakReference<Object>> textureIdRefMap = > new HashMap<>(); > 45: > 46: private static Map<String, MTLShader> shaderMap = new HashMap<>(); Can be final. 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. 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. 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`. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 85: > 83: for (Map.Entry<String, Integer> entry : samplers.entrySet()) { > 84: this.samplers.put(entry.getValue(), entry.getKey()); > 85: } Can be `samplers.forEach((key, val) -> this.samplers.put(val, key));` with better argument names. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 109: > 107: } else { > 108: return false; > 109: } `return nMetalShaderRef != 0`? modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 117: > 115: > 116: currentEnabledShader.textureIdRefMap.put(texUnit, new > WeakReference(tex)); > 117: MTLTexture mtlTex = (MTLTexture)tex; Raw types modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 197: > 195: > 196: private static native long nCreateMetalShader(long context, String > fragFuncName); > 197: private static native Map nGetUniformNameIdMap(long nMetalShader); Raw type modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line 30: > 28: import com.sun.glass.ui.Screen; > 29: import com.sun.javafx.geom.Rectangle; > 30: import com.sun.prism.CompositeMode; Unused modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line 39: > 37: public class MTLSwapChain implements MTLRenderTarget, Presentable, > GraphicsResource { > 38: > 39: private PresentableState pState; final modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line 59: > 57: @Override > 58: public boolean lockResources(PresentableState state) { > 59: No need for empty line. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line 87: > 85: MTLContext context = getContext(); > 86: context.flushVertexBuffer(); > 87: MTLGraphics g = (MTLGraphics) MTLGraphics.create(context, > stableBackbuffer); Unnecessary cast modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line 126: > 124: @Override > 125: public Graphics createGraphics() { > 126: Empty line modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line 148: > 146: long pTex = pState.getNativeFrameBuffer(); > 147: > 148: stableBackbuffer = > (MTLRTTexture)MTLRTTexture.create(getContext(), pTex, w, h, 0); Unnecessary cast modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line 166: > 164: > 165: @Override > 166: public void setOpaque(boolean opaque) { Should we warn somehow about this no-op? It could be confusing for the caller for this to "fail" silently. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line 41: > 39: > 40: private final MTLContext context; > 41: private long texPtr; final modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line 79: > 77: > 78: // We don't handle mipmap in shared texture yet. > 79: private MTLTexture(MTLTexture sharedTex, WrapMode newMode) { Raw type modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line 87: > 85: @Override > 86: protected Texture createSharedTexture(WrapMode newMode) { > 87: return new MTLTexture(this, newMode); Raw type modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line 97: > 95: int srcscan, boolean skipFlush) { > 96: > 97: switch (format.getDataType()) { I would use an exhaustive switch and possibly extract each branch into a method. This is a big switch with sub-switches, it's easy to get lost. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTextureData.java line 36: > 34: private long size; > 35: > 36: private MTLTextureData() { Unused constructor. Prevents the fields from being final. Doesn't look like it's needed. modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTextureResource.java line 32: > 30: public class MTLTextureResource<T extends MTLTextureData> extends > DisposerManagedResource<T> { > 31: > 32: boolean canDispose; private final modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLVramPool.java line 36: > 34: implements TextureResourcePool<MTLTextureData> { > 35: > 36: private static MTLVramPool theInstance = new MTLVramPool(); final modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLVramPool.java line 49: > 47: public long estimateTextureSize(int width, int height, PixelFormat > format) { > 48: return ((long) width) * ((long) height) * > 49: ((long) format.getBytesPerPixelUnit()); Unnecessary cast 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? ------------- PR Review: https://git.openjdk.org/jfx/pull/1824#pullrequestreview-3055919768 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231425136 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231643473 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231434571 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231476027 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231447687 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231481882 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231487233 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231509449 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231537374 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231566616 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231582333 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231544130 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231633067 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231635900 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231692608 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231711272 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231709236 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231731280 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231733285 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231702027 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231713202 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231716612 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231738770 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231749898 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231746162 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231757335 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231757753 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231761693 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231761972 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231763737 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231764851 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231766544 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231777814 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231779325 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231781861 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231790290 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231834264 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231836448 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231837112 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231837549 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231838620 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231840770 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231793697 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231795530 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231795928 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231802438 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231804350 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231807526 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231808072 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231809412 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231813640 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231816624 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231820659 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231824222 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231827607 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231829034 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231848617 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231846379 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231847525 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231855592 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231861918 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231863160 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231867919 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231844004 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231886182 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231887184 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231889067 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231891135 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231891964 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231893319 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231896842 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231899223 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231900351 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231900934 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231919533 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231904681 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231907827 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231911150 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231912646 PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2231913076
