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