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

[Ambarish Rapte]

On Thu, 16 Jan 2020 11:28:46 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 1 additional commit.

The fix itself looks good and seems safe for 14.
I do have few minor changes in test file and suggestions in fix code, please take a look.
Also verified that large sized snapshots are created correctly.

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1303:

> 1302:         if (height > maxTextureSize || width > maxTextureSize) {
> 1303:             // The requested size for the screenshot is to big to fit a single texture,
> 1304:             // so we need to take several snapshot tiles and merge them into single image (fixes JDK-8088198)

Should be `too` big to fit

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1306:

> 1305:             int verticalTileNb = (int) Math.ceil(height / (double) maxTextureSize);
> 1306:             int horizontalTileNb = (int) Math.ceil(width / (double) maxTextureSize);
> 1307:             for (int i = 0; i < horizontalTileNb; i++) {

A suggestion for this arithmetic.
int verticalTileNb = height / maxTextureSize + 1;
int horizontalTileNb = width / maxTextureSize + 1;
This will avoid the type casting and floating point operations. However I leave it to you to change or not.

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1312:

> 1311:                     int tileWidth = Math.min(maxTextureSize, width - xOffset);
> 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);

`int xOffset = i * maxTextureSize;`
`int tileWidth = Math.min(maxTextureSize, width - xOffset);`

These two lines should be moved to the outer loop of horizontal tabs.

tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 1:

> 1: package test.javafx.scene;
> 2: 

Copyright header should be added to all new files. You can copy the header from any other file ([header sample](https://github.com/openjdk/jfx/blob/20325e1c3ec4c4e81af74d3d43bf3a803dbe1a51/tests/system/src/test/java/test/javafx/scene/Snapshot2Test.java#L1).)
Only change would be that, this file would contain only year `2020` in the copyright header.

tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 39:

> 38:     public void teardownEach() {
> 39:     }
> 40: 

This empty method is not needed.

tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 65:

> 64: 
> 65: 

Please remove the extra empty lines, 3, 62, 65.

tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 59:

> 58:             Image image = rect.snapshot(params, null);
> 59:             assertEquals(VALUE_LARGER_THAN_TEXTURE_SIZE, (int) image.getHeight());
> 60:         });

Should add `assertNull(image);` before the `assertEquals`

tests/system/src/test/java/test/javafx/scene/Snapshot3Test.java line 11:

> 10: 
> 11: import static org.junit.Assert.*;
> 12: 

Usually we avoid wild imports and import only the specific classes.

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

Changes requested by arapte (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/68