On Thu, 14 May 2020 12:48:30 GMT, Frederic Thevenet <github.com+7450507+ftheve...@openjdk.org> wrote:
>> 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) > >> >> >> 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. > > I'll need to think about this a bit more, but maybe a good approach could be > to generally adopt your solution, but > still attempt to see if any of the snapshot's dimension can be divided > equally by 2 or 3 (while being less than > maxTextureSize) , and use that a a tile size. As the number of tiles > increases, it become less important to have same > sized tiles as you demonstrated so using maxTextureSize should be fine. This > way we get rid of the inner loop > direction logic (which I agree is verbose and kind of confusing), and still > have a chance to optimize the case where > tiled snapshots are made up of 4 tiles or less (which I see as being the most > frequent use case, in my experience > anyway). That sounds like a promising approach. ------------- PR: https://git.openjdk.java.net/jfx/pull/112
