RFR: 8236832: [macos 10.15] JavaFX Application hangs on video play on Cata…

[Alexander Matveev]

https://bugs.openjdk.java.net/browse/JDK-8236832

- This issue most likely caused by changes in AVFoundation APIs on macOS 10.15.
- Getting currentTime from AVPlayer was blocked if it is done from 
ProcessAudioTap callback (used for spectrum, equalizer and volume) on first 
call. If we reading currentTime after first callback returns, then it will not 
block. Fixed by not reading currentTime on first call and default it 0.

-

Commits:
 - c8438fd6: 8236832: [macos 10.15] JavaFX Application hangs on video play on 
Catalina

Changes: https://git.openjdk.java.net/jfx/pull/126/files
 Webrev: https://webrevs.openjdk.java.net/jfx/126/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8236832
  Stats: 23 lines in 4 files changed: 15 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jfx/pull/126.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/126/head:pull/126

PR: https://git.openjdk.java.net/jfx/pull/126

Re: [Integrated] RFR: 8238755: allow to create static lib for javafx.media on linux

[Johan Vos]

Changeset: ef2f9ce9
Author:Johan Vos 
Date:  2020-02-25 22:02:00 +
URL:   https://git.openjdk.java.net/jfx/commit/ef2f9ce9

8238755: allow to create static lib for javafx.media on linux

Reviewed-by: kcr, almatvee

! modules/javafx.base/src/main/java/com/sun/javafx/PlatformUtil.java
! modules/javafx.base/src/main/java/module-info.java
! 
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/ConnectionHolder.java
! modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/Locator.java
! 
modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/platform/PlatformManager.java
! 
modules/javafx.media/src/main/java/com/sun/media/jfxmediaimpl/platform/gstreamer/GSTPlatform.java
! 
modules/javafx.media/src/main/native/gstreamer/gstreamer-lite/gstreamer/gst/gst.c
! modules/javafx.media/src/main/native/gstreamer/plugins/av/fxavcodecplugin.c
! modules/javafx.media/src/main/native/gstreamer/plugins/fxplugins.c
! 
modules/javafx.media/src/main/native/jfxmedia/platform/gstreamer/GstPlatform.cpp

Re: PixelBuffer API threading model?

[Michael Paus]

Am 25.02.20 um 15:51 schrieb Neil C Smith:

In there, we have a small amount of locking between the
GStreamer callback and the OpenGL thread to cover buffer swap and
texture upload.  Being able to do likewise with the PixelBuffer API,
to swap or null the underlying buffer, would cover both use cases
here?  Swapping the underlying buffer also makes sense for a number of
APIs like GStreamer that ping pong between various memory locations,
otherwise we need to do some careful image caching.


The API can do some limited form of buffer swapping. I did this for a
proof of concept demo recently. Whether this works or not depends
on how much control you have on the allocation of the native buffer.

What I did was to allocate a contiguous native buffer which was twice
as large as needed for the image. Let's say the image was 1000x500 pixels.
I then allocated a buffer for 1000x1000 pixels.

I then could define in C two addresses for two buffers. The first one
at offset 0 and the second one in the middle of the memory.
The two addresses can then be forwarded independently to the native
renderer. When you now get a notification from your native renderer
that one of theses buffers has been written, you can tell that to the
PixelBuffer API in the callback function by appropriately setting the
viewport. You have to set the viewport to (0, 0, 1000, 500) or
(0, 500, 1000, 500) depending on which buffer part you have written.

This concept could even be extended to more than two buffers.
The key point is that the allocated memory must be contiguous.

Re: [Rev 01] RFR: 8237889: Update libxml2 to version 2.9.10

[Kevin Rushforth]

On Fri, 21 Feb 2020 20:25:30 GMT, Kevin Rushforth  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> Looks good. I did a full build / test on Windows, and a full build on Mac and 
> Linux.

@johanvos or @tiainen can one of you be the second reviewer? This isn't urgent, 
but we would like to get it in in the next week or two.

-

PR: https://git.openjdk.java.net/jfx/pull/120

Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH

[Kevin Rushforth]

On Tue, 25 Feb 2020 18:15:31 GMT, Bernhard M. Wiedemann 
 wrote:

>>> FWIW, I have scripts that will unpack the modular jar files and diff each 
>>> class
>> 
>> I agree that such specialized diff tools have some value, yet, there are 
>> also some limitations and downsides to them. E.g. you cannot simply tell 
>> another party what the expected sha256sum of a build result is.
>> 
>> https://www.suse.com/c/?p=42014  also has a section on problems with "the 
>> use of specialized comparison tools like [openSUSE's] ‘build-compare‘ "
>> 
>> I probably should write an FAQ entry about that topic...
>> 
>>> each released build is necessarily going to be different because you want a 
>>> unique time stamp and build number associated with it.
>> 
>> For release builds, it is important that other people can take the released 
>> sources and reproduce the same original binaries with the same release 
>> number (and ideally same timestamps) to easily verify that the build was 
>> clean (not corrupted by bad CPUs/RAM/HDDs or someone messing with the build 
>> machine).
>> I heard, some people even use that to save network bandwidth: add a small 
>> patch locally+remotely, build it locally, tell the world the new build hash, 
>> but have others upload their binaries with the right hash.
> 
> Hi, did you find time to review this?

No, I'm pretty backed up on reviews. It's on my queue, though.

-

PR: https://git.openjdk.java.net/jfx/pull/99

Re: RFR: 8238650: Allow to override buildDate with SOURCE_DATE_EPOCH

[Bernhard M . Wiedemann]

On Sat, 8 Feb 2020 11:14:29 GMT, Bernhard M. Wiedemann 
 wrote:

>> As an optional override, I am OK with the concept of having a way for the 
>> build to be reproducible.
>> 
>> FWIW, I have scripts that will unpack the modular jar files and diff each 
>> class as well as doing the same for a src.zip, and it's pretty easy to tell 
>> if only VersionInfo (which is the class that records the time stamps) has 
>> changed.
>> 
>> I note that in practice, this is useful for a certain class of builds (e.g., 
>> CI or nightly test builds), but each released build is necessarily going to 
>> be different because you want a unique time stamp and build number 
>> associated with it.
>> 
>> I will review this (probably some time next week) and would like @johanvos 
>> to review as well.
> 
>> FWIW, I have scripts that will unpack the modular jar files and diff each 
>> class
> 
> I agree that such specialized diff tools have some value, yet, there are also 
> some limitations and downsides to them. E.g. you cannot simply tell another 
> party what the expected sha256sum of a build result is.
> 
> https://www.suse.com/c/?p=42014  also has a section on problems with "the use 
> of specialized comparison tools like [openSUSE's] ‘build-compare‘ "
> 
> I probably should write an FAQ entry about that topic...
> 
>> each released build is necessarily going to be different because you want a 
>> unique time stamp and build number associated with it.
> 
> For release builds, it is important that other people can take the released 
> sources and reproduce the same original binaries with the same release number 
> (and ideally same timestamps) to easily verify that the build was clean (not 
> corrupted by bad CPUs/RAM/HDDs or someone messing with the build machine).
> I heard, some people even use that to save network bandwidth: add a small 
> patch locally+remotely, build it locally, tell the world the new build hash, 
> but have others upload their binaries with the right hash.

Hi, did you find time to review this?

-

PR: https://git.openjdk.java.net/jfx/pull/99

Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation

[Jeanette Winzenburg]

On Mon, 24 Feb 2020 17:15:02 GMT, Ajit Ghaisas  wrote:

>> Bug : https://bugs.openjdk.java.net/browse/JDK-8235480
>> 
>> Fix : Added the missed out RTL checks to the key mappings in 
>> TableViewBehaviorBase class.
>> 
>> Testing : Modified unit tests in TableViewKeyInputTest to take orientation 
>> as a parameter. The Left/Right key press tests have been modified to address 
>> LTR and RTL orientations.
>> 
>> Note : If this test modification is acceptable, I would like to address 
>> other similar tests separately. (I will create a test JBS issue and address 
>> later)
> 
> The pull request has been updated with 1 additional commit.

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewKeyInputTest.java
 line 1141:

> 1140: keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
> 1141: }
> 1142: keyboard.doKeyPress(KeyCode.SPACE,

having such if blocks in all tests regarding horizontal navigation feels like 
the parametrization isn't quite complete, IMO: the test method shouldn't need 
to worry, instead just call semantic horizontal navigation methods.

A simple change would be to extract that differentiation into a dedicated 
method, like f.i. forward/backward, test would get simpler (and make it easier 
to add those that are missing :)

@Test
public void testForwardFocus() {
sm.setCellSelectionEnabled(true);
sm.select(0, col0);
// current
//if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
//keyboard.doRightArrowPress(KeyModifier.getShortcutKey());
//} else {
//keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
//}
// extracted
forward(KeyModifier.getShortcutKey());
assertTrue("selected cell must still be selected", sm.isSelected(0, 
col0));
assertFalse("next cell must not be selected", sm.isSelected(0, col1));
TablePosition focusedCell = fm.getFocusedCell();
assertEquals("focused cell must moved to next", col1, 
focusedCell.getTableColumn());
}

/**
 * Orientation-aware horizontal navigation with arrow keys.
 * @param modifiers the modifiers to use on keyboard
 */
protected void forward(KeyModifier... modifiers) {
if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
keyboard.doRightArrowPress(modifiers);
} else {
keyboard.doLeftArrowPress(modifiers);
}
}

-

PR: https://git.openjdk.java.net/jfx/pull/114

Re: [Rev 02] RFR: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation

[Jeanette Winzenburg]

On Tue, 25 Feb 2020 15:04:05 GMT, Jeanette Winzenburg  
wrote:

>> The pull request has been updated with 1 additional commit.
> 
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewKeyInputTest.java
>  line 1141:
> 
>> 1140: keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
>> 1141: }
>> 1142: keyboard.doKeyPress(KeyCode.SPACE,
> 
> having such if blocks in all tests regarding horizontal navigation feels like 
> the parametrization isn't quite complete, IMO: the test method shouldn't need 
> to worry, instead just call semantic horizontal navigation methods.
> 
> A simple change would be to extract that differentiation into a dedicated 
> method, like f.i. forward/backward, test would get simpler (and make it 
> easier to add those that are missing :)
> 
> @Test
> public void testForwardFocus() {
> sm.setCellSelectionEnabled(true);
> sm.select(0, col0);
> // current
> //if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
> //keyboard.doRightArrowPress(KeyModifier.getShortcutKey());
> //} else {
> //keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
> //}
> // extracted
> forward(KeyModifier.getShortcutKey());
> assertTrue("selected cell must still be selected", sm.isSelected(0, 
> col0));
> assertFalse("next cell must not be selected", sm.isSelected(0, col1));
> TablePosition focusedCell = fm.getFocusedCell();
> assertEquals("focused cell must moved to next", col1, 
> focusedCell.getTableColumn());
> }
> 
> /**
>  * Orientation-aware horizontal navigation with arrow keys.
>  * @param modifiers the modifiers to use on keyboard
>  */
> protected void forward(KeyModifier... modifiers) {
> if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
> keyboard.doRightArrowPress(modifiers);
> } else {
> keyboard.doLeftArrowPress(modifiers);
> }
> }

Also, I'm a bit weary about the "else if" (vs a simple "else") - wouldn't it be 
some kind of setup error if the node orientation is neither rtl nor ltr? If so, 
I would add a test to check for it once.

-

PR: https://git.openjdk.java.net/jfx/pull/114

Re: PixelBuffer API threading model?

[Neil C Smith]

Hi,

On Tue, 25 Feb 2020 at 13:25, Kevin Rushforth
 wrote:
> This points out a flaw in the specification.
...
> To answer your other question about adding an API to PixelBuffer to swap
> out the underlying Buffer (to avoid yet another copy), I'm not sure how
> hard that would be (we currently assume that the buffer object itself is
> immutable)

Thanks for the info.  The code I'm working on is loosely ported from
some existing code that interfaces GStreamer with OpenGL texture
upload.  In there, we have a small amount of locking between the
GStreamer callback and the OpenGL thread to cover buffer swap and
texture upload.  Being able to do likewise with the PixelBuffer API,
to swap or null the underlying buffer, would cover both use cases
here?  Swapping the underlying buffer also makes sense for a number of
APIs like GStreamer that ping pong between various memory locations,
otherwise we need to do some careful image caching.

> I'm not 100% sure that
> running it in a Platform.runLater is sufficient in all cases, but will
> file a bug to investigate the threading and to document when it is safe
> to free a native DirectBuffer. It would still be useful to have your
> test case, too.

OK, will do.  I've run a "battle test" with 4k textures at 60fps,
freeing the whole native pipeline every 5sec.  Without
Platform::runLater it segfaults almost immediately.  Moving the free
into Platform::runLater seems to run happily for hours.  Caveat,
caveat, YMMV, etc. :-)

Many thanks,

Neil


-- 
Neil C Smith
Codelerity Ltd.
www.codelerity.com

Codelerity Ltd. is a company registered in England and Wales
Registered company number : 12063669
Registered office address : Office 4 219 Kensington High Street,
Kensington, London, England, W8 6BD

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 13:55:50 GMT, Nir Lisker  wrote:

>> Replying to @nlisker and @kevinrushforth general comments about the memory 
>> consumption of using a LinkedHashMap:
>> 
>> I agree that at the very least these should be lazy initialised when they 
>> are needed and that we should initially allocate a small capacity and load 
>> factor.
>> 
>> We could also have some sort of strategy where we could use arrays or lists 
>> if the number of listeners are less than some threshold (i.e. keeping the 
>> implementation with a similar overhead to the original) and then switch to 
>> the HashMap when it exceeds this threshold. This would make the code more 
>> complicated and I wonder if this is the worth the extra complexity.
>>  
>> I know that in our application, the thousands of listeners unregistering 
>> when using a TableView was stalling the JavaFX thread. 
>> 
>> I am happy to submit code that lazy initialises and minimises initial 
>> capacity and load factor as suggested or happy to take direction and 
>> suggestions from anyone with a deeper understanding of the code than myself.
> 
> I think that a starting point would be to decide on a spec for the listener 
> notification order: is it specified by their registration order, or that is 
> it unspecified, in which case we can take advantage of this for better 
> performance. Leaving is unspecified and restricting ourselves to having it 
> ordered is losing on both sides.

Even though the order of notifications is unspecified, the following tests fail 
if we use a HashMap vs LinkedHashMap i.e. the notifications are not in order of 
registration:

test.javafx.scene.control.TableViewTest > 
ensureSelectionIsCorrectWhenItemsChange FAILED
java.lang.AssertionError: expected:<0> but was:<-1>
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:470)
at org.junit.Assert.assertEquals(Assert.java:454)
at 
test.javafx.scene.control.TableViewTest.ensureSelectionIsCorrectWhenItemsChange(TableViewTest.java:333)

test.javafx.scene.control.TreeTableViewTest > 
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeTableViewTest.java:2042)

test.javafx.scene.control.TreeViewTest > test_rt27185 FAILED
java.lang.AssertionError: expected: but 
was:
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:145)
at 
test.javafx.scene.control.TreeViewTest.test_rt27185(TreeViewTest.java:603)

test.javafx.scene.control.TreeViewTest > 
test_rt27180_collapseBranch_laterSiblingAndChildrenSelected FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeViewTest.test_rt27180_collapseBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:973)

test.javafx.scene.control.TreeViewTest > 
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeViewTest.java:992)


These would need to be investigated to see why they assume this order.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: [Rev 02] RFR: 8239822: Intermittent unit test failures in RegionCSSTest

[Kevin Rushforth]

On Tue, 25 Feb 2020 11:51:09 GMT, Ajit Ghaisas  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> Marked as reviewed by aghaisas (Reviewer).

> Looks good to me. Just the dates in the copyright headers need to be updated.

They will get updated when I update copyrights of files modified this year to 
2020 (with automated scripts) with 
[JDK-8237504](https://bugs.openjdk.java.net/browse/JDK-8237504).

-

PR: https://git.openjdk.java.net/jfx/pull/124

Re: [Integrated] RFR: 8239822: Intermittent unit test failures in RegionCSSTest

[Kevin Rushforth]

Changeset: c3ee1a30
Author:Kevin Rushforth 
Date:  2020-02-25 14:01:57 +
URL:   https://git.openjdk.java.net/jfx/commit/c3ee1a30

8239822: Intermittent unit test failures in RegionCSSTest

Reviewed-by: aghaisas

! 
modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/StyleManagerTest.java
! 
modules/javafx.graphics/src/test/java/test/javafx/css/Node_cssStateTransition_Test.java
! 
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Tue, 25 Feb 2020 12:18:34 GMT, dannygonzalez 
 wrote:

>> I haven't done a detailed review yet, but I worry about the memory 
>> consumption and performance of using a Map for the simple cases where there 
>> is only a single (or very small number) of listeners. These methods are used 
>> in many more places than just the TableView / TreeTableView implementation.
> 
> Replying to @nlisker and @kevinrushforth general comments about the memory 
> consumption of using a LinkedHashMap:
> 
> I agree that at the very least these should be lazy initialised when they are 
> needed and that we should initially allocate a small capacity and load factor.
> 
> We could also have some sort of strategy where we could use arrays or lists 
> if the number of listeners are less than some threshold (i.e. keeping the 
> implementation with a similar overhead to the original) and then switch to 
> the HashMap when it exceeds this threshold. This would make the code more 
> complicated and I wonder if this is the worth the extra complexity.
>  
> I know that in our application, the thousands of listeners unregistering when 
> using a TableView was stalling the JavaFX thread. 
> 
> I am happy to submit code that lazy initialises and minimises initial 
> capacity and load factor as suggested or happy to take direction and 
> suggestions from anyone with a deeper understanding of the code than myself.

I think that a starting point would be to decide on a spec for the listener 
notification order: is it specified by their registration order, or that is it 
unspecified, in which case we can take advantage of this for better 
performance. Leaving is unspecified and restricting ourselves to having it 
ordered is losing on both sides.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

[Kevin Rushforth]

On Sun, 23 Feb 2020 14:57:44 GMT, Rony G. Flatscher 
 wrote:

>> OK, forgot to submit an explanatory text related to this fix. Posted [1] 
>> which explains the problem and the suggested solution for discussion. 
>> 
>> - This pull request relates to: 
>> 
>> 
>> - Brief background: the controller code for a FXML file can be written in
>>   any of the Java script languages that implement the javax.script framework
>>   introduced with Java 6
>> 
>> There are three possible types of script code definitions possible in fxml 
>> files:
>> 
>> 1. in an empty script element where the source attribute denotes an external 
>> file
>>that contains the code, e.g.:
>> 
>>  
>> 
>>This pull request uses the source attribute value as the filename.
>> 
>>There are no arguments to be supplied.
>> 
>> 2. text of a script element, e.g.:
>> 
>>   say 'code executed at:' .dateTime~new 
>> 
>>or:
>> 
>>   
>> 
>>This pull request uses the fxml-filename appended with the string
>>"-script_starting_at_line_" and the starting line number of the code.
>> 
>>There are no arguments to be supplied.
>> 
>>Reasonings:
>> 
>> - in the case that multiple fxml files employ script code it is 
>> important
>>   to learn the name of the fxml file that hosts the code that gets 
>> executed
>> 
>> - the dash after the fxml filename is meant to ease parsing
>> 
>> - in the case that there are multiple fx:script elements in a fxml 
>> file
>>   with inline code it is important to learn in which line the code 
>> starts
>>   that gets executed
>> 
>> - the decorated filename thereby becomes self documentary and 
>> unambiguous to
>>   the script code developer when debugging
>> 
>> 
>> 3. PCDATA text in an event attribute which by convention starts with "on", 
>> e.g. "onAction":
>> 
>>
>> 
>>This pull request uses the fxml-filename appended with a dash "-", 
>> appended with the
>>name of the event attribute, the string 
>> "_attribute_in_element_ending_at_line_" and
>>the ending line number of the element.
>> 
>>In the ScriptEventHandler constructor the decorated filename gets stored 
>> in addition
>>to the script and the scriptEngine.
>> 
>>Each time the event fires the event object argument will be stored in a 
>> copy of the
>>engineBindings with the index EVENT_KEY as well as with a single capacity 
>> Object array
>>stored with the index ScriptEngine.ARGV and the filename with the index
>>ScriptEngine.FILENAME. The script's evaluation will get this 
>> engineBindings as
>>its ENGINE_SCOPE Bindings supplied.
>> 
>>Reasonings:
>> 
>> - in the case that multiple fxml files employ script code it is 
>> important
>>   to learn the name of the fxml file that hosts the event code that 
>> gets
>>   executed
>> 
>> - the dash after the fxml filename is meant to ease parsing
>> 
>> - in the case that there are multiple event attributes in elements
>>   in a fxml file it is important to learn which event attribute in 
>> which
>>   element hosts the event script code that gets executed
>> 
>> - the decorated filename thereby becomes self documentary and 
>> unambiguous to
>>   the script code developer when debugging
>> 
>> - the structural change to the constructor (adding the decorated 
>> filename as
>>   a third argument) of the private static ScriptEventHandler class 
>> is confined
>>   to the FXMLLoader class and has no side effects
>> 
>> [1] "Ad suggested test unit for 'JDK-8234959 FXMLLoader does not populate 
>> ENGINE_SCOPE Bindings with FILENAME and ARGV'": 
>> 
> 
> Sorry, mixed up the link with the test unit WIP! :(
> 
> This is the correct link with the explanatory text related to this suggested 
> fix. Posted [1] which explains the problem and the suggested solution for 
> discussion. 
> [1] 'Ad suggested fix for "JDK-8234959 FXMLLoader does not populate 
> ENGINE_SCOPE Bindings with FILENAME and ARGV"': 
> 

@aghaisas can you also review this?

-

PR: https://git.openjdk.java.net/jfx/pull/122

Re: RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

[Rony G . Flatscher]

On Sun, 23 Feb 2020 14:54:32 GMT, Rony G. Flatscher 
 wrote:

>> …9: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
> 
> OK, forgot to submit an explanatory text related to this fix. Posted [1] 
> which explains the problem and the suggested solution for discussion. 
> 
> - This pull request relates to: 
> 
> 
> - Brief background: the controller code for a FXML file can be written in
>   any of the Java script languages that implement the javax.script framework
>   introduced with Java 6
> 
> There are three possible types of script code definitions possible in fxml 
> files:
> 
> 1. in an empty script element where the source attribute denotes an external 
> file
>that contains the code, e.g.:
> 
>  
> 
>This pull request uses the source attribute value as the filename.
> 
>There are no arguments to be supplied.
> 
> 2. text of a script element, e.g.:
> 
>   say 'code executed at:' .dateTime~new 
> 
>or:
> 
>   
> 
>This pull request uses the fxml-filename appended with the string
>"-script_starting_at_line_" and the starting line number of the code.
> 
>There are no arguments to be supplied.
> 
>Reasonings:
> 
> - in the case that multiple fxml files employ script code it is 
> important
>   to learn the name of the fxml file that hosts the code that gets 
> executed
> 
> - the dash after the fxml filename is meant to ease parsing
> 
> - in the case that there are multiple fx:script elements in a fxml 
> file
>   with inline code it is important to learn in which line the code 
> starts
>   that gets executed
> 
> - the decorated filename thereby becomes self documentary and 
> unambiguous to
>   the script code developer when debugging
> 
> 
> 3. PCDATA text in an event attribute which by convention starts with "on", 
> e.g. "onAction":
> 
>
> 
>This pull request uses the fxml-filename appended with a dash "-", 
> appended with the
>name of the event attribute, the string 
> "_attribute_in_element_ending_at_line_" and
>the ending line number of the element.
> 
>In the ScriptEventHandler constructor the decorated filename gets stored 
> in addition
>to the script and the scriptEngine.
> 
>Each time the event fires the event object argument will be stored in a 
> copy of the
>engineBindings with the index EVENT_KEY as well as with a single capacity 
> Object array
>stored with the index ScriptEngine.ARGV and the filename with the index
>ScriptEngine.FILENAME. The script's evaluation will get this 
> engineBindings as
>its ENGINE_SCOPE Bindings supplied.
> 
>Reasonings:
> 
> - in the case that multiple fxml files employ script code it is 
> important
>   to learn the name of the fxml file that hosts the event code that 
> gets
>   executed
> 
> - the dash after the fxml filename is meant to ease parsing
> 
> - in the case that there are multiple event attributes in elements
>   in a fxml file it is important to learn which event attribute in 
> which
>   element hosts the event script code that gets executed
> 
> - the decorated filename thereby becomes self documentary and 
> unambiguous to
>   the script code developer when debugging
> 
> - the structural change to the constructor (adding the decorated 
> filename as
>   a third argument) of the private static ScriptEventHandler class is 
> confined
>   to the FXMLLoader class and has no side effects
> 
> [1] "Ad suggested test unit for 'JDK-8234959 FXMLLoader does not populate 
> ENGINE_SCOPE Bindings with FILENAME and ARGV'": 
> 

Sorry, mixed up the link with the test unit WIP! :(

This is the correct link with the explanatory text related to this suggested 
fix. Posted [1] which explains the problem and the suggested solution for 
discussion. 
[1] 'Ad suggested fix for "JDK-8234959 FXMLLoader does not populate 
ENGINE_SCOPE Bindings with FILENAME and ARGV"': 


-

PR: https://git.openjdk.java.net/jfx/pull/122

Re: RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

[Rony G . Flatscher]

On Sat, 22 Feb 2020 15:39:35 GMT, Rony G. Flatscher 
 wrote:

> …9: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

OK, forgot to submit an explanatory text related to this fix. Posted [1] which 
explains the problem and the suggested solution for discussion. 

- This pull request relates to: 


- Brief background: the controller code for a FXML file can be written in
  any of the Java script languages that implement the javax.script framework
  introduced with Java 6

There are three possible types of script code definitions possible in fxml 
files:

1. in an empty script element where the source attribute denotes an external 
file
   that contains the code, e.g.:

 

   This pull request uses the source attribute value as the filename.

   There are no arguments to be supplied.

2. text of a script element, e.g.:

  say 'code executed at:' .dateTime~new 

   or:

  

   This pull request uses the fxml-filename appended with the string
   "-script_starting_at_line_" and the starting line number of the code.

   There are no arguments to be supplied.

   Reasonings:

- in the case that multiple fxml files employ script code it is 
important
  to learn the name of the fxml file that hosts the code that gets 
executed

- the dash after the fxml filename is meant to ease parsing

- in the case that there are multiple fx:script elements in a fxml file
  with inline code it is important to learn in which line the code 
starts
  that gets executed

- the decorated filename thereby becomes self documentary and 
unambiguous to
  the script code developer when debugging


3. PCDATA text in an event attribute which by convention starts with "on", e.g. 
"onAction":

   

   This pull request uses the fxml-filename appended with a dash "-", appended 
with the
   name of the event attribute, the string 
"_attribute_in_element_ending_at_line_" and
   the ending line number of the element.

   In the ScriptEventHandler constructor the decorated filename gets stored in 
addition
   to the script and the scriptEngine.

   Each time the event fires the event object argument will be stored in a copy 
of the
   engineBindings with the index EVENT_KEY as well as with a single capacity 
Object array
   stored with the index ScriptEngine.ARGV and the filename with the index
   ScriptEngine.FILENAME. The script's evaluation will get this engineBindings 
as
   its ENGINE_SCOPE Bindings supplied.

   Reasonings:

- in the case that multiple fxml files employ script code it is 
important
  to learn the name of the fxml file that hosts the event code that gets
  executed

- the dash after the fxml filename is meant to ease parsing

- in the case that there are multiple event attributes in elements
  in a fxml file it is important to learn which event attribute in which
  element hosts the event script code that gets executed

- the decorated filename thereby becomes self documentary and 
unambiguous to
  the script code developer when debugging

- the structural change to the constructor (adding the decorated 
filename as
  a third argument) of the private static ScriptEventHandler class is 
confined
  to the FXMLLoader class and has no side effects

[1] "Ad suggested test unit for 'JDK-8234959 FXMLLoader does not populate 
ENGINE_SCOPE Bindings with FILENAME and ARGV'": 


-

PR: https://git.openjdk.java.net/jfx/pull/122

RFR: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

[Rony G . Flatscher]

…9: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV

-

Commits:
 - 30539592: Removed/replaced white space.
 - 5c33d590: fix for  
JDK-8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME 
and ARGV

Changes: https://git.openjdk.java.net/jfx/pull/122/files
 Webrev: https://webrevs.openjdk.java.net/jfx/122/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8234959
  Stats: 18 lines in 1 file changed: 8 ins; 4 del; 6 mod
  Patch: https://git.openjdk.java.net/jfx/pull/122.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/122/head:pull/122

PR: https://git.openjdk.java.net/jfx/pull/122

Re: PixelBuffer API threading model?

[Kevin Rushforth]

Hi Neil,

I didn't look at your use case in detail, so I missed that you were 
talking about freeing the native memory that is backing your 
DirectBuffer. No, freeing the memory immediately after calling 
setImage(null) will not work. The JavaFX rendering is done in a 
different thread, which could still be using that buffer.


This points out a flaw in the specification. I'm not 100% sure that 
running it in a Platform.runLater is sufficient in all cases, but will 
file a bug to investigate the threading and to document when it is safe 
to free a native DirectBuffer. It would still be useful to have your 
test case, too.


To answer your other question about adding an API to PixelBuffer to swap 
out the underlying Buffer (to avoid yet another copy), I'm not sure how 
hard that would be (we currently assume that the buffer object itself is 
immutable). You can file an Enhancement request if you like, but isn't 
likely to be done any time soon.


-- Kevin


On 2/25/2020 2:42 AM, Neil C Smith wrote:

On Fri, 21 Feb 2020 at 12:39, Kevin Rushforth
 wrote:

I missed seeing this yesterday. Since you have a test program, can you
file a bug at:

https://urldefense.com/v3/__https://bugreport.java.com/__;!!GqivPVa7Brio!NbvpernCUtnKGCczrvpk22da9xKbHFu64OIolCd86BxXnVxOT2HFW47wx97PiUPVtuUh$

and include your test case?

Thanks Kevin.  I need to replicate without a particular dependency,
but will do ASAP.  Can I confirm that the behaviour outlined in my
first email is intended to be safe/supported usage then?

Incidentally, this also means switching machines - my main dev machine
with Mesa and an AMD GPU will kernel crash within about 40s using the
PixelBuffer API.  I'm fairly sure that's a Mesa bug but have not
tracked down what exactly the API is doing that causes it - no issues
with OpenGL code doing the same thing in principle.

Best wishes,

Neil

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 00:43:34 GMT, Kevin Rushforth  wrote:

>> Sorry for the interruption, send a PR that corrects the same problem.
> 
> I haven't done a detailed review yet, but I worry about the memory 
> consumption and performance of using a Map for the simple cases where there 
> is only a single (or very small number) of listeners. These methods are used 
> in many more places than just the TableView / TreeTableView implementation.

Replying to @nlisker and @kevinrushforth general comments about the memory 
consumption of using a LinkedHashMap:

I agree that at the very least these should be lazy initialised when they are 
needed and that we should initially allocate a small capacity and load factor.

We could also have some sort of strategy where we could use arrays or lists if 
the number of listeners are less than some threshold (i.e. keeping the 
implementation with a similar overhead to the original) and then switch to the 
HashMap when it exceeds this threshold. This would make the code more 
complicated and I wonder if this is the worth the extra complexity.
 
I know that in our application, the thousands of listeners unregistering when 
using a TableView was stalling the JavaFX thread. 

I am happy to submit code that lazy initialises and minimises initial capacity 
and load factor as suggested or happy to take direction and suggestions from 
anyone with a deeper understanding of the code than myself.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: [Rev 02] RFR: 8239822: Intermittent unit test failures in RegionCSSTest

[Ajit Ghaisas]

On Mon, 24 Feb 2020 19:36:47 GMT, Kevin Rushforth  wrote:

>> This is a fix for an intermittent test failure affecting 
>> `test.javafx.scene.layout.RegionCSSTest`. This turns out to be a test bug in 
>> `test.javafx.scene.CssStyleHelperTest`, which was added as part of 
>> [JDK-8237469](https://bugs.openjdk.java.net/browse/JDK-8237469). 
>> RegionCSSTest is a victim of this bug.
>> 
>> Except for the systemTests project, all unit tests are run in the same JVM, 
>> so each test class must ensure that any modified global state is restored 
>> after the tests are run. The CssStyleHelperTest leaves a userAgentStyleSheet 
>> set, which is a global (per Application) attribute, so any subsequent tests 
>> will be run with that stylesheet set. If the last test that is run in 
>> CssStyleHelperTest sets a style sheet with `"-fx-background"` then it will 
>> cause assertion failures in 78 of the tests in RegionCSSTest. 
>> 
>> The fix is to cleanup the global userAgentStyleSheet, along with any other 
>> state that is set, in an  `@AfterClass` cleanup method.
> 
> The pull request has been updated with 1 additional commit.

Marked as reviewed by aghaisas (Reviewer).

-

PR: https://git.openjdk.java.net/jfx/pull/124

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 00:45:06 GMT, Kevin Rushforth  wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>>  line 194:
>> 
>>> 193: private Map 
>>> invalidationListeners = new LinkedHashMap<>();
>>> 194: private Map, Integer> 
>>> changeListeners = new LinkedHashMap<>();
>>> 195: private T currentValue;
>> 
>> Two comments on this:
>> 
>> 1. The old implementation initialized these lazily, so we might want to 
>> preserve that behavior. I think it is reasonable since in most cases an 
>> observable won't have both types of listeners attached to it.
>> 2. Since we're dealing wither performance optimizations here, we should 
>> think about what the `initialCapcity` and `loadFactor` of these maps should 
>> be, as it can greatly increase performance when dealing with a large amount 
>> of entries.
> 
> Adding to this, we need to ensure that the simple case of a few listeners 
> doesn't suffer memory bloat. It may make sense to initially allocate a Map 
> with a small capacity and load factor, and then reallocate it if someone adds 
> more than a certain number of listeners.

Agree with the lazy initialisation and the initial setting of capacity and load 
factor to reduce memory footprint unless it's needed.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: [Rev 02] RFR: 8239822: Intermittent unit test failures in RegionCSSTest

[Dean Wookey]

On Mon, 24 Feb 2020 19:36:47 GMT, Kevin Rushforth  wrote:

>> This is a fix for an intermittent test failure affecting 
>> `test.javafx.scene.layout.RegionCSSTest`. This turns out to be a test bug in 
>> `test.javafx.scene.CssStyleHelperTest`, which was added as part of 
>> [JDK-8237469](https://bugs.openjdk.java.net/browse/JDK-8237469). 
>> RegionCSSTest is a victim of this bug.
>> 
>> Except for the systemTests project, all unit tests are run in the same JVM, 
>> so each test class must ensure that any modified global state is restored 
>> after the tests are run. The CssStyleHelperTest leaves a userAgentStyleSheet 
>> set, which is a global (per Application) attribute, so any subsequent tests 
>> will be run with that stylesheet set. If the last test that is run in 
>> CssStyleHelperTest sets a style sheet with `"-fx-background"` then it will 
>> cause assertion failures in 78 of the tests in RegionCSSTest. 
>> 
>> The fix is to cleanup the global userAgentStyleSheet, along with any other 
>> state that is set, in an  `@AfterClass` cleanup method.
> 
> The pull request has been updated with 1 additional commit.

Looks good to me. Just the dates in the copyright headers need to be updated.

-

Changes requested by dwookey (Author).

PR: https://git.openjdk.java.net/jfx/pull/124

Re: PixelBuffer API threading model?

[Neil C Smith]

On Fri, 21 Feb 2020 at 12:39, Kevin Rushforth
 wrote:
> I missed seeing this yesterday. Since you have a test program, can you
> file a bug at:
>
> https://bugreport.java.com/
>
> and include your test case?

Thanks Kevin.  I need to replicate without a particular dependency,
but will do ASAP.  Can I confirm that the behaviour outlined in my
first email is intended to be safe/supported usage then?

Incidentally, this also means switching machines - my main dev machine
with Mesa and an AMD GPU will kernel crash within about 40s using the
PixelBuffer API.  I'm fairly sure that's a Mesa bug but have not
tracked down what exactly the API is doing that causes it - no issues
with OpenGL code doing the same thing in principle.

Best wishes,

Neil


-- 
Neil C Smith
Codelerity Ltd.
www.codelerity.com

Codelerity Ltd. is a company registered in England and Wales
Registered company number : 12063669
Registered office address : Office 4 219 Kensington High Street,
Kensington, London, England, W8 6BD