RFR: 8271024: Implement macOS Metal Rendering Pipeline [v11]
Nir Lisker
nlisker at openjdk.org
Fri Jul 25 20:10:21 UTC 2025
On Tue, 22 Jul 2025 17:00:25 GMT, Ambarish Rapte <arapte at 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
More information about the openjfx-dev
mailing list