On Tue, 15 Oct 2024 22:47:38 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review changes
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/IntBgr.java
> line 68:
>
>> 66: int b = (pixel >> 16) & 0xff;
>> 67: return (255 << 24) | (r << 16) | (g << 8) | b;
>> 68: }
>
> Not sure if we optimizing here, but could eliminate two shifts like this:
>
> Suggestion:
>
> public int getArgb(int[] arr, int offset) {
> int pixel = arr[offset];
> int r = pixel & 0xff;
> int g = pixel & 0xff00;
> int b = pixel & 0xff0000;
> return (255 << 24) | (r << 16) | g | (b >> 16);
> }
There are many such cases, and I'm not sure if it is really important to
optimize this.
> modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/IntRgb.java
> line 43:
>
>> 41: public static final IntPixelAccessor accessor = Accessor.instance;
>> 42:
>> 43: public static IntToIntPixelConverter ToIntArgbPreConverter() {
>
> This method is not following Java naming conventions:
>
> Suggestion:
>
> public static IntToIntPixelConverter toIntArgbPreConverter() {
Yes, but again, the code style is kind of special here. All similar methods are
named like that...
> modules/javafx.graphics/src/main/java/com/sun/prism/Image.java line 225:
>
>> 223: // GRAY, RGB, BGRA_PRE, and INT_ARGB_PRE are directly supported
>> by Prism.
>> 224: // We'll need to convert all other formats that we might
>> encounter to one of the supported formats.
>> 225: // TODO: 3D - need a way to handle pre versus non-Pre
>
> Is this TODO still relevant?
Probably not. In any case, I don't know what the todo specifically wants us to
do.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803193449
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803195074
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803188704
