On Sat, 19 Oct 2024 16:24:42 GMT, John Hendrikx <jhendr...@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
