RFR: 8271024: Implement macOS Metal Rendering Pipeline [v17]
Ambarish Rapte
arapte at openjdk.org
Tue Aug 5 14:18:05 UTC 2025
On Mon, 4 Aug 2025 22:26:29 GMT, Nir Lisker <nlisker at openjdk.org> wrote:
>> Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision:
>>
>> kcr-andy: review comments
>
> modules/javafx.graphics/src/jslc/java/com/sun/scenario/effect/compiler/backend/hw/MSLBackend.java line 170:
>
>> 168: "ddx", "dfdx",
>> 169: "ddy", "dfdy",
>> 170: "intcast", "int");
>
> Not that it matters probably, but why are these immutable maps and not switches? If we know all the strings at compile time and the structure is used only for key-based retrievals, we're not gaining anything with a map.
We have maps for similar classes GLSLBackend and HLSLBackend. It seems good maintain the similarity.
> 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: Map<String, Long> devDetails = MTLPipeline.getInstance().getDeviceDetails();
>
> `getDeviceDetails()` returns a raw `Map`, but here it's cast to `Map<String, Long>`. Is it safe? We probably want to fix it in `GraphicsPipelin`.
The Map is created by the `MTLPipeline` class itself and passed to super class `GraphicsPipeline`. As we know the exact type, it is safe.
> modules/javafx.graphics/src/main/java/com/sun/prism/mtl/MTLTexture.java line 149:
>
>> 147: for (int colIndex = 0; colIndex < rowStride; colIndex += 3) {
>> 148: index = rowIndex + colIndex;
>> 149: arr32Bit[dstIndex++] = arr[index + 2];
>
> `arr` might be `null` here if `buf.hasArray() == false`, resulting in an NPE.
We use non-direct buffers, so it is very unlikely case. But adding a recovery path seems right thing for now.
Eventually we should evaluate if the buffer is always non-direct then hasArray check can be removed from all pipelines. I shall file a followup bug for this.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2254006543
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2254015712
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2254133522
More information about the openjfx-dev
mailing list