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