Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Tue, 28 Jan 2020 16:00:35 GMT, Frederic Thevenet wrote: >> I have tested using a recipient WritableImage with an IntARGB pixel format >> (so that is aligned with that of the tile), that I construct by supplying a >> PixelBuffer, and as expected that performance of the tiled code is much more >> in line with that of the non-tiled version. One overhead left is the >> allocation of the extra buffer, but it is far less significant. >> The problem with this approach is that the "best" pixel format (as in >> similar to that of RRT) isn't the same depending on the rendering pipeline >> (e.g. d3d is intARGB while es2 is byteBGRA) so that would require adding a >> fair amount of code to determine the best possible scenario depending on all >> the constraints (keeping in mind the caller can also provide its own >> WritableImage...) that in turn would need thorough testing. >> All in all, I agree the risk of regression is probably too great for this to >> make it into openjfx14 with so little time left. >> >> Instead, I will prototype Kevin's proposal to only use tiling when it would >> fail with the current code and use what I've learned here to propose a more >> robust fix targeted at openjfx15 > > I've made the change suggested by Kevin, and it works as expected. > It is not very elegant but it does provide relief with regard to the core > issue while avoiding risks of performance regressions. > > Now with regard to a follow-up PR targeted at the next release, I assume a > new issue needs first be filed into the JBS first; should I just file a new > bug via https://bugreport.java.com/bugreport (I don't have access to > https://bugs.openjdk.java.net)? The change looks like what I would expect, so I'll finish my review in the next day or so. @arapte will need to re-review it as well. Yes, we will need a new JBS bug ID for the follow-on. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Mon, 27 Jan 2020 18:45:17 GMT, Frederic Thevenet wrote: >> I thought of one possibility that might be worth looking into for a short >> term fix (i.e., could still go into openjfx14). Rather than relying on >> `PrismSettings.maxTextureSize` you could instead try to render the entire >> image (as is done today) in a try / catch block, and only fall back to >> tiling if there is an exception. That way there would be no concern over any >> possible performance regression, since the only time we would use tiling >> would be in the case where it fials today. >> >> What do you think? > > I have tested using a recipient WritableImage with an IntARGB pixel format > (so that is aligned with that of the tile), that I construct by supplying a > PixelBuffer, and as expected that performance of the tiled code is much more > in line with that of the non-tiled version. One overhead left is the > allocation of the extra buffer, but it is far less significant. > The problem with this approach is that the "best" pixel format (as in similar > to that of RRT) isn't the same depending on the rendering pipeline (e.g. d3d > is intARGB while es2 is byteBGRA) so that would require adding a fair amount > of code to determine the best possible scenario depending on all the > constraints (keeping in mind the caller can also provide its own > WritableImage...) that in turn would need thorough testing. > All in all, I agree the risk of regression is probably too great for this to > make it into openjfx14 with so little time left. > > Instead, I will prototype Kevin's proposal to only use tiling when it would > fail with the current code and use what I've learned here to propose a more > robust fix targeted at openjfx15 I've made the change suggested by Kevin, and it works as expected. It is not very elegant but it does provide relief with regard to the core issue while avoiding risks of performance regressions. Now with regard to a follow-up PR targeted at the next release, I assume a new issue needs first be filed into the JBS first; should I just file a new bug via https://bugreport.java.com/bugreport (I don't have access to https://bugs.openjdk.java.net)? - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Mon, 27 Jan 2020 15:30:27 GMT, Kevin Rushforth wrote: >> I would be very cautious of using multi-threading here. In any case, I think >> that the issues around absolute performance could be handled separately. >> >> Having said that, given my review comments, along with the concerns over >> performance regressions for those cases that will now be tiled, but formerly >> weren't, I no longer think this is a good candidate for openjfx14. This PR >> should be retargeted to the `master` branch for openjfx15. > > I thought of one possibility that might be worth looking into for a short > term fix (i.e., could still go into openjfx14). Rather than relying on > `PrismSettings.maxTextureSize` you could instead try to render the entire > image (as is done today) in a try / catch block, and only fall back to tiling > if there is an exception. That way there would be no concern over any > possible performance regression, since the only time we would use tiling > would be in the case where it fials today. > > What do you think? I have tested using a recipient WritableImage with an IntARGB pixel format (so that is aligned with that of the tile), that I construct by supplying a PixelBuffer, and as expected that performance of the tiled code is much more in line with that of the non-tiled version. One overhead left is the allocation of the extra buffer, but it is far less significant. The problem with this approach is that the "best" pixel format (as in similar to that of RRT) isn't the same depending on the rendering pipeline (e.g. d3d is intARGB while es2 is byteBGRA) so that would require adding a fair amount of code to determine the best possible scenario depending on all the constraints (keeping in mind the caller can also provide its own WritableImage...) that in turn would need thorough testing. All in all, I agree the risk of regression is probably too great for this to make it into openjfx14 with so little time left. Instead, I will prototype Kevin's proposal to only use tiling when it would fail with the current code and use what I've learned here to propose a more robust fix targeted at openjfx15 - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Mon, 27 Jan 2020 13:47:43 GMT, Kevin Rushforth wrote:
>>>
>>>
>>> > profiling a run of the benchmark shows that a lot of time is spent into
>>> > `IntTo4ByteSameConverter::doConvert`
>>>
>>> This is a bit naive, but what if you parallelize the code there? I didn't
>>> test that this produces the correct result, but you can try to replace the
>>> loops with this:
>>>
>>> ```
>>> IntStream.range(0, h).parallel().forEach(y -> {
>>> IntStream.range(0, w).parallel().forEach(x -> {
>>> int pixel = srcarr[srcoff++];
>>> dstarr[dstoff++] = (byte) (pixel );
>>> dstarr[dstoff++] = (byte) (pixel >> 8);
>>> dstarr[dstoff++] = (byte) (pixel >> 16);
>>> dstarr[dstoff++] = (byte) (pixel >> 24);
>>> });
>>> srcoff += srcscanints;
>>> dstoff += dstscanbytes;
>>> });
>>> ```
>>
>> I don't think this works as it is, as all threads race to increment `srcoff`
>> and `dstoff`.
>
> I would be very cautious of using multi-threading here. In any case, I think
> that the issues around absolute performance could be handled separately.
>
> Having said that, given my review comments, along with the concerns over
> performance regressions for those cases that will now be tiled, but formerly
> weren't, I no longer think this is a good candidate for openjfx14. This PR
> should be retargeted to the `master` branch for openjfx15.
I thought of one possibility that might be worth looking into for a short term
fix (i.e., could still go into openjfx14). Rather than relying on
`PrismSettings.maxTextureSize` you could instead try to render the entire image
(as is done today) in a try / catch block, and only fall back to tiling if
there is an exception. That way there would be no concern over any possible
performance regression, since the only time we would use tiling would be in the
case where it fials today.
What do you think?
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Sun, 26 Jan 2020 11:58:27 GMT, Frederic Thevenet
wrote:
>>>
>>>
>>> > the `WriteableImage` used to collate the tiles and the tiles returned
>>> > from the `RTTexture` have different pixel formats (`IntARGB` for the tile
>>> > and `byteBGRA` for the `WriteableImage`).
>>>
>>> Where did you see these?
>>
>> Simply by watching the value of `tile.getPixelReader().getPixelFormat()`
>> and `wimg.getPixelWriter().getPixelFormat()` before doing the copy at
>> https://github.com/openjdk/jfx/blob/4bc4417356ebd639567d315257a6bbe11344d9c2/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1315
>>>
>>> > Unfortunately it seems the only way to choose the pixel format for a
>>> > `WritableImage` is to initialize it with a `PixelBuffer`, but then one
>>> > can no longer use a `PixelWriter` to update it...
>>>
>>> You can update it with `PixelBuffer#updateBuffer`. I think that you will
>>> want to pass the stitched tile as the dirty region.
>>
>> Thanks. I'll look into it.
>
>>
>>
>> > profiling a run of the benchmark shows that a lot of time is spent into
>> > `IntTo4ByteSameConverter::doConvert`
>>
>> This is a bit naive, but what if you parallelize the code there? I didn't
>> test that this produces the correct result, but you can try to replace the
>> loops with this:
>>
>> ```
>> IntStream.range(0, h).parallel().forEach(y -> {
>> IntStream.range(0, w).parallel().forEach(x -> {
>> int pixel = srcarr[srcoff++];
>> dstarr[dstoff++] = (byte) (pixel );
>> dstarr[dstoff++] = (byte) (pixel >> 8);
>> dstarr[dstoff++] = (byte) (pixel >> 16);
>> dstarr[dstoff++] = (byte) (pixel >> 24);
>> });
>> srcoff += srcscanints;
>> dstoff += dstscanbytes;
>> });
>> ```
>
> I don't think this works as it is, as all threads race to increment `srcoff`
> and `dstoff`.
I would be very cautious of using multi-threading here. In any case, I think
that the issues around absolute performance could be handled separately.
Having said that, given my review comments, along with the concerns over
performance regressions for those cases that will now be tiled, but formerly
weren't, I no longer think this is a good candidate for openjfx14. This PR
should be retargeted to the `master` branch for openjfx15.
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Sun, 26 Jan 2020 11:40:06 GMT, Frederic Thevenet
wrote:
>>> the `WriteableImage` used to collate the tiles and the tiles returned from
>>> the `RTTexture` have different pixel formats (`IntARGB` for the tile and
>>> `byteBGRA` for the `WriteableImage`).
>>
>> Where did you see these?
>>
>>> Unfortunately it seems the only way to choose the pixel format for a
>>> `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
>>> no longer use a `PixelWriter` to update it...
>>
>> You can update it with `PixelBuffer#updateBuffer`. I think that you will
>> want to pass the stitched tile as the dirty region.
>
>>
>>
>> > the `WriteableImage` used to collate the tiles and the tiles returned from
>> > the `RTTexture` have different pixel formats (`IntARGB` for the tile and
>> > `byteBGRA` for the `WriteableImage`).
>>
>> Where did you see these?
>
> Simply by watching the value of `tile.getPixelReader().getPixelFormat()` and
> `wimg.getPixelWriter().getPixelFormat()` before doing the copy at
> https://github.com/openjdk/jfx/blob/4bc4417356ebd639567d315257a6bbe11344d9c2/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1315
>>
>> > Unfortunately it seems the only way to choose the pixel format for a
>> > `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
>> > no longer use a `PixelWriter` to update it...
>>
>> You can update it with `PixelBuffer#updateBuffer`. I think that you will
>> want to pass the stitched tile as the dirty region.
>
> Thanks. I'll look into it.
>
>
> > profiling a run of the benchmark shows that a lot of time is spent into
> > `IntTo4ByteSameConverter::doConvert`
>
> This is a bit naive, but what if you parallelize the code there? I didn't
> test that this produces the correct result, but you can try to replace the
> loops with this:
>
> ```
> IntStream.range(0, h).parallel().forEach(y -> {
> IntStream.range(0, w).parallel().forEach(x -> {
> int pixel = srcarr[srcoff++];
> dstarr[dstoff++] = (byte) (pixel );
> dstarr[dstoff++] = (byte) (pixel >> 8);
> dstarr[dstoff++] = (byte) (pixel >> 16);
> dstarr[dstoff++] = (byte) (pixel >> 24);
> });
> srcoff += srcscanints;
> dstoff += dstscanbytes;
> });
> ```
I don't think this works as it is, as all threads race to increment `srcoff`
and `dstoff`.
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Sat, 25 Jan 2020 20:24:54 GMT, Nir Lisker wrote:
>>> profiling a run of the benchmark shows that a lot of time is spent into
>>> `IntTo4ByteSameConverter::doConvert`
>>
>> This is a bit naive, but what if you parallelize the code there? I didn't
>> test that this produces the correct result, but you can try to replace the
>> loops with this:
>> IntStream.range(0, h).parallel().forEach(y -> {
>> IntStream.range(0, w).parallel().forEach(x -> {
>> int pixel = srcarr[srcoff++];
>> dstarr[dstoff++] = (byte) (pixel );
>> dstarr[dstoff++] = (byte) (pixel >> 8);
>> dstarr[dstoff++] = (byte) (pixel >> 16);
>> dstarr[dstoff++] = (byte) (pixel >> 24);
>> });
>> srcoff += srcscanints;
>> dstoff += dstscanbytes;
>> });
>
>> the `WriteableImage` used to collate the tiles and the tiles returned from
>> the `RTTexture` have different pixel formats (`IntARGB` for the tile and
>> `byteBGRA` for the `WriteableImage`).
>
> Where did you see these?
>
>> Unfortunately it seems the only way to choose the pixel format for a
>> `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
>> no longer use a `PixelWriter` to update it...
>
> You can update it with `PixelBuffer#updateBuffer`. I think that you will want
> to pass the stitched tile as the dirty region.
>
>
> > the `WriteableImage` used to collate the tiles and the tiles returned from
> > the `RTTexture` have different pixel formats (`IntARGB` for the tile and
> > `byteBGRA` for the `WriteableImage`).
>
> Where did you see these?
Simply by watching the value of `tile.getPixelReader().getPixelFormat()` and
`wimg.getPixelWriter().getPixelFormat()` before doing the copy at
https://github.com/openjdk/jfx/blob/4bc4417356ebd639567d315257a6bbe11344d9c2/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1315
>
> > Unfortunately it seems the only way to choose the pixel format for a
> > `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
> > no longer use a `PixelWriter` to update it...
>
> You can update it with `PixelBuffer#updateBuffer`. I think that you will want
> to pass the stitched tile as the dirty region.
Thanks. I'll look into it.
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Sat, 25 Jan 2020 19:55:23 GMT, Nir Lisker wrote:
>> With regard as to why the tiling version is significantly slower, though, I
>> do have a pretty good idea; as Kevin hinted, the pixel copy into a temporary
>> buffer before copying into the final image is where most the extra time is
>> spent.
>> The reason why it is so much slower is a little bit of a pity, though;
>> profiling a run of the benchmark shows that a lot of time is spent into
>> `IntTo4ByteSameConverter::doConvert`. As it turns out, the reason for this
>> is that, under Windows and the D3D pipeline anyway, the `WriteableImage`
>> used to collate the tiles and the tiles returned from the RTTexture have
>> different pixel formats (IntARGB for the tile and byteBGRA for the
>> `WriteableImage`).
>> So if we could use a `WriteableImage` with an IntARGB pixel format as the
>> recipient for the snapshot (at least as long as no image was provided by the
>> caller), I suspect that the copy would be much faster.
>> Unfortunately it seems the only way to choose the pixel format for a
>> `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
>> no longer use a `PixelWriter` to update it and it desn't seems to me that
>> there is a way to safely access the `PixelBuffer` from an image's reference
>> alone.
>> I'm pretty new to this code base though (which is quite large; I haven't
>> read it all quite yet... ;-), so hopefully there's a way to do that that has
>> simply eluded me so far.
>
>> profiling a run of the benchmark shows that a lot of time is spent into
>> `IntTo4ByteSameConverter::doConvert`
>
> This is a bit naive, but what if you parallelize the code there? I didn't
> test that this produces the correct result, but you can try to replace the
> loops with this:
> IntStream.range(0, h).parallel().forEach(y -> {
> IntStream.range(0, w).parallel().forEach(x -> {
> int pixel = srcarr[srcoff++];
> dstarr[dstoff++] = (byte) (pixel );
> dstarr[dstoff++] = (byte) (pixel >> 8);
> dstarr[dstoff++] = (byte) (pixel >> 16);
> dstarr[dstoff++] = (byte) (pixel >> 24);
> });
> srcoff += srcscanints;
> dstoff += dstscanbytes;
> });
> the `WriteableImage` used to collate the tiles and the tiles returned from
> the `RTTexture` have different pixel formats (`IntARGB` for the tile and
> `byteBGRA` for the `WriteableImage`).
Where did you see these?
> Unfortunately it seems the only way to choose the pixel format for a
> `WritableImage` is to initialize it with a `PixelBuffer`, but then one can no
> longer use a `PixelWriter` to update it...
You can update it with `PixelBuffer#updateBuffer`. I think that you will want
to pass the stitched tile as the dirty region.
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Fri, 24 Jan 2020 17:16:13 GMT, Frederic Thevenet
wrote:
>> I don't, to be honest.
>> The results for some dimensions (not always the same) can vary pretty
>> widely from one run to another, despite all my effort to repeat results and
>> remove outliers.
>> Out of curiosity, I also tried to eliminate the GC as possible culprit by
>> running it with epsilon, but it seems to make no significant difference.
>> I ran that test on a laptop with Integrated Intel graphics and no dedicated
>> vram (Intel UHD Graphics 620), though, so this might be why.
>> Maybe someone could try and run the bench on hardware with a discreet GPU?
>
> With regard as to why the tiling version is significantly slower, though, I
> do have a pretty good idea; as Kevin hinted, the pixel copy into a temporary
> buffer before copying into the final image is where most the extra time is
> spent.
> The reason why it is so much slower is a little bit of a pity, though;
> profiling a run of the benchmark shows that a lot of time is spent into
> `IntTo4ByteSameConverter::doConvert`. As it turns out, the reason for this is
> that, under Windows and the D3D pipeline anyway, the `WriteableImage` used to
> collate the tiles and the tiles returned from the RTTexture have different
> pixel formats (IntARGB for the tile and byteBGRA for the `WriteableImage`).
> So if we could use a `WriteableImage` with an IntARGB pixel format as the
> recipient for the snapshot (at least as long as no image was provided by the
> caller), I suspect that the copy would be much faster.
> Unfortunately it seems the only way to choose the pixel format for a
> `WritableImage` is to initialize it with a `PixelBuffer`, but then one can no
> longer use a `PixelWriter` to update it and it desn't seems to me that there
> is a way to safely access the `PixelBuffer` from an image's reference alone.
> I'm pretty new to this code base though (which is quite large; I haven't read
> it all quite yet... ;-), so hopefully there's a way to do that that has
> simply eluded me so far.
> profiling a run of the benchmark shows that a lot of time is spent into
> `IntTo4ByteSameConverter::doConvert`
This is a bit naive, but what if you parallelize the code there? I didn't test
that this produces the correct result, but you can try to replace the loops
with this:
IntStream.range(0, h).parallel().forEach(y -> {
IntStream.range(0, w).parallel().forEach(x -> {
int pixel = srcarr[srcoff++];
dstarr[dstoff++] = (byte) (pixel );
dstarr[dstoff++] = (byte) (pixel >> 8);
dstarr[dstoff++] = (byte) (pixel >> 16);
dstarr[dstoff++] = (byte) (pixel >> 24);
});
srcoff += srcscanints;
dstoff += dstscanbytes;
});
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Fri, 24 Jan 2020 16:55:47 GMT, Frederic Thevenet wrote: >>> Here are the results when running JavaFX 14-ea+7. >>> The columns of the table correspond the width of the target snapshot, while >>> the rows correspond to the height and the content of the cells is the >>> average time* spent (in ms) in `Node::snapshot` >>> (*) each test is ran 10 times and the elapsed time is averaged after >>> pruning outliers, using Grubbs' test. >>> >>> 10242048307240965120614471688192 >>> 10246.30427210.303935 15.052336 35.929304 >>> 23.860095 28.828812 35.315288 27.867205 >>> 204811.544367 21.156326 28.368750 28.887164 >>> 47.134738 54.354708 55.480251 56.722649 >>> 307215.503187 30.215269 41.304645 39.789648 >>> 82.255091 82.576379 96.618722 106.586547 >>> 409620.928336 38.768648 64.255423 52.608217 >>> 101.797347 132.516816 158.525192 166.872889 >>> 512028.693431 67.275306 68.090280 76.208412 >>> 133.974510 157.120373 182.329784 210.069066 >>> 614429.972591 54.751002 88.171906 104.489291 >>> 147.788597 185.185643 213.562819 228.643761 >>> 716833.668398 63.088490 98.756212 130.502678 >>> 196.367121 225.166481 239.328794 260.162501 >>> 819240.961901 87.067460 128.230351 178.127225 >>> 198.479068 225.806211 266.170239 325.967840 >> >> Any idea why 4096x1024 and 1024x4096 are so different? Same for 8192x1024 >> and 1024x8192. > > I don't, to be honest. > The results for some dimensions (not always the same) can vary pretty widely > from one run to another, despite all my effort to repeat results and remove > outliers. > Out of curiosity, I also tried to eliminate the GC as possible culprit by > running it with epsilon, but it seems to make a significant difference. > I ran that test on a laptop with Integrated Intel graphics and no dedicated > vram (Intel UHD Graphics 620), though, so this might be why. > Maybe someone could try and run the bench on hardware with a discreet GPU? With regard to why the tiling version is significantly slower, though, I do have a pretty good idea; as Kevin hinted, the pixel copy into a temporary buffer before copying into the final image is where most the extra time is spent. The reason why is is some much slower is a little bit of a pity, though; profiling a run of the benchmark shows that a lot of time is spent into `IntTo4ByteSameConverter::doConvert` and the reason for this turns out that this is due to the fact that, under Windows and the D3D pipeline anyway, the `WriteableImage` used to collate the tiles and the tiles returned from the RTTexture have different pixel formats (IntARGB for the tile and byteBGRA for the `WriteableImage`). So if we could use a `WriteableImage` with an IntARGB pixel format as the recipient for the snapshot (at least as long as no image was provided by the caller), I suspect that the copy would be much faster. Unfortunately it seems the only way to choose the pixel format for a `WritableImage` is to initialize it with a `PixelBuffer`, but then one can no longer use a `PixelWriter` to update it and it desn't seems to me that there is a way to safely access the `PixelBuffer` from an image's reference alone. I'm pretty new to this code base though (which is quite large; I haven't read it all quite yet... ;-), so hopefully there's a way to do that that has eluded me so far. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Fri, 24 Jan 2020 16:34:29 GMT, Nir Lisker wrote: >> And here are the results with the change in this PR, on the same machine >> under Windows 10: >> >> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 | >> |---|---|---|---|---|---|---|---|---| >> | 1024 | 6.957774 | 10.461498 | 14.721024 | 19.279638 | 47.508266 | >> 56.585089 | 64.522117 | 53.448326 | >> | 2048 | 10.990480 | 19.284461 | 28.235822 | 41.067555 | 92.818088 | >> 106.782334 | 120.907050 | 112.852554 | >> | 3072 | 15.642648 | 28.502151 | 59.541998 | 55.663658 | 130.654226 | >> 149.805330 | 205.212356 | 169.002232 | >> | 4096 | 19.882252 | 41.287722 | 59.493687 | 73.809264 | 169.212467 | >> 200.212097 | 247.934609 | 246.412543 | >> | 5120 | 49.986204 | 97.986781 | 126.127089 | 167.274104 | 217.063815 | >> 276.812929 | 307.276073 | 366.800463 | >> | 6144 | 66.546156 | 104.339809 | 150.171765 | 206.282107 | 272.486419 | >> 321.178983 | 365.822047 | 410.087087 | >> | 7168 | 66.894654 | 119.510866 | 176.002883 | 248.937222 | 314.721516 | >> 380.834398 | 430.858648 | 482.499047 | >> | 8192 | 67.040207 | 112.831977 | 161.852173 | 237.588749 | 319.667719 | >> 382.151556 | 437.810832 | 451.865266 | > >> Here are the results when running JavaFX 14-ea+7. >> The columns of the table correspond the width of the target snapshot, while >> the rows correspond to the height and the content of the cells is the >> average time* spent (in ms) in `Node::snapshot` >> (*) each test is ran 10 times and the elapsed time is averaged after pruning >> outliers, using Grubbs' test. >> >> 1024 2048307240965120614471688192 >> 1024 6.30427210.303935 15.052336 35.929304 >> 23.860095 28.828812 35.315288 27.867205 >> 2048 11.544367 21.156326 28.368750 28.887164 >> 47.134738 54.354708 55.480251 56.722649 >> 3072 15.503187 30.215269 41.304645 39.789648 >> 82.255091 82.576379 96.618722 106.586547 >> 4096 20.928336 38.768648 64.255423 52.608217 >> 101.797347 132.516816 158.525192 166.872889 >> 5120 28.693431 67.275306 68.090280 76.208412 >> 133.974510 157.120373 182.329784 210.069066 >> 6144 29.972591 54.751002 88.171906 104.489291 >> 147.788597 185.185643 213.562819 228.643761 >> 7168 33.668398 63.088490 98.756212 130.502678 >> 196.367121 225.166481 239.328794 260.162501 >> 8192 40.961901 87.067460 128.230351 178.127225 >> 198.479068 225.806211 266.170239 325.967840 > > Any idea why 4096x1024 and 1024x4096 are so different? Same for 8192x1024 and > 1024x8192. I don't, to be honest. The results for some dimensions (not always the same) can vary pretty widely from one run to another, despite all my effort to repeat results and remove outliers. Out of curiosity, I also tried to eliminate the GC as possible culprit by running it with epsilon, but it seems to make a significant difference. I ran that test on a laptop with Integrated Intel graphics and no dedicated vram (Intel UHD Graphics 620), though, so this might be why. Maybe someone could try and run the bench on hardware with a discreet GPU? - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Fri, 24 Jan 2020 16:26:38 GMT, Frederic Thevenet wrote: >> Here are the results when running JavaFX 14-ea+7. >> The columns of the table correspond the width of the target snapshot, while >> the rows correspond to the height and the content of the cells is the >> average time* spent (in ms) in `Node::snapshot` >> (*) each test is ran 10 times and the elapsed time is averaged after pruning >> outliers, using Grubbs' test. >> >> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 | >> |---|---|---|---|---|---|---|---|---| >> | 1024 | 6.304272 | 10.303935 | 15.052336 | 35.929304 | 23.860095 | >> 28.828812 | 35.315288 | 27.867205 | >> | 2048 | 11.544367 | 21.156326 | 28.368750 | 28.887164 | 47.134738 | >> 54.354708 | 55.480251 | 56.722649 | >> | 3072 | 15.503187 | 30.215269 | 41.304645 | 39.789648 | 82.255091 | >> 82.576379 | 96.618722 | 106.586547 | >> | 4096 | 20.928336 | 38.768648 | 64.255423 | 52.608217 | 101.797347 | >> 132.516816 | 158.525192 | 166.872889 | >> | 5120 | 28.693431 | 67.275306 | 68.090280 | 76.208412 | 133.974510 | >> 157.120373 | 182.329784 | 210.069066 | >> | 6144 | 29.972591 | 54.751002 | 88.171906 | 104.489291 | 147.788597 | >> 185.185643 | 213.562819 | 228.643761 | >> | 7168 | 33.668398 | 63.088490 | 98.756212 | 130.502678 | 196.367121 | >> 225.166481 | 239.328794 | 260.162501 | >> | 8192 | 40.961901 | 87.067460 | 128.230351 | 178.127225 | 198.479068 | >> 225.806211 | 266.170239 | 325.967840 | > > And here are the results with the change in this PR, on the same machine > under Windows 10: > > || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 | > |---|---|---|---|---|---|---|---|---| > | 1024 | 6.957774 | 10.461498 | 14.721024 | 19.279638 | 47.508266 | 56.585089 > | 64.522117 | 53.448326 | > | 2048 | 10.990480 | 19.284461 | 28.235822 | 41.067555 | 92.818088 | > 106.782334 | 120.907050 | 112.852554 | > | 3072 | 15.642648 | 28.502151 | 59.541998 | 55.663658 | 130.654226 | > 149.805330 | 205.212356 | 169.002232 | > | 4096 | 19.882252 | 41.287722 | 59.493687 | 73.809264 | 169.212467 | > 200.212097 | 247.934609 | 246.412543 | > | 5120 | 49.986204 | 97.986781 | 126.127089 | 167.274104 | 217.063815 | > 276.812929 | 307.276073 | 366.800463 | > | 6144 | 66.546156 | 104.339809 | 150.171765 | 206.282107 | 272.486419 | > 321.178983 | 365.822047 | 410.087087 | > | 7168 | 66.894654 | 119.510866 | 176.002883 | 248.937222 | 314.721516 | > 380.834398 | 430.858648 | 482.499047 | > | 8192 | 67.040207 | 112.831977 | 161.852173 | 237.588749 | 319.667719 | > 382.151556 | 437.810832 | 451.865266 | > Here are the results when running JavaFX 14-ea+7. > The columns of the table correspond the width of the target snapshot, while > the rows correspond to the height and the content of the cells is the average > time* spent (in ms) in `Node::snapshot` > (*) each test is ran 10 times and the elapsed time is averaged after pruning > outliers, using Grubbs' test. > > 1024 2048307240965120614471688192 > 1024 6.30427210.303935 15.052336 35.929304 > 23.860095 28.828812 35.315288 27.867205 > 2048 11.544367 21.156326 28.368750 28.887164 > 47.134738 54.354708 55.480251 56.722649 > 3072 15.503187 30.215269 41.304645 39.789648 > 82.255091 82.576379 96.618722 106.586547 > 4096 20.928336 38.768648 64.255423 52.608217 > 101.797347 132.516816 158.525192 166.872889 > 5120 28.693431 67.275306 68.090280 76.208412 > 133.974510 157.120373 182.329784 210.069066 > 6144 29.972591 54.751002 88.171906 104.489291 > 147.788597 185.185643 213.562819 228.643761 > 7168 33.668398 63.088490 98.756212 130.502678 > 196.367121 225.166481 239.328794 260.162501 > 8192 40.961901 87.067460 128.230351 178.127225 > 198.479068 225.806211 266.170239 325.967840 Any idea why 4096x1024 and 1024x4096 are so different? Same for 8192x1024 and 1024x8192. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Fri, 24 Jan 2020 16:03:39 GMT, Frederic Thevenet wrote: >> I've put together a small benchmark to measure the time it takes to >> snapshots into images of sizes varying from 1024*1024 to 8192*8192, by >> increment of 1024 in each dimension, which you can find here: >> https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java > > Here are the results when running JavaFX 14-ea+7. > The columns of the table correspond the width of the target snapshot, while > the rows correspond to the height and the content of the cells is the average > time* spent (in ms) in `Node::snapshot` > (*) each test is ran 10 times and the elapsed time is averaged after pruning > outliers, using Grubbs' test. > > || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 | > |---|---|---|---|---|---|---|---|---| > | 1024 | 6.304272 | 10.303935 | 15.052336 | 35.929304 | 23.860095 | 28.828812 > | 35.315288 | 27.867205 | > | 2048 | 11.544367 | 21.156326 | 28.368750 | 28.887164 | 47.134738 | > 54.354708 | 55.480251 | 56.722649 | > | 3072 | 15.503187 | 30.215269 | 41.304645 | 39.789648 | 82.255091 | > 82.576379 | 96.618722 | 106.586547 | > | 4096 | 20.928336 | 38.768648 | 64.255423 | 52.608217 | 101.797347 | > 132.516816 | 158.525192 | 166.872889 | > | 5120 | 28.693431 | 67.275306 | 68.090280 | 76.208412 | 133.974510 | > 157.120373 | 182.329784 | 210.069066 | > | 6144 | 29.972591 | 54.751002 | 88.171906 | 104.489291 | 147.788597 | > 185.185643 | 213.562819 | 228.643761 | > | 7168 | 33.668398 | 63.088490 | 98.756212 | 130.502678 | 196.367121 | > 225.166481 | 239.328794 | 260.162501 | > | 8192 | 40.961901 | 87.067460 | 128.230351 | 178.127225 | 198.479068 | > 225.806211 | 266.170239 | 325.967840 | And here are the results with the change in this PR, on the same machine under Windows 10: || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 | |---|---|---|---|---|---|---|---|---| | 1024 | 6.957774 | 10.461498 | 14.721024 | 19.279638 | 47.508266 | 56.585089 | 64.522117 | 53.448326 | | 2048 | 10.990480 | 19.284461 | 28.235822 | 41.067555 | 92.818088 | 106.782334 | 120.907050 | 112.852554 | | 3072 | 15.642648 | 28.502151 | 59.541998 | 55.663658 | 130.654226 | 149.805330 | 205.212356 | 169.002232 | | 4096 | 19.882252 | 41.287722 | 59.493687 | 73.809264 | 169.212467 | 200.212097 | 247.934609 | 246.412543 | | 5120 | 49.986204 | 97.986781 | 126.127089 | 167.274104 | 217.063815 | 276.812929 | 307.276073 | 366.800463 | | 6144 | 66.546156 | 104.339809 | 150.171765 | 206.282107 | 272.486419 | 321.178983 | 365.822047 | 410.087087 | | 7168 | 66.894654 | 119.510866 | 176.002883 | 248.937222 | 314.721516 | 380.834398 | 430.858648 | 482.499047 | | 8192 | 67.040207 | 112.831977 | 161.852173 | 237.588749 | 319.667719 | 382.151556 | 437.810832 | 451.865266 | - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Fri, 24 Jan 2020 15:55:50 GMT, Frederic Thevenet wrote: >> I haven't done any testing yet, but I have two comments on the patch: >> >> 1. Using the clamped texture size as the upper limit is the right thing to >> do, but `Prism.maxTexture` isn't guaranteed to be that size. The >> `Prism.maxTexture` constant represents the value to clamp the texture size >> to. In the event that D3D or OpenGL were to report a maximum less than the >> value of `Prism.maxTexture` (unlikely, but maybe on some low-end embedded >> systems?), then that value is what should be used. The way to get the >> clamped texture size is to call >> [`ResourceFactory::getMaximumTextureSize`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/prism/ResourceFactory.java#L222), >> but you can't do that directly from the scene graph code. >> >> 2. Allocating multiple `WritableImage` objects and using >> `PixelWriter::setPixels` to stitch them together will take more temporary >> memory, and is likely to be slower, than having the snapshot code write into >> a subimage of the larger allocated image directly. >> >> Having said that, the current proposed solution is still better than the >> current behavior is almost all cases, although there could be a performance >> hit in the case of an 8K x 8K image, which will now be tiled. I want to do a >> more careful review and some testing. If any other users of snapshot have >> any comments or concerns, they would be welcome. >> >> I think the best long-term solution might be to modify the >> [`QuantumToolkit::renderToImage`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java#L1490) >> method, although that would certainly be out of scope for JavaFX 14. > > I've put together a small benchmark to measure the time it takes to snapshots > into images of sizes varying from 1024*1024 to 8192*8192, by increment of > 1024 in each dimension, which you can find here: > https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java Here are the results when running JavaFX 14-ea+7. The columns of the table correspond the width of the target snapshot, while the rows correspond to the height and the content of the cells is the average time* spent (in ms) in `Node::snapshot` (*) each test is ran 10 times and the elapsed time is averaged after pruning outliers, using Grubbs' test. || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 | |---|---|---|---|---|---|---|---|---| | 1024 | 6.304272 | 10.303935 | 15.052336 | 35.929304 | 23.860095 | 28.828812 | 35.315288 | 27.867205 | | 2048 | 11.544367 | 21.156326 | 28.368750 | 28.887164 | 47.134738 | 54.354708 | 55.480251 | 56.722649 | | 3072 | 15.503187 | 30.215269 | 41.304645 | 39.789648 | 82.255091 | 82.576379 | 96.618722 | 106.586547 | | 4096 | 20.928336 | 38.768648 | 64.255423 | 52.608217 | 101.797347 | 132.516816 | 158.525192 | 166.872889 | | 5120 | 28.693431 | 67.275306 | 68.090280 | 76.208412 | 133.974510 | 157.120373 | 182.329784 | 210.069066 | | 6144 | 29.972591 | 54.751002 | 88.171906 | 104.489291 | 147.788597 | 185.185643 | 213.562819 | 228.643761 | | 7168 | 33.668398 | 63.088490 | 98.756212 | 130.502678 | 196.367121 | 225.166481 | 239.328794 | 260.162501 | | 8192 | 40.961901 | 87.067460 | 128.230351 | 178.127225 | 198.479068 | 225.806211 | 266.170239 | 325.967840 | - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Tue, 21 Jan 2020 21:53:29 GMT, Kevin Rushforth wrote: >>> >>> >>> Looks good to me. >>> Below is just an observation about time taken by the API, >>> Platform: Windows10, `maxTextureSize`: 4096 >>> For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` >>> takes ~100 ms, and each call to `setPixels()` takes ~30 ms. >>> >>> Please wait for one more approval before integrating. >> >> Do you have the same kind of measurements for similar uses cases without >> this patch? I'm expecting performances to remain the same for snapshots less >> than `maxTextureSize*maxTextureSize` in size, since there is no extra pixel >> copy in that case, and the rest of the code if globally unchanged. >> >> Still there exists a window for performance regressions, which for instance >> on Windows 10 w/ the D3D rendering pipeline would be when one of the >> dimension of a snapshot is >4096 and <8192: in that case a snapshot would >> require up to 4 extra copy pixels steps with this patch. >> This is due to the fact that the old code would accept to render in a >> texture of a size up to the non-clamped max texture size (8192 in D3D, 16384 >> in ES2), but because I couldn't find a clean way to access the non clamped >> maxTextureSize exposed by the render factory from the Scene class, I settled >> for Prsim.maxTextureSize, which is the clamped value (4096 by default). >> I haven't dug deep enough in the code to understand precisely why the max >> texture size is clamped to 4096 by default, but assuming that it is for a >> good reason wouldn't that also apply in that case? Or is it always safe to >> use the non-clamped value in that particular case? > > I haven't done any testing yet, but I have two comments on the patch: > > 1. Using the clamped texture size as the upper limit is the right thing to > do, but `Prism.maxTexture` isn't guaranteed to be that size. The > `Prism.maxTexture` constant represents the value to clamp the texture size > to. In the event that D3D or OpenGL were to report a maximum less than the > value of `Prism.maxTexture` (unlikely, but maybe on some low-end embedded > systems?), then that value is what should be used. The way to get the clamped > texture size is to call > [`ResourceFactory::getMaximumTextureSize`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/prism/ResourceFactory.java#L222), > but you can't do that directly from the scene graph code. > > 2. Allocating multiple `WritableImage` objects and using > `PixelWriter::setPixels` to stitch them together will take more temporary > memory, and is likely to be slower, than having the snapshot code write into > a subimage of the larger allocated image directly. > > Having said that, the current proposed solution is still better than the > current behavior is almost all cases, although there could be a performance > hit in the case of an 8K x 8K image, which will now be tiled. I want to do a > more careful review and some testing. If any other users of snapshot have any > comments or concerns, they would be welcome. > > I think the best long-term solution might be to modify the > [`QuantumToolkit::renderToImage`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java#L1490) > method, although that would certainly be out of scope for JavaFX 14. I've put together a small benchmark to measure the time it takes to snapshots into images of sizes varying from 1024*1024 to 8192*8192, by increment of 1024 in each dimension, which you can find here: https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Mon, 20 Jan 2020 11:24:05 GMT, Frederic Thevenet wrote: >> Looks good to me. >> Below is just an observation about time taken by the API, >> Platform: Windows10, `maxTextureSize`: 4096 >> For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` >> takes ~100 ms, and each call to `setPixels()` takes ~30 ms. >> >> Please wait for one more approval before integrating. > >> >> >> Looks good to me. >> Below is just an observation about time taken by the API, >> Platform: Windows10, `maxTextureSize`: 4096 >> For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` >> takes ~100 ms, and each call to `setPixels()` takes ~30 ms. >> >> Please wait for one more approval before integrating. > > Do you have the same kind of measurements for similar uses cases without this > patch? I'm expecting performances to remain the same for snapshots less than > `maxTextureSize*maxTextureSize` in size, since there is no extra pixel copy > in that case, and the rest of the code if globally unchanged. > > Still there exists a window for performance regressions, which for instance > on Windows 10 w/ the D3D rendering pipeline would be when one of the > dimension of a snapshot is >4096 and <8192: in that case a snapshot would > require up to 4 extra copy pixels steps with this patch. > This is due to the fact that the old code would accept to render in a texture > of a size up to the non-clamped max texture size (8192 in D3D, 16384 in ES2), > but because I couldn't find a clean way to access the non clamped > maxTextureSize exposed by the render factory from the Scene class, I settled > for Prsim.maxTextureSize, which is the clamped value (4096 by default). > I haven't dug deep enough in the code to understand precisely why the max > texture size is clamped to 4096 by default, but assuming that it is for a > good reason wouldn't that also apply in that case? Or is it always safe to > use the non-clamped value in that particular case? I haven't done any testing yet, but I have two comments on the patch: 1. Using the clamped texture size as the upper limit is the right thing to do, but `Prism.maxTexture` isn't guaranteed to be that size. The `Prism.maxTexture` constant represents the value to clamp the texture size to. In the event that D3D or OpenGL were to report a maximum less than the value of `Prism.maxTexture` (unlikely, but maybe on some low-end embedded systems?), then that value is what should be used. The way to get the clamped texture size is to call [`ResourceFactory::getMaximumTextureSize`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/prism/ResourceFactory.java#L222), but you can't do that directly from the scene graph code. 2. Allocating multiple `WritableImage` objects and using `PixelWriter::setPixels` to stitch them together will take more temporary memory, and is likely to be slower, than having the snapshot code write into a subimage of the larger allocated image directly. Having said that, the current proposed solution is still better than the current behavior is almost all cases, although there could be a performance hit in the case of an 8K x 8K image, which will now be tiled. I want to do a more careful review and some testing. If any other users of snapshot have any comments or concerns, they would be welcome. I think the best long-term solution might be to modify the [`QuantumToolkit::renderToImage`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java#L1490) method, although that would certainly be out of scope for JavaFX 14. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Mon, 20 Jan 2020 05:06:50 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 2 additional commits. > > Looks good to me. > Below is just an observation about time taken by the API, > Platform: Windows10, `maxTextureSize`: 4096 > For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` takes > ~100 ms, and each call to `setPixels()` takes ~30 ms. > > Please wait for one more approval before integrating. > > > Looks good to me. > Below is just an observation about time taken by the API, > Platform: Windows10, `maxTextureSize`: 4096 > For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` takes > ~100 ms, and each call to `setPixels()` takes ~30 ms. > > Please wait for one more approval before integrating. Do you have the same kind of measurements for similar uses cases without this patch? I'm expecting performances to remain the same for snapshots less than `maxTextureSize*maxTextureSize` in size, since there is no extra pixel copy in that case, and the rest of the code if globally unchanged. Still there exists a window for performance regressions, which for instance on Windows 10 w/ the D3D rendering pipeline would be when one of the dimension of a snapshot is >4096 and <8192: in that case a snapshot would require up to 4 extra copy pixels steps with this patch. This is due to the fact that the old code would accept to render in a texture of a size up to the non-clamped max texture size (8192 in D3D, 16384 in ES2), but because I couldn't find a clean way to access the non clamped maxTextureSize exposed by the render factory from the Scene class, I settled for Prsim.maxTextureSize, which is the clamped value (4096 by default). I haven't dug deep enough in the code to understand precisely why the max texture size is clamped to 4096 by default, but assuming that it is for a good reason wouldn't that also apply in that case? Or is it always safe to use the non-clamped value in that particular case? - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
On Mon, 20 Jan 2020 05:07:11 GMT, Frederic Thevenet 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. Looks good to me. Below is just an observation about time taken by the API, Platform: `Windows10`, `maxTextureSize`: 4096 For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` takes ~100 ms, and each call to `setPixels()` takes ~30 ms. Please wait for one more approval before integrating. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
> 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. - Added commits: - 8966936f: Wrapped long line - 8e44dea2: Refactored height & weight calculation to avoid unnecessary computations Changes: - all: https://git.openjdk.java.net/jfx/pull/68/files - new: https://git.openjdk.java.net/jfx/pull/68/files/50191728..8966936f Webrevs: - full: https://webrevs.openjdk.java.net/jfx/68/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/68/webrev.02-03 Stats: 10 lines in 1 file changed: 5 ins; 2 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/68.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/68/head:pull/68 PR: https://git.openjdk.java.net/jfx/pull/68
