On Tue, 17 Mar 2020 11:43:16 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:
>
> Revert changes in import statements
Overall, this looks quite good. In particular the tiled rendering, as
implemented by the `renderTile` method, should be
reasonably efficient.
My only high-level comment is that I'm somewhat skeptical of
`computeOptimumTileSize` to determine the size and
direction of tiling. I note that in the case of an image that is tiled in both
X and Y, there are at most 4 distinct
tile sizes if it doesn't fit evenly. In the case where only one of X or Y is
tiled, there are at most 2 distinct tile
sizes. Here is an example:
+-----------+-----------+ . +-------+
| | | . | |
| M | M | . | R |
| | | . | |
+-----------+-----------+ . +-------+
| | | . | |
| M | M | . | R |
| | | . | |
+-----------+-----------+ . +-------+
. . . .
+-----------+-----------+ . +-------+
| | | . | |
| M | M | . | R |
| | | . | |
+-----------+-----------+ . +-------+
| B | B | . | C |
+-----------+-----------+ . +-------+
Where `M` represents the middle set of tiles each with a size of `tileW x
tileH`. `R` is the right hand column of
tiles, `B` is bottom row, and `C` is corner.
Recognizing this, I wonder if it might be better to always use the maximum tile
size, but fill all of the middle tiles
of that size first, and then pick up the right and/or bottom edges as needed.
This will minimize thrashing (no more
than 3 changes of tile size), while avoiding the more complicated logic that
tries to keep the tiles all the same size
at the cost of smaller tiles, and which has to fall back to using uneven tiles
anyway. If you do it this way, there is
also no need to have code that switches the order of the inner loop. It will
naturally handle that.
Either way, I'd like to see some additional system tests that cover all of the
cases of X and Y fitting/not-fitting
exactly (and if you stick with your current approach, X or Y as the inner loop).
I left a couple inline comments as well.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1495:
> 1494: }
> 1495: //Copy tile's pixel into the target image
> 1496: targetImg.image.setPixels(xOffset, yOffset, w, h,
Typo: should be "pixels" (plural)
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1523:
> 1522:
> 1523: private int computeOptimumTileSize(int size, int maxSize){
> 1524: return computeOptimumTileSize(size, maxSize, null);
Minor: add a space before the `{` , although this will become a moot point if
you take my suggestion of eliminating
this method entirely.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1527:
> 1526:
> 1527: private int computeOptimumTileSize(int size, int maxSize,
> AtomicBoolean isDivExact) {
> 1528: // This method attempts to find the smallest exact
> divider for the provided `size`
I don't care for the use of `AtomicBoolean` to effect a pass-by-reference. You
aren't relying on the atomic nature, so
it just ends up being confusing. I recommend using `boolean[]`, although this
will become a moot point if you take my
suggestion of eliminating this method entirely.
-------------
PR: https://git.openjdk.java.net/jfx/pull/112
