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

[Jayathirth D V]

On Mon, 28 Oct 2024 17:36:56 GMT, Michael Strauß <mstrauss at 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 21 additional commits since the last revision:
> 
>  - Merge branch 'master' into feature/ximageloader
>  - Merge branch 'master' into feature/ximageloader
>  - review changes
>  - scanline stride always measured in bytes
>  - remove unused code
>  - fix line endings
>  - fix line endings
>  - add support for palette-based image formats
>  - review changes
>  - review changes
>  - ... and 11 more: https://git.openjdk.org/jfx/compare/300461ea...9dba94c1

Looks like current code is good approach to add plug-able support for different image formats in JavaFX.
I have gone through all the files and overall structure looks fine to me apart from minor comments.

I will use this code with sample app and do more testing and system runs and get back to you later.

modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line 195:

> 193:     }
> 194: 
> 195: 

No need for this new line.

modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line 342:

> 340: 
> 341:             if (loader != null) {
> 342:                 images = loadAll(loader, width, height, preserveAspectRatio, pixelScale, 1, smooth);

If this is constant for fixed-density images we should use variable/macro instead of literal value 1 Or add a comment mentioning why we are passing value 1.

modules/javafx.graphics/src/main/java/com/sun/javafx/iio/bmp/BMPImageLoaderFactory.java line 496:

> 494: 
> 495:         int[] outWH = ImageTools.computeDimensions(
> 496:             bih.biWidth, hght, (int)(w * imagePixelScale), (int)(h * imagePixelScale), preserveAspectRatio);

I see possibility of these calculations overflowing.

modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java line 126:

> 124:             int imageHeight = reader.getHeight(imageIndex);
> 125:             int[] widthHeight = ImageTools.computeDimensions(
> 126:                 (int)(imageWidth * screenPixelScale), (int)(imageHeight * screenPixelScale),

Again is there a possibility of overflow here?

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

PR Review: https://git.openjdk.org/jfx/pull/1593#pullrequestreview-2401414969
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1820519669
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1820531736
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1822031048
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1822066470