On Thu, 16 Jan 2020 17:00:47 GMT, Frederic Thevenet <github.com+7450507+ftheve...@openjdk.org> wrote:
>> This PR aims to address the following issue: JDK-8088198 Exception thrown >> from snapshot if dimensions are larger than max texture size >> >> In order to do that, it simply captures snapshots in multiple tiles of >> maxTextureSize^2 dimensions (or less, as needed), and then recomposes all >> the tiles into a a single image. >> Other than that, the logic used to do the actual snapshot is unchanged. >> >> Tests using the existing SnapshotCommon class have been added in a new file >> named Snapshot3Test under SystemTest/test/javafx/scene. >> These tests pass with the proposed fix, and fail without, throwing " >> java.lang.IllegalArgumentException: Unrecognized image loader: null" > > The pull request has been updated with 2 additional commits. I tested this fix against the repro code in https://github.com/javafxports/openjdk-jfx/issues/433 (which is [JDK-8222238](https://bugs.openjdk.java.net/browse/JDK-8222238)), but there is still an NPE. I'm not certain that this fix is supposed to solve that bug, but according to the comments, the root cause is the same as [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082), which is related to this one. It's worth to take a look to see if something was missed. modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1290: > 1289: int yMin = (int)Math.floor(y); > 1290: int xMax = (int)Math.ceil(x + w); > 1291: int yMax = (int)Math.ceil(y + h); While you're working in the area, this code can be written as int width, height; if (wimg == null) { int xMax = (int)Math.ceil(x + w); int yMax = (int)Math.ceil(y + h); width = Math.max(xMax - xMin, 1); height = Math.max(yMax - yMin, 1); wimg = new WritableImage(width, height); } else { to avoid unnecessary computations. modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1313: > 1312: int tileHeight = Math.min(maxTextureSize, height - > yOffset); > 1313: WritableImage tile = doSnapshotTile(scene, xMin + > xOffset, yMin + yOffset, tileWidth, tileHeight, root, transform, depthBuffer, > fill, camera, null); > 1314: wimg.getPixelWriter().setPixels(xOffset, yOffset, > tileWidth, tileHeight, tile.getPixelReader(), 0, 0); This line is too long and needs to break into 2. I think that the limit is 135 characters. modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1316: > 1315: } > 1316: } > 1317: } else { I would extract this code into its own method similar to `doSnapshotTile`: `assemble(scene, xMin, yMin, width, height, root, transform, depthBuffer, fill, camera, wimg, maxTextureSize);` (`assemble` is a bad name, I didn't think about a better one). The method can return he resulting `WritableImage`, but it is not needed since it is manipulated via "side-effects". I would, however, bring it line with the `else` clause - either both use `wimg = methodName(..., wimg, ...);` or just `methodName(..., wimg, ...);`. This is fine since the input `WritableImage` is never `null`. From a readability point of view, using return values seems better. ------------- PR: https://git.openjdk.java.net/jfx/pull/68
