RFR: 8306707: Support pluggable image loading via javax.imageio [v2]

[Michael Strauß]

On Tue, 15 Oct 2024 21:35:14 GMT, John Hendrikx <jhendrikx at 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/iio/ImageStorage.java line 201:
> 
>> 199:             return null;
>> 200:         }
>> 201:     }
> 
> Can we document why this is done?  If I had to guess: loading `XImageLoaderFactory`, even though it is present in this module, may fail loading dependent classes (`ImageIO`).

I've added a javadoc paragraph with more details.

> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java line 217:
> 
>> 215:         }
>> 216: 
>> 217:         return ByteBuffer.wrap(Arrays.copyOf(data, size - offset));
> 
> This doesn't look correct.  If `offset` isn't zero, then dropping the last few bytes seems incorrect.  It should start the copy from `offset`?
> 
> I would think this should be: `ByteBuffer.wrap(data, offset, size - offset)` (avoids copy but may waste some space).
> 
> Or if you still want to make a copy, then `Arrays.copyOfRange(data, offset, size)`.
> 
> Note: I have the impression that calling `getData` means the bank won't be modified anymore, so there may not be a need to copy it.

Good catch, I think it is sufficient to use `ByteBuffer.wrap(data, offset, size - offset)` in all cases.

> modules/javafx.graphics/src/main/java/com/sun/javafx/image/PixelUtils.java line 215:
> 
>> 213:         getB2BConverter(PixelGetter<ByteBuffer> src, PixelSetter<ByteBuffer> dst)
>> 214:     {
>> 215:         if (src ==        ByteRgba.getter) {
> 
> minor: I'd be in favor of fixing this formatting.  I know it means well, but it really isn't improving readability.  Perhaps it is more of a job for a `switch` or `Map` lookup.

The code style in this area is very... special. However, I'm hesitant to do a wholesale reformatting here. This will pollute the Git change log with not all that much benefits.

> modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 75:
> 
>> 73:  * <li><a href="http://www.libpng.org/pub/png/spec/">PNG</a></li>
>> 74:  * </ul>
>> 75:  * For all other formats, JavaFX uses the {@link javax.imageio Java Image I/O API} on supported platforms.
> 
> This doesn't look like a valid javadoc link; in Eclipse at least it doesn't do anything.  Perhaps:
> 
> Suggestion:
> 
>  * For all other formats, JavaFX uses the {@link javax.imageio.ImageIO Java Image I/O API} on supported platforms.

Must be an Eclipse problem. IntelliJ and the generated Javadocs correctly point to the `javax.imageio` package documentation.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803167608
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803168475
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803171651
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803167495