On Mon, 4 Aug 2025 22:26:29 GMT, Nir Lisker <nlis...@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
