On Tue, 15 Jul 2025 05:41:32 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 incrementally with one additional
> commit since the last revision:
>
> changes for running apps in eclipse
first batch of comments (java), mostly questions and minor suggestions. will
review the native code next.
modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
line 334:
> 332:
> 333: try {
> 334: FileWriter fragmentShaderHeaderFile = new
> FileWriter(headerFilesDir + FRAGMENT_SHADER_HEADER_FILE_NAME);
This writer is not buffered, meaning it will be too slow.
I'd say either to wrap it into `BufferedWriter`, or better yet - use
`StringBuilder`s everywhere and write the single generated string in one
operation.
modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
line 336:
> 334: FileWriter fragmentShaderHeaderFile = new
> FileWriter(headerFilesDir + FRAGMENT_SHADER_HEADER_FILE_NAME);
> 335:
> fragmentShaderHeaderFile.write(fragmentShaderHeader.toString());
> 336: fragmentShaderHeaderFile.write("#endif\n");
this line is weird - why is it here and not after L331?
modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
line 337:
> 335:
> fragmentShaderHeaderFile.write(fragmentShaderHeader.toString());
> 336: fragmentShaderHeaderFile.write("#endif\n");
> 337: fragmentShaderHeaderFile.close();
an exception on lines 335-336 will cause the writer not being closed.
can we use try-with-resources?
modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
line 346:
> 344:
> 345: try {
> 346: FileWriter objCHeaderFile = new FileWriter(headerFilesDir +
> objCHeaderFileName);
same issue here, need to close the writer properly.
modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
line 423:
> 421:
> 422: objCHeaderFile.write(" return nil;\n");
> 423: objCHeaderFile.write("};\n\n");
Not sure if it's worth doing, but perhaps this code can use a single
StringBuilder to build the code, then use `Files.writeString()` to write it
(this might require a bit of refactoring).
modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java
line 444:
> 442: }
> 443:
> 444: uniformsForShaderFile = uniformsForShaderFile.replace(" float2",
> " vector_float2");
feels like this code was written in 1997. Use a StringBuilder perhaps?
modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2SwapChain.java line
53:
> 51: // a value of zero corresponds to the windowing system-provided
> 52: // framebuffer object
> 53: long nativeDestHandle = 0;
`= 0` is not strictly necessary.
modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2SwapChain.java line
240:
> 238: @Override
> 239: public int getFboID() {
> 240: return (int)nativeDestHandle;
this results in loss of information. any possibility that it may backfire
somehow, by mapping two different `nativeDestHandle`s to the same `int`?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
159:
> 157: @Override
> 158: protected State updateRenderTarget(RenderTarget target, NGCamera
> camera, boolean depthTest) {
> 159: MTLLog.Debug("MTLContext.updateRenderTarget() :target = " +
> target + ", camera = " + camera + ", depthTest = " + depthTest);
general question: is this temporary? can we use maybe a better logging
mechanism, one that does not concatenate strings when disabled? works case,
surround this by `if(debug)` ?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
172:
> 170:
> 171: // Validate the camera before getting its computed data
> 172: if (camera instanceof NGDefaultCamera) {
suggestion: `if (camera instanceof NGDefaultCamera nc)` to avoid explicit cast
in L173
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
180:
> 178: double vw = camera.getViewWidth();
> 179: double vh = camera.getViewHeight();
> 180: if (targetWidth != vw || targetHeight != vh) {
would it make sense to detect the case when target(w/h) is very close to (w/h)
and skip scaling? or is this scenario unlikely?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
339:
> 337:
> 338: public long getMetalCommandQueue()
> 339: {
I like this style, but the rest of the file uses K&R braces...
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
350:
> 348: // if (checkDisposed()) return;
> 349: // nSetDeviceParametersFor2D(pContext);
> 350: }
remove the commented out code LL348-349, leave the comment?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
357:
> 355: // result of this call, hence the method is no-op.
> 356: // But overriding the method here for any future reference.
> 357: // if (checkDisposed()) return;
same here?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLContext.java line
489:
> 487: }
> 488:
> 489: private void printRawMatrix(String mesg) {
unused method?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLLog.java line 31:
> 29:
> 30: public class MTLLog {
> 31: public static void Debug(String str) {
Since `MTTLLog` is a home-grown facility:
perhaps we should consider allowing `Supplier<String>` lambdas (or
`Supplier<Object>`), or allow for `MessageFormat`'ted strings, to avoid
concatenations.
Lambdas may be faster, but they do require effectively final parameters.
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLMesh.java line 60:
> 58: @Override
> 59: public void dispose() {
> 60: disposerRecord.dispose();
two questions here:
1. why is `disposerRecord.dispose()`; here (and in `D3DMesh`, `ES2Mesh`) and
not in the `BaseGraphicResource` ?
2. any danger in the `dispose()` being called twice? (could it happen at all?)
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLMesh.java line 93:
> 91: }
> 92:
> 93: void traceDispose() {}
what's the purpose of this method?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPhongMaterial.java
line 104:
> 102: String logname = PhongMaterial.class.getName();
> 103: PlatformLogger.getLogger(logname).warning(
> 104: "Warning: Low on texture resources. Cannot
> create texture.");
I suppose this message is the same as elsewhere. Is it clear enough for the
user? Does the user have a clear idea of how to correct the issue?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLPipeline.java line
90:
> 88:
> 89: // This enables sharing of MTLCommandQueue between PRISM and
> GLASS
> 90: HashMap devDetails = (HashMap)
> MTLPipeline.getInstance().getDeviceDetails();
cast is unnecessary. suggestion:
`Map devDetails = MTLPipeline.getInstance().getDeviceDetails();`
I guess we've inherited the API based on the raw object here?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLRTTextureData.java
line 35:
> 33: @Override
> 34: public void dispose() {
> 35: if (pTexture != 0L) {
this code is weird: why doesn't call super.dispose()?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
line 75:
> 73: i *= 2;
> 74: }
> 75: return i;
interesting... would `1 << val` be simpler (barring overflow)?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
line 128:
> 126: return (Shader) m.invoke(null, new Object[]{this,
> shaderName, nameStream});
> 127: } catch (Throwable e) {
> 128: e.printStackTrace();
do we want to use a logger or the `stderr` is the best decision in this case?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
line 183:
> 181:
> 182: long pResource = nCreateTexture(context.getContextHandle() ,
> 183: (int) formatHint.ordinal(), (int) usageHint.ordinal(),
unnecessary casts to (int) x2
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
line 274:
> 272: Texture tex = createTexture(texFormat, Usage.DEFAULT,
> WrapMode.CLAMP_TO_EDGE, texWidth, texHeight);
> 273:
> 274: frame.releaseFrame();
could all these `frame.releaseFrame()` be moved to a `finally` block?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
line 305:
> 303: @Override
> 304: public int getRTTWidth(int w, Texture.WrapMode wrapMode) {
> 305: // Below debugging logic replicates D3DResoureFactory
is this code still needed? also L314
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLResourceFactory.java
line 396:
> 394: public void dispose() {
> 395: super.dispose();
> 396: context.dispose();
Q: should we call super.dispose() _after_ context.dispose()?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLShader.java line 57:
> 55: shaderMap.put(fragmentFunctionName, this);
> 56: } else {
> 57: throw new AssertionError("Failed to create Shader");
is `AssertionError` the right type here?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line
128:
> 126:
> 127: if (pState.getNativeFrameBuffer() == 0) {
> 128: System.err.println("Native backbuffer texture from Glass is
> nil.");
is `stderr` ok here?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLSwapChain.java line
143:
> 141: stableBackbuffer = null;
> 142: } /*else {
> 143: // RT-27554
is this still relevant?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line 97:
> 95: int srcscan, boolean skipFlush) {
> 96:
> 97: if (format.getDataType() == PixelFormat.DataType.INT) {
would a `switch` be better here?
modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line
122:
> 120: byte[] arr = buf.hasArray() ? buf.array() : null;
> 121:
> 122: if (format == PixelFormat.BYTE_BGRA_PRE || format ==
> PixelFormat.BYTE_ALPHA) {
also here, `switch` ?
modules/javafx.graphics/src/main/java/com/sun/scenario/effect/impl/hw/mtl/MTLShaderSource.java
line 36:
> 34: @Override
> 35: public InputStream loadSource(String name) {
> 36: // MSL shaders are compilend and linked into a MTLLibrary at
> build time.
-> compiled
-------------
PR Review: https://git.openjdk.org/jfx/pull/1824#pullrequestreview-3025473708
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210708247
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210709590
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210695663
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210697664
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210751940
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210757844
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210763281
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210767379
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210942089
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210945178
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210961660
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210974725
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210977355
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210977769
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2210983716
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211116140
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211133718
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211139193
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211147969
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211163123
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211170894
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211174999
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211177828
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211182961
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211193965
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211197823
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211205783
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211211057
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211305810
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211306463
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211310013
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211311714
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2211315024
