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

[Michael Strauß]

On Sat, 19 Oct 2024 16:24:42 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Michael Strauß has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - remove unused code
>>  - fix line endings
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java line 206:
> 
>> 204:                         : ImageStorage.ImageType.PALETTE,
>> 205:                     getByteBuffer(image.getRaster().getDataBuffer()),
>> 206:                     image.getWidth(), image.getHeight(), image.getWidth() * colorModel.getPixelSize(),
> 
> General comment here for all spots where you are calculating the stride; you can ask AWT this.  You don't know what kind of padding or rounding going on, and given how varied image formats can be, its unlikely to be always as simple as "round up to nearest byte".  I also think that having in some cases the stride being bytes and in others being bits is not a good idea.  So, how often before the switch asking AWT for the stride?
> 
>       SampleModel model = raster.getSampleModel();
> 
>       int stride = switch(model) {
>           case ComponentSampleModel m -> m.getScanlineStride();
>           case MultiPixelPackedSampleModel m -> m.getScanlineStride();
>           case SinglePixelPackedSampleModel m -> m.getScanlineStride();
>           default -> throw new IllegalStateException("Unsupported sample model: " + model);
>       }
> 
> (Blame AWT for not putting `getScanlineStride` as an abstract method in the base class).
> 
> You can then use this value for all locations, and IMHO should convert the indexed versions to use bytes, or at a minimum document in `ImageFrame` that `stride` no longer has its traditional meaning in all cases.  As I think that's very confusing, and a stride in bits is useless to have, I'd be very much in favor of having the stride back to being bytes in all cases.

Implemented as you suggested. Scanline stride is now also always measured in bytes.

> modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/BaseIndexedToByteConverter.java line 42:
> 
>> 40:         private final Boolean premultiplied;
>> 41: 
>> 42:         IndexedGetter(int[] colors, Boolean premultiplied) {
> 
> I think using `Boolean` to store 3 values is confusing.  Use an `enum`?

Replaced with `AlphaType`.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1808818039
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1808818298