On Mon, 9 Mar 2020 10:01:53 GMT, Frederic Thevenet
<github.com+7450507+ftheve...@openjdk.org> wrote:
>> Issue JDK-8088198, where an exception would be thrown when trying to capture
>> a snapshot whose final dimensions would be
>> larger than the running platform's maximum supported texture size, was
>> addressed in openjfx14. The fix, based around
>> the idea of capturing as many tiles of the maximum possible size and
>> re-compositing the final snapshot out of these, is
>> currently only attempted after the original, non-tiled, strategy has already
>> failed. This was decided to avoid any risk
>> of regressions, either in terms of performances and correctness, while still
>> offering some relief to the original
>> issue. This follow-on issue aims to propose a fix to the original issue,
>> that is able to correctly decide on the best
>> snapshot strategy (tiled or not) to adopt before applying it and ensure best
>> performances possible when tiling is
>> necessary while still introducing no regressions compared to the original
>> solution.
>
> Frederic Thevenet has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Avoid useless width and height calculation
The change seems good to me. Suggested few minor corrections and a query.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1493:
> 1492: }
> 1493: else {
> 1494: rt.readPixels(buffer, rt.getContentX(),
> rt.getContentY(), w, h);
Please correct the `if` `else` formatting here and other places.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1527:
> 1526: private int computeOptimumTileSize(int size, int maxSize,
> AtomicBoolean isDivExact) {
> 1527: // This methods attempts to find the smallest integer
> divider for the provided `size`
> 1528: // while the result of the division is less than
> `maxSize`.
This **method** attempts to....
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1575:
> 1574: }
> 1575: // Find out it is is possible to divide up the
> image in tiles of the same size
> 1576: AtomicBoolean exactWidthDivFound = new
> AtomicBoolean(false);
Find out **if it** is possible...
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1600:
> 1599: }
> 1600: }
> 1601: }
The `if (exactHeightDivFound.get())` and `else` block here look effectively
same. Could you please take a re-look or
explain what am I missing here.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1576:
> 1575: // Find out it is is possible to divide up the
> image in tiles of the same size
> 1576: AtomicBoolean exactWidthDivFound = new
> AtomicBoolean(false);
> 1577: int tileWidth = computeOptimumTileSize(w,
> maxTextureSize, exactWidthDivFound);
`exactWidthDivFound` is unused, may be can skip creating a named variable and
just pass a unnamed `AtomicBoolean` in
next call to `computeOptimumTileSize`
-------------
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/112
