[Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

[Nir Lisker]

On Thu, 16 Jan 2020 17:00:47 GMT, Frederic Thevenet <github.com+7450507+fthevenet at 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