Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties

2021-09-06 Thread Frederic Thevenet
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote: > This PR fixes a bug that was introduced in #454. > > Since this fix might impact the performance considerations in the original > PR, I ran a performance benchmark against the previous `ChangeListener`-based > implementation, which

Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties

2021-09-06 Thread Frederic Thevenet
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote: > This PR fixes a bug that was introduced in #454. > > Since this fix might impact the performance considerations in the original > PR, I ran a performance benchmark against the previous `ChangeListener`-based > implementation, which

Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties

2021-09-06 Thread Frederic Thevenet
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote: > This PR fixes a bug that was introduced in #454. > > Since this fix might impact the performance considerations in the original > PR, I ran a performance benchmark against the previous `ChangeListener`-based > implementation, which

Re-examine the risks of JDK-8264770 breaking third party libraries and applications.

2021-08-26 Thread Frederic Thevenet
Hi, A change was introduced In JDK-8264770 that swaps the use of ChangeListeners to InvalidationListeners within the internal implementation of BidirectionalBinding [1]. While this change shouldn't normally affect third party applications, it turned out to break the scrolling facilities

Re: RFR: 8259680: Need API to query states of CAPS LOCK and NUM LOCK keys

2021-01-24 Thread Frederic Thevenet
On Sun, 24 Jan 2021 18:09:03 GMT, Nir Lisker wrote: > Formally, OpenJFX N supports JDK N-1, so for 17 we should be able to use > features from 16. It was decided a while back not to force-fail on older JDK > versions and let it evolve naturally. That's the bit of information I was missing,

Re: RFR: 8259680: Need API to query states of CAPS LOCK and NUM LOCK keys

2021-01-24 Thread Frederic Thevenet
On Sat, 23 Jan 2021 23:11:10 GMT, Nir Lisker wrote: >> The JavaFX API does not provide a way to get the state of CAPS LOCK or NUM >> LOCK on the keyboard. Being able to read the lock state would allow an >> application to inform the user that caps lock was enabled for passwords or >> other

Re: New API to read Caps-Lock and Num-Lock state

2021-01-17 Thread Frederic Thevenet
ice) On Mon, 18 Jan 2021, 12:29 am Frederic Thevenet, mailto:thevenet.f...@free.fr>> wrote: Hi,  From the perspective of the application developer, I think throwing a runtime exception if the key does not exists on a given platform is potentially risky, as the API provided

Re: New API to read Caps-Lock and Num-Lock state

2021-01-17 Thread Frederic Thevenet
Hi, From the perspective of the application developer, I think throwing a runtime exception if the key does not exists on a given platform is potentially risky, as the API provided no hints that a given key might not exists an other platforms, and this oversight will only manifest itself at

Integrated: 8211294: ScrollPane content is blurry with 125% scaling

2021-01-07 Thread Frederic Thevenet
On Thu, 24 Sep 2020 18:57:13 GMT, Frederic Thevenet wrote: > This PR aims to fix the blurriness to sometimes affects some controls (such > as TextArea) in a scene when rendered with a scaling factor that is not an > integer (typically when viewed on a HiDPI screen with a 125%, 15

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v6]

2021-01-05 Thread Frederic Thevenet
On Tue, 5 Jan 2021 06:32:25 GMT, Ambarish Rapte wrote: >>> >>> >>> Apologies for delay on this review, Change looks good but I did observe an >>> issue with layout of TabPane header area when the header is set to be shown >>> on Left or Right side. The last tab-header's border seems to be

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v6]

2021-01-04 Thread Frederic Thevenet
On Mon, 4 Jan 2021 17:04:41 GMT, Ambarish Rapte wrote: > > > Apologies for delay on this review, Change looks good but I did observe an > issue with layout of TabPane header area when the header is set to be shown > on Left or Right side. The last tab-header's border seems to be clipped. >

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v6]

2020-12-23 Thread Frederic Thevenet
endering glitches, though I'm not yet familiar enough with the code to > see if it is really the case. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: Addressed comments from review - Changes: - all: https://

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v5]

2020-12-21 Thread Frederic Thevenet
endering glitches, though I'm not yet familiar enough with the code to > see if it is really the case. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: Added a test - Changes: - all: https://git.openjdk.java.ne

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-21 Thread Frederic Thevenet
On Fri, 18 Dec 2020 16:54:46 GMT, Kevin Rushforth wrote: >> So that leaves two follow-on bugs then: >> 1. Rendering a cached node doesn't match rendering it directly even when the >> transform is unchanged. >> 2. TextFlow: methods copied from Region have not picked up subsequent >> changes in

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-18 Thread Frederic Thevenet
On Fri, 18 Dec 2020 11:39:04 GMT, Frederic Thevenet wrote: >> I did some testing with your latest patch, and I don't see any problem when >> dragging a window between screens with different scales. It seems to be >> recalculating the cached insets and using them in layout

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-18 Thread Frederic Thevenet
On Thu, 17 Dec 2020 23:43:39 GMT, Kevin Rushforth wrote: >> Your latest patch looks exactly like what I would expect. I need to do some >> multi-screen testing on Windows with different scale factors. >> >>> ... the repaint for the affected Region does not seem to occur right away >>> on

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-17 Thread Frederic Thevenet
On Thu, 17 Dec 2020 00:24:49 GMT, Kevin Rushforth wrote: >> There is still a remaining scaling issue, I'm afraid: snapped insets >> coordinates are cached after they've been computed and only updated when >> the inset (or a related property, such as shape or border) changes or if >> the

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v4]

2020-12-17 Thread Frederic Thevenet
endering glitches, though I'm not yet familiar enough with the code to > see if it is really the case. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: Invalidate cached snapped inset values when screen scale has

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-16 Thread Frederic Thevenet
On Wed, 16 Dec 2020 18:14:42 GMT, Frederic Thevenet wrote: >> Region is part of the public API, so any change to increase the visibility >> of fields or methods would require a new enhancement with a CSR and >> justification as to why it is needed as public API. We wou

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v3]

2020-12-16 Thread Frederic Thevenet
endering glitches, though I'm not yet familiar enough with the code to > see if it is really the case. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: computeChildArea methods in TextFlow should take screen scaling in

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-16 Thread Frederic Thevenet
On Wed, 16 Dec 2020 17:24:10 GMT, Kevin Rushforth wrote: >>> >>> >>> Good catch. Yes, `TextFlow` has the same problem, and ought to be fixed as >>> part of this, probably by deleting that method and using the public methods >>> in Region. It seems wholly unnecessary to use the copied method,

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-16 Thread Frederic Thevenet
On Wed, 16 Dec 2020 16:39:37 GMT, Kevin Rushforth wrote: > > > Good catch. Yes, `TextFlow` has the same problem, and ought to be fixed as > part of this, probably by deleting that method and using the public methods > in Region. It seems wholly unnecessary to use the copied method, since the

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-16 Thread Frederic Thevenet
On Wed, 16 Dec 2020 13:36:27 GMT, Kevin Rushforth wrote: >> I was thinking, while I'm at it I might as well update the copyright notice >> for Region.java; should it be 2020 or 2021 (or left alone)? > > That file was modified earlier in the year without the copyright year being > updated, so

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-16 Thread Frederic Thevenet
On Wed, 16 Dec 2020 12:55:49 GMT, Frederic Thevenet wrote: >> In looking at the other instances where snap-to-pixel is done correctly for >> Insets, the above should use `snapSpace{X,Y}` rather than `snapSize{X,Y}` > > I agree that it is better to separate the scrollpane is

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling

2020-12-16 Thread Frederic Thevenet
On Wed, 16 Dec 2020 00:42:24 GMT, Kevin Rushforth wrote: >> For completeness, here is the patch for the snap-to-pixel issue: >> >> diff --git >> a/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java >> b/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java

Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v2]

2020-12-16 Thread Frederic Thevenet
endering glitches, though I'm not yet familiar enough with the code to > see if it is really the case. Frederic Thevenet has updated the pull request incrementally with two additional commits since the last revision: - Fixed region doesn't account for screen scalling when enforcing snap-to-

Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-12-14 Thread Frederic Thevenet
On Tue, 15 Dec 2020 02:02:37 GMT, Kevin Rushforth wrote: >>> >>> >>> One more comment: given the quality problems that necessarily arise when >>> the translation of a cached image is not on an integer boundary, part of >>> the solution might be to snap the cached image to a pixel boundary as

Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-12-14 Thread Frederic Thevenet
On Sat, 12 Dec 2020 22:31:56 GMT, Kevin Rushforth wrote: > > > One more comment: given the quality problems that necessarily arise when the > translation of a cached image is not on an integer boundary, part of the > solution might be to snap the cached image to a pixel boundary as is done

Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-12-14 Thread Frederic Thevenet
On Sat, 12 Dec 2020 22:31:56 GMT, Kevin Rushforth wrote: >> I spent a bit of time looking at this. I think the root cause of the problem >> is in ScrollPane itself. It is attempting to layout its children by doing a >> snap to pixel (meaning that the final scaled translation should be an >>

Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-12-05 Thread Frederic Thevenet
On Fri, 4 Dec 2020 23:41:38 GMT, Kevin Rushforth wrote: > > > I hope to take a look at this, along with other pending reviews, early next > week. There is still some time to get this into 16 if we can find a robust > fix. That's great news, thanks! - PR:

Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-12-04 Thread Frederic Thevenet
On Wed, 21 Oct 2020 12:35:34 GMT, Kevin Rushforth wrote: >> Hello, >> Did anyone get a chance to look into this? >> Thanks! > > Not yet. I took a quick look, and this is helpful in pointing out where the > problem is, but I don't know whether it is the right solution to simply round > the

Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-10-21 Thread Frederic Thevenet
On Fri, 25 Sep 2020 09:32:46 GMT, Frederic Thevenet wrote: >> The visual representation corresponds with digits, so there can be tests >> that check if the numbers are what we expect >> them to be. It's good that this is not windows-only, so that it can be >>

Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-09-25 Thread Frederic Thevenet
On Fri, 25 Sep 2020 07:47:41 GMT, Johan Vos wrote: > > > The visual representation corresponds with digits, so there can be tests that > check if the numbers are what we expect > them to be. It's good that this is not windows-only, so that it can be > tackled on linux as well. But what is

Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-09-24 Thread Frederic Thevenet
On Thu, 24 Sep 2020 18:57:13 GMT, Frederic Thevenet wrote: > This PR aims to fix the blurriness to sometimes affects some controls (such > as TextArea) in a scene when rendered with > a scaling factor that is not an integer (typically when viewed on a HiDPI > screen with a 125%,

RFR: 8211294: [windows] TextArea content is blurry with 125% scaling

2020-09-24 Thread Frederic Thevenet
This PR aims to fix the blurriness to sometimes affects some controls (such as TextArea) in a scene when rendered with a scaling factor that is not an integer (typically when viewed on a HiDPI screen with a 125%, 150% or 175% output scaling). Please note that regardless of what the JBS issue

Integrated: 8238954: Improve performance of tiled snapshot rendering

2020-07-01 Thread Frederic Thevenet
On Wed, 12 Feb 2020 13:21:03 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 openjf

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v16]

2020-07-01 Thread Frederic Thevenet
ing is > necessary while still introducing no regressions compared to the original > solution. Frederic Thevenet has updated the pull request incrementally with two additional commits since the last revision: - Mark variables as final - Using for loops instead of while - Ch

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v15]

2020-07-01 Thread Frederic Thevenet
On Wed, 1 Jul 2020 13:42:12 GMT, Ambarish Rapte wrote: >> Frederic Thevenet has updated the pull request incrementally with one >> additional commit since the last revision: >> >> - Removed unused imports in Snapshot2Test >> - Fixed comments in QuantumTool

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v15]

2020-07-01 Thread Frederic Thevenet
ing 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: - Removed unused imports in Snapshot2Test - Fixed comments in Quantu

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v14]

2020-07-01 Thread Frederic Thevenet
On Tue, 30 Jun 2020 21:22:14 GMT, Ambarish Rapte wrote: >> Frederic Thevenet has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Prevent attempt to render tiles with a 0 sized dimension. > > modules/javafx.

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v14]

2020-06-30 Thread Frederic Thevenet
ressions compared to the original > solution. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: Prevent attempt to render tiles with a 0 sized dimension. - Changes: - all: https://git.openjdk.java.net/jfx/pull/112

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]

2020-06-30 Thread Frederic Thevenet
On Mon, 29 Jun 2020 22:46:26 GMT, Kevin Rushforth wrote: >> I think I found the problem in the tiling logic that leads to the macOS >> failures. You need to check that the remainder >> width or height is > 0. Also, it looks like you have the "B" and "R" loops >> backwards, which is a bit

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]

2020-06-30 Thread Frederic Thevenet
On Mon, 29 Jun 2020 21:32:33 GMT, Kevin Rushforth wrote: >> Frederic Thevenet has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fill test image with a bilinear gradient instead of random noise. > > modules

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]

2020-06-29 Thread Frederic Thevenet
On Mon, 29 Jun 2020 16:16:58 GMT, Frederic Thevenet wrote: >> Thanks for your review. >> >> I don't have access to a Mac so I can't check that directly. >> The tests pass on both my Windows 10 and Ubuntu 20.04 environments and the >> stack isn't very h

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]

2020-06-29 Thread Frederic Thevenet
On Mon, 29 Jun 2020 14:35:00 GMT, Frederic Thevenet wrote: >> I observed that the added tests are failing on mac machine(Mojave 10.14.6), >> but they do pass on windows10. Can you >> please verify ? Timeout and Unrecognized Image loader errors from the log, >> test.ja

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]

2020-06-29 Thread Frederic Thevenet
ing 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: Fill test image with a bilinear gradient instead of random noise. - Ch

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]

2020-06-29 Thread Frederic Thevenet
On Mon, 29 Jun 2020 14:02:11 GMT, Ambarish Rapte wrote: >> 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

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v12]

2020-06-19 Thread Frederic Thevenet
ing 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: Crawling pixels in writableImage with parallel stream is a bad idea. -

Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v11]

2020-06-17 Thread Frederic Thevenet
ing 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: Changed wrong value for image size (for these tests we *don't* want it to be divida

Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-06-17 Thread Frederic Thevenet
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 tha

Re: [Rev 09] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-06-17 Thread Frederic Thevenet
ing 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: Removed trailing whitespaces - Changes: - all: https://git.op

Re: [Rev 08] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-06-17 Thread Frederic Thevenet
ing 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: Added tiled snapshots tests - Changes: - all: https://git.op

Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-06-15 Thread Frederic Thevenet
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?

Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-06-13 Thread Frederic Thevenet
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 >>>

Re: [Rev 07] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-06-12 Thread Frederic Thevenet
ing 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: Changed tile walking logic - Changes: - all: https://git.op

Re: [Rev 06] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-06-12 Thread Frederic Thevenet
ing 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 to import statements - Changes: - all: https://git.op

Re: [Rev 05] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-06-12 Thread Frederic Thevenet
ing is > necessary while still introducing no regressions compared to the original > solution. Frederic Thevenet has updated the pull request incrementally with three additional commits since the last revision: - Use boolean[] instead of AtomicBoolean to effec pass-by-reference - Fixed typo

Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-05-14 Thread Frederic Thevenet
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/

Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-05-14 Thread Frederic Thevenet
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/

Re: [Rev 03] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-04-15 Thread Frederic Thevenet
On Tue, 17 Mar 2020 15:31:20 GMT, Frederic Thevenet wrote: >>> >>> >>> Now that the tiling is done in the `QuantumRenderer` level, I'll bring back >>> [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082). Can't this >>> tiling be used

Re: [Rev 03] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-17 Thread Frederic Thevenet
On Tue, 17 Mar 2020 14:19:45 GMT, Frederic Thevenet wrote: >> Now that the tiling is done in the `QuantumRenderer` level, I'll bring back >> [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082). Can't this >> tiling be used to fix that? > >> >>

Re: [Rev 03] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-17 Thread Frederic Thevenet
On Tue, 17 Mar 2020 13:28:57 GMT, Nir Lisker wrote: > > > Now that the tiling is done in the `QuantumRenderer` level, I'll bring back > [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082). Can't this > tiling be used to fix that? It won't help with thing in their current state,

Re: [Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-17 Thread Frederic Thevenet
ing 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.op

Re: [Rev 02] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-14 Thread Frederic Thevenet
On Fri, 13 Mar 2020 17:24:02 GMT, Ambarish Rapte wrote: >> Frederic Thevenet has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid useless width and height calculation > > modules/javafx.graphics/src/main/

Re: [Rev 02] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-14 Thread Frederic Thevenet
On Fri, 13 Mar 2020 17:26:35 GMT, Ambarish Rapte wrote: >> Frederic Thevenet has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid useless width and height calculation > > modules/javafx.graphics/src/main/

Re: [Rev 03] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-14 Thread Frederic Thevenet
ing 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: Fixed code style and typo following review. - Changes: - all:

Re: RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-10 Thread Frederic Thevenet
On Tue, 10 Mar 2020 11:13:54 GMT, Frederic Thevenet wrote: >> ### 15-internal: >> >> >> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | >> |---|---|---|---|---|---|---|---|---|---| >> | 1024 | 5.381051 | 9.261115 | 14.033219

Re: RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-10 Thread Frederic Thevenet
On Tue, 10 Mar 2020 10:29:38 GMT, Frederic Thevenet wrote: >> ### 14-internal >> >> >> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | >> |---|---|---|---|---|---|---|---|---|---| >> | 1024 | 5.740508 | 9.337537 | 13.489849

Re: RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-10 Thread Frederic Thevenet
On Tue, 10 Mar 2020 10:29:31 GMT, Frederic Thevenet wrote: >> ### 14-ea+9 >> >> >> || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | >> |---|---|---|---|---|---|---|---|---|---| >> | 1024 | 5.607827 | 9.380503 | 13.835523 | 17.514362 | 2

Re: RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-10 Thread Frederic Thevenet
On Tue, 10 Mar 2020 10:29:11 GMT, Frederic Thevenet wrote: >> On My windows10 machine, I can observe 20 to 30 % reduction in time to take >> snapshot. >> Can you please capture the time the way you did >> [here](https://github.com/openjdk/jfx/pull/68#issuecomment-57819

Re: RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-10 Thread Frederic Thevenet
On Tue, 10 Mar 2020 06:05:53 GMT, Ambarish Rapte wrote: >> I finally got a chance to do some more extensive testing when running this >> patch with the es2 pipeline on Linux. >> It works as expected, and from what I saw, using a IntARGB pixelBuffer when >> no WritableImage is provided avoids

Re: [Rev 01] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-09 Thread Frederic Thevenet
On Sun, 8 Mar 2020 12:47:52 GMT, Nir Lisker wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1296: > >> 1295: int width = Math.max(xMax - xMin, 1); >> 1296: int height = Math.max(yMax -

Re: [Rev 02] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-09 Thread Frederic Thevenet
> 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

Re: [Rev 01] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-06 Thread Frederic Thevenet
> 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

Re: [Rev 01] RFR: 8238954: Improve performance of tiled snapshot rendering

2020-03-06 Thread Frederic Thevenet
On Fri, 6 Mar 2020 08:28:34 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java > line 1527: > >> 1526: private int computeOptimumTileSize(int size, int

Re: RFR: 8238954: Improve performance of tiled snapshot rendering

2020-02-28 Thread Frederic Thevenet
On Wed, 12 Feb 2020 14:57:33 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 ope

RFR: 8238954: Improve performance of tiled snapshot rendering

2020-02-28 Thread Frederic Thevenet
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

Re: RFR: 8238954: Improve performance of tiled snapshot rendering

2020-02-28 Thread Frederic Thevenet
On Wed, 12 Feb 2020 13:21:03 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.

Re: [Rev 04] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-31 Thread Frederic Thevenet
On Fri, 31 Jan 2020 13:01:51 GMT, Kevin Rushforth wrote: >> Since @kevinrushforth and @arapte have completed their review, is this ready >> to integrate? >> I'm a little confused by the fact this has both `rfr` and `ready` labels >> attached; is this an expected behaviour? > > Yes, this is

Re: [Rev 04] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-31 Thread Frederic Thevenet
On Thu, 30 Jan 2020 08:15:39 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 1 additional commit. > > With my basic testing, the change looks good, scaled up and scaled down > snapshots seem correct visually. > > After this change the tiled snapshot will be limited by

Re: [Rev 04] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-29 Thread Frederic Thevenet
On Wed, 29 Jan 2020 16:33:16 GMT, Nir Lisker wrote: >> The code changes look good to me. As long as you are willing to address the >> follow-on issues for openjfx15, I'll approve this PR for openjfx14, once I >> run my last test. >> >> I did a fair bit of testing of the functionality, which

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-28 Thread Frederic Thevenet
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

Re: [Rev 04] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-28 Thread Frederic Thevenet
> 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

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-27 Thread Frederic Thevenet
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

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-26 Thread Frederic Thevenet
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`

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-26 Thread Frederic Thevenet
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

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-24 Thread Frederic Thevenet
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 t

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-24 Thread Frederic Thevenet
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 |

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-24 Thread Frederic Thevenet
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: >

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-24 Thread Frederic Thevenet
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 gu

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-24 Thread Frederic Thevenet
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,

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-20 Thread Frederic Thevenet
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

Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-17 Thread Frederic Thevenet
On Fri, 17 Jan 2020 11:28:03 GMT, Frederic Thevenet wrote: >> I tested this fix against the repro code in >> https://github.com/javafxports/openjdk-jfx/issues/433 (which is >> [JDK-838](https://bugs.openjdk.java.net/browse/JDK-838)), but there >> is still

Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-17 Thread Frederic Thevenet
On Thu, 16 Jan 2020 16:08:05 GMT, Nir Lisker wrote: >> The pull request has been updated with 2 additional commits. > > modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1316: > >> 1315: } >> 1316: } >> 1317: } else { > > I would extract

Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-17 Thread Frederic Thevenet
> 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

Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-17 Thread Frederic Thevenet
On Thu, 16 Jan 2020 17:00:32 GMT, Nir Lisker wrote: >> The pull request has been updated with 2 additional commits. > > I tested this fix against the repro code in > https://github.com/javafxports/openjdk-jfx/issues/433 (which is >

Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-16 Thread Frederic Thevenet
On Thu, 16 Jan 2020 14:26:24 GMT, Kevin Rushforth wrote: >> I think it is not worth it. > > I agree. I do like the suggestion to rename the variables, though. done in 501917284e0490d16b1831fcd854e31a779449b9 - PR: https://git.openjdk.java.net/jfx/pull/68

Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-16 Thread Frederic Thevenet
On Thu, 16 Jan 2020 10:47:26 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1303: > >> 1302: if (height > maxTextureSize || width > maxTextureSize) { >> 1303:

Re: [Rev 01] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-16 Thread Frederic Thevenet
On Thu, 16 Jan 2020 10:58:12 GMT, Ambarish Rapte wrote: >> The pull request has been updated with 1 additional commit. > > modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 1312: > >> 1311: int tileWidth = Math.min(maxTextureSize, width - >> xOffset); >>

Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

2020-01-16 Thread Frederic Thevenet
> 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

  1   2   >