Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
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 still shows better performance for the new
> implementation:
>
>
> @State(Scope.Benchmark)
> public class BindingBenchmark {
> DoubleProperty property1 = new SimpleDoubleProperty();
> DoubleProperty property2 = new SimpleDoubleProperty();
>
> public BindingBenchmark() {
> property2.bindBidirectional(property1);
> }
>
> @Benchmark
> public void benchmark() {
> for (int i = 0; i < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
OK I see. I didn't realize GA is due for tomorrow already.
I had September 14 in mind as a release date, but realize now that this is the
date for OpenJDK.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
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 still shows better performance for the new
> implementation:
>
>
> @State(Scope.Benchmark)
> public class BindingBenchmark {
> DoubleProperty property1 = new SimpleDoubleProperty();
> DoubleProperty property2 = new SimpleDoubleProperty();
>
> public BindingBenchmark() {
> property2.bindBidirectional(property1);
> }
>
> @Benchmark
> public void benchmark() {
> for (int i = 0; i < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
I see. This is unfortunate though, as this will definitely break some
applications in ways potentially difficult to diagnose.
Maybe this won't be too bad if the window until 17.0.1 is small, but still, I'm
afraid third party developers not following these discussions might be in for
some head scratching.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
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 still shows better performance for the new
> implementation:
>
>
> @State(Scope.Benchmark)
> public class BindingBenchmark {
> DoubleProperty property1 = new SimpleDoubleProperty();
> DoubleProperty property2 = new SimpleDoubleProperty();
>
> public BindingBenchmark() {
> property2.bindBidirectional(property1);
> }
>
> @Benchmark
> public void benchmark() {
> for (int i = 0; i < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
Am I correct in assuming that, as is, this PR is only going to be merged into
openJFX 18 ?
If so, because it addresses a regression introduced in jfx 17, we'll probably
want to have this one in 17 as well on day one, if its not too late.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re-examine the risks of JDK-8264770 breaking third party libraries and applications.
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 used by the widely used rich text widget RichTextFX [2]. After a short investigation, I believe the root cause lies within another library by the same author called ReactFX [3], which aims to bring reactive programming techniques to JavaFX; in order to do so it seems to expands on but also sometime overrides the built-in bindings and events mechanisms. Now, I do believe that this is probably an exceptional case, and it is also quite possible that it is the result of an unsafe/incorrect use of internal implementations by the third party library, but with that said I can't help but feeling a bit nervous about the wider implications of that change with regard to compatibility of existing apps and OpenJFX 17. At the very least I believe it is important to raise awareness about potential compatibility issues among the community. For your information, I reached out to the maintainers of RichTextFX and proposed a workaround (replacing the use of a bidirectional binding by a pair of explicit ChangeListeners), which, while not very elegant, seems to fix the particular issue I discovered [4], but unfortunately it seems development on the underlying ReactFX is no longer active (last commit in 2016). -- Fred [1] https://bugs.openjdk.java.net/browse/JDK-8264770 [2] https://github.com/FXMisc/RichTextFX [3] https://github.com/ReactFX/reactfx.github.io [4] https://github.com/FXMisc/Flowless/issues/97
Re: RFR: 8259680: Need API to query states of CAPS LOCK and NUM LOCK keys
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, thanks. - PR: https://git.openjdk.java.net/jfx/pull/385
Re: RFR: 8259680: Need API to query states of CAPS LOCK and NUM LOCK keys
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 usages where the keyboard input might not be echoed. It would also
>> allow an application to do spell checking / auto-correction that might
>> ordinarily be skipped when typing all upper-case letters.
>>
>> We need an equivalent JavaFX API to the existing AWT
>> `java.awt.ToolKit::getLockingKeyState` method. A natural place to put this
>> in JavaFX is in the `javafx.application.Platform` class, so we propose to
>> create a new `Platform::isKeyLocked` method, which will take a `KeyCode` --
>> either `CAPS` or `NUM_LOCK` -- and return an `Optional` indicating
>> whether or not that key is in the locked or "on" state. If we can't read the
>> key state on a particular platform, we will return `Optional.empty()`,
>> rather than throwing a runtime exception as AWT does.
>>
>> I have provided both an automated Robot test and a manual test. The latter
>> is needed primarily because we can't set the CAPS lock on Mac using Robot,
>> but also because we want way to test the case where the user has enabled
>> CAPS lock before the program starts.
>
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/Application.java line
> 764:
>
>> 762: default:
>> 763: return Optional.empty();
>> 764: }
>
> This could be a switch expression:
> return switch (lockState) {
> case KeyEvent.KEY_LOCK_OFF -> Optional.of(false);
> case KeyEvent.KEY_LOCK_ON -> Optional.of(true);
> default -> Optional.empty();
> }
> Also, do we use a space after `switch`? I see many instances both with and
> without in the current source.
Do we plan to drop support for JDK11 for OpenJFX 17? If not, we'll need to keep
to the classic Switch statement, as Switch expressions were only introduced in
JDK12 or 13 as a preview.
That said, since 17 is going to be the next LTS release, maybe the plan is to
bump the dependency for OpenJFX then?
-
PR: https://git.openjdk.java.net/jfx/pull/385
Re: New API to read Caps-Lock and Num-Lock state
I realize that my suggestion was somewhat unclear, apology about that.
I wasn't suggesting that we return null to count as the third state, but
indeed that we leverage the isPresent state of the optional, alongside
with the two states provided by the Boolean, e.g:
isKeyLocked(KeyCode.NUM_LOCK).ifPresent(locked-> {
// The num_lock key exists on the current platform
if (locked){
// React to the key being locked
} else {
// Or to the key not being locked
});
On 17/01/2021 18:31, Jonathan Giles wrote:
A method returning Optional should never return null, as that undoes
the contract that Optional is supposed to represent, which is to avoid
NPEs by never being null itself, only empty in the absence of a
returned value. For this reason, an Optional should be considered two
state rather than three state.
-- Jonathan
(Tapped on a touch device)
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 no hints that a given key
might
not exists an other platforms, and this oversight will only manifest
itself at runtime, on said platform.
With the other two proposed solution (three-state return and Checked
exception) the API itself reminds its would be consumer that they
should
consider a case where the operation is invalid.
I'm personally not keen on checked exceptions in such a scenario
either,
as it would require an extra API to check for the existence of a
given
key on the current platform, or condemn developers to implement
conditional logic via exception catching (or require developer to
know
what specific keys exists on what platform before-hand to do the
check).
Given all that, my personal preference would go to a three state
return
solution.
On that topic, is there anything that would preclude the use of an
Optional to represent these three states, if we want to
avoid
introducing new enums? It seems to me its semantics aligns quite
nicely
with the problem at hand.
Fred
On 16/01/2021 17:41, Kevin Rushforth wrote:
> Hi Jonathan,
>
> Thanks for the suggestion. I thought about it as well. We could do
> that, but it seems like overkill. I'll reconsider if enough others
> favor the idea. As for the exception, my thinking is to use
> UnsupportedOperationException, which is what the equivalent AWT
method
> uses. FWIW, I don't generally like checked exceptions; we don't
define
> any such exceptions in JavaFX and I wouldn't want to start now.
>
> -- Kevin
>
>
> On 1/16/2021 12:46 AM, Jonathan Giles wrote:
>> Hi friends,
>>
>> Just to throw out an alternate API approach (which I would not go
>> anywhere near close to saying is a better approach), we could
>> consider a three-value enum getter API:
>>
>> public static KeyLockState Platform::getKeyLockState(KeyCode
keyCode);
>>
>> Where KeyLockState = { LOCKED, NOT_LOCKED, NOT_PRESENT }
>>
>> I'll be the first to argue against yet another enum, but I
wanted to
>> throw this out there as an alternate approach that avoids the
needs
>> for checked exceptions (which is what I assume is meant by
'throw an
>> exception', as opposed to throwing a runtime exception).
>>
>> -- Jonathan
>>
>> On Sat, Jan 16, 2021 at 6:40 AM Kevin Rushforth
>> mailto:kevin.rushfo...@oracle.com>
<mailto:kevin.rushfo...@oracle.com
<mailto:kevin.rushfo...@oracle.com>>> wrote:
>>
>> For JavaFX 17, I am planning to add a minor enhancement to
read the
>> state of the keyboard lock keys, specifically, Caps-Lock,
>> Num-Lock, and
>> maybe Scroll-Lock (although I might defer the latter to a
future
>> version
>> since it will be more difficult to test, and doesn't seem as
>> useful).
>>
>> This is currently tracked by JDK-8259680 [1].
>>
>> The proposed API would be something like:
>>
>> public static boolean Platform::isKeyLocked(KeyCode
>> keyCode);
>>
>> One question is whether we should throw an exception if the
key
>> state
>> cannot be read on a particular system (e.g., Num Lock on
macOS),
>> which
>> is what the similar AWT API does. I don't have a strong
opinion on
>>
Re: New API to read Caps-Lock and Num-Lock state
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 runtime, on said platform.
With the other two proposed solution (three-state return and Checked
exception) the API itself reminds its would be consumer that they should
consider a case where the operation is invalid.
I'm personally not keen on checked exceptions in such a scenario either,
as it would require an extra API to check for the existence of a given
key on the current platform, or condemn developers to implement
conditional logic via exception catching (or require developer to know
what specific keys exists on what platform before-hand to do the check).
Given all that, my personal preference would go to a three state return
solution.
On that topic, is there anything that would preclude the use of an
Optional to represent these three states, if we want to avoid
introducing new enums? It seems to me its semantics aligns quite nicely
with the problem at hand.
Fred
On 16/01/2021 17:41, Kevin Rushforth wrote:
Hi Jonathan,
Thanks for the suggestion. I thought about it as well. We could do
that, but it seems like overkill. I'll reconsider if enough others
favor the idea. As for the exception, my thinking is to use
UnsupportedOperationException, which is what the equivalent AWT method
uses. FWIW, I don't generally like checked exceptions; we don't define
any such exceptions in JavaFX and I wouldn't want to start now.
-- Kevin
On 1/16/2021 12:46 AM, Jonathan Giles wrote:
Hi friends,
Just to throw out an alternate API approach (which I would not go
anywhere near close to saying is a better approach), we could
consider a three-value enum getter API:
public static KeyLockState Platform::getKeyLockState(KeyCode keyCode);
Where KeyLockState = { LOCKED, NOT_LOCKED, NOT_PRESENT }
I'll be the first to argue against yet another enum, but I wanted to
throw this out there as an alternate approach that avoids the needs
for checked exceptions (which is what I assume is meant by 'throw an
exception', as opposed to throwing a runtime exception).
-- Jonathan
On Sat, Jan 16, 2021 at 6:40 AM Kevin Rushforth
mailto:kevin.rushfo...@oracle.com>> wrote:
For JavaFX 17, I am planning to add a minor enhancement to read the
state of the keyboard lock keys, specifically, Caps-Lock,
Num-Lock, and
maybe Scroll-Lock (although I might defer the latter to a future
version
since it will be more difficult to test, and doesn't seem as
useful).
This is currently tracked by JDK-8259680 [1].
The proposed API would be something like:
public static boolean Platform::isKeyLocked(KeyCode
keyCode);
One question is whether we should throw an exception if the key
state
cannot be read on a particular system (e.g., Num Lock on macOS),
which
is what the similar AWT API does. I don't have a strong opinion on
that
poont, although I wouldn't want to throw an exception if the
keyboard
doesn't have the key in question, as long the system is able to
read the
state accurately.
Comments are welcome.
-- Kevin
[1] https://bugs.openjdk.java.net/browse/JDK-8259680
Integrated: 8211294: ScrollPane content is blurry with 125% scaling
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%, 150% or 175% > output scaling). > > Please note that regardless of what the JBS issue (and therefore the title of > this PR) states, this does not appear to be a Windows only issue as I have > observed the same issue on Linux (Ubuntu 20.04). > > The following conditions are necessary for the blurriness to appear, but do > not guarantee that it will: > > - The node, or one of its parents, must have set `Node::cacheProperty` to > `true` > > - The scaling factor (as detected automatically or explicitly set via > glass.win/gtk.uiScale) must be a non integer number (e.g. 1.25, 1.5, 175) > > - The effective layout X or Y coordinates for the cached node must be != 0; > > Under these conditions, the translate coordinates for the transform used when > the cached image for a node is rendered to the screen may be non integer > numbers, which is the cause for the blurriness. > > Based on these observations, this PR fixes the issue by simply rounding the > translate coordinates (using `Math.round`) before the transform is applied in > `renderCacheToScreen()` and as far as I can tell, it fixes the blurriness in > all the previously affected applications (both trivial test cases or with > complex scenegraphs) and does not appear to introduce other noticeable visual > artifacts. > > Still, there might be a better place somewhere else higher up in the call > chain where this should be addressed as it could maybe be the root cause for > other rendering glitches, though I'm not yet familiar enough with the code to > see if it is really the case. This pull request has now been integrated. Changeset: 9c84c77b Author:Frederic Thevenet Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/9c84c77b Stats: 161 lines in 3 files changed: 136 ins; 12 del; 13 mod 8211294: ScrollPane content is blurry with 125% scaling Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v6]
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 clipped.
>>> Please check the code below. This is a very specific scenario, If tab's
>>> label is changed from "Tab" to "tab" then the issue does not occur. I did
>>> not observe any other such scenarios yet.
>>>
>>> ```
>>> import javafx.application.Application;
>>> import javafx.scene.Scene;
>>> import javafx.scene.control.TabPane;
>>> import javafx.scene.control.Tab;
>>> import javafx.stage.Stage;
>>> import javafx.geometry.Side;
>>>
>>> public class TabPaneExample extends Application {
>>> public static void main(String[] args) {
>>> launch(args);
>>> }
>>>
>>> public void start(Stage primaryStage) {
>>> Tab tab0 = new Tab("Tab 0");
>>> Tab tab1 = new Tab("Tab 1");
>>> Tab tab2 = new Tab("Tab 2");
>>> TabPane tabPane = new TabPane();
>>> tabPane.getTabs().addAll(tab0, tab1, tab2);
>>> tabPane.setSide(Side.LEFT);
>>>
>>> Scene scene = new Scene(tabPane);
>>> primaryStage.setScene(scene);
>>> primaryStage.show();
>>> }
>>> }
>>> ```
>>>
>>> 
>>> | 
>>> Output with this fix. | Output without this fix.
>>>
>>> Edit: Also, please note the difference of space between letter `a` and `b`
>>> in Tab's label.
>>
>> At which screen scale did you observe this?
>
>> At which screen scale did you observe this?
>
> Both border clip and font spacing issues are observed only at 125% scale.
>
>
>> Update: If I run your test program using a build directly from the target
>> branch, I see what you are seeing with the clipped tab pane border. That is
>> likely an unrelated bug that has since been fixed.
>
> I also see same behavior. Border clip issue does not occur with latest code
> but the difference in font spacing is observed(only at 125%)
I'm suspecting that the this fix might be revealing a issue related to
[JDK-8199592](https://bugs.openjdk.java.net/browse/JDK-8199592).
Keeping in mind that the revision on which this fix is based does *not* include
the fix for JDK-8199592, a way to confirm that would be to rebase that fix on
top of the latest rev and see if the test program still exhibits the same
behaviour.
I'll try to do that later on today.
-
PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v6]
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.
> Please check the code below. This is a very specific scenario, If tab's label
> is changed from "Tab" to "tab" then the issue does not occur. I did not
> observe any other such scenarios yet.
>
> ```
> import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.TabPane;
> import javafx.scene.control.Tab;
> import javafx.stage.Stage;
> import javafx.geometry.Side;
>
> public class TabPaneExample extends Application {
> public static void main(String[] args) {
> launch(args);
> }
>
> public void start(Stage primaryStage) {
> Tab tab0 = new Tab("Tab 0");
> Tab tab1 = new Tab("Tab 1");
> Tab tab2 = new Tab("Tab 2");
> TabPane tabPane = new TabPane();
> tabPane.getTabs().addAll(tab0, tab1, tab2);
> tabPane.setSide(Side.LEFT);
>
> Scene scene = new Scene(tabPane);
> primaryStage.setScene(scene);
> primaryStage.show();
> }
> }
> ```
>
> 
> | 
> Output with this fix. | Output without this fix.
>
> Edit: Also, please note the difference of space between letter `a` and `b` in
> Tab's label.
At which screen scale did you observe this?
-
PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v6]
> 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 (and therefore the title of > this PR) states, this does not appear to be a Windows only issue as I have > observed the same issue on Linux (Ubuntu 20.04). > > The following conditions are necessary for the blurriness to appear, but do > not guarantee that it will: > > - The node, or one of its parents, must have set `Node::cacheProperty` to > `true` > > - The scaling factor (as detected automatically or explicitly set via > glass.win/gtk.uiScale) must be a non integer number (e.g. 1.25, 1.5, 175) > > - The effective layout X or Y coordinates for the cached node must be != 0; > > Under these conditions, the translate coordinates for the transform used when > the cached image for a node is rendered to the screen may be non integer > numbers, which is the cause for the blurriness. > > Based on these observations, this PR fixes the issue by simply rounding the > translate coordinates (using `Math.round`) before the transform is applied in > `renderCacheToScreen()` and as far as I can tell, it fixes the blurriness in > all the previously affected applications (both trivial test cases or with > complex scenegraphs) and does not appear to introduce other noticeable visual > artifacts. > > Still, there might be a better place somewhere else higher up in the call > chain where this should be addressed as it could maybe be the root cause for > other rendering 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://git.openjdk.java.net/jfx/pull/308/files - new: https://git.openjdk.java.net/jfx/pull/308/files/969f23f0..12db9288 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=308=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=308=04-05 Stats: 12 lines in 2 files changed: 7 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/308.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/308/head:pull/308 PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v5]
> 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 (and therefore the title of > this PR) states, this does not appear to be a Windows only issue as I have > observed the same issue on Linux (Ubuntu 20.04). > > The following conditions are necessary for the blurriness to appear, but do > not guarantee that it will: > > - The node, or one of its parents, must have set `Node::cacheProperty` to > `true` > > - The scaling factor (as detected automatically or explicitly set via > glass.win/gtk.uiScale) must be a non integer number (e.g. 1.25, 1.5, 175) > > - The effective layout X or Y coordinates for the cached node must be != 0; > > Under these conditions, the translate coordinates for the transform used when > the cached image for a node is rendered to the screen may be non integer > numbers, which is the cause for the blurriness. > > Based on these observations, this PR fixes the issue by simply rounding the > translate coordinates (using `Math.round`) before the transform is applied in > `renderCacheToScreen()` and as far as I can tell, it fixes the blurriness in > all the previously affected applications (both trivial test cases or with > complex scenegraphs) and does not appear to introduce other noticeable visual > artifacts. > > Still, there might be a better place somewhere else higher up in the call > chain where this should be addressed as it could maybe be the root cause for > other rendering 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.net/jfx/pull/308/files - new: https://git.openjdk.java.net/jfx/pull/308/files/c448691b..969f23f0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=308=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=308=03-04 Stats: 106 lines in 1 file changed: 106 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/308.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/308/head:pull/308 PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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 those methods > > I filed the following follow-on bugs: > 1. [JDK-8258694](https://bugs.openjdk.java.net/browse/JDK-8258694) : > Rendering cached nodes with sub-pixel translation is blurry > 2. [JDK-8258697](https://bugs.openjdk.java.net/browse/JDK-8258697) : > TextFlow: methods copied from Region have diverged I've added a test that verifies that the inset values for a scrollpane in a scene are snapped to whole pixels, given the actual screen scale. The test passes without this fix for screen scale of 100% or 200% but fails at 125%, 150% and 175% With this fix, it passes at all screen scale. - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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 as I would expect. >> Do you have a test case that shows the problem? > > Unfortunately I've only seen it within an application with a fairly complex > scene graph, which make isolating the issue tricky. > > I'm still trying to reproduce it a minimal sample, but in the mean time, > please have a look at the a screen capture below, which should at least help > clarify the issue I'm observing: > >  > > This snippet is captured after the window had been moved from a screen scaled > to 100% to a second one scaled to 150%; notice how the controls in the "Chart > Properties" pane only snap to pixel when the mouse pointer enters the pane. Digging deeper into my own code, it appears that there are several areas where I need to account for screen scale within the application's code itself (i.e. I use coordinates from mouseEvent to set the layout of some elements, and these are not snapped), so it probably safe to ignore these issues in the context of the PR. - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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 moving the window to another screen, but only once the Region is >>> interacted with (e.g. hovering the mouse over it). >> >> This seems like a different (and preexisting) bug then. I think we're up to >> three follow-on issues if I haven't lost track: >> >> 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 those methods >> 3. Need to Render and layout scene out when scale changes (either when >> moving to a screen with a different scale or when the scale changes as the >> result of changing the setting of the current screen) > > 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 as I would expect. > Do you have a test case that shows the problem? Unfortunately I've only seen it within an application with a fairly complex scene graph, which make isolating the issue tricky. I'm still trying to reproduce it a minimal sample, but in the mean time, please have a look at the a screen capture below, which should at least help clarify the issue I'm observing:  This snippet is captured after the window had been moved from a screen scaled to 100% to a second one scaled to 150%; notice how the controls in the "Chart Properties" pane only snap to pixel when the mouse pointer enters the pane. - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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 snapToPixel property changes. >> >> This means that on a setup with more than one screen with different scales, >> when moving the application's window from one screen to the other, controls >> that were snapped on the first screen nor longer are once moved on the >> second one, and therefore appear blurry. >> >> We could possibly listen to `Window::renderScaleXProperty` and >> `Window::renderScaleYProperty` and call ` updateSnappedInsets`& >> `requestLayout` when one changes? > > Good point. We would need some way to invalidate the cached values if the > screen scale changes. > >> We could possibly listen to `Window::renderScaleXProperty` and >> `Window::renderScaleYProperty` and call `updateSnappedInsets` & >> `requestLayout` when one changes? > > A layout will happen anyway as the result of a Window moving to another > screen, so really it's only the cached snapped insets that need to be > recalculated. I don't know whether a listener is the best approach, since it > would need to handle the case where the Window changes (either because the > scene changed or the scene was added to a new window), which would present > similar challenges to what we face with treeVisible. > > Another possibility is for `updateSnappedInsets` to also cache the scaleX/Y > at which the snapped insets were computed. The `snappedX` methods could > check the current scale against the cached scale (a simple == test) and call > `updateSnappedInsets` if the scale changes. I've opted for the cache and check for screen scale solution, as I agree it presents less risks of leaked or missing listeners down the road. There's a slight unexpected drawback I've noticed, though, which is that the repaint for the affected Region does not seem to occur right away on moving the window to another screen, but only once the Region is interacted with (e.g. hovering the mouse over it). - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v4]
> 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 (and therefore the title of > this PR) states, this does not appear to be a Windows only issue as I have > observed the same issue on Linux (Ubuntu 20.04). > > The following conditions are necessary for the blurriness to appear, but do > not guarantee that it will: > > - The node, or one of its parents, must have set `Node::cacheProperty` to > `true` > > - The scaling factor (as detected automatically or explicitly set via > glass.win/gtk.uiScale) must be a non integer number (e.g. 1.25, 1.5, 175) > > - The effective layout X or Y coordinates for the cached node must be != 0; > > Under these conditions, the translate coordinates for the transform used when > the cached image for a node is rendered to the screen may be non integer > numbers, which is the cause for the blurriness. > > Based on these observations, this PR fixes the issue by simply rounding the > translate coordinates (using `Math.round`) before the transform is applied in > `renderCacheToScreen()` and as far as I can tell, it fixes the blurriness in > all the previously affected applications (both trivial test cases or with > complex scenegraphs) and does not appear to introduce other noticeable visual > artifacts. > > Still, there might be a better place somewhere else higher up in the call > chain where this should be addressed as it could maybe be the root cause for > other rendering 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 changes. - Changes: - all: https://git.openjdk.java.net/jfx/pull/308/files - new: https://git.openjdk.java.net/jfx/pull/308/files/5720f03d..c448691b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=308=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=308=02-03 Stats: 23 lines in 1 file changed: 23 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/308.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/308/head:pull/308 PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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 wouldn't do that just >> to fix this bug. >> >> Does TextFlow really need the package-scope snap methods from Region? I was >> thinking you would modify the copied `computeChildXXXAreaWidth/Height` >> methods to call the public `snapSpaceX/Y` methods (the ones that don't take >> `snap` as a parameter). > > Looking the git blame, it appears the orignal and copy diverged on commit > 55cf799de20b6fbf9ee31850b75c34389c8a28f2 "Baseline offset depends on layout > bounds which are calculated during layout". > I really can't say if TextFlow needs that as well, so I left it out and just > adapted the methods as discussed above. > > Maybe someone more knowledgeable about font rendering and the TextFlow > implementation can comment on whether or not it should be added to TextFlow > (presumably in a follow up?) There is still a remain 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 snapToPixel property changes. This means that on a setup with more than one screen with different scales, when moving the application's window from one screen to the other, controls that were snapped on the first screen nor longer are once moved on the second one, and therefore appear blurry. We could possibly listen to `Window::renderScaleXProperty` and `Window::renderScaleYProperty` and call ` updateSnappedInsets`& `requestLayout` when one changes? - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v3]
> 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 (and therefore the title of > this PR) states, this does not appear to be a Windows only issue as I have > observed the same issue on Linux (Ubuntu 20.04). > > The following conditions are necessary for the blurriness to appear, but do > not guarantee that it will: > > - The node, or one of its parents, must have set `Node::cacheProperty` to > `true` > > - The scaling factor (as detected automatically or explicitly set via > glass.win/gtk.uiScale) must be a non integer number (e.g. 1.25, 1.5, 175) > > - The effective layout X or Y coordinates for the cached node must be != 0; > > Under these conditions, the translate coordinates for the transform used when > the cached image for a node is rendered to the screen may be non integer > numbers, which is the cause for the blurriness. > > Based on these observations, this PR fixes the issue by simply rounding the > translate coordinates (using `Math.round`) before the transform is applied in > `renderCacheToScreen()` and as far as I can tell, it fixes the blurriness in > all the previously affected applications (both trivial test cases or with > complex scenegraphs) and does not appear to introduce other noticeable visual > artifacts. > > Still, there might be a better place somewhere else higher up in the call > chain where this should be addressed as it could maybe be the root cause for > other rendering 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 account. - Changes: - all: https://git.openjdk.java.net/jfx/pull/308/files - new: https://git.openjdk.java.net/jfx/pull/308/files/4087c062..5720f03d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=308=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=308=01-02 Stats: 14 lines in 1 file changed: 0 ins; 6 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/308.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/308/head:pull/308 PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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, since the >>> value of `snap` is always set to `isSnapPixel()`, which is what the public >>> methods do. >> >> Actually I think the copy was necessary for the other methods in this >> delimited block: `computeChildXXXAreaWidth/Height` because members from >> Region are indeed inaccessible. >> These copies are outdated and should be updated as they use the deprecated >> `snapSpace()` (instead of `snapSpaceX/Y()`) which does not take into account >> non square screen ratio. >> >> I assume we have no other choice than copy the new versions of these from >> Region into TextFlow? Unless you think we have reasons to reassess why these >> where made package-private in the first place ? > > 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 wouldn't do that just > to fix this bug. > > Does TextFlow really need the package-scope snap methods from Region? I was > thinking you would modify the copied `computeChildXXXAreaWidth/Height` > methods to call the public `snapSpaceX/Y` methods (the ones that don't take > `snap` as a parameter). Looking the git blame, it appears the orignal and copy diverged on commit 55cf799de20b6fbf9ee31850b75c34389c8a28f2 "Baseline offset depends on layout bounds which are calculated during layout". I really can't say if TextFlow needs that as well, so I left it out and just adapted the methods as discussed above. Maybe someone more knowledgeable about font rendering and the TextFlow implementation can comment on whether or not it should be added to TextFlow (presumably in a follow up?) - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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 > value of `snap` is always set to `isSnapPixel()`, which is what the public > methods do. Actually I think the copy was necessary for the other methods in this delimited block: `computeChildXXXAreaWidth/Height` because members from Region are indeed inaccessible. These copies are outdated and should be updated as they use the deprecated `snapSpace()` (instead of `snapSpaceX/Y()`) which does not take into account non square screen ratio. I assume we have no other choice than copy the new versions of these from Region into TextFlow? Unless you think we have reasons to reassess why these where made package-private in the first place ? - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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 it will be updated by a script when @arapte implements > [JDK-8254101](https://bugs.openjdk.java.net/browse/JDK-8254101) early next > week. So to avoid a possible merge conflict, you can just leave it alone. I've been looking for other occurrences of the same issue in other places, and came across this one, in TextFlow: https://github.com/openjdk/jfx/blob/f2928d9506b928d991d7474e36bc5a0b24d3b017/modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java#L614 >From what I gather, this is likely to cause the same result. - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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 issue from the cache
> rendering one.
>
> I've reverted my initial changes and applied the changes you proposed (using
> snapSpaceX/Y as suggested in your latest comment).
> I'll also add a test case soon.
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)?
-
PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling
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
>> index 565d52b516..00c0f6da61 100644
>> --- a/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java
>> +++ b/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java
>> @@ -989,17 +989,11 @@ public class Region extends Parent {
>> /** Called to update the cached snapped insets */
>> private void updateSnappedInsets() {
>> final Insets insets = getInsets();
>> -if (_snapToPixel) {
>> -snappedTopInset = Math.ceil(insets.getTop());
>> -snappedRightInset = Math.ceil(insets.getRight());
>> -snappedBottomInset = Math.ceil(insets.getBottom());
>> -snappedLeftInset = Math.ceil(insets.getLeft());
>> -} else {
>> -snappedTopInset = insets.getTop();
>> -snappedRightInset = insets.getRight();
>> -snappedBottomInset = insets.getBottom();
>> -snappedLeftInset = insets.getLeft();
>> -}
>> +final boolean snap = isSnapToPixel();
>> +snappedTopInset = snapSizeY(insets.getTop(), snap);
>> +snappedRightInset = snapSizeX(insets.getRight(), snap);
>> +snappedBottomInset = snapSizeY(insets.getBottom(), snap);
>> +snappedLeftInset = snapSizeX(insets.getLeft(), snap);
>> }
>>
>> /**
>>
>> We will need a test case for this. You can see
>> [UIRenderSceneTest.java](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/javafx/scene/UIRenderSceneTest.java)
>> for an example of a test that forces Hi-DPI scaling.
>
> 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 issue from the cache
rendering one.
I've reverted my initial changes and applied the changes you proposed (using
snapSpaceX/Y as suggested in your latest comment).
I'll also add a test case soon.
-
PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: ScrollPane content is blurry with 125% scaling [v2]
> 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 (and therefore the title of > this PR) states, this does not appear to be a Windows only issue as I have > observed the same issue on Linux (Ubuntu 20.04). > > The following conditions are necessary for the blurriness to appear, but do > not guarantee that it will: > > - The node, or one of its parents, must have set `Node::cacheProperty` to > `true` > > - The scaling factor (as detected automatically or explicitly set via > glass.win/gtk.uiScale) must be a non integer number (e.g. 1.25, 1.5, 175) > > - The effective layout X or Y coordinates for the cached node must be != 0; > > Under these conditions, the translate coordinates for the transform used when > the cached image for a node is rendered to the screen may be non integer > numbers, which is the cause for the blurriness. > > Based on these observations, this PR fixes the issue by simply rounding the > translate coordinates (using `Math.round`) before the transform is applied in > `renderCacheToScreen()` and as far as I can tell, it fixes the blurriness in > all the previously affected applications (both trivial test cases or with > complex scenegraphs) and does not appear to introduce other noticeable visual > artifacts. > > Still, there might be a better place somewhere else higher up in the call > chain where this should be addressed as it could maybe be the root cause for > other rendering 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-pixel. - Revert "Round up the translation coordinates when rendering a cached node to screen to prevent it from appearing blurry" This reverts commit cce931d3 - Changes: - all: https://git.openjdk.java.net/jfx/pull/308/files - new: https://git.openjdk.java.net/jfx/pull/308/files/cce931d3..4087c062 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=308=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=308=00-01 Stats: 13 lines in 2 files changed: 0 ins; 6 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/308.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/308/head:pull/308 PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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 is
>>> done in this PR, but we would need to ensure that this doesn't impact
>>> smooth scrolling of a TextArea.
>>
>> I've done some quick tests with the minimal sample below, and could not
>> notice any performance impact with this patch compared to 15.0.1.
>>
>> java
>> public class TextScroll extends Application {
>>
>> @Override
>> public void start(Stage stage) throws Exception {
>> var textArea = new TextArea();
>> var txt = new
>> File(getClass().getResource("/Scrollpane.java").getFile());
>> textArea.setText(Files.readString(txt.toPath(),
>> StandardCharsets.UTF_8));
>> var root = new ScrollPane(textArea);
>> root.setFitToHeight(true);
>> root.setFitToWidth(true);
>> Scene scene;
>> scene = new Scene(root, 800, 600);
>> stage.setTitle("Scroll test");
>> stage.setScene(scene);
>> stage.show();
>> }
>>
>> public static void main(String[] args) {
>> launch(args);
>> }
>> }
>
> Actually, when I mentioned smooth scrolling, it wasn't performance I was
> thinking of, it was the ability to scroll by fractional pixel amounts, but
> with the default snap-to-pixel enabled, that won't happen anyway.
>
> I'll look into the other questions you raised tomorrow, specifically what we
> might want to do about rendering the cache, and also your question about
> snapToPixel.
>
> As for the specific problem with ScrollPane, I found the cause of that one.
> There is a (long-standing from the look of it) bug in
> [`Region::updateSnappedInsets`](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java#L990).
> It attempts to account for snap-to-pixel, but does so incorrectly, using
> `Math.ceil` without taking screen scaling into account (that is, without
> calling the `snapXxxx` methods). The correct code should be something like:
>
> final boolean snap = isSnapToPixel();
> snappedTopInset = snapSizeY(insets.getTop(), snap);
> snappedRightInset = snapSizeX(insets.getRight(), snap);
> snappedBottomInset = snapSizeY(insets.getBottom(), snap);
> snappedLeftInset = snapSizeX(insets.getLeft(), snap);
>
> This fixes the ScrollPane problem without needing to modify the cached
> rendering code.
Thanks for that.
I am wondering; is it ok to potentially address both sub-issues discussed here
(Scrollpane snaping vs cache rendering) under the same JBS bug? Or would it be
better to address them under separate issues?
Speaking of which, the JBS issue title is in fact misleading, as the issue
appears to be not specific to Windows; what's the best practice in such cases;
rename the issue? Open a new one? Leave it as is?
-
PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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 in
> this PR, but we would need to ensure that this doesn't impact smooth
> scrolling of a TextArea.
I've done some quick tests with the minimal sample below, and could not notice
any performance impact with this patch compared to 15.0.1.
java
public class TextScroll extends Application {
@Override
public void start(Stage stage) throws Exception {
var textArea = new TextArea();
var txt = new
File(getClass().getResource("/Scrollpane.java").getFile());
textArea.setText(Files.readString(txt.toPath(),
StandardCharsets.UTF_8));
var root = new ScrollPane(textArea);
root.setFitToHeight(true);
root.setFitToWidth(true);
Scene scene;
scene = new Scene(root, 800, 600);
stage.setTitle("Scroll test");
stage.setScene(scene);
stage.show();
}
public static void main(String[] args) {
launch(args);
}
}
-
PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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
>> integer value), but it is failing to do so. This is mostly not a problem
>> when caching is disabled, since our text rendering does sub-pixel
>> antialiasing that looks crisp even at non-integer boundaries. However,
>> translating an already-rendered image by a non-integer boundary will cause
>> the blurriness we are seeing. There is another issue with the Y translation
>> which isn't 0 even when not using a ScrollPane.
>>
>> I'll continue looking at this in the coming week.
>
> 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 in
> this PR, but we would need to ensure that this doesn't impact smooth
> scrolling of a TextArea.
Further investigations on my part raised one more question, which hopefully you
can answer:
To which extend should `setSnapToPixel` ensure children of a region are indeed
snap to whole pixel coordinates?
To make it clearer, please consider the following sample:
java
public class Blur extends Application {
@Override
public void start(final Stage stage) throws Exception {
var root = new Pane();
root.setSnapToPixel(true);
var ctrl = new CheckBox("Cached");
ctrl.setLayoutX(0.5);
ctrl.setLayoutY(0.5);
ctrl.cacheProperty().bind(ctrl.selectedProperty());
ctrl.setSelected(true);
var ctrl2 = new Button("Foo");
ctrl2.setLayoutX(0.5);
ctrl2.setLayoutY(30.5);
ctrl2.cacheProperty().bind(ctrl.selectedProperty());
var ctrl3 = new Button("Bar");
ctrl3.setLayoutY(60);
ctrl3.cacheProperty().bind(ctrl.selectedProperty());
root.getChildren().addAll(ctrl, ctrl2, ctrl3);
Scene scene;
scene = new Scene(root, 200, 200);
stage.setTitle("Blur test");
stage.setScene(scene);
stage.show();
}
public static void main(String[] args) {
launch(args);
}
}
In this sample, LayoutX and Y properties are deliberately set to non integer
values for the first two controls (the last one serves as a visual baseline,
but `snapToPixel` is set to true. Also clicking the check box toggles caching
for all controls.
Here's what it looks looks **at 100% scaling**, (OpenJFX 15.0.1), with cache
enabled:
and with cache disabled:
What is the legitimate result to expect here; should
`root.setSnapToPixel(true);` override `setLayoutX(0.5);` and align everything
for crisp rendering (as is my understanding)? Or am I misunderstanding the
scope of `setSnapToPixel` and it has no effect when layout is set explicitly?
-
PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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 transform values when rendering the cache to the screen. Hi, Is there anything I can do to help get this moving forward again? As mentioned before, I agree the proposed fix might be simply be addressing the symptoms rather than the root cause and I'm willing to keep working on it but I'd really appreciate a second opinion and some insights as to where to direct my attention if this fix proves to be not enough. For what it's worth, I've been living with this fix on my dev machine for a while now, and it does provide the expected relief to the underlying issue, without me noticing any obvious regressions, neither visually nor performance-wise (I haven't run any targeted benchmarks, though). With JDK-8199592 fixed, I think it'd be great if we could tackle all of the scaling issues in 16 (might be getting a bit late now, though) - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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 >> tackled on linux as well. But what is not clear >> to me: does this require a physical HiDPI screen, or is setting the scale >> factor manually good enough to reproduce the >> bug? > >> >> >> 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 not clear >> to me: does this require a physical HiDPI screen, or is setting the scale >> factor manually good enough to reproduce the >> bug? > > The issue will appear consistently as long as the conditions I listed are > met, regardless of the actual number of > pixels the physical screen can display. For instance you'll see the problem, > if you apply a 125% scaling on 1080p > screen (a common configuration on 13'' laptops). Also, it will occur > regardless of whether the scaling is applied at > the OS level and picked up by javafx or if it is only set for a single > application using the `glass.xxx.uiScale` > property. Hello, Did anyone get a chance to look into this? Thanks! - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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 not clear > to me: does this require a physical HiDPI screen, or is setting the scale > factor manually good enough to reproduce the > bug? The issue will appear consistently as long as the conditions I listed are met, regardless of the actual number of pixels the physical screen can display. For instance you'll see the problem, if you apply a 125% scaling on 1080p screen (a common configuration on 13'' laptops). Also, it will occur regardless of whether the scaling is applied at the OS level and picked up by javafx or if it is only set for a single application using the `glass.xxx.uiScale` property. - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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%, 150% or 175% output > scaling). Please note that regardless of what the JBS issue (and therefore > the title of this PR) states, this does not > appear to be a Windows only issue as I have observed the same issue on Linux > (Ubuntu 20.04). > The following conditions are necessary for the blurriness to appear, but do > not guarantee that it will: > > - The node, or one of its parents, must have set `Node::cacheProperty` to > `true` > > - The scaling factor (as detected automatically or explicitly set via > glass.win/gtk.uiScale) must be a non integer number > (e.g. 1.25, 1.5, 175) > > - The effective layout X or Y coordinates for the cached node must be != 0; > > Under these conditions, the translate coordinates for the transform used when > the cached image for a node is rendered > to the screen may be non integer numbers, which is the cause for the > blurriness. > Based on these observations, this PR fixes the issue by simply rounding the > translate coordinates (using `Math.round`) > before the transform is applied in `renderCacheToScreen()` and as far as I > can tell, it fixes the blurriness in all the > previously affected applications (both trivial test cases or with complex > scenegraphs) and does not appear to introduce > other noticeable visual artifacts. Still, there might be a better place > somewhere else higher up in the call chain > where this should be addressed as it could maybe be the root cause for other > rendering glitches, though I'm not yet > familiar enough with the code to see if it is really the case. Also, I don't really have an idea on how this could be tested other than visually, so I'm open to suggestions. - PR: https://git.openjdk.java.net/jfx/pull/308
RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
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 (and therefore the title of this PR) states, this does not appear to be a Windows only issue as I have observed the same issue on Linux (Ubuntu 20.04). The following conditions are necessary for the blurriness to appear, but do not guarantee that it will: - The node, or one of its parents, must have set `Node::cacheProperty` to `true` - The scaling factor (as detected automatically or explicitly set via glass.win/gtk.uiScale) must be a non integer number (e.g. 1.25, 1.5, 175) - The effective layout X or Y coordinates for the cached node must not be != 0; Under these conditions, the translate coordinates for the transform used when the cached image for a node is rendered to the screen may be non integer numbers, which is the cause for the blurriness. Based on these observations, this PR fixes the issue by simply rounding the translate coordinates (using `Math.round`) before the transform is applied in `renderCacheToScreen()` and as far as I can tell, it fixes the blurriness in all the previously affected applications (both trivial test cases or with complex scenegraphs) and does not appear to introduce other noticeable visual artifacts. Still, there might be a better place somewhere else higher up in the call chain where this should be addressed as it could maybe be the root cause for other rendering glitches, though I'm not yet familiar enough with the code to see if it is really the case. - Commit messages: - Round up the translation coordinates when rendering a cached node to screen to prevent it from appearing blurry Changes: https://git.openjdk.java.net/jfx/pull/308/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=308=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8211294 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/308.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/308/head:pull/308 PR: https://git.openjdk.java.net/jfx/pull/308
Integrated: 8238954: Improve performance of tiled snapshot rendering
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. 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. This pull request has now been integrated. Changeset: 32584dba Author:Frederic Thevenet Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/32584dba Stats: 395 lines in 3 files changed: 64 ins; 313 del; 18 mod 8238954: Improve performance of tiled snapshot rendering Reviewed-by: arapte, kcr - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v16]
> 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 two additional commits since the last revision: - Mark variables as final - Using for loops instead of while - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/d8083075..58cc2000 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.15 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.14-15 Stats: 37 lines in 1 file changed: 3 ins; 14 del; 20 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
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v15]
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 QuantumToolkit >> - Only initialize RTT cache if needed > > Marked as reviewed by arapte (Reviewer). I have read Ambarish's code more closely and I am now convinced it is indeed easier to understand; beyond the change from "while" to "for" loops, it introduces an extra set of variables which avoids the same "m/b/rTileWidth/Height" vars to have to designate different things depending in which loop they're used (M, B, or R). In hindsight, this is why I had such a hard time naming them. I have run the automated tests on Windows and Ubuntu (w/ forceGPU=true) and it passes. The benchmark I made also produces similar results. Someone will still need to run the tests on macOS, I'm afraid. If it's not too much of a problem now that the PR's been approved, I think it would beneficial to make the change, especially considering that this bit of code might be used as a starting point for other issues requiring tiling. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v15]
> 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: - Removed unused imports in Snapshot2Test - Fixed comments in QuantumToolkit - Only initialize RTT cache if needed - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/c0f7d14f..d8083075 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.14 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.13-14 Stats: 10 lines in 2 files changed: 3 ins; 2 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
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v14]
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.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
> line 1638:
>
>> 1637: renderWholeImage(x, y, w, h, rf, pImage);
>> 1638: }
>> 1639: params.platformImage = pImage;
>
> I tried to write this code using for loop to make it little easy for reader
> to understand. This is just a suggestion, I
> leave it to you to whether replace it or not,
> int mTileWidth = computeTileSize(w, maxTextureSize);
> int mTileHeight = computeTileSize(h, maxTextureSize);
> IntBuffer buffer = IntBuffer.allocate(mTileWidth * mTileHeight);
>
> int mTileXOffset = 0;
> int mTileYOffset = 0;
> for (mTileXOffset = 0; (mTileXOffset + mTileWidth) <= w; mTileXOffset +=
> mTileWidth) {
> for (mTileYOffset = 0; (mTileYOffset + mTileHeight) <= h; mTileYOffset
> += mTileHeight) {
> renderTile(x, mTileXOffset, y, mTileYOffset, mTileWidth,
> mTileHeight,
> buffer, rf, tileRttCache, pImage);
> }
> }
>
> int rTileXOffset = mTileXOffset;
> int rTileWidth = w - rTileXOffset;
> if (rTileWidth > 0) {
> for (int rTileYOffset = 0; (rTileYOffset + mTileHeight) <= h;
> rTileYOffset += mTileHeight) {
> renderTile(x, rTileXOffset, y, rTileYOffset, rTileWidth,
> mTileHeight,
> buffer, rf, tileRttCache, pImage);
> }
> }
>
> int bTileYOffset = mTileYOffset;
> int bTileHeight = h - bTileYOffset;
> if (bTileHeight > 0) {
> for (int bTileXOffset = 0; (bTileXOffset + mTileWidth) <= w;
> bTileXOffset += mTileWidth) {
> renderTile(x, bTileXOffset, y, bTileYOffset, mTileWidth,
> bTileHeight,
> buffer, rf, tileRttCache, pImage);
> }
> }
>
> if (rTileWidth > 0 && bTileHeight > 0) {
> renderTile(x, rTileXOffset, y, bTileYOffset, rTileWidth, bTileHeight,
> buffer, rf, tileRttCache, pImage);
> }
Thanks for you suggestion.
The original code is still too fresh in my mind for me to really be able to say
which version is easier to understand,
so I'll let others with a fresh pair of eyes be the judge of that. However I
think your code is arguably more concise -
and overall more elegant - so I'd be happy to swap my version for yours,
provided it is considered at least as readable
as the original. WDYT?
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v14]
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- > 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: Prevent attempt to render tiles with a 0 sized dimension. - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/efccc4d8..c0f7d14f Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.13 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.12-13 Stats: 27 lines in 1 file changed: 12 ins; 8 del; 7 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
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]
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 confusing. > > Actually, it also fails the same way on Ubuntu 20.04 as it does on macOS if I > force HW acceleration on Ubuntu. It's off > by default when running in a VM, so is using the SW pipeline, which doesn't > have a hard limit (so that could explain > the heap problem you were facing). Ensuring that the tile size is > 0 fixes > it on Linux and Mac. I've pushed the changes discussed above, and verified that the tests now pass on a Ubuntu 20.04 VM with `prism.forceGpu=true` (and that they indeed failed prior to those changes). - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]
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/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
> line 1612:
>
>> 1611: int bTileHeight = tileHeight;
>> 1612: while (bTileHeight == tileHeight && bOffset <
>> h) {
>> 1613: renderTile(x, xOffset, y, bOffset,
>> mTileWidth, bTileHeight, buffer, rf, tileRttCache,
>> pImage);
>
> It looks like the "B" and the "R" loops are reversed. This isn't causing any
> actual problems, but is confusing since it
> doesn't match the comment or the diagram above. The comment for this block
> says "B" tiles, but actually, it is the "R"
> tiles in the diagram that this is looping over. At the end of the main loop,
> `mTileWIdth` and `mTileHeight` will be set
> to the size of the corner tile. Given this, the tiles of `mTileWidth` X
> `tileHeight` will be the right hand column.
> Once you fix this, you will need to surround this `while` loop in a check for
> `if (mTileWidth > 0)`
You're right: the comments and variable names are swapped. Will fix it (and
obviously add the extra condition that
prevents attempts to render 0 width/height tiles). Thanks.
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]
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 helpful (it simply >> indicates the renderImage method returned null). >> Running the test with the unpatched version of the >> `QuantumToolkit::renderToImage` method would typically result in >> such a stack, but there are many other possible reasons. >> With regard to the choice of random noise for the test image, my idea was to >> be able to catch any misalignment caused >> by tiling in the final snapshot: using a single color or a simple pattern >> would not necessarily help catching such >> errors. Using a complex image could do the trick, and avoid the broken >> aspect you mentioned, but it would need to be >> very large (>4096x4096), and I was not sure it would be wise to add such a >> large binary resource to the repo. So I >> settled for random noise, since it was the simplest way to generate an image >> which guaranties any misalignment would be >> caught by comparing pixels 1 to 1. > > I have changed the test setup to fill up the scene with a gradient instead of > random noise; it should be as able to > catch misalignment, while not looking like something's gone horribly wrong. Also, while running the tests again on a ubuntu 20.04 VM, I encountered 2 occurences where one of the tests failed, out of ~8 runs in total. In one case, the error where similar to yours: > Task :systemTests:test test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameSizeImm[3] FAILED java.lang.IllegalArgumentException: Unrecognized image loader: null at javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278) at javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53) at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1342) at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136) at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214) at test.javafx.scene.Snapshot2Test.lambda$doTestTiledSnapshotImm$12(Snapshot2Test.java:375) 148 tests completed, 1 failed While in the other, it pointed to an Out Of Memory Error: Task :systemTests:test test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameHeightDefer[3] FAILED java.lang.OutOfMemoryError: Java heap space at java.base/java.nio.HeapByteBuffer.(HeapByteBuffer.java:63) at java.base/java.nio.ByteBuffer.allocate(ByteBuffer.java:351) at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.createPlatformImage(QuantumToolkit.java:1426) at javafx.graphics/javafx.scene.image.Image.(Image.java:740) at javafx.graphics/javafx.scene.image.WritableImage.(WritableImage.java:77) at test.javafx.scene.Snapshot2Test.doTestTiledSnapshotDefer(Snapshot2Test.java:377) at test.javafx.scene.Snapshot2Test.testSnapshot2x2TilesSameHeightDefer(Snapshot2Test.java:489) 148 tests completed, 1 failed Again, in my environment, it only failed one test, and not all the time, but this could be the same root cause. Two ways we could find out would be by either increasing the max heap size for the test runner or use a smaller image and force the prism.maxTextureSize property to something less than 4096 to trigger tiling anyway. Unfortunately, I could not find out how to achieve either of these (simply passing the properties to the JVM running the Gradle wrapper seem to have no effect). - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]
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.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameWidthImm[0] FAILED >> java.lang.IllegalArgumentException: Unrecognized image loader: null >> at >> javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278) >> at >> javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53) >> at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1342) >> at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136) >> at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214) >> at >> test.javafx.scene.Snapshot2Test.lambda$doTestTiledSnapshotImm$12(Snapshot2Test.java:364) >> >> test.javafx.scene.Snapshot2Test > testSnapshot2x1TilesSameSizeDefer[0] FAILED >> junit.framework.AssertionFailedError: Timeout waiting for snapshot >> callback >> at >> test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:157) >> at >> test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:183) >> at >> test.javafx.scene.Snapshot2Test.doTestTiledSnapshotDefer(Snapshot2Test.java:378) >> at >> test.javafx.scene.Snapshot2Test.testSnapshot2x1TilesSameSizeDefer(Snapshot2Test.java:459) >> >> I would suggest to fill the image with a single or preferably some pattern >> of multiple recognizable colors instead of >> noise. The noise gives a broken feel. It might confuse the verifications of >> any future fixes in this area. A well >> defined color would be nice. > > 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 helpful (it simply > indicates the renderImage method returned null). > Running the test with the unpatched version of the > `QuantumToolkit::renderToImage` method would typically result in > such a stack, but there are many other possible reasons. > With regard to the choice of random noise for the test image, my idea was to > be able to catch any misalignment caused > by tiling in the final snapshot: using a single color or a simple pattern > would not necessarily help catching such > errors. Using a complex image could do the trick, and avoid the broken aspect > you mentioned, but it would need to be > very large (>4096x4096), and I was not sure it would be wise to add such a > large binary resource to the repo. So I > settled for random noise, since it was the simplest way to generate an image > which guaranties any misalignment would be > caught by comparing pixels 1 to 1. I have changed the test setup to fill up the scene with a gradient instead of random noise; it should be as able to catch misalignment, while not looking like something's gone horribly wrong. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]
> 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: Fill test image with a bilinear gradient instead of random noise. - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/a01aaa20..efccc4d8 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.12 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.11-12 Stats: 16 lines in 1 file changed: 11 ins; 0 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
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]
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 snapshot and do a pixel-wise comparison with the original image. >> >> I've added the new tests to the existing `Snapshot2Test.java`. > > 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.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameWidthImm[0] FAILED > java.lang.IllegalArgumentException: Unrecognized image loader: null > at > javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278) > at > javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53) > at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1342) > at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136) > at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214) > at > test.javafx.scene.Snapshot2Test.lambda$doTestTiledSnapshotImm$12(Snapshot2Test.java:364) > > test.javafx.scene.Snapshot2Test > testSnapshot2x1TilesSameSizeDefer[0] FAILED > junit.framework.AssertionFailedError: Timeout waiting for snapshot > callback > at > test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:157) > at > test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:183) > at > test.javafx.scene.Snapshot2Test.doTestTiledSnapshotDefer(Snapshot2Test.java:378) > at > test.javafx.scene.Snapshot2Test.testSnapshot2x1TilesSameSizeDefer(Snapshot2Test.java:459) > > I would suggest to fill the image with a single or preferably some pattern of > multiple recognizable colors instead of > noise. The noise gives a broken feel. It might confuse the verifications of > any future fixes in this area. A well > defined color would be nice. 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 helpful (it simply indicates the renderImage method returned null). Running the test with the unpatched version of the `QuantumToolkit::renderToImage` method would typically result in such a stack, but there are many other possible reasons. With regard to the choice of random noise for the test image, my idea was to be able to catch any misalignment caused by tiling in the final snapshot: using a single color or a simple pattern would not necessarily help catching such errors. Using a complex image could do the trick, and avoid the broken aspect you mentioned, but it would need to be very large (>4096x4096), and I was not sure it would be wise to add such a large binary resource to the repo. So I settled for random noise, since it was the simplest way to generate an image which guaranties any misalignment would be caught by comparing pixels 1 to 1. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v12]
> 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: Crawling pixels in writableImage with parallel stream is a bad idea. - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/4752d83e..a01aaa20 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.11 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.10-11 Stats: 16 lines in 1 file changed: 7 ins; 0 del; 9 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
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v11]
> 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: Changed wrong value for image size (for these tests we *don't* want it to be dividable by 2 nor 3) - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/82746316..4752d83e Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.10 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.09-10 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 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
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 09] 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: Removed trailing whitespaces - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/2951d538..82746316 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.09 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.08-09 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 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
Re: [Rev 08] 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: Added tiled snapshots tests - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/8f0c2d22..2951d538 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.08 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.07-08 Stats: 170 lines in 1 file changed: 168 ins; 0 del; 2 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
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 07] 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: Changed tile walking logic - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/0642096a..8f0c2d22 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.07 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.06-07 Stats: 80 lines in 1 file changed: 23 ins; 17 del; 40 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
Re: [Rev 06] 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 to import statements - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/1fb04e56..0642096a Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.06 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.05-06 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
Re: [Rev 05] 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 three additional commits since the last revision: - Use boolean[] instead of AtomicBoolean to effec pass-by-reference - Fixed typo in comment - Removed fully qualified name when declaring RTTexture - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/7f5f2890..1fb04e56 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.05 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.04-05 Stats: 126 lines in 1 file changed: 31 ins; 79 del; 16 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
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 03] RFR: 8238954: Improve performance of tiled snapshot rendering
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 to fix that? >> >> It won't help with things in their current state, as the `RenderToImage` >> method that is modified in this PR is >> currently called only by the snapshot feature in `Scene` But of course I >> suspect the same technique could also be used >> to solve [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082) and >> now is indeed a good time to investigate >> it further. I'll have a look to see if it is possible to factorize the >> tiling implementation for both classes of >> issues, in which case it would make sense to do prior to merging this PR. If >> it is different enough that the >> implementation cannot be shared but only the general idea, then I suggest we >> address it in a different PR. What do you >> all think ? > > At first glance, the NPE in > [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082) occurs in the > Prism layer, > which is one level _below_ Quantum where the tiling is currently implemented, > so I'm not sure tit is reachable from > there; if we want the code to be shared, it looks like it would need to be > moved even further down (maybe in the > `ResourceFactory`?) Also, while reusing code is generally the way to go, in > such lower layers, very closely > intertwined with the actual rendering, I'm afraid that insisting on having a > "one-size-fits-all" implementation might > get in the way of necessary case-by-case optimizations, so I'd like to have > someone with a deeper knowledge of the > code base to weight in before starting work in that direction. Maybe > @kevinrushforth could advise? Hi everyone, This PR hasn't seen much activity in a while, so I though I would give it a gentle kick to hopefully get it moving again ;) As explained above, I feel a little stalled at the moment, as we need to decide whether or not it is a good idea to try and address all or part of JDK-8189082 within the scope of this PR, and I don't feel like I can settle that on my own. Thanks. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 03] RFR: 8238954: Improve performance of tiled snapshot rendering
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? > >> >> >> 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, as the `RenderToImage` > method that is modified in this PR is currently > called only by the snapshot feature in `Scene` But of course I suspect the > same technique could also be used to solve > [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082) and now is > indeed a good time to investigate it > further. I'll have a look to see if it is possible to factorize the tiling > implementation for both classes of issues, > in which case it would make sense to do prior to merging this PR. If it is > different enough that the implementation > cannot be shared but only the general idea, then I suggest we address it in a > different PR. What do you all think ? At first glance, the NPE in [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082) occurs in the Prism layer, which is one level _below_ Quantum where the tiling is currently implemented, so I'm not sure tit is reachable from there; if we want the code to be shared, it looks like it would need to be moved even further down (maybe in the `ResourceFactory`?) Also, while reusing code is generally the way to go, in such lower layers, very closely intertwined with the actual rendering, I'm afraid that insisting on having a "one-size-fits-all" implementation might get in the way of necessary case-by-case optimizations, so I'd like to have someone with a deeper knowledge of the code base to weight in before starting work in that direction. Maybe @kevinrushforth could advise? - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 03] RFR: 8238954: Improve performance of tiled snapshot rendering
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, as the `RenderToImage` method that is modified in this PR is currently called only by the snapshot feature in `Scene` But of course I suspect the same technique could also be used to solve [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082) and now is indeed a good time to investigate it further. I'll have a look to see if it is possible to factorize the tiling implementation for both classes of issues, in which case it would make sense to do prior to merging this PR. If it is different enough that the implementation cannot be shared but only the general idea, then I suggest we address it in a different PR. What do you all think ? - 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
Re: [Rev 02] RFR: 8238954: Improve performance of tiled snapshot rendering
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/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. Functionally, both side of the condition actually do the exact same thing, but the side effects of which dimension is processed in the inner loop sometime have measurable consequences in terms of performances; I've added the following comment to the code to better explain this: // In order to minimize the number of time we have to resize the underlying // surface for capturing a tile, choose a dimension that has an exact divider // (if any) to be processed in the inner most loop. // E.g. looping on width then height in the example bellow requires four // surface resizing, whereas the opposite requires only two: // // for (w;;)for (h;;) // for(h;;) for(w;;) // - - // | | | | | | // | 1 | 3 | | 1 | 2 | //h | | |h | | | // - - // | 2 | 4 | | 3 | 4 | // - - // w w - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 02] RFR: 8238954: Improve performance of tiled snapshot rendering
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/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` I've made a change so that null can be passed to the method if the AtomicBoolean instance is not needed. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 03] 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: Fixed code style and typo following review. - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/9c5b0d4a..50ac8c22 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.02-03 Stats: 154 lines in 1 file changed: 52 ins; 85 del; 17 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
Re: RFR: 8238954: Improve performance of tiled snapshot rendering
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 | 20.608201 | 26.159817 | 33.599632 >> | 36.669261 | 43.042338 | 46.086088 | >> | 2048 | 9.752862 | 17.698869 | 27.004541 | 38.437578 | 52.297443 | >> 60.757880 | 68.101838 | 80.162117 | 93.852856 | >> | 3072 | 15.564961 | 27.304138 | 40.255866 | 56.636476 | 80.472402 | >> 86.346635 | 105.154089 | 121.048263 | 130.458981 | >> | 4096 | 19.436113 | 35.556343 | 53.277865 | 71.623899 | 95.814932 | >> 122.543003 | 136.833771 | 160.199834 | 178.356125 | >> | 5120 | 27.246498 | 65.875784 | 73.171492 | 103.380029 | 126.486761 | >> 147.666102 | 165.833885 | 199.005331 | >> 220.659671 | | 6144 | 31.843301 | 62.101937 | 93.646729 | 125.531512 | >> 150.914608 | 175.553034 | 209.835003 | >> 241.114596 | 253.512648 | | 7168 | 40.507918 | 70.843435 | 101.075064 | >> 137.284040 | 165.808501 | 197.015259 | >> 254.286955 | 304.928104 | 299.992601 | | 8192 | 43.206941 | 80.290957 | >> 121.946965 | 157.016439 | 193.509481 | >> 243.514969 | 268.151933 | 359.562281 | 352.102850 | | 9216 | 49.529493 | >> 90.895186 | 149.422784 | 179.512616 | >> 217.260338 | 267.610592 | 309.706685 | 354.950852 | 383.275751 | >>  > > I've uploaded 3 sets of results, from 3 different implementations: > > 1. **14-ea+9** is the implementation merged into openjfx14 following #68; it > only use tiling if the original > implementation fails; on Windows that would typically be when the snapshot > dimensions are larger than 8192 pixels. > 2. **14-internal** uses the same tiling implementation than the above, but > start using tiling as soon as snapshot > dimensions are larger than `PrismSettings.maxTextureSize`; i.e. typically > 4096 pixels. NB: This implementation was > never merged into openJFX; results are only provided for comparison's sake. > 3. **15-internal** uses the tiling implementation proposed in the PR. > Compared to the ones above, it attempt to align > pixel formats to avoid the cost of transformation from one format to another > (e.g. ByteBRGA to IntARGB) and it tries to > divide up the final snapshots into tiles of the same dimensions to prevent > creating a new GPU surface for every tile. > **Please note that these results are for the best possible scenario with > regard to the above optimizations; in the worst > case scenario (a user provided image with a different pixel format and no > way to divide the snapshot into equal size > tiles), then the performances are the same as that of implementation 2** > > My conclusions from the above results are twofold: > > - Tiling has a cost in terms of performance, but as far as I can tell it > remains the only practical way to work around > the underlying issue (i.e. taking snapshot larger than the supported > texture size) and the optimizations proposed in > this PR arguably do mitigate that cost, even if it is only true in some of > the cases. > > - The original implementation, which is preserved in 14-ea+9, ignores texture > clamping to 4096 which means it doesn't has > to resort to tiling until the target snapshot size is >8192 on d3d or 16384 > on es2. > If this is an incorrect behaviour on the part of the original implementation > (which I'm led to believe is the case), it > gives it an unfair advantage in this benchmark, i.e. it is faster because is > does things it shouldn't do. If however it > turns out it is actually safe to ignore clamping when taking snapshot, then > it would make sense to do so in the > implementation proposed by this PR as well. By the way, the code for the above benchmark is available here: https://github.com/fthevenet/snapshot-perf-meter Would it make sense to include it into the OpenJFX repo, maybe under jfx/apps/performance ? - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering
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 | 17.611105 | 38.898909 | 48.165735 >> | 53.596876 | 49.449740 | 66.032570 | >> | 2048 | 9.845097 | 17.799415 | 26.109529 | 34.607728 | 79.345622 | >> 94.082500 | 107.777644 | 100.901349 | 135.826890 | >> | 3072 | 14.654498 | 26.183649 | 39.781191 | 51.871491 | 113.010307 | >> 143.613631 | 184.883820 | 167.076202 | 200.852633 >> | | 4096 | 18.706278 | 36.115871 | 51.477296 | 68.457649 | 156.240888 | >> 186.159272 | 222.876505 | 237.387683 | >> 290.125942 | | 5120 | 50.566276 | 106.465632 | 140.506406 | 161.687151 | >> 203.644875 | 237.260330 | 279.108632 | >> 311.002566 | 371.704115 | | 6144 | 53.501341 | 106.726656 | 160.191733 | >> 216.969484 | 264.996201 | 287.375425 | >> 335.294473 | 365.035267 | 419.995978 | | 7168 | 66.422026 | 110.882355 | >> 187.978455 | 239.014528 | 308.817056 | >> 335.838550 | 394.270828 | 445.987300 | 506.974069 | | 8192 | 60.315442 | >> 108.770069 | 164.424088 | 205.330331 | >> 305.201833 | 343.846336 | 392.867668 | 454.540147 | 503.808112 | | 9216 | >> 71.070811 | 132.708328 | 188.411172 | >> 256.130225 | 320.028449 | 400.748559 | 471.542252 | 595.355103 | 589.240851 | >>  > > ### 15-internal: > > > || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | > |---|---|---|---|---|---|---|---|---|---| > | 1024 | 5.381051 | 9.261115 | 14.033219 | 20.608201 | 26.159817 | 33.599632 > | 36.669261 | 43.042338 | 46.086088 | > | 2048 | 9.752862 | 17.698869 | 27.004541 | 38.437578 | 52.297443 | 60.757880 > | 68.101838 | 80.162117 | 93.852856 | > | 3072 | 15.564961 | 27.304138 | 40.255866 | 56.636476 | 80.472402 | > 86.346635 | 105.154089 | 121.048263 | 130.458981 | > | 4096 | 19.436113 | 35.556343 | 53.277865 | 71.623899 | 95.814932 | > 122.543003 | 136.833771 | 160.199834 | 178.356125 | > | 5120 | 27.246498 | 65.875784 | 73.171492 | 103.380029 | 126.486761 | > 147.666102 | 165.833885 | 199.005331 | > 220.659671 | | 6144 | 31.843301 | 62.101937 | 93.646729 | 125.531512 | > 150.914608 | 175.553034 | 209.835003 | > 241.114596 | 253.512648 | | 7168 | 40.507918 | 70.843435 | 101.075064 | > 137.284040 | 165.808501 | 197.015259 | > 254.286955 | 304.928104 | 299.992601 | | 8192 | 43.206941 | 80.290957 | > 121.946965 | 157.016439 | 193.509481 | > 243.514969 | 268.151933 | 359.562281 | 352.102850 | | 9216 | 49.529493 | > 90.895186 | 149.422784 | 179.512616 | > 217.260338 | 267.610592 | 309.706685 | 354.950852 | 383.275751 | >  I've uploaded 3 sets of results, from 3 different implementations: 1. **14-ea+9** is the implementation merged into openjfx14 following #68; it only use tiling if the original implementation fails; on Windows that would typically be when the snapshot dimensions are larger than 8192 pixels. 2. **14-internal** uses the same tiling implementation than the above, but start using tiling as soon as snapshot dimensions are larger than `PrismSettings.maxTextureSize`; i.e. typically 4096 pixels. NB: This implementation was never merged into openJFX; results are only provided for comparison's sake. 3. **15-internal** uses the tiling implementation proposed in the PR. Compared to the ones above, it attempt to align pixel formats to avoid the cost of transformation from one format to another (e.g. ByteBRGA to IntARGB) and it tries to divide up the final snapshots into tiles of the same dimensions to prevent creating a new GPU surface for every tile. **Please note that these results are for the best possible scenario with regard to the above optimizations; in the worst case scenario (a user provided image with a different pixel format and no way to divide the snapshot into equal size tiles), then the performances are the same as that of implementation 2** My conclusions from the above results are twofold: - Tiling has a cost in terms of performance, but as far as I can tell it remains the only practical way to work around the underlying issue (i.e. taking snapshot larger than the supported texture size) and the optimizations proposed in this PR arguably do mitigate that cost, even if it is only true in some of the cases. - The original implementation, which is preserved in 14-ea+9, ignores texture clamping to 4096 which means it doesn't has to resort to tiling until the target snapshot size is >8192 on d3d or 16384 on
Re: RFR: 8238954: Improve performance of tiled snapshot rendering
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 | 21.776304 | 27.918815 >> | 36.929480 | 35.647317 | 74.274598 | >> | 2048 | 11.271960 | 17.774451 | 26.468146 | 34.555745 | 42.699573 | >> 52.027568 | 74.481450 | 68.349699 | 135.902502 | >> | 3072 | 13.953309 | 26.320822 | 42.652327 | 51.534650 | 65.202531 | >> 78.406419 | 103.512338 | 109.125337 | 214.305998 | >> | 4096 | 17.776236 | 35.800101 | 57.483720 | 70.221125 | 98.024908 | >> 106.510784 | 109.940266 | 150.731465 | 265.632840 | >> | 5120 | 22.756799 | 44.809926 | 69.324931 | 85.589418 | 106.649450 | >> 137.122404 | 146.470563 | 149.845737 | 371.508604 >> | | 6144 | 26.218716 | 53.103478 | 78.157447 | 122.467283 | 129.644202 | >> 162.408603 | 160.861559 | 172.865197 | >> 460.078507 | | 7168 | 31.095109 | 60.251328 | 91.299751 | 133.693702 | >> 143.813523 | 154.496892 | 171.049670 | >> 186.519441 | 527.541763 | | 8192 | 34.609033 | 71.258569 | 107.084557 | >> 143.252194 | 163.108854 | 165.619014 | >> 178.134480 | 220.085044 | 514.600669 | | 9216 | 69.133873 | 135.713363 | >> 198.200679 | 277.606301 | 355.598066 | >> 480.710904 | 450.875668 | 577.701980 | 623.690198 | >>  > > ### 14-internal > > > || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | > |---|---|---|---|---|---|---|---|---|---| > | 1024 | 5.740508 | 9.337537 | 13.489849 | 17.611105 | 38.898909 | 48.165735 > | 53.596876 | 49.449740 | 66.032570 | > | 2048 | 9.845097 | 17.799415 | 26.109529 | 34.607728 | 79.345622 | 94.082500 > | 107.777644 | 100.901349 | 135.826890 | > | 3072 | 14.654498 | 26.183649 | 39.781191 | 51.871491 | 113.010307 | > 143.613631 | 184.883820 | 167.076202 | 200.852633 > | | 4096 | 18.706278 | 36.115871 | 51.477296 | 68.457649 | 156.240888 | > 186.159272 | 222.876505 | 237.387683 | > 290.125942 | | 5120 | 50.566276 | 106.465632 | 140.506406 | 161.687151 | > 203.644875 | 237.260330 | 279.108632 | > 311.002566 | 371.704115 | | 6144 | 53.501341 | 106.726656 | 160.191733 | > 216.969484 | 264.996201 | 287.375425 | > 335.294473 | 365.035267 | 419.995978 | | 7168 | 66.422026 | 110.882355 | > 187.978455 | 239.014528 | 308.817056 | > 335.838550 | 394.270828 | 445.987300 | 506.974069 | | 8192 | 60.315442 | > 108.770069 | 164.424088 | 205.330331 | > 305.201833 | 343.846336 | 392.867668 | 454.540147 | 503.808112 | | 9216 | > 71.070811 | 132.708328 | 188.411172 | > 256.130225 | 320.028449 | 400.748559 | 471.542252 | 595.355103 | 589.240851 | >  ### 15-internal: || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | |---|---|---|---|---|---|---|---|---|---| | 1024 | 5.381051 | 9.261115 | 14.033219 | 20.608201 | 26.159817 | 33.599632 | 36.669261 | 43.042338 | 46.086088 | | 2048 | 9.752862 | 17.698869 | 27.004541 | 38.437578 | 52.297443 | 60.757880 | 68.101838 | 80.162117 | 93.852856 | | 3072 | 15.564961 | 27.304138 | 40.255866 | 56.636476 | 80.472402 | 86.346635 | 105.154089 | 121.048263 | 130.458981 | | 4096 | 19.436113 | 35.556343 | 53.277865 | 71.623899 | 95.814932 | 122.543003 | 136.833771 | 160.199834 | 178.356125 | | 5120 | 27.246498 | 65.875784 | 73.171492 | 103.380029 | 126.486761 | 147.666102 | 165.833885 | 199.005331 | 220.659671 | | 6144 | 31.843301 | 62.101937 | 93.646729 | 125.531512 | 150.914608 | 175.553034 | 209.835003 | 241.114596 | 253.512648 | | 7168 | 40.507918 | 70.843435 | 101.075064 | 137.284040 | 165.808501 | 197.015259 | 254.286955 | 304.928104 | 299.992601 | | 8192 | 43.206941 | 80.290957 | 121.946965 | 157.016439 | 193.509481 | 243.514969 | 268.151933 | 359.562281 | 352.102850 | | 9216 | 49.529493 | 90.895186 | 149.422784 | 179.512616 | 217.260338 | 267.610592 | 309.706685 | 354.950852 | 383.275751 |  - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering
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-578192074) >> for previous PR > > ### 14-ea+9 > > > || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | > |---|---|---|---|---|---|---|---|---|---| > | 1024 | 5.607827 | 9.380503 | 13.835523 | 17.514362 | 21.776304 | 27.918815 > | 36.929480 | 35.647317 | 74.274598 | > | 2048 | 11.271960 | 17.774451 | 26.468146 | 34.555745 | 42.699573 | > 52.027568 | 74.481450 | 68.349699 | 135.902502 | > | 3072 | 13.953309 | 26.320822 | 42.652327 | 51.534650 | 65.202531 | > 78.406419 | 103.512338 | 109.125337 | 214.305998 | > | 4096 | 17.776236 | 35.800101 | 57.483720 | 70.221125 | 98.024908 | > 106.510784 | 109.940266 | 150.731465 | 265.632840 | > | 5120 | 22.756799 | 44.809926 | 69.324931 | 85.589418 | 106.649450 | > 137.122404 | 146.470563 | 149.845737 | 371.508604 > | | 6144 | 26.218716 | 53.103478 | 78.157447 | 122.467283 | 129.644202 | > 162.408603 | 160.861559 | 172.865197 | > 460.078507 | | 7168 | 31.095109 | 60.251328 | 91.299751 | 133.693702 | > 143.813523 | 154.496892 | 171.049670 | > 186.519441 | 527.541763 | | 8192 | 34.609033 | 71.258569 | 107.084557 | > 143.252194 | 163.108854 | 165.619014 | > 178.134480 | 220.085044 | 514.600669 | | 9216 | 69.133873 | 135.713363 | > 198.200679 | 277.606301 | 355.598066 | > 480.710904 | 450.875668 | 577.701980 | 623.690198 | >  ### 14-internal || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | |---|---|---|---|---|---|---|---|---|---| | 1024 | 5.740508 | 9.337537 | 13.489849 | 17.611105 | 38.898909 | 48.165735 | 53.596876 | 49.449740 | 66.032570 | | 2048 | 9.845097 | 17.799415 | 26.109529 | 34.607728 | 79.345622 | 94.082500 | 107.777644 | 100.901349 | 135.826890 | | 3072 | 14.654498 | 26.183649 | 39.781191 | 51.871491 | 113.010307 | 143.613631 | 184.883820 | 167.076202 | 200.852633 | | 4096 | 18.706278 | 36.115871 | 51.477296 | 68.457649 | 156.240888 | 186.159272 | 222.876505 | 237.387683 | 290.125942 | | 5120 | 50.566276 | 106.465632 | 140.506406 | 161.687151 | 203.644875 | 237.260330 | 279.108632 | 311.002566 | 371.704115 | | 6144 | 53.501341 | 106.726656 | 160.191733 | 216.969484 | 264.996201 | 287.375425 | 335.294473 | 365.035267 | 419.995978 | | 7168 | 66.422026 | 110.882355 | 187.978455 | 239.014528 | 308.817056 | 335.838550 | 394.270828 | 445.987300 | 506.974069 | | 8192 | 60.315442 | 108.770069 | 164.424088 | 205.330331 | 305.201833 | 343.846336 | 392.867668 | 454.540147 | 503.808112 | | 9216 | 71.070811 | 132.708328 | 188.411172 | 256.130225 | 320.028449 | 400.748559 | 471.542252 | 595.355103 | 589.240851 |  - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering
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 >> swapping around pixel components, like under d3d. I've also verified than >> the patch's behaviour is correct when a >> writable image is provided as a parameter to Node.snapshot, whether the >> underlying image is actually a PlatformImage or >> a wrapper for a PixelBuffer, which corrects another limitation of the >> previous implementation. > > 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-578192074) > for previous PR ### 14-ea+9 || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 |9216 | |---|---|---|---|---|---|---|---|---|---| | 1024 | 5.607827 | 9.380503 | 13.835523 | 17.514362 | 21.776304 | 27.918815 | 36.929480 | 35.647317 | 74.274598 | | 2048 | 11.271960 | 17.774451 | 26.468146 | 34.555745 | 42.699573 | 52.027568 | 74.481450 | 68.349699 | 135.902502 | | 3072 | 13.953309 | 26.320822 | 42.652327 | 51.534650 | 65.202531 | 78.406419 | 103.512338 | 109.125337 | 214.305998 | | 4096 | 17.776236 | 35.800101 | 57.483720 | 70.221125 | 98.024908 | 106.510784 | 109.940266 | 150.731465 | 265.632840 | | 5120 | 22.756799 | 44.809926 | 69.324931 | 85.589418 | 106.649450 | 137.122404 | 146.470563 | 149.845737 | 371.508604 | | 6144 | 26.218716 | 53.103478 | 78.157447 | 122.467283 | 129.644202 | 162.408603 | 160.861559 | 172.865197 | 460.078507 | | 7168 | 31.095109 | 60.251328 | 91.299751 | 133.693702 | 143.813523 | 154.496892 | 171.049670 | 186.519441 | 527.541763 | | 8192 | 34.609033 | 71.258569 | 107.084557 | 143.252194 | 163.108854 | 165.619014 | 178.134480 | 220.085044 | 514.600669 | | 9216 | 69.133873 | 135.713363 | 198.200679 | 277.606301 | 355.598066 | 480.710904 | 450.875668 | 577.701980 | 623.690198 |  - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 01] RFR: 8238954: Improve performance of tiled snapshot rendering
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 - yMin, 1);
>> 1297: if (wimg == null) {
>
> The changes here do not need to be reverted, this is still extra calculation.
Well spotted. I've re-applied that change.
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 02] 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. The pull request has been updated with 1 additional commit. - Added commits: - 9c5b0d4a: Avoid useless width and height calculation Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/9ec2d25c..9c5b0d4a Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.01-02 Stats: 8 lines in 1 file changed: 4 ins; 2 del; 2 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
Re: [Rev 01] 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. The pull request has been updated with 1 additional commit. - Added commits: - 9ec2d25c: Added comments to computeOptimumTileSize. Also, Ensure isDivExact is set to false if needed. Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/e13b5a06..9ec2d25c Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.00-01 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 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
Re: [Rev 01] RFR: 8238954: Improve performance of tiled snapshot rendering
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 maxSize,
>> AtomicBoolean isDivExact) {
>> 1527: for (int n = 2; n <= 6; n++) {
>> 1528: int optimumSize = size / n;
>
> It would be helpful if you add explanation about the constants 2 and 6 here.
Good point.
I've added the following comment:
>// This methods attempts to find the smallest integer divider for the provided
>`size`
>// while the result of the division is less than `maxSize`.
>// It tests all potential dividers from 2 to 6 and returns the result of the
>division
>// if all conditions can be satisfied or, failing that, `maxSize`.
>// The value for `isDivExact` reflects whether or not an exact divider could
>be found.
Please note that my choice of 6 for the largest attempted divider is totally
arbitrary; we ideally want to maximize the chances to find an exact divider,
but we also need to avoid making the tiles too small in the process and it
seemed a good compromise.
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering
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 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. > > I've marked this PR as a WIP for the time being because I've only tested it > on the d3d rendering pipeline so far, so I think it is too early to call for > a formal review yet. > Still, I welcome feedback if someone wants to have a look at it already. > > In a nutshell, this is what this PR does: > > - Reverts changes from 8088198 > - Implements tiling within `QuantumToolkit::renderToImage` instead > - Gets the maxTextureSize directly from the `ResourceFactory` instance > instead of relying on `PrismSettings.maxTextureSize` (which may be wrong in > the event the maxTexture size supported by the hardware is less than the > clamped value set in PrismSettings) > - Tries to align the PixelFomat for the target `WritableImage` with that of > the `RTTexture` when possible, since converting IntARGB to ByteBGRA (or the > other way round) is a major cost factor when copying large amounts of pixels. > - Avoids as much as possible having to resize the tile in between subsequent > calls to `RTTexture::readPixels`, as it is another major cost factor, under > the d3d rendering pipeline at least. (Native method > `D3DResourceFactory_nReadPixels` attempts to reuse the same surface in > between calls but is forced to create a new one if dimensions are not exactly > the same, which is quite costly). > > TODO: > > - [x] Make sure chosen PixelFormat is optimum when using es2 pipeline. > - [ ] Consider using subtexture pixel read with es2 (SW and d3d don't support > it) as a better alternative to relying on same size tiles to avoid surface > thrashing. 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 swapping around pixel components, like under d3d. I've also verified than the patch's behaviour is correct when a writable image is provided as a parameter to Node.snapshot, whether the underlying image is actually a PlatformImage or a wrapper for a PixelBuffer, which corrects another limitation of the previous implementation. - PR: https://git.openjdk.java.net/jfx/pull/112
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. - Commits: - e13b5a06: Use tiling in QuantumToolkit::renderToImage when requested image is larger than max texture size - b914a48d: Revert "8088198: Exception thrown from snapshot if dimensions are larger than max texture size" Changes: https://git.openjdk.java.net/jfx/pull/112/files Webrev: https://webrevs.openjdk.java.net/jfx/112/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8238954 Stats: 173 lines in 2 files changed: 84 ins; 50 del; 39 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
Re: RFR: 8238954: Improve performance of tiled snapshot rendering
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. > 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. I've marked this PR as a WIP for the time being because I've only tested it on the d3d rendering pipeline so far, so I think it is too early to call for a formal review yet. Still, I welcome feedback if someone wants to have a look at it already. In a nutshell, this is what this PR does: - Reverts changes from 8088198 - Implements tiling within `QuantumToolkit::renderToImage` instead - Gets the maxTextureSize directly from the `ResourceFactory` instance instead of relying on `PrismSettings.maxTextureSize` (which may be wrong in the event the maxTexture size supported by the hardware is less than the clamped value set in PrismSettings) - Tries to align the PixelFomat for the target `WritableImage` with that of the `RTTexture` when possible, since converting IntARGB to ByteBGRA (or the other way round) is a major cost factor when copying large amounts of pixels. - Avoids as much as possible having to resize the tile in between subsequent calls to `RTTexture::readPixels`, as it is another major cost factor, under the d3d rendering pipeline at least. (Native method `D3DResourceFactory_nReadPixels` attempts to reuse the same surface in between calls but is forced to create a new one if dimensions are not exactly the same, which is quite costly). TODO: - Make sure chosen PixelFormat is optimum when using es2 pipeline. - Consider using subtexture pixel read with es2 (SW and d3d don't support it) as a better alternative to relying on same size tiles to avoid surface thrashing. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 04] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 ready for you to integrate. The `rfr` label is intentionally > left there, since there might be review comments or questions that are still > being addressed. In this case, there are no outstanding questions, so go > ahead and integrate it. Either @arapte or I will sponsor it. Ok, thank you for the clarification. /integrate - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 04] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 dimensions of the > `WritableImage`. > We can not create a `WritableImage` of dimension `(8192 * 3, 8192 * 3)` or > greater. > `new WritableImage(8192 * 3, 8192 * 3)` causes an exception. > java.lang.IllegalArgumentException: capacity < 0: (-1879048192 < 0) > at java.base/java.nio.Buffer.createCapacityException(Buffer.java:257) > This is an existing behavior of `WritableImage`. > May be we should consider to wrap and re-throw the exception and update the > API JavaDoc. > Anyway not a stopper for this PR. > Approving. 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? - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 04] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 reinforces the >> decision to only try tiling as a fallback when an exception occurs, since >> there could otherwise be functional regressions in addition to performance >> regressions. >> >> With the latest update to the PR, any case that works today will continue to >> behave exactly as it does today. The worst that will happen is that the >> tiling might produce very slightly different results for interpolated >> colors, or possibly fail with a different exception (e.g., if a 4K x 4K >> texture is still too big for some hypothetical device, or if the user passes >> in a `WritableImage` created with a `PixelBuffer` (see below)). >> >> I added a verbose println to snapshot to validate my testing: >> >> 1. Snapshot with image size = 5k x 5k (should still fit on Windows D3D with >> no tiling) >> 2. Snapshot with image size = 20k x 3k (should tile in X) >> 3. Snapshot with image size = 3K x 20k (should tile in Y) >> 4. Snapshot with image size = 20K x 20k (should tile in both X and Y) >> >> RESULT: All of the above work as expected >> >> 5. Modify the jfx runtime to force tiling, set the tile size to 128 bytes, >> and run various tests (e.g., a stress test of tiling) >> RESULT: Everything looks visually correct, but four of the tests in >> `test.robot.test3d.Snapshot3DTest` fail with a color mismatch [can be >> addressed in a follow-up bug] >> >> 6. Pass in a `WritableImage` created with a `PixelBuffer` object (NIO buffer) >> RESULT: It works without tiling and throws an exception with tiling [can be >> addressed in a follow-up bug] >> >> 7. Pass in a larger than needed `WritableImage` (e.g., using a viewport that >> is large enough to cause tiling, but smaller than the passed in image) into >> snapshot when using tiling. Is the entire image cleared? >> RESULT: I still need to run this test, but even if it doesn't work correctly >> when tiling, it wouldn't be a regression, and could be addressed in a >> follow-up bug. >> >> Issues to file for follow-on bug(s): >> 1. Improve performance of tiled snapshot rendering >> * As part of this, we need to get max texture size from the toolkit >> * Consider moving the tiled rendering into `QuantumRenderer` thread >> * Reuse a single temporary buffer rather than creating a new one for >> each tile >> 2. Passing in a `WritableImage` created with a `PixelBuffer` object (NIO >> buffer) fails if tiling is needed. I'll file this one. >> 3. Pixel accuracy issues with tiled image; this can be seen in >> `Snapshot3DTest` by instrumenting the code to force tiling with a tile size >> of 128. It looks OK visually, but fails the color comparison test. I'll file >> this one. >> 4. Presuming that the above test 7 produces incorrect results, file a >> follow-up bug to fix it. > >> Improve performance of tiled snapshot rendering > > I'll have a closer look at improving `IntTo4ByteSameConverter::doConvert`. Yes, I'm fine with working further on this and the related issues for openjfx15, and I have fairly good idea on how to approach it. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 render the entire >> image (as is done today) in a try / catch block, and only fall back to >> tiling if there is an exception. That way there would be no concern over any >> possible performance regression, since the only time we would use tiling >> would be in the case where it fials today. >> >> What do you think? > > I have tested using a recipient WritableImage with an IntARGB pixel format > (so that is aligned with that of the tile), that I construct by supplying a > PixelBuffer, and as expected that performance of the tiled code is much more > in line with that of the non-tiled version. One overhead left is the > allocation of the extra buffer, but it is far less significant. > The problem with this approach is that the "best" pixel format (as in similar > to that of RRT) isn't the same depending on the rendering pipeline (e.g. d3d > is intARGB while es2 is byteBGRA) so that would require adding a fair amount > of code to determine the best possible scenario depending on all the > constraints (keeping in mind the caller can also provide its own > WritableImage...) that in turn would need thorough testing. > All in all, I agree the risk of regression is probably too great for this to > make it into openjfx14 with so little time left. > > Instead, I will prototype Kevin's proposal to only use tiling when it would > fail with the current code and use what I've learned here to propose a more > robust fix targeted at openjfx15 I've made the change suggested by Kevin, and it works as expected. It is not very elegant but it does provide relief with regard to the core issue while avoiding risks of performance regressions. Now with regard to a follow-up PR targeted at the next release, I assume a new issue needs first be filed into the JBS first; should I just file a new bug via https://bugreport.java.com/bugreport (I don't have access to https://bugs.openjdk.java.net)? - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 04] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
> 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 > tiles into a a single image. > Other than that, the logic used to do the actual snapshot is unchanged. > > Tests using the existing SnapshotCommon class have been added in a new file > named Snapshot3Test under SystemTest/test/javafx/scene. > These tests pass with the proposed fix, and fail without, throwing " > java.lang.IllegalArgumentException: Unrecognized image loader: null" The pull request has been updated with 1 additional commit. - Added commits: - fce986e9: Only attempt tiling on exception Changes: - all: https://git.openjdk.java.net/jfx/pull/68/files - new: https://git.openjdk.java.net/jfx/pull/68/files/8966936f..fce986e9 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/68/webrev.04 - incr: https://webrevs.openjdk.java.net/jfx/68/webrev.03-04 Stats: 12 lines in 1 file changed: 4 ins; 4 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/68.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/68/head:pull/68 PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 >> performance regressions for those cases that will now be tiled, but formerly >> weren't, I no longer think this is a good candidate for openjfx14. This PR >> should be retargeted to the `master` branch for openjfx15. > > 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 render the entire > image (as is done today) in a try / catch block, and only fall back to tiling > if there is an exception. That way there would be no concern over any > possible performance regression, since the only time we would use tiling > would be in the case where it fials today. > > What do you think? I have tested using a recipient WritableImage with an IntARGB pixel format (so that is aligned with that of the tile), that I construct by supplying a PixelBuffer, and as expected that performance of the tiled code is much more in line with that of the non-tiled version. One overhead left is the allocation of the extra buffer, but it is far less significant. The problem with this approach is that the "best" pixel format (as in similar to that of RRT) isn't the same depending on the rendering pipeline (e.g. d3d is intARGB while es2 is byteBGRA) so that would require adding a fair amount of code to determine the best possible scenario depending on all the constraints (keeping in mind the caller can also provide its own WritableImage...) that in turn would need thorough testing. All in all, I agree the risk of regression is probably too great for this to make it into openjfx14 with so little time left. Instead, I will prototype Kevin's proposal to only use tiling when it would fail with the current code and use what I've learned here to propose a more robust fix targeted at openjfx15 - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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`).
>>
>> Where did you see these?
>>
>>> Unfortunately it seems the only way to choose the pixel format for a
>>> `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
>>> no longer use a `PixelWriter` to update it...
>>
>> You can update it with `PixelBuffer#updateBuffer`. I think that you will
>> want to pass the stitched tile as the dirty region.
>
>>
>>
>> > 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`).
>>
>> Where did you see these?
>
> Simply by watching the value of `tile.getPixelReader().getPixelFormat()` and
> `wimg.getPixelWriter().getPixelFormat()` before doing the copy at
> https://github.com/openjdk/jfx/blob/4bc4417356ebd639567d315257a6bbe11344d9c2/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1315
>>
>> > Unfortunately it seems the only way to choose the pixel format for a
>> > `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
>> > no longer use a `PixelWriter` to update it...
>>
>> You can update it with `PixelBuffer#updateBuffer`. I think that you will
>> want to pass the stitched tile as the dirty region.
>
> Thanks. I'll look into it.
>
>
> > 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 result, but you can try to replace the
> loops with this:
>
> ```
> IntStream.range(0, h).parallel().forEach(y -> {
> IntStream.range(0, w).parallel().forEach(x -> {
> int pixel = srcarr[srcoff++];
> dstarr[dstoff++] = (byte) (pixel );
> dstarr[dstoff++] = (byte) (pixel >> 8);
> dstarr[dstoff++] = (byte) (pixel >> 16);
> dstarr[dstoff++] = (byte) (pixel >> 24);
> });
> srcoff += srcscanints;
> dstoff += dstscanbytes;
> });
> ```
I don't think this works as it is, as all threads race to increment `srcoff`
and `dstoff`.
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 result, but you can try to replace the
>> loops with this:
>> IntStream.range(0, h).parallel().forEach(y -> {
>> IntStream.range(0, w).parallel().forEach(x -> {
>> int pixel = srcarr[srcoff++];
>> dstarr[dstoff++] = (byte) (pixel );
>> dstarr[dstoff++] = (byte) (pixel >> 8);
>> dstarr[dstoff++] = (byte) (pixel >> 16);
>> dstarr[dstoff++] = (byte) (pixel >> 24);
>> });
>> srcoff += srcscanints;
>> dstoff += dstscanbytes;
>> });
>
>> 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`).
>
> Where did you see these?
>
>> Unfortunately it seems the only way to choose the pixel format for a
>> `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
>> no longer use a `PixelWriter` to update it...
>
> You can update it with `PixelBuffer#updateBuffer`. I think that you will want
> to pass the stitched tile as the dirty region.
>
>
> > 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`).
>
> Where did you see these?
Simply by watching the value of `tile.getPixelReader().getPixelFormat()` and
`wimg.getPixelWriter().getPixelFormat()` before doing the copy at
https://github.com/openjdk/jfx/blob/4bc4417356ebd639567d315257a6bbe11344d9c2/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1315
>
> > Unfortunately it seems the only way to choose the pixel format for a
> > `WritableImage` is to initialize it with a `PixelBuffer`, but then one can
> > no longer use a `PixelWriter` to update it...
>
> You can update it with `PixelBuffer#updateBuffer`. I think that you will want
> to pass the stitched tile as the dirty region.
Thanks. I'll look into it.
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 the >>> average time* spent (in ms) in `Node::snapshot` >>> (*) each test is ran 10 times and the elapsed time is averaged after >>> pruning outliers, using Grubbs' test. >>> >>> 10242048307240965120614471688192 >>> 10246.30427210.303935 15.052336 35.929304 >>> 23.860095 28.828812 35.315288 27.867205 >>> 204811.544367 21.156326 28.368750 28.887164 >>> 47.134738 54.354708 55.480251 56.722649 >>> 307215.503187 30.215269 41.304645 39.789648 >>> 82.255091 82.576379 96.618722 106.586547 >>> 409620.928336 38.768648 64.255423 52.608217 >>> 101.797347 132.516816 158.525192 166.872889 >>> 512028.693431 67.275306 68.090280 76.208412 >>> 133.974510 157.120373 182.329784 210.069066 >>> 614429.972591 54.751002 88.171906 104.489291 >>> 147.788597 185.185643 213.562819 228.643761 >>> 716833.668398 63.088490 98.756212 130.502678 >>> 196.367121 225.166481 239.328794 260.162501 >>> 819240.961901 87.067460 128.230351 178.127225 >>> 198.479068 225.806211 266.170239 325.967840 >> >> Any idea why 4096x1024 and 1024x4096 are so different? Same for 8192x1024 >> and 1024x8192. > > I don't, to be honest. > The results for some dimensions (not always the same) can vary pretty widely > from one run to another, despite all my effort to repeat results and remove > outliers. > Out of curiosity, I also tried to eliminate the GC as possible culprit by > running it with epsilon, but it seems to make a significant difference. > I ran that test on a laptop with Integrated Intel graphics and no dedicated > vram (Intel UHD Graphics 620), though, so this might be why. > Maybe someone could try and run the bench on hardware with a discreet GPU? With regard to why the tiling version is significantly slower, though, I do have a pretty good idea; as Kevin hinted, the pixel copy into a temporary buffer before copying into the final image is where most the extra time is spent. The reason why is is some much slower is a little bit of a pity, though; profiling a run of the benchmark shows that a lot of time is spent into `IntTo4ByteSameConverter::doConvert` and the reason for this turns out that this is due to the fact that, under Windows and the D3D pipeline anyway, 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`). So if we could use a `WriteableImage` with an IntARGB pixel format as the recipient for the snapshot (at least as long as no image was provided by the caller), I suspect that the copy would be much faster. Unfortunately it seems the only way to choose the pixel format for a `WritableImage` is to initialize it with a `PixelBuffer`, but then one can no longer use a `PixelWriter` to update it and it desn't seems to me that there is a way to safely access the `PixelBuffer` from an image's reference alone. I'm pretty new to this code base though (which is quite large; I haven't read it all quite yet... ;-), so hopefully there's a way to do that that has eluded me so far. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 | 14.721024 | 19.279638 | 47.508266 | >> 56.585089 | 64.522117 | 53.448326 | >> | 2048 | 10.990480 | 19.284461 | 28.235822 | 41.067555 | 92.818088 | >> 106.782334 | 120.907050 | 112.852554 | >> | 3072 | 15.642648 | 28.502151 | 59.541998 | 55.663658 | 130.654226 | >> 149.805330 | 205.212356 | 169.002232 | >> | 4096 | 19.882252 | 41.287722 | 59.493687 | 73.809264 | 169.212467 | >> 200.212097 | 247.934609 | 246.412543 | >> | 5120 | 49.986204 | 97.986781 | 126.127089 | 167.274104 | 217.063815 | >> 276.812929 | 307.276073 | 366.800463 | >> | 6144 | 66.546156 | 104.339809 | 150.171765 | 206.282107 | 272.486419 | >> 321.178983 | 365.822047 | 410.087087 | >> | 7168 | 66.894654 | 119.510866 | 176.002883 | 248.937222 | 314.721516 | >> 380.834398 | 430.858648 | 482.499047 | >> | 8192 | 67.040207 | 112.831977 | 161.852173 | 237.588749 | 319.667719 | >> 382.151556 | 437.810832 | 451.865266 | > >> 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 the >> average time* spent (in ms) in `Node::snapshot` >> (*) each test is ran 10 times and the elapsed time is averaged after pruning >> outliers, using Grubbs' test. >> >> 1024 2048307240965120614471688192 >> 1024 6.30427210.303935 15.052336 35.929304 >> 23.860095 28.828812 35.315288 27.867205 >> 2048 11.544367 21.156326 28.368750 28.887164 >> 47.134738 54.354708 55.480251 56.722649 >> 3072 15.503187 30.215269 41.304645 39.789648 >> 82.255091 82.576379 96.618722 106.586547 >> 4096 20.928336 38.768648 64.255423 52.608217 >> 101.797347 132.516816 158.525192 166.872889 >> 5120 28.693431 67.275306 68.090280 76.208412 >> 133.974510 157.120373 182.329784 210.069066 >> 6144 29.972591 54.751002 88.171906 104.489291 >> 147.788597 185.185643 213.562819 228.643761 >> 7168 33.668398 63.088490 98.756212 130.502678 >> 196.367121 225.166481 239.328794 260.162501 >> 8192 40.961901 87.067460 128.230351 178.127225 >> 198.479068 225.806211 266.170239 325.967840 > > Any idea why 4096x1024 and 1024x4096 are so different? Same for 8192x1024 and > 1024x8192. I don't, to be honest. The results for some dimensions (not always the same) can vary pretty widely from one run to another, despite all my effort to repeat results and remove outliers. Out of curiosity, I also tried to eliminate the GC as possible culprit by running it with epsilon, but it seems to make a significant difference. I ran that test on a laptop with Integrated Intel graphics and no dedicated vram (Intel UHD Graphics 620), though, so this might be why. Maybe someone could try and run the bench on hardware with a discreet GPU? - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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: >> https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java > > 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 the average > time* spent (in ms) in `Node::snapshot` > (*) each test is ran 10 times and the elapsed time is averaged after pruning > outliers, using Grubbs' test. > > || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 | > |---|---|---|---|---|---|---|---|---| > | 1024 | 6.304272 | 10.303935 | 15.052336 | 35.929304 | 23.860095 | 28.828812 > | 35.315288 | 27.867205 | > | 2048 | 11.544367 | 21.156326 | 28.368750 | 28.887164 | 47.134738 | > 54.354708 | 55.480251 | 56.722649 | > | 3072 | 15.503187 | 30.215269 | 41.304645 | 39.789648 | 82.255091 | > 82.576379 | 96.618722 | 106.586547 | > | 4096 | 20.928336 | 38.768648 | 64.255423 | 52.608217 | 101.797347 | > 132.516816 | 158.525192 | 166.872889 | > | 5120 | 28.693431 | 67.275306 | 68.090280 | 76.208412 | 133.974510 | > 157.120373 | 182.329784 | 210.069066 | > | 6144 | 29.972591 | 54.751002 | 88.171906 | 104.489291 | 147.788597 | > 185.185643 | 213.562819 | 228.643761 | > | 7168 | 33.668398 | 63.088490 | 98.756212 | 130.502678 | 196.367121 | > 225.166481 | 239.328794 | 260.162501 | > | 8192 | 40.961901 | 87.067460 | 128.230351 | 178.127225 | 198.479068 | > 225.806211 | 266.170239 | 325.967840 | 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 | 14.721024 | 19.279638 | 47.508266 | 56.585089 | 64.522117 | 53.448326 | | 2048 | 10.990480 | 19.284461 | 28.235822 | 41.067555 | 92.818088 | 106.782334 | 120.907050 | 112.852554 | | 3072 | 15.642648 | 28.502151 | 59.541998 | 55.663658 | 130.654226 | 149.805330 | 205.212356 | 169.002232 | | 4096 | 19.882252 | 41.287722 | 59.493687 | 73.809264 | 169.212467 | 200.212097 | 247.934609 | 246.412543 | | 5120 | 49.986204 | 97.986781 | 126.127089 | 167.274104 | 217.063815 | 276.812929 | 307.276073 | 366.800463 | | 6144 | 66.546156 | 104.339809 | 150.171765 | 206.282107 | 272.486419 | 321.178983 | 365.822047 | 410.087087 | | 7168 | 66.894654 | 119.510866 | 176.002883 | 248.937222 | 314.721516 | 380.834398 | 430.858648 | 482.499047 | | 8192 | 67.040207 | 112.831977 | 161.852173 | 237.588749 | 319.667719 | 382.151556 | 437.810832 | 451.865266 | - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 guaranteed to be that size. The >> `Prism.maxTexture` constant represents the value to clamp the texture size >> to. In the event that D3D or OpenGL were to report a maximum less than the >> value of `Prism.maxTexture` (unlikely, but maybe on some low-end embedded >> systems?), then that value is what should be used. The way to get the >> clamped texture size is to call >> [`ResourceFactory::getMaximumTextureSize`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/prism/ResourceFactory.java#L222), >> but you can't do that directly from the scene graph code. >> >> 2. Allocating multiple `WritableImage` objects and using >> `PixelWriter::setPixels` to stitch them together will take more temporary >> memory, and is likely to be slower, than having the snapshot code write into >> a subimage of the larger allocated image directly. >> >> Having said that, the current proposed solution is still better than the >> current behavior is almost all cases, although there could be a performance >> hit in the case of an 8K x 8K image, which will now be tiled. I want to do a >> more careful review and some testing. If any other users of snapshot have >> any comments or concerns, they would be welcome. >> >> I think the best long-term solution might be to modify the >> [`QuantumToolkit::renderToImage`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java#L1490) >> method, although that would certainly be out of scope for JavaFX 14. > > 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: > https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java 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 the average time* spent (in ms) in `Node::snapshot` (*) each test is ran 10 times and the elapsed time is averaged after pruning outliers, using Grubbs' test. || 1024 |2048 |3072 |4096 |5120 |6144 |7168 |8192 | |---|---|---|---|---|---|---|---|---| | 1024 | 6.304272 | 10.303935 | 15.052336 | 35.929304 | 23.860095 | 28.828812 | 35.315288 | 27.867205 | | 2048 | 11.544367 | 21.156326 | 28.368750 | 28.887164 | 47.134738 | 54.354708 | 55.480251 | 56.722649 | | 3072 | 15.503187 | 30.215269 | 41.304645 | 39.789648 | 82.255091 | 82.576379 | 96.618722 | 106.586547 | | 4096 | 20.928336 | 38.768648 | 64.255423 | 52.608217 | 101.797347 | 132.516816 | 158.525192 | 166.872889 | | 5120 | 28.693431 | 67.275306 | 68.090280 | 76.208412 | 133.974510 | 157.120373 | 182.329784 | 210.069066 | | 6144 | 29.972591 | 54.751002 | 88.171906 | 104.489291 | 147.788597 | 185.185643 | 213.562819 | 228.643761 | | 7168 | 33.668398 | 63.088490 | 98.756212 | 130.502678 | 196.367121 | 225.166481 | 239.328794 | 260.162501 | | 8192 | 40.961901 | 87.067460 | 128.230351 | 178.127225 | 198.479068 | 225.806211 | 266.170239 | 325.967840 | - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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, and each call to `setPixels()` takes ~30 ms. >>> >>> Please wait for one more approval before integrating. >> >> Do you have the same kind of measurements for similar uses cases without >> this patch? I'm expecting performances to remain the same for snapshots less >> than `maxTextureSize*maxTextureSize` in size, since there is no extra pixel >> copy in that case, and the rest of the code if globally unchanged. >> >> Still there exists a window for performance regressions, which for instance >> on Windows 10 w/ the D3D rendering pipeline would be when one of the >> dimension of a snapshot is >4096 and <8192: in that case a snapshot would >> require up to 4 extra copy pixels steps with this patch. >> This is due to the fact that the old code would accept to render in a >> texture of a size up to the non-clamped max texture size (8192 in D3D, 16384 >> in ES2), but because I couldn't find a clean way to access the non clamped >> maxTextureSize exposed by the render factory from the Scene class, I settled >> for Prsim.maxTextureSize, which is the clamped value (4096 by default). >> I haven't dug deep enough in the code to understand precisely why the max >> texture size is clamped to 4096 by default, but assuming that it is for a >> good reason wouldn't that also apply in that case? Or is it always safe to >> use the non-clamped value in that particular case? > > 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 guaranteed to be that size. The > `Prism.maxTexture` constant represents the value to clamp the texture size > to. In the event that D3D or OpenGL were to report a maximum less than the > value of `Prism.maxTexture` (unlikely, but maybe on some low-end embedded > systems?), then that value is what should be used. The way to get the clamped > texture size is to call > [`ResourceFactory::getMaximumTextureSize`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/prism/ResourceFactory.java#L222), > but you can't do that directly from the scene graph code. > > 2. Allocating multiple `WritableImage` objects and using > `PixelWriter::setPixels` to stitch them together will take more temporary > memory, and is likely to be slower, than having the snapshot code write into > a subimage of the larger allocated image directly. > > Having said that, the current proposed solution is still better than the > current behavior is almost all cases, although there could be a performance > hit in the case of an 8K x 8K image, which will now be tiled. I want to do a > more careful review and some testing. If any other users of snapshot have any > comments or concerns, they would be welcome. > > I think the best long-term solution might be to modify the > [`QuantumToolkit::renderToImage`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java#L1490) > method, although that would certainly be out of scope for JavaFX 14. 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: https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 call to `doSnapshotTile()` takes > ~100 ms, and each call to `setPixels()` takes ~30 ms. > > Please wait for one more approval before integrating. > > > 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, and each call to `setPixels()` takes ~30 ms. > > Please wait for one more approval before integrating. Do you have the same kind of measurements for similar uses cases without this patch? I'm expecting performances to remain the same for snapshots less than `maxTextureSize*maxTextureSize` in size, since there is no extra pixel copy in that case, and the rest of the code if globally unchanged. Still there exists a window for performance regressions, which for instance on Windows 10 w/ the D3D rendering pipeline would be when one of the dimension of a snapshot is >4096 and <8192: in that case a snapshot would require up to 4 extra copy pixels steps with this patch. This is due to the fact that the old code would accept to render in a texture of a size up to the non-clamped max texture size (8192 in D3D, 16384 in ES2), but because I couldn't find a clean way to access the non clamped maxTextureSize exposed by the render factory from the Scene class, I settled for Prsim.maxTextureSize, which is the clamped value (4096 by default). I haven't dug deep enough in the code to understand precisely why the max texture size is clamped to 4096 by default, but assuming that it is for a good reason wouldn't that also apply in that case? Or is it always safe to use the non-clamped value in that particular case? - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 an NPE. I'm not certain that this fix is supposed to solve that >> bug, but according to the comments, the root cause is the same as >> [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082), which is >> related to this one. It's worth to take a look to see if something was >> missed. > >> >> >> I tested this fix against the repro code in >> [javafxports/openjdk-jfx#433](https://github.com/javafxports/openjdk-jfx/issues/433) >> (which is [JDK-838](https://bugs.openjdk.java.net/browse/JDK-838)), >> but there is still an NPE. I'm not certain that this fix is supposed to >> solve that bug, but according to the comments, the root cause is the same as >> [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082), which is >> related to this one. It's worth to take a look to see if something was >> missed. > > Although JDK-8189082 (and potentially others) are indeed likely to share the > same root cause, the fix proposed here won't have an effect on anything else > than snapshots, since the tiling is done at the Scene level rather than > within QuantumToolkit. > I purposefully choose to take the "easy" way out to limit the potential > side-effects of the change, but I agree that on the other hand supporting > tiling when needed directly in QuantumToolkit::renderToImage would have the > potential to solve a whole category of issues. > Also, from a very pragmatic angle, because of the increased complexity and > scope and the risks that come with it, I'm not very optimistic that this > change could realistically make it into jfx14 and to be honest I'm quite > eager to get the screenshot feature in my app working properly ASAP. > > I'd be willing to try and work on a more generic fix under the umbrella of > JDK-8189082, that'd be targeted at 15, while still having this fix included > as part 14; but that means this should (ideally) be reverted once it becomes > useless. > I don't know if the idea of having a "temporary fix" approach seems > acceptable in that context. Wel,l upon closer inspection it appears `QuantumToolkit::renderToImage` is only really used by `Scene::doSnapshot` anyway. Rendering of a the content of an NGSubScene (the underlying issue in JDK-8189082) is handled in a completely different code path, and I suspect this is also true for Canvas, where I hear a similar kind of issue exists. So from my (admittedly limited) understanding, fixing all problems at once would requires handling the tiling transparently within `RTTexture` (or more exactly inside all of its GraphicsPipeline specifics implementations) which seems quite a bit more complicated and risky. - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 this code into its own method similar to `doSnapshotTile`:
>
> `assemble(scene, xMin, yMin, width, height, root, transform, depthBuffer,
> fill, camera, wimg, maxTextureSize);`
>
> (`assemble` is a bad name, I didn't think about a better one).
>
> The method can return he resulting `WritableImage`, but it is not needed
> since it is manipulated via "side-effects". I would, however, bring it line
> with the `else` clause - either both use `wimg = methodName(..., wimg, ...);`
> or just `methodName(..., wimg, ...);`. This is fine since the input
> `WritableImage` is never `null`. From a readability point of view, using
> return values seems better.
I'm not 100% convinced this would really add much to the readability of the
code; I extracted the code from `doSnapshotTile` in its own method because it
is called twice (on both sides of the `if (height > maxTextureSize || width >
maxTextureSize)` condition, actually), but this isn't the case here.
I've got no strong feeling against it either, so I don't know; anybody else
care to comment?
-
PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
> 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 > tiles into a a single image. > Other than that, the logic used to do the actual snapshot is unchanged. > > Tests using the existing SnapshotCommon class have been added in a new file > named Snapshot3Test under SystemTest/test/javafx/scene. > These tests pass with the proposed fix, and fail without, throwing " > java.lang.IllegalArgumentException: Unrecognized image loader: null" The pull request has been updated with 2 additional commits. - Added commits: - 8966936f: Wrapped long line - 8e44dea2: Refactored height & weight calculation to avoid unnecessary computations Changes: - all: https://git.openjdk.java.net/jfx/pull/68/files - new: https://git.openjdk.java.net/jfx/pull/68/files/50191728..8966936f Webrevs: - full: https://webrevs.openjdk.java.net/jfx/68/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/68/webrev.02-03 Stats: 10 lines in 1 file changed: 5 ins; 2 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/68.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/68/head:pull/68 PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
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 > [JDK-838](https://bugs.openjdk.java.net/browse/JDK-838)), but there > is still an NPE. I'm not certain that this fix is supposed to solve that bug, > but according to the comments, the root cause is the same as > [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082), which is > related to this one. It's worth to take a look to see if something was missed. > > > I tested this fix against the repro code in > [javafxports/openjdk-jfx#433](https://github.com/javafxports/openjdk-jfx/issues/433) > (which is [JDK-838](https://bugs.openjdk.java.net/browse/JDK-838)), > but there is still an NPE. I'm not certain that this fix is supposed to solve > that bug, but according to the comments, the root cause is the same as > [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082), which is > related to this one. It's worth to take a look to see if something was missed. Although JDK-8189082 (and potentially others) are indeed likely to share the same root cause, the fix proposed here won't have an effect on anything else than snapshots, since the tiling is done at the Scene level rather than within QuantumToolkit. I purposefully choose to take the "easy" way out to limit the potential side-effects of the change, but I agree that on the other hand supporting tiling when needed directly in QuantumToolkit::renderToImage would have the potential to solve a whole category of issues. Also, from a very pragmatic angle, because of the increased complexity and scope and the risks that come with it, I'm not very optimistic that this change could realistically make it into jfx14 and to be honest I'm quite eager to get the screenshot feature in my app working properly ASAP. I'd be willing to try and work on a more generic fix under the umbrella of JDK-8189082, that'd be targeted at 15, while still having this fix included as part 14; but that means this should (ideally) be reverted once it becomes useless. I don't know if the idea of having a "temporary fix" approach seems acceptable in that context. - 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
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
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: // The requested size for the screenshot is to big to fit
>> a single texture,
>> 1304: // so we need to take several snapshot tiles and merge
>> them into single image (fixes JDK-8088198)
>
> Should be `too` big to fit
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
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); >> 1312: int tileHeight = Math.min(maxTextureSize, height - >> yOffset); >> 1313: WritableImage tile = doSnapshotTile(scene, xMin + >> xOffset, yMin + yOffset, tileWidth, tileHeight, root, transform, >> depthBuffer, fill, camera, null); > > `int xOffset = i * maxTextureSize;` > `int tileWidth = Math.min(maxTextureSize, width - xOffset);` > > These two lines should be moved to the outer loop of horizontal tabs. done in 501917284e0490d16b1831fcd854e31a779449b9 - PR: https://git.openjdk.java.net/jfx/pull/68
Re: [Rev 02] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size
> 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 > tiles into a a single image. > Other than that, the logic used to do the actual snapshot is unchanged. > > Tests using the existing SnapshotCommon class have been added in a new file > named Snapshot3Test under SystemTest/test/javafx/scene. > These tests pass with the proposed fix, and fail without, throwing " > java.lang.IllegalArgumentException: Unrecognized image loader: null" The pull request has been updated with 2 additional commits. - Added commits: - 50191728: Several changes following code review - d4ecb734: Removed Snapshot3Test.java and re-enabled ignored tests in Snapshot2Test.java Changes: - all: https://git.openjdk.java.net/jfx/pull/68/files - new: https://git.openjdk.java.net/jfx/pull/68/files/9986809d..50191728 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/68/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/68/webrev.01-02 Stats: 82 lines in 3 files changed: 1 ins; 71 del; 10 mod Patch: https://git.openjdk.java.net/jfx/pull/68.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/68/head:pull/68 PR: https://git.openjdk.java.net/jfx/pull/68
