Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8088198: Exception thrown from snapshot if dimensions are larger than max texture size #68

Closed
wants to merge 7 commits into from
Closed

8088198: Exception thrown from snapshot if dimensions are larger than max texture size #68

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4bc4417
8088198: Exception thrown from snapshot if dimensions are larger than…
fthevenet Dec 14, 2019
9986809
Revert formatting changes
fthevenet Dec 24, 2019
d4ecb73
Removed Snapshot3Test.java and re-enabled ignored tests in Snapshot2T…
fthevenet Jan 16, 2020
5019172
Several changes following code review
fthevenet Jan 16, 2020
8e44dea
Refactored height & weight calculation to avoid unnecessary computations
fthevenet Jan 17, 2020
8966936
Wrapped long line
fthevenet Jan 17, 2020
fce986e
Only attempt tiling on exception
fthevenet Jan 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 53 additions & 20 deletions modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
View file
Open in desktop
Expand Up @@ -1285,9 +1285,6 @@ static WritableImage doSnapshot(Scene scene,
Node root, BaseTransform transform, boolean depthBuffer,
Paint fill, Camera camera, WritableImage wimg) {

Toolkit tk = Toolkit.getToolkit();
Toolkit.ImageRenderingContext context = new Toolkit.ImageRenderingContext();

int xMin = (int)Math.floor(x);
int yMin = (int)Math.floor(y);
int xMax = (int)Math.ceil(x + w);
fthevenet marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -1301,11 +1298,55 @@ static WritableImage doSnapshot(Scene scene,
height = (int)wimg.getHeight();
}

int maxTextureSize = PrismSettings.maxTextureSize;
if (height > maxTextureSize || width > maxTextureSize) {
// The requested size for the screenshot is too big to fit a single texture,
// so we need to take several snapshot tiles and merge them into single image (fixes JDK-8088198)
int numVerticalTiles = (int) Math.ceil(height / (double) maxTextureSize);
int numHorizontalTiles = (int) Math.ceil(width / (double) maxTextureSize);
for (int i = 0; i < numHorizontalTiles; i++) {
int xOffset = i * maxTextureSize;
int tileWidth = Math.min(maxTextureSize, width - xOffset);
for (int j = 0; j < numVerticalTiles; j++) {
int yOffset = j * maxTextureSize;
int tileHeight = Math.min(maxTextureSize, height - yOffset);
fthevenet marked this conversation as resolved.
Show resolved Hide resolved
WritableImage tile = doSnapshotTile(scene, xMin + xOffset, yMin + yOffset, tileWidth, tileHeight, root, transform, depthBuffer, fill, camera, null);
fthevenet marked this conversation as resolved.
Show resolved Hide resolved
wimg.getPixelWriter().setPixels(xOffset, yOffset, tileWidth, tileHeight, tile.getPixelReader(), 0, 0);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% convinced this would really add much to the readability of the code; I extracted the code from doSnapshotTile in its own method because it is called twice (on both sides of the if (height > maxTextureSize || width > maxTextureSize) condition, actually), but this isn't the case here.
I've got no strong feeling against it either, so I don't know; anybody else care to comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't have a strong opinion, so I'm OK with you leaving it as-is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine to leave as-is.

} else {
// The requested size for the screenshot fits max texture size,
// so we can directly return the one generated tile and avoid the extra pixel copy.
wimg = doSnapshotTile(scene, xMin, yMin, width, height, root, transform, depthBuffer, fill, camera, wimg);
}

// if this scene belongs to some stage
// we need to mark the entire scene as dirty
// because dirty logic is buggy
if (scene != null && scene.peer != null) {
scene.setNeedsRepaint();
}

return wimg;
}

/**
* Capture a single snapshot tile
*/
private static WritableImage doSnapshotTile(Scene scene,
int x, int y, int w, int h,
Node root, BaseTransform transform, boolean depthBuffer,
Paint fill, Camera camera, WritableImage tileImg) {
Toolkit tk = Toolkit.getToolkit();
Toolkit.ImageRenderingContext context = new Toolkit.ImageRenderingContext();
if (tileImg == null) {
tileImg = new WritableImage(w, h);
}
setAllowPGAccess(true);
context.x = xMin;
context.y = yMin;
context.width = width;
context.height = height;
context.x = x;
context.y = y;
context.width = w;
context.height = h;
context.transform = transform;
context.depthBuffer = depthBuffer;
context.root = root.getPeer();
Expand All @@ -1316,8 +1357,8 @@ static WritableImage doSnapshot(Scene scene,
// temporarily adjust camera viewport to the snapshot size
cameraViewWidth = camera.getViewWidth();
cameraViewHeight = camera.getViewHeight();
camera.setViewWidth(width);
camera.setViewHeight(height);
camera.setViewWidth(w);
camera.setViewHeight(h);
NodeHelper.updatePeer(camera);
context.camera = camera.getPeer();
} else {
Expand All @@ -1334,10 +1375,10 @@ static WritableImage doSnapshot(Scene scene,
}

Toolkit.WritableImageAccessor accessor = Toolkit.getWritableImageAccessor();
context.platformImage = accessor.getTkImageLoader(wimg);
context.platformImage = accessor.getTkImageLoader(tileImg);
setAllowPGAccess(false);
Object tkImage = tk.renderToImage(context);
accessor.loadTkImage(wimg, tkImage);
accessor.loadTkImage(tileImg, tkImage);

if (camera != null) {
setAllowPGAccess(true);
Expand All @@ -1346,15 +1387,7 @@ static WritableImage doSnapshot(Scene scene,
NodeHelper.updatePeer(camera);
setAllowPGAccess(false);
}

// if this scene belongs to some stage
// we need to mark the entire scene as dirty
// because dirty logic is buggy
if (scene != null && scene.peer != null) {
scene.setNeedsRepaint();
}

return wimg;
return tileImg;
}

/**
Expand Down
13 changes: 4 additions & 9 deletions tests/system/src/test/java/test/javafx/scene/Snapshot2Test.java
View file
Open in desktop
Expand Up @@ -41,7 +41,6 @@
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
Expand Down Expand Up @@ -373,26 +372,22 @@ public void testSnapshotScaleNodeDefer() {
doTestSnapshotScaleNodeDefer(3, 3);
}

// TODO: Re-enable this test when RT-22073 is fixed
@Ignore @Test
@Test
public void testSnapshotBigXScaleNodeImm() {
doTestSnapshotScaleNodeImm(100, 1);
}

// TODO: Re-enable this test when RT-22073 is fixed
@Ignore @Test
@Test
public void testSnapshotBigXScaleNodeDefer() {
doTestSnapshotScaleNodeDefer(100, 1);
}

// TODO: Re-enable this test when RT-22073 is fixed
@Ignore @Test
@Test
public void testSnapshotBigYScaleNodeImm() {
doTestSnapshotScaleNodeImm(1, 200);
}

// TODO: Re-enable this test when RT-22073 is fixed
@Ignore @Test
@Test
public void testSnapshotBigYScaleNodeDefer() {
doTestSnapshotScaleNodeDefer(1, 200);
}
Expand Down