On Thu, 17 Oct 2024 19:25:30 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> This PR is an improved version of #1093. >> >> JavaFX can load BMP, GIF, PNG, and JPEG images with its built-in image >> loaders. It has been a long-standing request to support more image formats, >> most notably (but not limited to) SVG. However, adding more built-in image >> loaders is a significant effort not only in creating the functionality, but >> also in maintaining the additional dependencies. >> >> This will probably not happen any time soon, so we are left with three >> alternatives: >> 1. Accept the fact that JavaFX will never be able to load additional image >> formats. >> 2. Create a public image loader API, and hope that developers in the JavaFX >> ecosystem will create image loader plugins. >> 3. Leverage the existing Java Image I/O API. >> >> From these options, I think we should simply support existing Java APIs; >> both because it is the shortest and most realistic path forward, but also >> because I don't think it is sensible to bifurcate pluggable image loading in >> the Java ecosystem. >> >> Of course, Java Image I/O is a part of the `java.desktop` module, which as >> of now, all JavaFX applications require. However, it has been noted in the >> previous PR that we shouldn't lock JavaFX into the `java.desktop` dependency >> even further. >> >> I've improved this PR to not permanently require the `java.desktop` >> dependency: if the module is present, then JavaFX will use Image I/O for >> image formats that it can't load with the built-in loaders; if the module is >> not present, only the built-in loaders are available. >> >> I have prepared a small sample application that showcases how the feature >> can be used to load SVG images in a JavaFX application: >> https://github.com/mstr2/jfx-imageio-sample > > Michael Strauß has updated the pull request incrementally with two additional > commits since the last revision: > > - remove unused code > - fix line endings Reviewed latest changes. 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. 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`? ------------- PR Review: https://git.openjdk.org/jfx/pull/1593#pullrequestreview-2379607376 PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1807410432 PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1807416469
