On Mon, 28 Oct 2024 17:36:56 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 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
