Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Mon, 15 Jun 2020 15:55:25 GMT, Frederic Thevenet wrote: >> 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'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). > > What kind of tests do you have in mind? More specifically do you mean simply > adding tests that expand on the existing > `doTestSnapshotScaleNodeDefer`and `doTestSnapshotScaleNodeImm` (which > basically just prove that taking a snapshot > returns a non-null image of the expected size)? Or do you think we need to > include a test that proves the snapshot > produced by tiling is entirely faithful to the original, pixel-wise? I went ahead and wrote a bunch of tests that: 1. Setup a scene to display an `ImageView` of a selected dimensions chosen to trigger tiling in different ways when taking snapshots. 2. Fill up the image with noise. 3. Take a snapshot and do a pixel-wise comparison with the original image. I've added the new tests to the existing `Snapshot2Test.java`. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Sat, 9 May 2020 17:43:08 GMT, Kevin Rushforth wrote: > [...] 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). What kind of tests do you have in mind? More specifically do you mean simply adding tests that expand on the existing `doTestSnapshotScaleNodeDefer`and `doTestSnapshotScaleNodeImm` (which basically just prove that taking a snapshot returns a non-null image of the expected size)? Or do you think we need to include a test that proves the snapshot produced by tiling is entirely faithful to the original, pixel-wise? - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Sat, 16 May 2020 14:20:10 GMT, Kevin Rushforth wrote: >>> >>> >>> 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. I have re-written the way we walk through tiles as per the discussion above. It isn't really more concise than the previous iteration but it is probably more straight forward and less surprising. It also handles more cases with the least amount of tile resizing than the previous code, so I think it is a clear improvement. What I'm not terribly happy with is the variable names: I must admit I quickly ran out of idea for naming the various instances of tiles sizes and offsets, so I reused the "M, R, B, C" terminology used above. However I'm worried it might seem a bit obscure when taken out of the context of this discussion, so if anyone have better names to suggest, I'd be happy to change these. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Thu, 14 May 2020 12:48:30 GMT, Frederic Thevenet 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
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Sat, 9 May 2020 17:28:52 GMT, Kevin Rushforth wrote: >> Frederic Thevenet has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert changes in import statements > > 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). - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Mon, 11 May 2020 15:30:28 GMT, Nir Lisker wrote:
>> Frederic Thevenet has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Revert changes in import statements
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
> line 1483:
>
>> 1482: IntBuffer buffer, ResourceFactory
>> rf, QuantumImage tileImg, QuantumImage
>> targetImg) { 1483: com.sun.prism.RTTexture rt =
>> tileImg.getRT(w, h, rf);
>> 1484: if (rt == null) {
>
> Any reason why the fully qualified name is needed?
Not that I can think of, probably some unfortunate copy/paste.
I'll fix that.
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Tue, 17 Mar 2020 11:43:16 GMT, Frederic Thevenet
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
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1483:
> 1482: IntBuffer buffer, ResourceFactory
> rf, QuantumImage tileImg, QuantumImage
> targetImg) { 1483: com.sun.prism.RTTexture rt =
> tileImg.getRT(w, h, rf);
> 1484: if (rt == null) {
Any reason why the fully qualified name is needed?
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1502:
> 1501: private void renderWholeImage(int x, int y, int w, int h,
> ResourceFactory rf, QuantumImage pImage) {
> 1502: com.sun.prism.RTTexture rt = pImage.getRT(w, h, rf);
> 1503: if (rt == null) {
Same questions for `RTTexture`
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
On Tue, 17 Mar 2020 11:43:16 GMT, Frederic Thevenet
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
Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/50ac8c22..7f5f2890 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.04 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.03-04 Stats: 112 lines in 1 file changed: 79 ins; 28 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/112.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/112/head:pull/112 PR: https://git.openjdk.java.net/jfx/pull/112
