Re: RFR: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
On Tue, 2 Nov 2021 18:11:38 GMT, Andrew Brygin wrote:
> GlassRunnable uses jni environment (jEnv) associated with the main
> application thread both for run() and dealloc() methods. Both these methods
> are supposed to be scheduled for execution on the main thread:
>
> if (jEnv != NULL)
> {
> GlassRunnable *runnable = [[GlassRunnable alloc]
> initWithRunnable:(*env)->NewGlobalRef(env, jRunnable)];
> [runnable performSelectorOnMainThread:@selector(run) withObject:nil
> waitUntilDone:NO];
> }
>
>
> However, it appears that on macOS 12 only the run() method is executed the
> main thread, whereas the dealloc() method is executed on the
> InvokeLaterDispatcher thread, that leads to usage of the main thread jni env
> in the context of another thread. This problem is more visible on aarch64,
> where the thread check is triggered by the W^X machinery, but the problem is
> present on x64 as well.
>
> Proposed fix just encapsulates all jni-related work in the run() method,
> reducing risks to misuse the jni environment of the main thread.
The fix looks good. All my testing so far looks good. I'll finish my testing
today.
-
PR: https://git.openjdk.java.net/jfx/pull/661
Re: RFR: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
On Wed, 3 Nov 2021 12:13:35 GMT, Kevin Rushforth wrote: > The fix looks good. All my testing so far looks good. I'll finish my testing > today. same here. - PR: https://git.openjdk.java.net/jfx/pull/661
Re: RFR: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces [v2]
On Fri, 4 Jun 2021 05:04:17 GMT, Michael Strauß wrote: >> This PR fixes an issue when building OpenJFX on Windows and command-line >> arguments contain paths with spaces. >> >> The problem is that on Windows, `ant` is invoked via `cmd`, which leads to >> quotes being interpreted twice. This can be fixed with the option `cmd /s`, >> which prevents interpreting quotes on the rest of the command line. The >> result is as if the rest of the command line had been typed verbatim at the >> command prompt. > > Michael Strauß has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'master' into fixes/JDK-8267059 > - Use cmd /s option when building on Windows @drmarmac The issue with accepting the TOU should now have been fixed, so if you click the agree box again, your review comment should be restored. - PR: https://git.openjdk.java.net/jfx/pull/499
Re: RFR: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces [v2]
On Wed, 3 Nov 2021 13:12:14 GMT, Erik Joelsson wrote: >> Michael Strauß has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - Merge branch 'master' into fixes/JDK-8267059 >> - Use cmd /s option when building on Windows > > @drmarmac The issue with accepting the TOU should now have been fixed, so if > you click the agree box again, your review comment should be restored. @erikj79 thanks, doesn't seem to have worked though. - PR: https://git.openjdk.java.net/jfx/pull/499
Re: RFR: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces [v2]
On Fri, 4 Jun 2021 05:04:17 GMT, Michael Strauß wrote: >> This PR fixes an issue when building OpenJFX on Windows and command-line >> arguments contain paths with spaces. >> >> The problem is that on Windows, `ant` is invoked via `cmd`, which leads to >> quotes being interpreted twice. This can be fixed with the option `cmd /s`, >> which prevents interpreting quotes on the rest of the command line. The >> result is as if the rest of the command line had been typed verbatim at the >> command prompt. > > Michael Strauß has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'master' into fixes/JDK-8267059 > - Use cmd /s option when building on Windows Tested this on Windows, seems to fix the issue. - Marked as reviewed by drmar...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jfx/pull/499
Re: RFR: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
On Tue, 2 Nov 2021 18:11:38 GMT, Andrew Brygin wrote:
> GlassRunnable uses jni environment (jEnv) associated with the main
> application thread both for run() and dealloc() methods. Both these methods
> are supposed to be scheduled for execution on the main thread:
>
> if (jEnv != NULL)
> {
> GlassRunnable *runnable = [[GlassRunnable alloc]
> initWithRunnable:(*env)->NewGlobalRef(env, jRunnable)];
> [runnable performSelectorOnMainThread:@selector(run) withObject:nil
> waitUntilDone:NO];
> }
>
>
> However, it appears that on macOS 12 only the run() method is executed the
> main thread, whereas the dealloc() method is executed on the
> InvokeLaterDispatcher thread, that leads to usage of the main thread jni env
> in the context of another thread. This problem is more visible on aarch64,
> where the thread check is triggered by the W^X machinery, but the problem is
> present on x64 as well.
>
> Proposed fix just encapsulates all jni-related work in the run() method,
> reducing risks to misuse the jni environment of the main thread.
Testing completed. I ran tests on three different macOS systems:
M1 running macOS 12.0.1 beta
Intel x64 system running macOS 12.0.1 beta
Intel x64 system (MacBook Pro) running 10.15.7
-
Marked as reviewed by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/661
Re: RFR: 8275723: Crash on macOS 12 in GlassRunnable::dealloc
On Tue, 2 Nov 2021 18:11:38 GMT, Andrew Brygin wrote:
> GlassRunnable uses jni environment (jEnv) associated with the main
> application thread both for run() and dealloc() methods. Both these methods
> are supposed to be scheduled for execution on the main thread:
>
> if (jEnv != NULL)
> {
> GlassRunnable *runnable = [[GlassRunnable alloc]
> initWithRunnable:(*env)->NewGlobalRef(env, jRunnable)];
> [runnable performSelectorOnMainThread:@selector(run) withObject:nil
> waitUntilDone:NO];
> }
>
>
> However, it appears that on macOS 12 only the run() method is executed the
> main thread, whereas the dealloc() method is executed on the
> InvokeLaterDispatcher thread, that leads to usage of the main thread jni env
> in the context of another thread. This problem is more visible on aarch64,
> where the thread check is triggered by the W^X machinery, but the problem is
> present on x64 as well.
>
> Proposed fix just encapsulates all jni-related work in the run() method,
> reducing risks to misuse the jni environment of the main thread.
This indeed fixes the issue, and doesn't cause regression.
-
Marked as reviewed by jvos (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/661
Re: RFR: 8273096: Add support for H.265/HEVC to JavaFX Media
On Thu, 21 Oct 2021 07:51:20 GMT, Alexander Matveev wrote: > - Added support for H.265/HEVC for all 3 platforms. > - Support is added only for .mp4 files over FILE/HTTP/HTTPS protocols. HTTP > Live Streaming with H.265/HEVC is not supported. > - On Windows mfwrapper was introduced which uses Media Foundation APIs to > decode HEVC. > - 10 and 12-bit HEVC was tested and also supported, however due to graphics > pipeline not supporting 10-bit YUV rendering we will use color converter to > convert video frame to 8-bit before sending it for rendering. > - Resolution upto 4k is supported. > > Additional runtime dependency requirements: > Windows: Windows 10 with HEVC Video Extensions installed. > macOS: macOS High Sierra and later > Linux: at least libavcodec56 and libswscale5 > > Additional build dependency: > Linux: libswscale-dev This adds an additional supported encoding type, so we will need a CSR for this. The specification can be the changes to the docs in `javafx/scene/media/package.html` Sorry for the delay in looking at this. I get this failure on Ubuntu 20.04 Linux running the oow2010-2.mp4 sample media file: ** (java:72365): WARNING **: 07:18:02.597: Failed to load plugin '.../build/sdk/lib/libavplugin-ffmpeg-58.so': libswscale.so.5: cannot open shared object file: No such file or directory Regarding the additional runtime dependencies: > Additional runtime dependency requirements: > Windows: Windows 10 with HEVC Video Extensions installed. > macOS: macOS High Sierra and later > Linux: at least libavcodec56 and libswscale5 The added dependency on Linux on `libswscale5` is a problem, at least as implemented in the current version of the PR. It's OK to require additional dependencies at runtime in order to play H.265 media files, but we need to continue to be able to play media that doesn't use H.265 without any additional requirements. This means that the loading of `libswscale5.so` needs to be optional; we can't link with it at build time, but need to dynamically load it at runtime, and only use it, or fail if not present, when decoding media formats that require it. Similarly, it is fine to make the playing of H.265 videos dependent on macOS 10.13 High Sierra or later, but unless we bump our current minimum platform across the board, we need to be able to play videos that don't use H.265 on macOS 10.12 Sierra. The same thing applies to Windows, where we need to be able to play non-H.265 media without the HEVC Video Extensions. - PR: https://git.openjdk.java.net/jfx/pull/649
RFR: 8268642: Improve property system to facilitate correct usage
Based on previous discussions, this PR attempts to improve the JavaFX property
system by enforcing correct API usage in several cases that are outlined below.
It also streamlines the API by deprecating untyped APIs in favor of typed APIs
that better express intent.
### 1. Behavioral changes for regular bindings
var target = new SimpleObjectProperty(bean, "target");
var source = new SimpleObjectProperty(bean, "source");
target.bind(source);
target.bindBidirectional(source);
_Before:_ `RuntimeException` --> "bean.target: A bound value cannot be set."
_After:_ `IllegalStateException` --> "bean.target: Bidirectional binding cannot
target a bound property."
var target = new SimpleObjectProperty(bean, "target");
var source = new SimpleObjectProperty(bean, "source");
var other = new SimpleObjectProperty(bean, "other");
source.bind(other);
target.bindBidirectional(source);
_Before:_ no error
_After:_ `IllegalArgumentException` --> "bean.source: Bidirectional binding
cannot target a bound property."
var target = new SimpleObjectProperty(bean, "target");
var source = new SimpleObjectProperty(bean, "source");
target.bindBidirectional(source);
target.bind(source);
_Before:_ no error
_After:_ `IllegalStateException` --> "bean.target: Cannot bind a property that
is targeted by a bidirectional binding."
### 2. Behavioral changes for content bindings
var target = new SimpleListProperty(bean, "target");
var source = new SimpleListProperty(bean, "source");
target.bindContent(source);
target.bindContentBidirectional(source);
_Before:_ no error
_After:_ `IllegalStateException` --> "bean.target: Bidirectional content
binding cannot target a bound collection."
var target = new SimpleListProperty(bean, "target");
var source = new SimpleListProperty(bean, "source");
var other = new SimpleListProperty();
source.bindContent(other);
target.bindContentBidirectional(source);
_Before:_ no error
_After:_ `IllegalArgumentException` --> "bean.source: Bidirectional content
binding cannot target a bound collection."
var target = new SimpleListProperty(bean, "target");
var source = new SimpleListProperty(bean, "source");
target.bindContentBidirectional(source);
target.bindContent(source);
_Before:_ no error
_After:_ `IllegalStateException` --> "bean.target: Cannot bind a collection
that is targeted by a bidirectional content binding."
var target = new SimpleListProperty(bean, "target");
var source = new SimpleListProperty(bean, "source");
target.bindContent(source);
target.set(FXCollections.observableArrayList());
_Before:_ no error
_After:_ `IllegalStateException` --> "bean.target: Cannot set the value of a
content-bound property."
var target = new SimpleListProperty(bean, "target");
var source = new SimpleListProperty(bean, "source");
target.bindContent(source);
target.add("foo");
_Before_: no error, but binding is broken because the lists are in an
inconsistent state
_After:_ `RuntimeExeption` via `Thread.UncaughtExceptionHandler` --> "Illegal
list modification: Content binding was removed because the lists are
out-of-sync."
### 3. Align un-binding of unidirectional content bindings with regular
unidirectional bindings
The API of unidirectional content bindings is aligned with regular
unidirectional bindings because the semantics are similar. Like `unbind()`,
`unbindContent(Object)` should not need an argument because there can only be a
single content binding. For this reason, `unbindContent(Object)` will be
deprecated in favor of `unbindContent()`:
void bindContent(ObservableList source);
+ void unbindContent();
+ boolean isContentBound();
+ @Deprecated(since = "18", forRemoval = true)
void unbindContent(Object object);
### 4. Replace untyped binding APIs with typed APIs
The following untyped APIs will be marked for removal in favor of typed
replacements:
In `javafx.beans.binding.Bindings`:
@Deprecated(since = "18", forRemoval = true)
void unbindBidirectional(Object property1, Object property2)
@Deprecated(since = "18", forRemoval = true)
void unbindContentBidirectional(Object obj1, Object obj2)
@Deprecated(since = "18", forRemoval = true)
void unbindContent(Object obj1, Object obj2)
In `ReadOnlyListProperty`, `ReadOnlySetProperty`, `ReadOnlyMapProperty`:
@Deprecated(since = "18", forRemoval = true)
void unbindContentBidirectional(Object object)
~~5. Support un-binding bidirectional conversion bindings with typed API
At this moment, `StringProperty` is the only property implementation that
offers bidirectional conversion bindings, where the `StringProperty` is bound
to a property of another type:~~
void bindBidirectional(Property other, StringConverter converter)
~~The inherited `Property.unbindBidirectional(Property)` API cannot be
used to unbind a conversion binding, because it only accepts arguments of type
`Property`.
`StringProperty` solves this issue by adding an untyped overload:
`unbindBidirectional(Object)`.~~
~~I propose to relax the definition of `Property
Re: RFR: 8268642: Improve property system to facilitate correct usage
On Tue, 27 Jul 2021 23:15:10 GMT, Michael Strauß wrote:
> Based on previous discussions, this PR attempts to improve the JavaFX
> property system by enforcing correct API usage in several cases that are
> outlined below. It also streamlines the API by deprecating untyped APIs in
> favor of typed APIs that better express intent.
>
> ### 1. Behavioral changes for regular bindings
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> target.bind(source);
> target.bindBidirectional(source);
>
> _Before:_ `RuntimeException` --> "bean.target: A bound value cannot be set."
> _After:_ `IllegalStateException` --> "bean.target: Bidirectional binding
> cannot target a bound property."
>
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> var other = new SimpleObjectProperty(bean, "other");
> source.bind(other);
> target.bindBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalArgumentException` --> "bean.source: Bidirectional binding
> cannot target a bound property."
>
>
> var target = new SimpleObjectProperty(bean, "target");
> var source = new SimpleObjectProperty(bean, "source");
> target.bindBidirectional(source);
> target.bind(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot bind a property
> that is targeted by a bidirectional binding."
>
>
> ### 2. Behavioral changes for content bindings
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.bindContentBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Bidirectional content
> binding cannot target a bound collection."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> var other = new SimpleListProperty();
> source.bindContent(other);
> target.bindContentBidirectional(source);
>
> _Before:_ no error
> _After:_ `IllegalArgumentException` --> "bean.source: Bidirectional content
> binding cannot target a bound collection."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContentBidirectional(source);
> target.bindContent(source);
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot bind a collection
> that is targeted by a bidirectional content binding."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.set(FXCollections.observableArrayList());
>
> _Before:_ no error
> _After:_ `IllegalStateException` --> "bean.target: Cannot set the value of a
> content-bound property."
>
>
> var target = new SimpleListProperty(bean, "target");
> var source = new SimpleListProperty(bean, "source");
> target.bindContent(source);
> target.add("foo");
>
> _Before_: no error, but binding is broken because the lists are in an
> inconsistent state
> _After:_ `RuntimeExeption` via `Thread.UncaughtExceptionHandler` --> "Illegal
> list modification: Content binding was removed because the lists are
> out-of-sync."
>
>
> ### 3. Align un-binding of unidirectional content bindings with regular
> unidirectional bindings
> The API of unidirectional content bindings is aligned with regular
> unidirectional bindings because the semantics are similar. Like `unbind()`,
> `unbindContent(Object)` should not need an argument because there can only be
> a single content binding. For this reason, `unbindContent(Object)` will be
> deprecated in favor of `unbindContent()`:
>
> void bindContent(ObservableList source);
> + void unbindContent();
> + boolean isContentBound();
>
> + @Deprecated(since = "18", forRemoval = true)
> void unbindContent(Object object);
>
>
> ### 4. Replace untyped binding APIs with typed APIs
> The following untyped APIs will be marked for removal in favor of typed
> replacements:
> In `javafx.beans.binding.Bindings`:
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindBidirectional(Object property1, Object property2)
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContentBidirectional(Object obj1, Object obj2)
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContent(Object obj1, Object obj2)
>
>
> In `ReadOnlyListProperty`, `ReadOnlySetProperty`, `ReadOnlyMapProperty`:
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContentBidirectional(Object object)
>
>
> ~~5. Support un-binding bidirectional conversion bindings with typed API
> At this moment, `StringProperty` is the only property implementation that
> offers bidirectional conversion bindings, where the `StringProperty` is bound
> to a property of another type:~~
>
> void bindBidirectional(Property other, StringConve
Re: RFR: 8268642: Improve property system to facilitate correct usage
On Tue, 27 Jul 2021 23:15:10 GMT, Michael Strauß wrote: > 4. Replace untyped binding APIs with typed APIs We have not yet finished the discussion on this. As currently proposed, I am not in favor of this. It will take me some time to go through it in enough detail to reply. - Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/590
Re: RFR: 8268642: Improve property system to facilitate correct usage
On Tue, 27 Jul 2021 23:15:10 GMT, Michael Strauß wrote: > 3. Align un-binding of unidirectional content bindings with regular > unidirectional bindings Same comment as for item 4. We need to finish the discussion of whether and how to do this. - PR: https://git.openjdk.java.net/jfx/pull/590
Re: RFR: 8089589: [ListView] ScrollBar content moves toward-backward during scrolling. [v4]
On Fri, 16 Apr 2021 10:45:18 GMT, Johan Vos wrote: >> This PR introduces a refactory for VirtualFlow, fixing a number of issues >> reported about inconsistent scrolling speed (see >> https://bugs.openjdk.java.net/browse/JDK-8089589) >> The problem mentioned in the JBS issue (and in related issues) is that the >> VirtualFlow implementation depends on cell index and cell count, instead of >> on pixel count. The latter is unknown when the VirtualFlow is created, and >> pre-calculating the size of a large set of items would be very expensive. >> Therefore, this PR uses a combination of a gradual calculation of the total >> size in pixels (estimatedSize) and a smoothing part that prevents the >> scrollback to scroll in the reverse direction as the requested change. >> This PR currently breaks a number of tests that hard-coded depend on a >> number of evaluations. This is inherit to the approach of this PR: if we >> want to estimate the total size, we need to do some additional calculations. >> In this PR, I try to balance between consistent behavior and performance. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > Process reviewer comments, uncomment commented test, remove stderr modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 926: > 924: // the absolute offset is changed accordingly. > 925: adjustAbsoluteOffset(); > 926: } This seems to violate the invariant `setPosition(...)` ≙ `positionProperty().set(...)`. Your comment indicates that this is by design, but how do you prevent people from invoking `positionProperty().set(...)` and getting a different behavior? - PR: https://git.openjdk.java.net/jfx/pull/398
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v3]
On Tue, 2 Nov 2021 10:15:41 GMT, Florian Kirmaier wrote: >> After thinking about this issue for some time, I've now got a solution. >> I know put the scene in the state it is, before is was shown, when the >> dirtyNodes are unset, the whole scene is basically considered dirty. >> This has the drawback of rerendering, whenever a window is "reshown", but it >> restores sanity about memory behaviour, which should be considered more >> important. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8269907 > We now require the rendering lock when cleaning up dirty nodes. To do so, > we moved some code required for snapshot into a reusable method. > To solidify my understanding - the "RenderScenegraph" is only allowed to be > changed when the render lock is held. This scene graph is represented by all > these NG classes, correct? Correct. When the renderLock is not held, the renderer thread is free to render it and can be sure it won't change. I'll take a look at the updated fix. - PR: https://git.openjdk.java.net/jfx/pull/584
