Re: Moving src.zip out of the lib directory of the JavaFX SDK

2021-06-24 Thread Kevin Rushforth
Since it seems that this change will cause Eclipse to not find the 
sources without changes on their part (see Tom's message), and might not 
be as important any more for Netbeans users, it's less clear to me that 
we should make this change. Netbeans users who download and use the 
JavaFX SDK can already use the documented workaround of adding the 
individual jar files rather than the entire lib directory to the module 
path.


Anyone feel strongly that we should still do this?

-- Kevin


On 6/24/2021 2:37 AM, Ty Young wrote:
Netbeans no longer defaults to creating Ant based projects unlike 
years ago & there has been, IIRC, some talk on further retiring 
support for it and Maven works just fine provided that you use the 
JavaFX Maven plugin*.



Still maybe worth fixing since support isn't completely removed and 
there may be cases where one might want to download and use a JavaFX 
SDK dist.



* the situation with IDE JavaFX support is complicated due to a 
project created outside Netbeans not set up with hacks Netbeans needs 
for green run button but there is an in-IDE option to do javafx:run 
and intellij needs a custom run action created to do javafx:run AFAIK, 
but I digress.



On 6/14/2021 1:15 PM, Kevin Rushforth wrote:
We deliver a set of modular jars in the lib directory of the 
standalone JavaFX SDK. We also deliver src.zip for use by IDEs into 
that same directory. If you add the lib directory to your 
application's module path in your IDE, it will try to load src.zip as 
if it were a jar file, and will fail. This is a pain point for 
developers using the SDK. This problem has been raised on the mailing 
list a couple of times, and I think it's time to fix it. This issue 
is tracked in JBS by JDK-8258499 [1].


I propose to move the src.zip file from the lib directory to the top 
directory of the SDK.


Alternatively, we could create a new directory for src.zip (either a 
sibling of lib or sub-directory under lib). However, I think it would 
be easier to find in the top dir of the SDK, and I don't see the need 
for a new directory just to hold src.zip.


Before I create the PR and the associated CSR, I'd like to hear 
developer's opinions on this.


-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8258499





Re: Proposed schedule for JavaFX 17

2021-06-24 Thread Kevin Rushforth
As a reminder, Rampdown Phase 1 (RDP1) for JavaFX 17 starts on July 8, 
2021 at 16:00 UTC (09:00 Pacific time), which is two weeks from today.


During rampdown of JavaFX 17, the "master" branch of the jfx repo will 
be open for JavaFX 18 fixes.


Please allow sufficient time for any feature that needs a CSR. New 
features should be far enough along in the review process that you can 
finalize the CSR before Thursday, July 1, or it is likely to miss the 
window for this release, in which case it can be targeted for JavaFX 18.


We will follow the same process as in previous releases for getting 
fixes into JavaFX 17 during rampdown. I'll send a message with detailed 
information later, but candidates for fixing during RDP1 are P1-P3 bugs 
(as long as they are not risky) and test or doc bugs of any priority. 
Some small enhancements might be considered during RDP1, but they 
require explicit approval; the bar will be high for such requests.


-- Kevin


On 3/30/2021 7:30 AM, Kevin Rushforth wrote:

I just noticed a copy/paste error:

> We plan to fork a jfx16 stabilization branch at RDP1.

That should be a "jfx17" branch.

-- Kevin


On 3/30/2021 7:28 AM, Kevin Rushforth wrote:

 Here is the proposed schedule for JavaFX 17.

RDP1: Jul 8, 2021 (aka “feature freeze”)
RDP2: Jul 29, 2021
Freeze: Aug 19, 2021
GA: Sep 7, 2021

We plan to fork a jfx16 stabilization branch at RDP1. The GA date is 
expected to be one week ahead of JDK 17, matching what we did for 16.


As was done for the previous release, the start of RDP1, the start of 
RDP2, and the code freeze will be 16:00 UTC on the respective dates.


Please let Johan or me know if you have any questions.

-- Kevin







Re: RFR: 8269147: Update GStreamer to version 1.18.4

2021-06-24 Thread Kevin Rushforth
On Thu, 24 Jun 2021 02:50:08 GMT, Alexander Matveev  
wrote:

> - GStreamer updated to 1.18.4.
>  - Tested on all platforms with all formats.

The code changes look good. I tested it on all three platforms. No problems.

@tiainen can be the second reviewer on this.

-

Marked as reviewed by kcr (Lead).

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


Re: [External] : Re: Cleanup JavaFX apps, tests, and scripts

2021-06-23 Thread Kevin Rushforth



> IDEs will need to adjust their files accordingly.

For the first step (the removal covered by JDK-8269259), I think I got 
them all for IntelliJ and Eclipse, but that will need to be verified.


I want to hear if there are any other comments, so I'll send a PR later 
this week or on Monday.


-- Kevin


On 6/23/2021 4:43 PM, Nir Lisker wrote:

Sounds good. I never understood the current organization scheme.

IDEs will need to adjust their files accordingly.

On Thu, Jun 24, 2021 at 1:31 AM Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


I missed one. I also propose to delete:

tests/functional/*

This was one of the directories that prompted this discussion in the
first place. It was on my working list to delete and I missed
copying it
into this email.

-- Kevin


On 6/23/2021 3:27 PM, Kevin Rushforth wrote:
> We discussed earlier the idea of cleaning up some of the unused
> programs and eventually reorganizing the apps and test directories.
>
> As a first step, I filed JDK-8269259 [1] in which I propose to
delete
> the following applications, tests, and scripts that are either
> obsolete or unmaintained:
>
> apps/performance/*
>
> apps/tests/HelloTest
>
> apps/toys/FXSlideShow
> apps/toys/Industrial
> apps/toys/Shape3DToy
> apps/toys/StretchyGrid
> apps/toys/TouchSuite
>
> tests/performance/VMPerformance
>
> tools/*
>
> While some of them might be useful, they aren't in their current
form,
> and it is likely not worth the effort to fix them. They will be
in the
> repo history if anyone really needs them.
>
> If anyone objects to a specific program or subdirectory in the
above
> list, let me know how you are using it or why you think it is still
> useful.
>
> To put this in context, this is step 1 of a multipart effort to
reduce
> unmaintained or obsolete applications, tests, and scripts in our
repo.
>
> When we are all done, the test directory will contain automated and
> manual tests that are built on a regular basis (and it should be
> straightforward to run the manual tests). The apps directory
will just
> contain the samples [2].
>
> The following directories will be examined during this extended
effort.
>
> apps/
>   performance/
>   tests/
>   toys/
>
> tests/
>   functional/
>   manual/
>   performance/
>
> tools/
>   gltrace/
>   scripts/
>
> As mentioned at the beginning, step 1 is to identify those programs
> that will be deleted. That way we don't expend any more effort
on them
> when we do subsequent steps.
>
> I expect the rest will be done incrementally, and include (not
> necessarily in order):
>
> 1. Wire up the programs under tests/manual to the build,
possibly with
> a new gradle task. If it isn't built as part of "gradle test" then
> that new task needs to be added to "gradle all"
>
> 2. Wire up the programs under tests/performance to the build,
probably
> the same build task as used in step 1.
>
> 3. Move the remaining test programs from apps/toys/* and
apps/tests/*
> to tests/manual/ -- since we currently use many of these in manual
> testing, they need to continue to be built by either "gradle
all" or
> "gradle test", and be easily able to run even if step 1 isn't done.
>
> 4. If there are any remaining test programs in apps/performance,
move
> them to tests/performance (currently I propose to delete them
all, so
> this step will go away).
>
> Comments?
>
> -- Kevin
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8269259
<https://bugs.openjdk.java.net/browse/JDK-8269259>
>
> [2] As a separate effort -- not directly associated with this
cleanup
> -- the samples could possibly be forked and maintained elsewhere as
> long as they are easy to download, build and run. Anything
related to
> apps/samples should be discussed in a separate email thread.
>





Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v29]

2021-06-23 Thread Kevin Rushforth
On Wed, 23 Jun 2021 22:37:11 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Corrected vertex shader

Tested on Mac and Linux. Works fine again.

-

Marked as reviewed by kcr (Lead).

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


Re: Cleanup JavaFX apps, tests, and scripts

2021-06-23 Thread Kevin Rushforth

I missed one. I also propose to delete:

tests/functional/*

This was one of the directories that prompted this discussion in the 
first place. It was on my working list to delete and I missed copying it 
into this email.


-- Kevin


On 6/23/2021 3:27 PM, Kevin Rushforth wrote:
We discussed earlier the idea of cleaning up some of the unused 
programs and eventually reorganizing the apps and test directories.


As a first step, I filed JDK-8269259 [1] in which I propose to delete 
the following applications, tests, and scripts that are either 
obsolete or unmaintained:


apps/performance/*

apps/tests/HelloTest

apps/toys/FXSlideShow
apps/toys/Industrial
apps/toys/Shape3DToy
apps/toys/StretchyGrid
apps/toys/TouchSuite

tests/performance/VMPerformance

tools/*

While some of them might be useful, they aren't in their current form, 
and it is likely not worth the effort to fix them. They will be in the 
repo history if anyone really needs them.


If anyone objects to a specific program or subdirectory in the above 
list, let me know how you are using it or why you think it is still 
useful.


To put this in context, this is step 1 of a multipart effort to reduce 
unmaintained or obsolete applications, tests, and scripts in our repo.


When we are all done, the test directory will contain automated and 
manual tests that are built on a regular basis (and it should be 
straightforward to run the manual tests). The apps directory will just 
contain the samples [2].


The following directories will be examined during this extended effort.

apps/
  performance/
  tests/
  toys/

tests/
  functional/
  manual/
  performance/

tools/
  gltrace/
  scripts/

As mentioned at the beginning, step 1 is to identify those programs 
that will be deleted. That way we don't expend any more effort on them 
when we do subsequent steps.


I expect the rest will be done incrementally, and include (not 
necessarily in order):


1. Wire up the programs under tests/manual to the build, possibly with 
a new gradle task. If it isn't built as part of "gradle test" then 
that new task needs to be added to "gradle all"


2. Wire up the programs under tests/performance to the build, probably 
the same build task as used in step 1.


3. Move the remaining test programs from apps/toys/* and apps/tests/* 
to tests/manual/ -- since we currently use many of these in manual 
testing, they need to continue to be built by either "gradle all" or 
"gradle test", and be easily able to run even if step 1 isn't done.


4. If there are any remaining test programs in apps/performance, move 
them to tests/performance (currently I propose to delete them all, so 
this step will go away).


Comments?

-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8269259

[2] As a separate effort -- not directly associated with this cleanup 
-- the samples could possibly be forked and maintained elsewhere as 
long as they are easy to download, build and run. Anything related to 
apps/samples should be discussed in a separate email thread.






Cleanup JavaFX apps, tests, and scripts

2021-06-23 Thread Kevin Rushforth
We discussed earlier the idea of cleaning up some of the unused programs 
and eventually reorganizing the apps and test directories.


As a first step, I filed JDK-8269259 [1] in which I propose to delete 
the following applications, tests, and scripts that are either obsolete 
or unmaintained:


apps/performance/*

apps/tests/HelloTest

apps/toys/FXSlideShow
apps/toys/Industrial
apps/toys/Shape3DToy
apps/toys/StretchyGrid
apps/toys/TouchSuite

tests/performance/VMPerformance

tools/*

While some of them might be useful, they aren't in their current form, 
and it is likely not worth the effort to fix them. They will be in the 
repo history if anyone really needs them.


If anyone objects to a specific program or subdirectory in the above 
list, let me know how you are using it or why you think it is still useful.


To put this in context, this is step 1 of a multipart effort to reduce 
unmaintained or obsolete applications, tests, and scripts in our repo.


When we are all done, the test directory will contain automated and 
manual tests that are built on a regular basis (and it should be 
straightforward to run the manual tests). The apps directory will just 
contain the samples [2].


The following directories will be examined during this extended effort.

apps/
  performance/
  tests/
  toys/

tests/
  functional/
  manual/
  performance/

tools/
  gltrace/
  scripts/

As mentioned at the beginning, step 1 is to identify those programs that 
will be deleted. That way we don't expend any more effort on them when 
we do subsequent steps.


I expect the rest will be done incrementally, and include (not 
necessarily in order):


1. Wire up the programs under tests/manual to the build, possibly with a 
new gradle task. If it isn't built as part of "gradle test" then that 
new task needs to be added to "gradle all"


2. Wire up the programs under tests/performance to the build, probably 
the same build task as used in step 1.


3. Move the remaining test programs from apps/toys/* and apps/tests/* to 
tests/manual/ -- since we currently use many of these in manual testing, 
they need to continue to be built by either "gradle all" or "gradle 
test", and be easily able to run even if step 1 isn't done.


4. If there are any remaining test programs in apps/performance, move 
them to tests/performance (currently I propose to delete them all, so 
this step will go away).


Comments?

-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8269259

[2] As a separate effort -- not directly associated with this cleanup -- 
the samples could possibly be forked and maintained elsewhere as long as 
they are easy to download, build and run. Anything related to 
apps/samples should be discussed in a separate email thread.




Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v28]

2021-06-23 Thread Kevin Rushforth
On Wed, 23 Jun 2021 18:21:20 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Grouped vec3's together

You need a corresponding change in the vertex shader.

modules/javafx.graphics/src/main/resources/com/sun/prism/es2/glsl/main.vert 
line 40:

> 38: vec3 attn;
> 39: float range;
> 40: vec3 dir;

These need to be reordered to match the changes in the fragment shaders. As it 
is, it causes a shader link error.

-

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


Re: Moving src.zip out of the lib directory of the JavaFX SDK

2021-06-23 Thread Kevin Rushforth
Are there any IDE users who are currently having problems as a result of 
this? If not, I'll retarget this for a future release.


-- Kevin


On 6/14/2021 1:15 PM, Kevin Rushforth wrote:
We deliver a set of modular jars in the lib directory of the 
standalone JavaFX SDK. We also deliver src.zip for use by IDEs into 
that same directory. If you add the lib directory to your 
application's module path in your IDE, it will try to load src.zip as 
if it were a jar file, and will fail. This is a pain point for 
developers using the SDK. This problem has been raised on the mailing 
list a couple of times, and I think it's time to fix it. This issue is 
tracked in JBS by JDK-8258499 [1].


I propose to move the src.zip file from the lib directory to the top 
directory of the SDK.


Alternatively, we could create a new directory for src.zip (either a 
sibling of lib or sub-directory under lib). However, I think it would 
be easier to find in the top dir of the SDK, and I don't see the need 
for a new directory just to hold src.zip.


Before I create the PR and the associated CSR, I'd like to hear 
developer's opinions on this.


-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8258499





Re: RFR: 8267554: Support loading stylesheets from data-URIs [v4]

2021-06-23 Thread Kevin Rushforth
On Tue, 22 Jun 2021 23:15:55 GMT, Michael Strauß  wrote:

>> This PR extends data URI support to allow stylesheets to be loaded from data 
>> URIs.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   simplified branch in StyleManager.loadStylesheetUnPrivileged

The API and spec changes look good. As I mentioned in a comment added to the 
CSR, it is ready to move to "Proposed". Once we are farther along in the code 
review, I'll formally review the CSR.

One thing I noticed is that the diffs don't exactly match what you pasted in 
the CSR (looks like a copy/paste issue). The `Scene::setUserAgentStylesheet` 
and `SubScene::setUserAgentStylesheet` methods each have three unchanged lines 
that are marked with the `+` diff indicator. Can you double-check?

I'll review the implementation next.

-

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


Re: RFR: 8264137: Suppress deprecation and removal warnings of internal methods [v2]

2021-06-23 Thread Kevin Rushforth
On Wed, 23 Jun 2021 11:16:09 GMT, Ajit Ghaisas  wrote:

>> This PR suppresses -PLINT="removal" warnings and fixes -PLINT="deprecation" 
>> warnings. I have created separate commits for each type of warnings for the 
>> ease of review.
>> 
>> `gradle --info -PLINT="deprecation,removal"` -- results in warnings during 
>> build without this PR and results in no warnings with this PR.
>
> Ajit Ghaisas has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Suppress/Fix LINT-deprecation warnings from tests
>  - Suppress LINT-removal warnings from tests

Test changes look good, too.

-

Marked as reviewed by kcr (Lead).

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


Integrated: 8268915: WebKit build fails with Xcode 12.5

2021-06-23 Thread Kevin Rushforth
On Thu, 17 Jun 2021 14:52:53 GMT, Kevin Rushforth  wrote:

> The JavaFX WebKit build fails with Xcode 12.5 + MacOS 11.3 SDK. This is 
> related to the added C++20 support where some of the system include files now 
> do `#include `. Because the macOS file system is case insensitive, 
> it matches a file named `VERSION` in the sqlite source directory. That file 
> is just a plain text file (not used by our build at all) so the fix is to 
> rename it to `VERSION.txt`. I've done a CI build on all three platforms. 
> @jgneff has verified that renaming this file fixes the build for XCode 12.5.

This pull request has now been integrated.

Changeset: 8e11b94f
Author:Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/8e11b94ff90d6640c8e7a1dc0da83599b9d16b84
Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod

8268915: WebKit build fails with Xcode 12.5

Reviewed-by: ajoseph

-

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


Re: RFR: 8267554: Support loading stylesheets from data-URIs [v3]

2021-06-22 Thread Kevin Rushforth
On Mon, 21 Jun 2021 15:04:26 GMT, Michael Strauß  wrote:

>> An overloaded method with a new signature is still a new method that adds to 
>> the public API, although in an easy to understand way. I agree that the same 
>> argument could be made for it as for `loadBinary(URL)`, but if neither are 
>> needed, then maybe we would be better off to deprecate the existing method 
>> (not as part of this enhancement, of course), rather than adding a similar 
>> one that also isn't needed as part of the public API. On the other hand, if 
>> there is a good reason, I don't see a problem adding this new method.
>> 
>> If you do propose to add this API, you will need to add an appropriate 
>> `@since` tag, and you should write an API-level test for this method as part 
>> of `StylesheetTest`.
>
> It seems to me that the only useful thing an application developer could do 
> with a `Stylesheet` instance is to inspect its contents (the relevant APIs in 
> javafx.css are public, after all). That seems like a niche use case, but to 
> the extent that there are applications out there that inspect binary 
> stylesheets, having the option to load them from a stream seems at least 
> marginally useful.
> 
> I slightly lean towards adding this public API also to not having to 
> introducing a new helper class _just_ for this one benign method. If you have 
> a stronger opinion on this, please go ahead and make the call.

Avoiding the need for an accessor isn't sufficient reason, but I do like the 
symmetry with the existing `loadBinary(URL)` method. Also, one can create a 
`Stylesheet` by parsing a text file using the various `parse` methods of 
`CssParser`, so there is ample precedent for doing this. I have no objection to 
the public method.

-

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v27]

2021-06-22 Thread Kevin Rushforth
On Sat, 19 Jun 2021 13:46:54 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated float constant registers

I don't have any more comments or concerns. If you choose to reorder the 
variables or make other simple changes, I'll re-approve.

-

Marked as reviewed by kcr (Lead).

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v27]

2021-06-22 Thread Kevin Rushforth
On Tue, 22 Jun 2021 10:28:16 GMT, Ambarish Rapte  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated float constant registers
>
> modules/javafx.graphics/src/main/resources/com/sun/prism/es2/glsl/main.vert 
> line 50:
> 
>> 48: 
>> 49: varying vec4 lightTangentSpacePositions[3];
>> 50: varying vec4 lightTangentSpaceDirections[3];
> 
> `lightTangentSpacePositions` is pre existing variable. I think the name 
> `lightTangentSpacePositions` is misguiding here. We use this variable to hold 
> a point to light vector in tangent space. I think the name should have been 
> something like, `tangentSpacePointToLightVec` or 
> `pointToLightVecInTangentSpace`. Again this is not mandatory for this PR, But 
> it did confuse me a bit while reading.
> 
> The other variable name `lightTangentSpaceDirections` sounds correct, as it 
> holds the lights direction in tangent space, but if we change the existing 
> variable then similarly this variable name should be also be changed.

Since this is preexisting, I probably wouldn't change it as part of this PR, 
but instead it could be done as part of the future work to support more than 3 
lights.

-

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


RFR: 8231558: [macos] Platform.exit causes assertion error on macOS 10.15 or later

2021-06-22 Thread Kevin Rushforth
This is a fix for the assertion error message that is printed to the console on 
macOS 10.15 or later when an application calls `Platform.exit` while a `Stage` 
is showing.

The root cause is a latent bug in the JavaFX glass code that was revealed by an 
apparent change of behavior in macOS. A few of the object deallocation methods, 
which are called by the Objective C auto-release mechanism, use the standard 
`GET_MAIN_JENV` macro to get the JNI environment. The macro will print an 
assertion warning if Java has been detached. I instrumented the code and can 
see that `GlassViewDelegate::dealloc` is now called after the 
`GlassApplication` main loop has detached Java. Since we don't control when the 
dealloc method is called, it is not correct to do the assertion check in those 
cases. Some of the dealloc methods already skip this assertion check by 
grabbing the jEnv pointer directly, so we need to fix the others. I added a new 
variant of the macro called `GET_MAIN_JENV_NOWARN` with a comment indicating 
that is suitable for use by the dealloc methods.

In addition to verifying that the test program attached to JBS now exits 
cleanly with no assertion failure message, I added an automated system test 
that fails on macOS before the fix and passes after the fix. On other platforms 
it passes already.

-

Commit messages:
 - 8231558: [macos] Platform.exit causes assertion error on macOS 10.15 or later

Changes: https://git.openjdk.java.net/jfx/pull/540/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=540=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8231558
  Stats: 208 lines in 7 files changed: 204 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jfx/pull/540.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/540/head:pull/540

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


Re: split code and tests in a PR

2021-06-22 Thread Kevin Rushforth
When I am doing a review and want to test a PR, I do something similar 
to what Johan mentioned. I download the PR to my local fork, verify it 
(often with manual testing in addition to the provided tests), and then 
backout the code changes to see whether the provided automated test 
would catch the failure.


The Skara bots don't run any tests at all. What Skara does do is run a 
PR check that looks for the results of a GitHub actions test run, which 
is run in the context of the personal fork of the PR author, and present 
those results to the reviewers of the PR. So we already get this level 
of testing.


I can't think of a good way to automate running the new / modified tests 
without the fix. I don't think we want Skara to get into the business of 
understanding enough about the different projects (jfx, jdk, skara 
itself, etc) to be able to split a PR into multiple parts. Also, we 
don't run any tests either in the bots themselves or in the openjdk 
group on GitHub, only on individual user's personal forks. Anything we 
do would be down to whatever we could run in the GitHub actions on the 
developer's personal fork.


-- Kevin


On 6/22/2021 9:06 AM, Nir Lisker wrote:

I also thought it would be useful for the bot to run the tests after the
patch and notify on a failure. I didn't think about applying the new tests
to the old code with "hopes" for a failure, I like this idea. Providing 2
downloads is also useful.

One thing we need to be careful with is that an older test can fail with a
new patch even though the tests that come with the patch pass because the
patch could have broken some dependent code (which could also mean that the
problem is in the dependent code).
How expensive is it for the bot to run the tests?

On Tue, Jun 22, 2021 at 6:31 PM Johan Vos  wrote:


Hi,

I really like the automation that Skara is providing. It saves time for
committers as well as for reviewers. In order to increase productivity even
more (without decreasing quality), there might be an additional step we
might somehow automate. Many PR's contain changes in java code and in test
code. What I typically do as a first step of a review, is to first run the
tests on the latest code, then apply the PR, and run the tests again.
Clearly, no test should fail in the second run, while it would be ok (and
desired) that at least 1 tests fails in the first run.

My typical approach for this is to download the diff, and manually split it
in 2 parts: code + tests. I then apply the part with the tests, and run
them. Next, I apply the part with the code, and re-run the tests.
Clearly, this is not enough for a review, but if we automate this, it gives
an additional indication that a PR is going in the right direction.

I'm not sure how easy/hard this would be to implement. The bot should be
able to detect the `src/test/java`, and provide 2 downloads in the comments
of the PR ("download test diff" and "download code diff"). It can also
execute those steps in a github action, and fail if the resulting tests are
failing after applying the code diff.

Thoughts?

- Johan





Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v27]

2021-06-22 Thread Kevin Rushforth
On Tue, 22 Jun 2021 11:25:42 GMT, Ambarish Rapte  wrote:

> Both Point and Spot light are not correctly applied to Mesh when number of 
> faces is increased to more than 60.
> Following are steps to reproduce with Point light with existing source 
> without this PR.
> As this is an existing issue we can address it in a follow on.

@arapte can you file a JBS bug for this?

-

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


Re: RFR: 8264137: Suppress deprecation and removal warnings of internal methods

2021-06-21 Thread Kevin Rushforth
On Mon, 21 Jun 2021 11:24:39 GMT, Ajit Ghaisas  wrote:

> This PR suppresses -PLINT="removal" warnings and fixes -PLINT="deprecation" 
> warnings. I have created separate commits for each type of warnings for the 
> ease of review.
> 
> `gradle --info -PLINT="deprecation,removal"` -- results in warnings during 
> build without this PR and results in no warnings with this PR.

Looks good.

-

Marked as reviewed by kcr (Lead).

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


Re: RFR: 8264137: Suppress deprecation and removal warnings of internal methods

2021-06-21 Thread Kevin Rushforth
On Mon, 21 Jun 2021 11:24:39 GMT, Ajit Ghaisas  wrote:

> This PR suppresses -PLINT="removal" warnings and fixes -PLINT="deprecation" 
> warnings. I have created separate commits for each type of warnings for the 
> ease of review.
> 
> `gradle --info -PLINT="deprecation,removal"` -- results in warnings during 
> build without this PR and results in no warnings with this PR.

I'd like a second pair of eyes on this. The changes are very straight-forward, 
but there are a lot of them and it would be easy to have a copy-paste error and 
use `snapSizeX` instead of `snapSizeY` or vice versa.

-

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


Re: RFR: 8264137: Suppress deprecation and removal warnings of internal methods

2021-06-21 Thread Kevin Rushforth
On Mon, 21 Jun 2021 11:24:39 GMT, Ajit Ghaisas  wrote:

> This PR suppresses -PLINT="removal" warnings and fixes -PLINT="deprecation" 
> warnings. I have created separate commits for each type of warnings for the 
> ease of review.
> 
> `gradle --info -PLINT="deprecation,removal"` -- results in warnings during 
> build without this PR and results in no warnings with this PR.

I tested this patch, and verified that it does eliminate the deprecation and 
removal warnings when building the sdk.

There are still a few warnings when compiling the tests. I ran the following 
command:


$ gradle --continue --info -PLINT="deprecation,removal" test


I get the following deprecation warnings:


modules\javafx.graphics\src\shims\java\javafx\scene\input\GestureEventShim.java:48:
 warning: [deprecation] GestureEvent(Object,EventTarget,EventType) in GestureEvent has been deprecated
modules\javafx.graphics\src\test\java\test\javafx\css\StylesheetTest.java:628: 
warning: [deprecation] toURL() in File has been deprecated
modules\javafx.graphics\src\test\java\test\javafx\scene\Scenegraph_eventHandlers_Test.java:342:
 warning: [deprecation] EventType() in EventType has been deprecated
modules\javafx.controls\src\test\java\test\javafx\scene\chart\XYChartTest.java:92:
 warning: [deprecation] set(S,V,StyleOrigin) in CssMetaData has been deprecated
modules\javafx.controls\src\test\java\test\javafx\scene\control\LabeledTest.java:768:
 warning: [deprecation] set(S,V,StyleOrigin) in CssMetaData has been deprecated
modules\javafx.controls\src\test\java\test\javafx\scene\control\TreeItemTest.java:201:
 warning: [deprecation] getNodeLevel(TreeItem) in TreeView has been 
deprecated
modules\javafx.controls\src\test\java\test\javafx\scene\control\TreeItemTest.java:208:
 warning: [deprecation] getNodeLevel(TreeItem) in TreeView has been 
deprecated
modules\javafx.controls\src\test\java\test\javafx\scene\control\TreeItemTest.java:217:
 warning: [deprecation] getNodeLevel(TreeItem) in TreeView has been 
deprecated
modules\javafx.controls\src\test\java\test\javafx\scene\control\TreeItemTest.java:231:
 warning: [deprecation] getNodeLevel(TreeItem) in TreeView has been 
deprecated
modules\javafx.controls\src\test\java\test\javafx\scene\control\TreeItemTest.java:232:
 warning: [deprecation] getNodeLevel(TreeItem) in TreeView has been 
deprecated
modules\javafx.fxml\src\test\java\test\com\oracle\javafx\fxml\test\TestLoadPerformance.java:107:
 warning: [deprecation] XMLReaderFactory in org.xml.sax.helpers has been 
deprecated
tests\system\src\test\java\test\robot\com\sun\glass\ui\monocle\input\devices\TestTouchDevices.java:46:
 warning: [deprecation] newInstance() in Class has been deprecated
tests\system\src\test\java\test\robot\javafx\embed\swing\JFXPanelTest.java:95: 
warning: [deprecation] BUTTON1_MASK in InputEvent has been deprecated
tests\system\src\test\java\test\robot\javafx\embed\swing\JFXPanelTest.java:96: 
warning: [deprecation] BUTTON1_MASK in InputEvent has been deprecated
tests\system\src\test\java\test\robot\javafx\embed\swing\NonFocusableJFXPanelTest.java:74:
 warning: [deprecation] BUTTON1_MASK in InputEvent has been deprecated
tests\system\src\test\java\test\robot\javafx\embed\swing\NonFocusableJFXPanelTest.java:75:
 warning: [deprecation] BUTTON1_MASK in InputEvent has been deprecated


and the following removal warnings:


modules\javafx.base\src\shims\java\javafx\util\converter\DateTimeStringConverterShim.java:34:
 warning: [removal] timeStyle in DateTimeStringConverter has been deprecated 
and marked for removal
modules\javafx.base\src\shims\java\javafx\util\converter\DateTimeStringConverterShim.java:38:
 warning: [removal] pattern in DateTimeStringConverter has been deprecated and 
marked for removal
modules\javafx.base\src\shims\java\javafx\util\converter\DateTimeStringConverterShim.java:42:
 warning: [removal] dateStyle in DateTimeStringConverter has been deprecated 
and marked for removal
modules\javafx.base\src\shims\java\javafx\util\converter\DateTimeStringConverterShim.java:46:
 warning: [removal] getDateFormat() in DateTimeStringConverter has been 
deprecated and marked for removal
modules\javafx.base\src\shims\java\javafx\util\converter\DateTimeStringConverterShim.java:50:
 warning: [removal] dateFormat in DateTimeStringConverter has been deprecated 
and marked for removal
modules\javafx.base\src\shims\java\javafx\util\converter\DateTimeStringConverterShim.java:54:
 warning: [removal] locale in DateTimeStringConverter has been deprecated and 
marked for removal


You can either fix it as part of this bug fix or else file a follow-on bug for 
the tests.

-

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


Re: RFR: 8267554: Support loading stylesheets from data-URIs [v2]

2021-06-21 Thread Kevin Rushforth
On Sun, 20 Jun 2021 00:22:57 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java line 301:
>> 
>>> 299:  * css version or if an I/O error occurs while reading from the 
>>> stream
>>> 300:  */
>>> 301: public static Stylesheet loadBinary(InputStream stream) throws 
>>> IOException {
>> 
>> Why do you need to add this public method to the API? I don't see any 
>> discussion as to why JavaFX applications need it. It looks like it is just 
>> being used internally by `StyleManager`. Unless there is a compelling reason 
>> to add this to the API, you will need to make this method package-scope and 
>> use an accessor to access it from `StyleManager`.
>
> I don't know why JavaFX applications would need this API. But the argument 
> should be the same argument as for `loadBinary(URL)`, which is also only used 
> internally by `StyleManager`. I think that this isn't opening up any new 
> effective API surface, because it's arguably just another overload of the 
> existing API.

An overloaded method with a new signature is still a new method that adds to 
the public API, although in an easy to understand way. I agree that the same 
argument could be made for it as for `loadBinary(URL)`, but if neither are 
needed, then maybe we would be better off to deprecate the existing method (not 
as part of this enhancement, of course), rather than adding a similar one that 
also isn't needed as part of the public API. On the other hand, if there is a 
good reason, I don't see a problem adding this new method.

If you do propose to add this API, you will need to add an appropriate `@since` 
tag, and you should write an API-level test for this method as part of 
`StylesheetTest`.

-

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


Re: RFR: 8267554: Support loading stylesheets from data-URIs

2021-06-18 Thread Kevin Rushforth
On Fri, 18 Jun 2021 01:23:50 GMT, Michael Strauß  wrote:

> This PR extends data URI support to allow stylesheets to be loaded from data 
> URIs.

I looked at just the public API and spec, and have two comments:

* I don't see any justification for adding 
`Stylesheet::loadBinary(InputStream)` to the public API. See my comments inline.
* Do you intend to support setting a user agent stylesheet via a data URL? The 
docs should be explicit about whether or not a data URI can be used for the 
following methods:
  * 
[Application::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/application/Application.java#L521)
  * 
[Scene::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/Scene.java#L1697)
  * 
[SubScene::setUserAgentStylesheet](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/SubScene.java#L687)

modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java line 301:

> 299:  * css version or if an I/O error occurs while reading from the 
> stream
> 300:  */
> 301: public static Stylesheet loadBinary(InputStream stream) throws 
> IOException {

Why do you need to add this public method to the API? I don't see any 
discussion as to why JavaFX applications need it. It looks like it is just 
being used internally by `StyleManager`. Unless there is a compelling reason to 
add this to the API, you will need to make this method package-scope and use an 
accessor to access it from `StyleManager`.

-

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


Re: RFR: 8269026: PasswordField doesn't render bullet character on Android [v2]

2021-06-18 Thread Kevin Rushforth
On Fri, 18 Jun 2021 14:45:21 GMT, Jose Pereda  wrote:

>> This PR modifies the PasswordField's bullet character used on Android, as 
>> the current unicode code is not supported for most fonts, including Roboto.
>
> Jose Pereda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address feedback from reviewer

Marked as reviewed by kcr (Lead).

-

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


Re: RFR: 8269026: PasswordField doesn't render bullet character on Android

2021-06-18 Thread Kevin Rushforth
On Fri, 18 Jun 2021 14:08:25 GMT, Jose Pereda  wrote:

> This PR modifies the PasswordField's bullet character used on Android, as the 
> current unicode code is not supported for most fonts, including Roboto.

Marked as reviewed by kcr (Lead).

modules/javafx.controls/src/android/java/javafx/scene/control/skin/TextFieldSkinAndroid.java
 line 82:

> 80: return String.valueOf(BULLET).repeat(txt.length());
> 81: } else {
> 82: return txt;

Should this call `return super.maskText(txt);` in case the implementation of 
the superclass method ever changes? Either is OK with me.

-

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v26]

2021-06-18 Thread Kevin Rushforth
On Thu, 17 Jun 2021 19:31:29 GMT, Kevin Rushforth  wrote:

>> Nir Lisker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed method call in glsl shaders
>
> modules/javafx.graphics/src/main/native-prism-d3d/hlsl/vsConstants.h line 58:
> 
>> 56: float4x3mBones[MAX_BONES] : register(c35);
>> 57: 
>> 58: float4  gReserved240[16] : register(c240);
> 
> `gReserved240` is now at the wrong location (it should be 245), so if it were 
> ever used it would be a problem. It should be updated to avoid confusion at 
> least.

I think the size should be updated to 11 as well (since we probably don't want 
to go past 256).

-

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v26]

2021-06-17 Thread Kevin Rushforth
On Thu, 17 Jun 2021 00:38:56 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed method call in glsl shaders

I did a full test run on 5 different system, including manual tests on 4 of 
them:

Windows 10 w/ Intel graphics
Linux w/ NVIDIA graphics (no manual testing)
Linux VM guest running on Windows 10 host
Mac w/ discrete graphics
Mac w/ integrated graphics

No problems detected. All looks good.

I think there could be some additional tuning done for point lights, but that 
could be looked at in a follow-on fix.

I reviewed the CSR and it is ready to be Finalized.

I finished reviewing the shader code, and left a couple comments on the HLSL 
shaders.

modules/javafx.graphics/src/main/native-prism-d3d/hlsl/psConstants.h line 28:

> 26: // see 

RFR: 8268915: WebKit build fails with Xcode 12.5

2021-06-17 Thread Kevin Rushforth
The JavaFX WebKit build fails with Xcode 12.5 + MacOS 11.3 SDK. This is related 
to the added C++20 support where some of the system include files now do 
`#include `. Because the macOS file system is case insensitive, it 
matches a file named `VERSION` in the sqlite source directory. That file is 
just a plain text file (not used by our build at all) so the fix is to rename 
it to `VERSION.txt`. I've done a CI build on all three platforms. @jgneff has 
verified that renaming this file fixes the build for XCode 12.5.

-

Commit messages:
 - 8268915: WebKit build fails with Xcode 12.5

Changes: https://git.openjdk.java.net/jfx/pull/535/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=535=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8268915
  Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/535.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/535/head:pull/535

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v25]

2021-06-16 Thread Kevin Rushforth
On Wed, 16 Jun 2021 14:08:24 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix for glsl shaders

The glsl `computeLight` methods are still calling the now-commented-out 
alternative shaders, so it fails at load time. See, for example, 
[main1Light.frag#L129](https://github.com/openjdk/jfx/blob/c9fb2452cffad5e9256e4ef7dbb733a69a655dac/modules/javafx.graphics/src/main/resources/com/sun/prism/es2/glsl/main1Light.frag#L129).
 The rest of the changes look fine.

-

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


Re: Command Line Tools for Xcode 12.4 → 12.5

2021-06-16 Thread Kevin Rushforth
Our production builds use Xcode 12.4 [1].  The GitHub Actions build 
haven't caught up yet. See JDK-8266218 [2].


-- Kevin

[1] https://github.com/openjdk/jfx/blob/master/build.properties#L93
[2] https://bugs.openjdk.java.net/browse/JDK-8266218


On 6/16/2021 10:54 AM, John Neffenger wrote:

On 6/16/21 10:21 AM, John Neffenger wrote:

4. Finally look up what the GitHub Actions are using (12.4).


Oops, make that "Finally look up what the project uses (12.4)."

For the record, our GitHub Actions use Xcode_11.3.1. [1]

John

[1]: 
https://github.com/openjdk/jfx/blob/master/.github/workflows/submit.yml#L202




Re: Command Line Tools for Xcode 12.4 → 12.5

2021-06-16 Thread Kevin Rushforth

Can you file a bug? We will take a look at it.

Thanks.

-- Kevin


On 6/16/2021 9:27 AM, John Neffenger wrote:

Just a heads-up about using the latest Xcode 12.5 ...

I use the Command Line Tools for Xcode 12.4 (at 431 MB) to build 
JavaFX on macOS as an alternative to the full Xcode package (at 11.86 
GB). Thank you, Arunprasad Rajkumar! [1]


Then Apple Software Update installed the latest Command Line Tools for 
Xcode 12.5, and my builds of JavaFX with WebKit started failing. You 
can find out what version you're using with the command:


  $ pkgutil --pkg-info=com.apple.pkg.CLTools_Executables
  package-id: com.apple.pkg.CLTools_Executables
  version: 12.5.0.0.1.1617976050
  volume: /
  location: /
  install-time: 1623800794
  groups: com.apple.FindSystemFiles.pkg-group

You can download and revert to the 12.4 release below:

  More Downloads
  https://developer.apple.com/download/all/?q=12.4

Note that the OpenJFX project uses version 12.4 (build.properties):

  jfx.build.macosx.xcode.version=Xcode12.4+1.0

The errors using version 12.5 start with:

  .../modules/javafx.web/src/main/native/Source/ThirdParty/
    sqlite/./version:1:1: error: expected unqualified-id

  /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/
    usr/include/c++/v1/cstddef:49:9: error: no member named
    'ptrdiff_t' in the global namespace

and end with:

  PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
  Preprocessed source(s) and associated run script(s) are located at:
  clang: note: diagnostic msg: /var/folders/...
  clang: note: diagnostic msg: /var/folders/...
  clang: note: diagnostic msg: Crash backtrace is located in
  clang: note: diagnostic msg: /Users/john/Library/Logs/
DiagnosticReports/clang__.crash
  clang: note: diagnostic msg: (choose the .crash file that
 corresponds to your crash)
  clang: note: diagnostic msg:

John

[1]: https://github.com/openjdk/jfx/pull/13





Re: RFR: 8268219: hlsprogressbuffer should provide PTS after GStreamer update

2021-06-15 Thread Kevin Rushforth
On Sat, 12 Jun 2021 00:10:55 GMT, Alexander Matveev  
wrote:

> - Reverted JDK-8268152 fix in gstbaseparse.c, since it is no longer needed.
>  - Our hlsprogressbuffer outputs buffers in time format, but without any PTS. 
> After GStreamer update mpregparser no longer tries to figure out timestamps 
> if stream in time format and it will assume that upstream provides 
> timestamps. Fixed by providing starting timestamp at the beginning or after 
> seek. In this case mpegparser able to figure out timestamps and will provide 
> them for each buffer downstream.
>  - Segment start was also incorrect it should be seek position, otherwise 
> after seek playback waits for seek time. For example if we seek to 2 min, 
> mediaplayer hangs for 2 min and only after that resumes playback. I think it 
> worked before, since mpegparser handled PTS before.

Looks good. Tested on all three platforms.

-

Marked as reviewed by kcr (Lead).

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v24]

2021-06-15 Thread Kevin Rushforth
On Mon, 14 Jun 2021 01:06:25 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated links in glsl shaders

I also see the same problem as Ambarish does when I run it on my older MacBook 
Pro with integrated graphics. I tracked it down to a bug in the newly added 
`computeLight` method in the GLSL shaders. See my inline comments. When I apply 
that fix it works fine for me on my older MacBook. Additionally, my VirtualBox 
Linux system now works as expected. Basically, you were getting lucky that it 
worked at all on the graphics cards it did work on.

While you are making changes to the shaders, I think it's time to change the 
GLSL shaders to use `computeSpotlightFactor` and comment (or ifdef) out the 
other two, and similarly change the HLSL 

Re: RFR: 8267418: IntelliJ build and test of JavaFX does not work [v4]

2021-06-15 Thread Kevin Rushforth
On Wed, 2 Jun 2021 07:11:54 GMT, Marius Hanl  wrote:

>> ~~Question: I was wondering, should I create a ticket for this as well? 
>> Given the fact that I don't have an https://bugs.openjdk.java.net account, I 
>> need to use the official bug reporting tool, which looked a bit overkill to 
>> me since someone needs to check my created ticket, while this PR is only 
>> affecting the IntelliJ IDE with OpenJFX and not the JavaFX platform 
>> directly.~~
>> EDIT: Thank you, Kevin. :)
>> 
>> This PR fixes the errors you get when cloning and working with OpenJFX in 
>> IntelliJ IDE:
>> - The **.idea/misc.xml** is modified to use **JDK_11** as language level 
>> instead of JDK_8. 
>> -> This is the language level shown inside the **Project Structure**. (File 
>> -> Project Structure...)
>> - The **.idea/base.iml, .idea/controls.iml, .idea/fxml.iml, .idea/web.iml, 
>> .idea/graphics.iml** are modified to include/recognize the shims (as test 
>> resource, this is very similar to the configuration inside the .classpath 
>> file from Eclipse)
>> - EDIT: The projects are now recognized by IntelliJ-gradle 
>> (**.idea/gradle.xml**, **.idea/compiler.xml**)
>> 
>> **-> With this, I can run all normal tests with IntelliJ**
>> 
>> ### What I couldn't fix yet (When I tried, it looked like IntelliJ is 
>> overriding the settings on next gradle reload): 
>> - IntelliJ is not detecting javafx.graphic inside the shims
>> - All javafx.* dependencies are not found for the system tests
>> 
>> **-> If someone has a solution, feel free to comment :)**
>
> Marius Hanl has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reverted whitespace made by IntelliJ

Good find. If this works, it seems like a good compromise to me. We don't 
download any `*-sources` or `*-javadoc` artifacts in our build, but it seems 
that IDEs do.

If we add this exclusion, I think we should do it using separate bug ID and a 
separate PR (requiring two reviewers, since this is an area we need to be 
careful about). As part of that, we would need to update the 
[`gradle/README.txt`](https://github.com/openjdk/jfx/blob/master/gradle/README.txt)
 file to let people know why the `*-sources` / `*-javadoc`  exclusion is there.

-

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


Integrated: 8264139: Suppress removal warnings for Security Manager methods

2021-06-15 Thread Kevin Rushforth
On Thu, 3 Jun 2021 23:34:38 GMT, Kevin Rushforth  wrote:

> This PR adds the necessary `@SuppressWarnings("removal")` annotations for the 
> recently-integrated security manager deprecation, [JEP 
> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
> 
> There are four commits:
> 
> 1. 678b026 : A patch generated by @wangweij to add annotations to the runtime 
> (`modules/*/src/main/java`) using the same automated tooling that he used as 
> part of the implementation of JEP 411.
> 2. 9698e87 : Same as above for the tests.
> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a 
> static import) was causing a removal warning.
> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added 
> to an entire class, or to a large method where only a small part of the 
> method uses a deprecated method. This was done using similar techniques to 
> the following fixes that Weijun did in openjdk/jdk#4172.
>  
> The first two commits represent the bulk of the changes. Other than scanning 
> to ensure that there are no obvious errors, and testing, they probably don't 
> need the same level of scrutiny as the manual changes do.
> 
> I tested this on all three platforms by doing a build / test with `JDK_HOME` 
> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the 
> build with `gradle -PLINT=removal` and verified that there were removal 
> warnings for the security manager APIs without this fix and none with this 
> fix.
> 
> NOTE: The following files under `modules/javafx.web/src/android` and 
> `modules/javafx.web/src/ios` were not processed by the automated tool. As I 
> have no way to compile them, I chose not to manually fix them either, but 
> doing so would be trivial as a follow-up fix if desired.
> 
> 
> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java

This pull request has now been integrated.

Changeset: c81a7226
Author:Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/c81a722614e46844c285d4a4a623352ef227da87
Stats: 605 lines in 194 files changed: 461 ins; 19 del; 125 mod

8264139: Suppress removal warnings for Security Manager methods

Co-authored-by: Weijun Wang 
Co-authored-by: Kevin Rushforth 
Reviewed-by: aghaisas, arapte

-

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


Moving src.zip out of the lib directory of the JavaFX SDK

2021-06-14 Thread Kevin Rushforth
We deliver a set of modular jars in the lib directory of the standalone 
JavaFX SDK. We also deliver src.zip for use by IDEs into that same 
directory. If you add the lib directory to your application's module 
path in your IDE, it will try to load src.zip as if it were a jar file, 
and will fail. This is a pain point for developers using the SDK. This 
problem has been raised on the mailing list a couple of times, and I 
think it's time to fix it. This issue is tracked in JBS by JDK-8258499 [1].


I propose to move the src.zip file from the lib directory to the top 
directory of the SDK.


Alternatively, we could create a new directory for src.zip (either a 
sibling of lib or sub-directory under lib). However, I think it would be 
easier to find in the top dir of the SDK, and I don't see the need for a 
new directory just to hold src.zip.


Before I create the PR and the associated CSR, I'd like to hear 
developer's opinions on this.


-- Kevin

[1] https://bugs.openjdk.java.net/browse/JDK-8258499



Re: RFR: 8268642: Expand the javafx.beans.property.Property documentation

2021-06-14 Thread Kevin Rushforth
On Mon, 14 Jun 2021 17:06:34 GMT, Michael Strauß  wrote:

> * Expand the `Property.bind` and `Property.bindBidirectional` documentation
> * Change the name of the formal parameter of `Property.bind` to "source" 
> (currently, it is inconsistently named "observable", "rawObservable" or 
> "newObservable")

If this is go forward, it will need a CSR. Before you spend time creating one, 
I want to do a preliminary read through this, since it is a somewhat large doc 
change.

-

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v19]

2021-06-14 Thread Kevin Rushforth
On Mon, 14 Jun 2021 08:50:25 GMT, Michael Strauß  wrote:

>> This PR adds support for loading images from [inline data 
>> URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely 
>> supported by web browsers. This enables developers to package small images 
>> in CSS files, rather than separately deploying the images alongside the CSS 
>> file.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   changes

Looks good.

I reviewed the CSR, so you can move that to Finalized now.

-

Marked as reviewed by kcr (Lead).

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v17]

2021-06-12 Thread Kevin Rushforth
On Wed, 9 Jun 2021 14:20:41 GMT, Michael Strauß  wrote:

>> This PR adds support for loading images from [inline data 
>> URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely 
>> supported by web browsers. This enables developers to package small images 
>> in CSS files, rather than separately deploying the images alongside the CSS 
>> file.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   doc

I reviewed and tested the implementation. I left a couple questions inline.

modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line 
314:

> 312: }
> 313: 
> 314: theStream = new 
> ByteArrayInputStream(dataUri.getData());

By parsing the Data URI before calling `ImageTools.createInputStream` our 
internal handling will take precedence over a custom handler that an 
application might have installed. I'm not sure this is what we want.

modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 185:

> 183: @Override
> 184: public String toString() {
> 185: if (originalData.length() < 30) {

I think this should be `< 32`. As it stands, if length is `30` or `31` the data 
portion of the string will be 31 characters with either 2 or 3 middle chars 
replaced by `"..."`. In those cases it would be better to just use the original 
data.

-

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


Re: RFR: 8268219: Investigate gstmpegaudioparse PTS issue

2021-06-12 Thread Kevin Rushforth
On Sat, 12 Jun 2021 00:10:55 GMT, Alexander Matveev  
wrote:

> - Reverted JDK-8268152 fix in gstbaseparse.c, since it is no longer needed.
>  - Our hlsprogressbuffer outputs buffers in time format, but without any PTS. 
> After GStreamer update mpregparser no longer tries to figure out timestamps 
> if stream in time format and it will assume that upstream provides 
> timestamps. Fixed by providing starting timestamp at the beginning or after 
> seek. In this case mpegparser able to figure out timestamps and will provide 
> them for each buffer downstream.
>  - Segment start was also incorrect it should be seek position, otherwise 
> after seek playback waits for seek time. For example if we seek to 2 min, 
> mediaplayer hangs for 2 min and only after that resumes playback. I think it 
> worked before, since mpegparser handled PTS before.

@sashamatveev Since I presume you are done investigating the problem, can you 
change the title of the JBS bug and this PR to be more description of the 
problem being fixed?

-

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v22]

2021-06-11 Thread Kevin Rushforth
On Fri, 11 Jun 2021 01:30:27 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed review comments

I tested this on a somewhat older physical Linux box yesterday. It runs better 
than it does on my VirtuaBox VM, but there are still regressions.

When running the old attenuation.LightingSample sample with the SpotLight 
patch, on Linux only the last light is applied. For example, if all three 
lights are selected, only the magenta light is applied. If both red and blue 
lights are selected, only the blue light is applied. If the red light is 
selected by itself then it is applied. Without the patch, all selected lights 
are applied (up to max of 3). The same behavior happens with the new sample 
when using spot lights.

I wonder if this is 

Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight

2021-06-11 Thread Kevin Rushforth
On Wed, 9 Jun 2021 18:22:31 GMT, Andreas Heger 
 wrote:

> The inconsistent illumination happens on Macs with retina displays only if 
> the 3D shape is placed in a SubScene. The light sources are located with 
> wrong coordinates in sub scenes and this causes a different illumination. The 
> wrong coordinates for the light sources come from the fact that the retina 
> pixel scale factors are not used in a SubScene.
> 
> With this pull request, the retina pixel scale factors will be also used in 
> SubScenes and this should resolve the bug 
> [https://bugs.openjdk.java.net/browse/JDK-8255015](url)

The fix looks good. I tested it both in isolation and with PR #334 and it works 
on both a retina and non-retina display.

If you have time to write an automated test, that would be useful, but if not 
then a manual test would be OK.

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGSubScene.java 
line 211:

> 209: // The pixel scale factors must be set to the rttGraphics, 
> otherwise the position
> 210: // of the lights will not be scaled correctly on retina 
> displays
> 211: // See https://bugs.openjdk.java.net/browse/JDK-8255015

We typically don't put pointers to bug IDs in the code (we used to, and still 
sometimes do in some special cases, but here it isn't needed).

-

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


Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight

2021-06-09 Thread Kevin Rushforth
On Wed, 9 Jun 2021 18:22:31 GMT, Andreas Heger 
 wrote:

> The inconsistent illumination happens on Macs with retina displays only if 
> the 3D shape is placed in a SubScene. The light sources are located with 
> wrong coordinates in sub scenes and this causes a different illumination. The 
> wrong coordinates for the light sources come from the fact that the retina 
> pixel scale factors are not used in a SubScene.
> 
> With this pull request, the retina pixel scale factors will be also used in 
> SubScenes and this should resolve the bug 
> [https://bugs.openjdk.java.net/browse/JDK-8255015](url)

@andreas-heger Welcome to the `jfx` project. At a quick glance, the fix looks 
promising. Have you tested this on Windows with Hi-DPI to make sure there is no 
impact? Would you be able add an automated test case that fails (only on Mac 
retina) without the fix and passes (on all platforms) with your fix? Hi-DPI 
fixes are often tricky to test in an automated test, so if not, we can use the 
existing manual test.

@nlisker this is the same problem I noted while testing PR #334. Clearly I had 
forgotten that it was not only a preexisting bug, but a known bug that was 
already filed. I intend to test this alone and in connection with your PR.

-

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v16]

2021-06-09 Thread Kevin Rushforth
On Sat, 5 Jun 2021 17:31:17 GMT, Michael Strauß  wrote:

>> This PR adds support for loading images from [inline data 
>> URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely 
>> supported by web browsers. This enables developers to package small images 
>> in CSS files, rather than separately deploying the images alongside the CSS 
>> file.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   added comment, simplified DataURI.matchScheme()

The docs look good. I left one minor comment inline.

You can update the CSR when ready. As I mentioned in a comment in the CSR, the 
`Specification` section should consist of the diffs to the API documentation. 
Make sure you note what API element (e.g., class doc, `Image(String url)` 
constructor, and so forth) each change is for, if it isn't clear from the 
diffs. You can omit the removal of the `` html tags and the change from 
`example.` to `example:` from the diffs in the CSR.

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 95:

> 93:  * the protocol handlers that are registered for the application.
> 94:  *
> 95:  * If a URL uses the "data" scheme, the data must be base64-encoded

I think these two sentences should be in the same paragraph.

-

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


Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses

2021-06-09 Thread Kevin Rushforth
On Wed, 21 Apr 2021 18:17:27 GMT, Michael Strauß  wrote:

> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as 
> well as the corresponding `:focus-visible` and `:focus-within` CSS 
> pseudo-classes.

I took an initial pass at the API docs. I have a couple global comments.

First, can you reorder the properties and their associated methods such that 
they are grouped together? If the docs are on the (private) field, which is 
what we usually do, it should be followed immediately by the associated setter 
(if any), getter, and property methods.

The order should be:


/**
 * EXISTING API DOCS FOR FOCUSED PROPERTY
 */
private FocusPropertyBase focused;

protected final void setFocused(boolean value) {

public final boolean isFocused() {

public final ReadOnlyBooleanProperty focusedProperty() {

/**
 * API DOCS FOR NEW FOCUSVISIBLE PROPERTY
 */
private FocusPropertyBase focusVisible;

public final boolean isFocusVisible() {

public final ReadOnlyBooleanProperty focusVisibleProperty() {

...


Second, the two new properties need both a `@defaultValue` and an `@since NN` 
tag. You can optimistically use `@since 17` or you can use `@since 18` (the 
latter is more likely).

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8167:

> 8165: /**
> 8166:  * Indicates whether this {@code Node} should visibly indicate 
> focus.
> 8167:  * This flag is set when a node acquired input focus via keyboard 
> navigation,

`acquired` --> `acquires`

-

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


Re: Support :focus-visible CSS pseudoclass

2021-06-08 Thread Kevin Rushforth

Thanks to Tom and Pedro for their replies. That's good to know.

Michael: It does sound like this is a generally useful feature to do in 
the near future, so it makes sense to proceed with the review. I presume 
that the API as you have described it in the JBS description is what you 
are currently proposing? If so, I'll take a look at the API + docs in 
the next couple of days and we can take it from there.


As with any non-trivial feature, we won't rush it in -- it will go in 
when it's ready -- so it may already be too late for JavaFX 17, 
especially given the care that needs to be taken for any new CSS 
feature, and the number of other things in flight for JavaFX 17.


-- Kevin



On 6/7/2021 11:55 PM, Tom Schindl wrote:

Hi,

As an application developer I highly appreciate this built-in CSS 
support.


We had to roll our own version for focus-visible and focus-within when 
implementing our custom controls and L but we would leverage this 
built-in support to remove complexity from our codebase.


Beside Microsofts new L many Web-UI-Toolkits use this 
focus-visible-feature (and emulate it while not all browsers support it).


How we use focus-visible:

I think focus-visible has a fairly narrow scope so we use it to show
input focus differently depending on the way an element got focused
(mainly mouse-activation vs rest)

How we use focus-within:

We use focus-within to visiually indicate the area the focus is 
currently in by changing by lifting or coloring that area.


Tom

Am 08.06.21 um 01:08 schrieb Kevin Rushforth:
I'd be interested in hearing from application developers as to 
whether and how they would use such a feature. As with all 
non-trivial features, we don't want to do it if only one or two 
developers are asking for it. This seems a reasonably new CSS 
feature; Firefox just added support in FF 85 released earlier this 
year and Safari still doesn't. That suggests it might be a bit early 
to adopt this for JavaFX.


Also, one thing that isn't clear is whether there would be any user 
visible change using the standard themes. I presume that the answer 
is "no", meaning that unless your stylesheets do anything with 
focus-visible, it will not affect the look and feel of the app. That 
should be stated explicitly


I also presume that there would be no application visible changes 
when using the standard themes, other than the presence of the two 
new read-only boolean properties?


-- Kevin


On 6/7/2021 3:46 PM, Michael Strauß wrote:

I have expanded the scope of the proposal to include the :focus-within
pseudo-class, which completes the set of focus-related pseudo-classes.

The updated text of the proposal can be found in the JBS ticket:
https://bugs.openjdk.java.net/browse/JDK-8268225

I'm looking forward to comments.






Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v21]

2021-06-08 Thread Kevin Rushforth
On Mon, 7 Jun 2021 23:38:44 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed doc review comments

One more comment about this PR. As I mentioned earlier, I am no longer able to 
test lighting on my Ubuntu 20.04 Linux Guest running in VirtualBox on my 
Windows 10 Host. In this mode I have to set `-Dprism.forceGPU=true` to get any 
3D support at all, so it isn't a supported config, but point lights used to run 
before spotlight support was added to the shaders, and no longer do. This is 
not a stopper, but makes it harder for me to test (I need to arrange to get 
temporary access to a physical Linux box).

-

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


Re: [EXTERNAL] Looked-up color fails for -fx-background-color in JavaFX CSS file

2021-06-08 Thread Kevin Rushforth

If you have a test program, can you file it here?

https://bugreport.java.com/

Thanks.

-- Kevin


On 6/8/2021 2:12 PM, Duquette, Will (US 397J) wrote:

The problem arises when I switch out the stylesheet for an active GUI, with 
code like this:

root.getStylesheets().clear()
root.getStylesheets().add(styleSheetURL)

It's the clear() that's the problem.  If add the new stylesheet and then remove 
the old one, no messages are produced.

I have a simple test program, if you'd like to have this submitted to a bug 
tracker.

Will Duquette

On 6/8/21, 1:33 PM, "openjfx-dev on behalf of Duquette, Will (US 397J)" 
 wrote:

 Howdy!

 I'm migrating a code-base from JavaFX 8 to JavaFX 11, and I'm seeing some 
weird behavior.  I have some CSS that looks like this (in part)

 .root {
  -theme-button: #0679DE;
 }

 .button-primary {
  -fx-background-color: -theme-button;
 }

 It's been working great in JavaFX 8 for quite a long while.  When I load 
the same app, I get messages like this written to the console.

 Jun 08, 2021 10:40:07 AM javafx.scene.CssStyleHelper calculateValue
 WARNING: Caught 'java.lang.ClassCastException: class java.lang.String 
cannot be cast to class javafx.scene.paint.Paint (java.lang.String is in module 
java.base of loader 'bootstrap'; javafx.scene.paint.Paint is in module 
javafx.graphics@11.0.11 of loader 'platform')' while converting value for 
'-fx-background-color' from rule '*.button-primary' in stylesheet 
file:/Users/will/github/dgs-tool/out/production/dgs-lib/dsn-dark.css

 Surprisingly, the color seems to still get set—but warnings written to the 
console are worrying.

 If I replace -theme-button with #0679DE in the .button-primary style, the 
message goes away.

 I'm seeing this for -fx-background-color and -fx-border-color, but not for 
any other places where I use looked-up colors.

 Any ideas?







Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v21]

2021-06-08 Thread Kevin Rushforth
On Mon, 7 Jun 2021 23:38:44 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed doc review comments

I've finished reviewing the code for everything except the shaders themselves. 
That will be later this week I hope.

I left a few comments on the tests. I also have one suggestion for an 
additional system test. It would be useful to test that a `Spotlight` with 
values that simulate a `PointLight` -- `direction` of `(0, 0, 1)`, `falloff` of 
0, `innerAngle` of 0, `outerAngle` of 180 -- produce the same results as a 
`PointLight`.

tests/system/src/test/java/test/javafx/scene/lighting3D/LightingTest.java line 
59:

> 57: //  |/
> 58: //
> 59: public class LightingTest {

This class can be `abstract`.


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v21]

2021-06-08 Thread Kevin Rushforth
On Mon, 7 Jun 2021 23:38:44 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed doc review comments

I reviewed the API docs, and they look good. Can you update the CSR with the 
latest docs and move it to "Proposed"?

-

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


Re: [External] : Re: Convenience factories for Border and Background

2021-06-08 Thread Kevin Rushforth
I think that the convenience methods should just cover the most common 
cases, so I'd rather skip the dotted and dashed variants. It is a good 
question as to whether there ought to be a variant that takes width. I 
wouldn't do that as the only method, though. I'd lean towards not taking 
the width. Once you start getting into more parameters you can just use 
the constructors without much more trouble.


As for the names, I have a slight preference for Border.stroke and 
Background.fill.


-- Kevin


On 6/8/2021 4:25 AM, Nir Lisker wrote:
Are dashed and dotted used frequently? I find that I only use solid 
unless I'm doing something fancy.


On Tue, Jun 8, 2021 at 5:21 AM Michael Strauß > wrote:


What do you think about this variation?

    Border.solid(Paint color, double width) ->
        new Border(new BorderStroke(color, BorderStrokeStyle.SOLID,
null, new BorderWidths(width)))

    Border.dashed(Paint color, double width)  ->
        new Border(new BorderStroke(color, BorderStrokeStyle.DASHED,
null, new BorderWidths(width)))

    Border.dotted(Paint color, double width) ->
        new Border(new BorderStroke(color, BorderStrokeStyle.DOTTED,
null, new BorderWidths(width)))

    Background.fill(Paint color) ->
        new BackgroundFill(color, null, null)

This gives developers a good deal of customizability before needing to
fall back to using constructors.


Am Di., 8. Juni 2021 um 03:21 Uhr schrieb Nir Lisker
mailto:nlis...@gmail.com>>:
>
> The new API:
>
> 1. `Border.of(Paint stroke)` or `Border.stroke(Paint stroke)`
that does
> `new Border(new BorderStroke(Paint stroke ,
BorderStrokeStyle.SOLID, null,
> null));`
> 2. `Background.of((Paint fill)` or `Background.fill(Paint fill)`
that does
> `new Background(new BackgroundFill(Paint fill, null, null));`
>
> I don't mind either name choice.
>





Re: RFR: 8244075: Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is removed from Scene [v2]

2021-06-08 Thread Kevin Rushforth
On Tue, 8 Jun 2021 11:48:20 GMT, Ambarish Rapte  wrote:

>> Issue:
>> There are several issues related to Accelerator of MenuItem of a ConextMenu 
>> set on Control.
>> 1. Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is 
>> removed from Scene
>> 2. Accelerator is not updated correctly when the Control is removed from a 
>> Scene and Added to a different Scene
>> 3. Accelerator is not removed from Scene when the anchor node is removed 
>> from Scene and then it's ContextMenu is set to null
>> 
>> Fix:
>> The accelerator should be added or removed correctly according to the Scene 
>> property of the anchor node. 
>> The issue#3 in above list is only fixed for Button and fails for other 
>> Controls. A test is added for this scenario and I shall report a new issue 
>> to address it.
>> Added tests that fails before and pass after the fix.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update @ignore() with bugid

Marked as reviewed by kcr (Lead).

-

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


Re: [External] : Re: Convenience factories for Border and Background

2021-06-07 Thread Kevin Rushforth
If I recall, there were a few developers that chimed in. It's a simple 
enough addition -- at least your original proposal (not the suggestion 
of mirroring the Color API, which I don't like) -- that it seems OK to me.


Can you repost your currently proposed API and see if those who might 
like to use it are satisfied with it?


-- Kevin


On 6/7/2021 4:41 PM, Nir Lisker wrote:

Does this constitute sufficient interest in the enhancement?

On Thu, May 13, 2021 at 6:41 PM Michael Strauß 
mailto:michaelstr...@gmail.com>> wrote:


Another option could be to mirror the `Color` API in both `Border` and
`Background`, like in the following examples:

Color.rgb(125, 100, 75)
Border.rgb(125, 100, 75)
Background.rgb(125, 100, 75)

Color.gray(127)
Border.gray(127)
Background.gray(127)

Color.web("orange", 0.5)
Border.web("orange", 0.5)
Background.web("orange", 0.5)

We could also mirror the named color constants, which would enable a
very compact syntax:

StackPane pane = new StackPane();
pane.setBorder(Border.RED);
pane.setBackground(Background.BLUE);

This is very similar to how "red" or "blue" are valid values for
"-fx-border" or "-fx-background" in CSS.





Re: Support :focus-visible CSS pseudoclass

2021-06-07 Thread Kevin Rushforth
I'd be interested in hearing from application developers as to whether 
and how they would use such a feature. As with all non-trivial features, 
we don't want to do it if only one or two developers are asking for it. 
This seems a reasonably new CSS feature; Firefox just added support in 
FF 85 released earlier this year and Safari still doesn't. That suggests 
it might be a bit early to adopt this for JavaFX.


Also, one thing that isn't clear is whether there would be any user 
visible change using the standard themes. I presume that the answer is 
"no", meaning that unless your stylesheets do anything with 
focus-visible, it will not affect the look and feel of the app. That 
should be stated explicitly


I also presume that there would be no application visible changes when 
using the standard themes, other than the presence of the two new 
read-only boolean properties?


-- Kevin


On 6/7/2021 3:46 PM, Michael Strauß wrote:

I have expanded the scope of the proposal to include the :focus-within
pseudo-class, which completes the set of focus-related pseudo-classes.

The updated text of the proposal can be found in the JBS ticket:
https://bugs.openjdk.java.net/browse/JDK-8268225

I'm looking forward to comments.




Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses

2021-06-07 Thread Kevin Rushforth
On Wed, 21 Apr 2021 18:17:27 GMT, Michael Strauß  wrote:

> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as 
> well as the corresponding `:focus-visible` and `:focus-within` CSS 
> pseudo-classes.

I note that the discussion hasn't finished on the openjfx-dev mailing list as 
to whether to accept this feature. The proposed API and docs in this PR can be 
used to inform the discussion, but it's premature to start the code review.

If it ends up going forward, it will need a CSR and 2 reviewers.

-

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]

2021-06-07 Thread Kevin Rushforth
On Sun, 6 Jun 2021 16:19:46 GMT, Nir Lisker  wrote:

> > So it looks like `computeSpotlightFactor` is the winner. Does that match 
> > your observations? It might be worth some further tuning.
> 
> Yes, on Ubuntu. On Win, `computeSpotlightFactor3` was the best for me. I 
> detailed my findings in the topmost comment.

Oh, right. I didn't read that carefully.

In that case, using `computeSpotlightFactor` in the GLSL shaders and 
`computeSpotlightFactor3` in the HLSL shaders seems like the way to go. If we 
are settled on this, the two you aren't using should be commented out or 
`#ifdef`ed out.

> What further tuning are you thinking of?

I don't have anything specific in mind. I was just wondering if there might be 
additional optimizations that one could do the `computeSpotlightFactor` method. 
Even if so, it could probably be done in a follow-on bug. The more interesting 
question is whether the gap between what we have now and what is possible for 
simple lights justifies additional shaders. Maybe not for point lights, but we 
need to at least consider it when you add directional lights.

-

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v20]

2021-06-07 Thread Kevin Rushforth
On Sun, 6 Jun 2021 17:23:45 GMT, Nir Lisker  wrote:

>> modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 144:
>> 
>>> 142: /**
>>> 143:  * The angle of the spotlight's inner cone, in degrees (as shown 
>>> in the class doc image). A point whose angle to the
>>> 144:  * light's position is less than this angle is not attenuated by 
>>> the spotlight factor ({@code spot = 1}). At larger
>> 
>> The term `angle to the light position` isn't precise since you need a second 
>> vector to form an angle. In the class docs, you use the wording `point whose 
>> vector to the light position forms an angle {@code theta} with the direction 
>> of the light`, which I like. Maybe you could work that into this description 
>> somehow?
>
> It can become cumbersome. Something like `A point whose whose vector to the 
> light's position and the direction of the light form an angle that is less 
> than this angle is not attenuated by the spotlight factor`  or `A point whose 
> vector to the light's position forms an angle {@code theta} with the 
> direction of the light, and {@code theta} is smaller than this angle, is not 
> attenuated by the spotlight factor.` is a lot to digest.
> 
> I think that the meaning is intuitive. Maybe I can defined what an "angle of 
> a point to the light" is in the class doc and use that definition in the 
> `innerAngle` and `outerAngle` docs.

Yeah, I see your point. And yes, I think it would help to define that term in 
the class docs.

-

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


Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v20]

2021-06-05 Thread Kevin Rushforth
On Thu, 3 Jun 2021 15:54:08 GMT, Nir Lisker  wrote:

>> Added a SpotLight only to the D3D pipeline currently.
>> 
>> ### API discussion points
>> 
>> - [X]  Added `SpotLight` as a subclass of `LightBase`. However, it could 
>> also be a subclass of `PointLight` as it's a point light with direction and 
>> extra factors. I saw that `scenario.effect.light.SpotLight` extends its 
>> respective `PointLight`, but it's not a perfect analogy. In the end, I think 
>> it's a questions of whether `PointLight` will be expanded in a way which 
>> doesn't not suit `SpotLight`, and I tend to think that the answer is no.
>> 
>> - [X] The inner and outer angles are the "diameter angles" as shown 
>> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types).
>>   I, personally, find it more intuitive that these are the "radius angles", 
>> so half these angles, as used in the spotlight factor formula. Do you think 
>> I can change this or do you prefer the current definition of the angles?
>> 
>> - [x] The current implementation uses an ad-hoc direction property (using a 
>> `Point3D`). It crossed my mind that we could use the rotation transforms of 
>> the node to control the direction instead, just like we use the 
>> translation/layout of the node to get the position (there is an internal 
>> Affine3D transform for lights, not sure why `AmbientLight` needs it). 
>> Wouldn't that make more sense? When I rotate the light I would expect to see 
>> a change in direction.
>> 
>> ### Implementation discussion points
>> 
>> - [ ] I've gotten advice from a graphics engineer to treat point lights as 
>> spot lights with a 360 degrees coverage, which simplifies a few places. We 
>> can still try to optimize for a point light by looking at the light 
>> parameters: `falloff = 0` and `outerAngle = 180`. These possible 
>> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the 
>> pixel/fragment shaders in the form of 3 different ways to compute the 
>> spotlight factor (the `computeLightN` methods). We need to check which of 
>> these give the best results.
>> 
>> ## Performance
>> 
>> Testing 3 point lights and comparing this branch with `master` using a 1000 
>> division sphere, 200 meshes, and 5000 meshes.
>> Using an AMD RX 470 4GB GPU.
>> 
>> In this branch, there is a possible CPU optimization for checking the light 
>> type and using precalculated values (in `D3DMeshView.cc` for d3d and 
>> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing 
>> the spotlight factor contributions (`computeSpotlightFactor`, 
>> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out 
>> different branching and shortcuts.
>> 
>> ### Results
>> The CPU "optimizations" made no difference, which is understandable 
>> considering it will not be the bottleneck. We can remove these if we want to 
>> simplify, though maybe if we allow a large number of lights it could make a 
>> difference (I doubt it). I don't have a strong preference either way.
>> 
>> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu).
>> 
>> **Win 10**
>> Compared with the `master` branch, this patch shows 5-10 fps drop in the 
>> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several 
>> occasions and got different results in terms of absolute numbers, but the 
>> relative performance difference remained more or less the same. Out of the 3 
>> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no 
>> "optimizations", gives slightly better performance.
>> 
>> **Ubuntu 18**
>> The mesh 200 test always gave 60 fps because it is locked to this fps, so we 
>> can't measure the real GPU performance change.
>> The mesh 5000 test shows 2-6 fps drop from master, with 
>> `computeSpotlightFactor` > `computeSpotlightFactor2`  > 
>> `computeSpotlightFactor3` at in terms of performance (~2 fps difference 
>> each).
>> 
>> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. 
>> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave 
>> the best performances.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated docs per review

The updated docs look good (although I haven't looked at the javadoc-generated 
docs yet) with a couple minor comments.

modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 52:

> 50:  * 
> 51:  *  {@link #innerAngleProperty() inner angle}: the angle of the inner 
> cone (see image below)
> 52:  *  {@link #outerAngleProperty() outer angle}: the angle of the outer 
> cone (see image below)

I recommend changing these to `innerAngle` and `outerAngle` (one word, camel 
case) to match the names of the properties.

modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 144:

> 142: /**
> 143:  * The angle of 

Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]

2021-06-05 Thread Kevin Rushforth
On Sat, 22 May 2021 02:27:41 GMT, Nir Lisker  wrote:

> Did you test the different `computeSpotlightFactor` methods in the shader? I 
> got some 4 fps difference between the best and worst cases.

Here are the performance numbers on my Mac (using the discrete `AMD Radeon Pro 
5300M` graphics chipset) using the using the LightingSample test program with a 
mesh of 1000 quads rendered with 3 point lights and no attenuation:

| shader compute method | fps |
|   |  |
| computeSpotlightFactor | 10.75 |
| computeSpotlightFactor2 | 9.40 |
| computeSpotlightFactor3 | 7.70 |

So it looks like computeSpotlightFactor is the winner. Does that match your 
observations? It might be worth some further tuning.

-

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v14]

2021-06-05 Thread Kevin Rushforth
On Sat, 5 Jun 2021 15:42:22 GMT, Michael Strauß  wrote:

>> This PR adds support for loading images from [inline data 
>> URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely 
>> supported by web browsers. This enables developers to package small images 
>> in CSS files, rather than separately deploying the images alongside the CSS 
>> file.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed review comments

The docs updates look good, with a few minor comments.

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 68:

> 66:  * images from a specified URL.
> 67:  *
> 68:  * Supported image formats are:

Can you revert this formatting change to avoid unnecessary diffs? (I mean the 
one that moved the sentence up to be on the same line as the ``. The removal 
of the unneeded `` is fine)

The reason for this is to avoid extra diffs in public API. It will make it 
easier to see what needs to go into the CSR.

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 76:

> 74:  * 
> 75:  *
> 76:  * Images can be resized as they are loaded (for example to reduce the 
> amount of

Revert

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 619:

> 617:  * Constructs an {@code Image} with content loaded from the 
> specified URL.
> 618:  *
> 619:  * @param url a resource path, file path or URL

Please add a comma after `file path` (we prefer the Oxford comma in our docs).

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 632:

> 630:  * using the specified parameters.
> 631:  *
> 632:  * @param url a resource path, file path or URL

Comma after `file path`

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 647:

> 645:  * using the specified parameters.
> 646:  *
> 647:  * @param url a resource path, file path or URL

Comma after `file path`

-

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v13]

2021-06-05 Thread Kevin Rushforth
On Sat, 5 Jun 2021 15:44:38 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/javafx/css/converter/URLConverter.java 
>> line 98:
>> 
>>> 96: }
>>> 97: 
>>> 98: private URL resolve(String stylesheetUrl, String resource) {
>> 
>> Why was this change done? It seems unnecessary and possibly unwanted.
>
> `resource` is never `null` at this point, and it is already trimmed. Also, 
> contrary to the comment, I cannot see this method being used anywhere in 
> tests, so I made it private.

If that is guaranteed by all callers of this method (which is easier to check 
now that you made it private), this is fine.

-

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v13]

2021-06-05 Thread Kevin Rushforth
On Sat, 5 Jun 2021 15:49:16 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/StyleManager.java 
>> line 776:
>> 
>>> 774: } else {
>>> 775: logger.warning("Error loading 
>>> image: " + url);
>>> 776: }
>> 
>> So if I understand this correctly, an Image is first loaded via the standard 
>> mechanism, and if that fails for any reason (`image.isError()`) a second 
>> attempt is made through `DataURI` ?
>> 
>> Would it not be better to register a new protocol so `new Image` would just 
>> succeed for data uri's ?
>
> In this specific case, image loading has failed for some reason. The call to 
> `DataURI.tryParse` is only there to potentially call `DataURI.toString()` for 
> a truncated log output, instead of logging the entire `url` String. If 
> data-URI truncation is not wanted, this code can be reverted.

Maybe add a brief comment to that effect?

-

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v14]

2021-06-05 Thread Kevin Rushforth
On Sat, 5 Jun 2021 15:42:22 GMT, Michael Strauß  wrote:

>> This PR adds support for loading images from [inline data 
>> URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely 
>> supported by web browsers. This enables developers to package small images 
>> in CSS files, rather than separately deploying the images alongside the CSS 
>> file.
>
> Michael Strauß has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed review comments

Yes, it is a good question as to whether we should add Data URI support in the 
core of JavaFX or provide it via some other means. I agree that we wouldn't 
want to implement it in JavaFX with a pluggable protocol handler, so if we go 
that route we will end up pushing it off to the app.

The other alternative that was mentioned (as a comment in the JBS bug) was to 
push for direct support in the JDK. Since that was requested some years ago and 
rejected -- see [JDK-4188739](https://bugs.openjdk.java.net/browse/JDK-4188739) 
-- I doubt that is likely.

My gut feel is that adding Data URI support to JavaFX is a worthwhile feature, 
but it might warrant further discussion.

-

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v13]

2021-06-05 Thread Kevin Rushforth
On Sat, 5 Jun 2021 15:08:29 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 
>> 185:
>> 
>>> 183: 
>>> 184: return originalUri.substring(0, originalUri.length() - 
>>> originalData.length())
>>> 185: + originalData.substring(0, 14) + "..." + 
>>> originalData.substring(originalData.length() - 14);
>> 
>> Why truncate it?
>
> The idea was to not clutter logs with large pieces of data. Do you think that 
> the entire data string should be logged?

Your explanation is fine. And since this is an internal class, it doesn't 
matter all that much.

-

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


Re: RFR: 8267551: Support loading images from inline data-URIs [v13]

2021-06-05 Thread Kevin Rushforth
On Sat, 5 Jun 2021 15:17:09 GMT, Michael Strauß  wrote:

>> In that case, it might be clearer and simpler to just call `trim()` on the 
>> input String before doing anything with it, unless there is a reason not to.
>
> My idea was to avoid repeatedly allocating a new String with the data 
> contained in the URI. Since `matchScheme` is called at least twice (maybe 
> more), that would be a total of at least three copies of the URI data until 
> we parse it. Granted, this would only apply for URIs that contain leading or 
> trailing whitespace (since `trim()` is specified to return the same String 
> instance if it contains no leading or trailing whitespace).

I see. How about something like this then?


if (uri == null || uri.isEmpty()) {
return false;
}

if (Character.isWhiteSpace(uri.charAt(0)) {
uri = uri.trim();
}

if (uri.length() < 6) {
return false;
}


The rest of the method can then work on a trimmed string.

-

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


[jfx11u] Integrated: 8268152: gstmpegaudioparse does not provides timestamps for HLS MP3 streams

2021-06-05 Thread Kevin Rushforth
On Sat, 5 Jun 2021 14:00:38 GMT, Kevin Rushforth  wrote:

> This is a clean backport of a GStreamer audio fix for a regression caused by 
> the recent GStreamer 1.18.3 update in 
> [JDK-8262365](https://bugs.openjdk.java.net/browse/JDK-8262365). I tested it 
> on all three platforms.

This pull request has now been integrated.

Changeset: 032368a7
Author:Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx11u/commit/032368a7bd9fd53b4448b1c26e78c95eca3dae49
Stats: 10 lines in 2 files changed: 10 ins; 0 del; 0 mod

8268152: gstmpegaudioparse does not provides timestamps for HLS MP3 streams

Backport-of: ee032387badadb41ed36de745aea3c0a074b79bd

-

PR: https://git.openjdk.java.net/jfx11u/pull/22


Re: RFR: 8267551: Support loading images from inline data-URIs [v13]

2021-06-05 Thread Kevin Rushforth
On Sat, 5 Jun 2021 14:56:15 GMT, Michael Strauß  wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 
>> 47:
>> 
>>> 45: }
>>> 46: 
>>> 47: int firstNonWhitespace = 0, length = uri.length();
>> 
>> Why do you need to trim leading spaces? The input URL strings should already 
>> be trimmed by the caller.
>
> I implemented it in this way so that this requirement is not imposed onto the 
> caller, similar to `java.net.URL` does not impose this requirement onto its 
> callers. I can imagine that `DataURI` might be used in other places, so it 
> might make sense to make it more robust with regards to leading whitespace?

In that case, it might be clearer and simpler to just call `trim()` on the 
input String before doing anything with it, unless there is a reason not to.

-

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


[jfx11u] RFR: 8268152: gstmpegaudioparse does not provides timestamps for HLS MP3 streams

2021-06-05 Thread Kevin Rushforth
This is a clean backport of a GStreamer audio fix for a regression caused by 
the recent GStreamer 1.18.3 update in 
[JDK-8262365](https://bugs.openjdk.java.net/browse/JDK-8262365). I tested it on 
all three platforms.

-

Commit messages:
 - 8268152: gstmpegaudioparse does not provides timestamps for HLS MP3 streams

Changes: https://git.openjdk.java.net/jfx11u/pull/22/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx11u=22=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8268152
  Stats: 10 lines in 2 files changed: 10 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx11u/pull/22.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx11u pull/22/head:pull/22

PR: https://git.openjdk.java.net/jfx11u/pull/22


Re: [External] : Re: platformWidth

2021-06-05 Thread Kevin Rushforth
Oh, I see. Yes, it looks like you are right. Since Windows is the only 
platform using platformWidth (and platformHeight), it might be possible 
to refactor it, but only if we can guarantee that (width * 
platformScaleX) == platformWidth, which would allow the Windows glass 
code to compute it for each screen (since there is no per-platform 
subclass of Screen). Alternatively, we could add a comment to the effect 
that those variables are unused for platforms other than Windows.


-- Kevin


On 6/4/2021 12:43 AM, Johan Vos wrote:

Hi John, Kevin,

Thanks for the replies.
The relation between actual resolution of the Screen and the JavaFX 
rendering is kept in the renderScale and outputScale, and does not 
come from platformWidth. The value of platformWidth is never used 
apart from the 2 methods I pointed out above.


The first reason I started this is because it is already complex with 
a viewScale, platformScale, renderScale and outputScale, and I don't 
see where the platformWidth  fits in this picture -- especially since 
the latter is not used at all on linux/mac.


The second reason is that I am looking into a more generic way to deal 
with output dimensions that don't match the dimensions of the JavaFX 
main texture. We currently support linear scaling with those scale 
factors, but for some local work, I added rotation as well (e.g. to 
allow rendering on a portrait screen when the source is in landscape 
coordinates). This works surprisingly well by only manipulating the 
transformation in ES2SwapChain.prepare (adding the rotation there). I 
am doing that now conditionally, based on (yet even more) system 
properties, but it might make sense to add an "orientation" parameter 
to the Screen. But before adding something, I first try to see if 
there is something unused that can be removed -- hence my question 
about platformWidth/Height.


- Johan

On Fri, Jun 4, 2021 at 12:04 AM Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>> wrote:


The platformWidth / platformHeight can also be different from the
width
/ height on Linux with Hi-DPI scaling, at least in theory, since the
Screen constructor is called using different parameters for the
platform
values. Mac is different because of how the retina scaling is
handled on
macOS -- even on a retina display, platformWidth/platformHeight ==
width/height and platformScale{X,Y} == 1.

-- Kevin


On 6/3/2021 2:38 PM, John Hendrikx wrote:
> Perhaps an example will help from my machine.
>
> I'm running on a 4k screen at 150% scaling. These are the values:
>
> outputScaleX = 1.5
> outputScaleY = 1.5
> platformScaleX = 1.5
> platformScaleY = 1.5
> width = 2560
> height = 1440
> visibleWidth = 2560
> visibleHeight = 1400
> platformWidth = 3840
> platformHeight = 2160
>
> So it seems that platform values report the actual resolution of
the
> Screen / Monitor. Probably they're what is being reported by
Windows
> and stored there by JavaFX to derive the actual width/height it
should
> use while taking scaling into account.
>
> You didn't ask about the difference between visibleHeight and
height.
> But just incase, the visibleHeight is 40 pixels smaller than
height,
> as this space is taken up by the task bar.
>
> --John
>
>
> On 01/06/2021 13:27, Johan Vos wrote:
>> Hi,
>>
>> com.sun.glass.ui.Screen has 3 width (and height) parameters,
each with a
>> getter:
>> * width
>> * visibleWidth
>> * platformWidth
>>
>> The latter seems to be used only in the windows build,
>> via screen.containsPlatformRect and
screen.portionIntersectsPlatformRect
>>
>> I don't really understand what the goal of this
platformWidth/Height
>> is. It
>> seems a bit artificial if this is not used on the other
platforms, but I
>> might be missing the bigger purpose of this?
>>
>> - Johan
>>





Re: [External] : Re: Surface the ImageLoader API

2021-06-04 Thread Kevin Rushforth

Two thoughts come to mind.

1. While I agree that it isn't necessary to do everything all at once, I 
don't agree that a full featured Image I/O API is off the table as a 
future enhancement. Whatever you end up proposing to do here should 
ideally fit into what we would want to do in the future in this space. 
At the very least we need to have a discussion about how the subset you 
propose would or would not fit in. I'm not suggesting that we need to 
plan for every possible unknown (and in some cases unknowable) future 
need, but there are some obvious things we might want to do in this 
space. Writing images and dealing with image meta-data (both of which 
you listed) are two of the main ones. We need to make sure that we don't 
preclude any likely future direction. Otherwise we could be left with an 
API that would need to be deprecated and replaced in order to fit into a 
future Image I/O-like API.


2. Whether or not the ImageLoader implementation classes might be good 
API isn't really the point. I rather doubt that defining a lookalike API 
in the public javafx.scene.image package that merely wraps those classes 
is what we want. Maybe you'll end up with an API that is somewhat close 
to that, but starting with an implementation to create the proposed API 
is backwards. One of the big things to consider is how image readers are 
registered: SPI? explicit registration API? something else?


As for next steps, the first sentence of your latest email is a good 
start at a brief summary of what you are looking for:


> ...a feature to support third-party image format plugins, with the 
intention of decoding images for use in the JavaFX scene graph.


I'd recommend fleshing this out a bit, possibly with some mocked up API 
(high-level). If you haven't already looked at it, the JEP template [1] 
is good for describing non-trivial features such as what you propose. 
While we don't generally do JEPs for JavaFX APIs any more, the template 
can be a useful way to organize the information needed to evaluate the 
proposal. It gets you think about thinks like motivation, goals / 
non-goals, API, alternatives, testing, etc.


If there is general buy-in that a pluggable image loader is worth doing, 
the main things to hash out before this can proceed are the two I 
mentioned above: 1) whether and how it would fit into a larger Image 
reader/writer API; and 2) what form the API should take.


-- Kevin

[1] https://openjdk.java.net/jeps/2


On 6/4/2021 3:10 PM, Michael Strauß wrote:

Let me restate my intentions to be more clear:

What I'm asking for is a feature to support third-party image format
plugins, with the intention of decoding images for use in the JavaFX
scene graph.

I think the following things should remain explicit non-goals:
- writing images
- transcoding images
- reading or writing metadata

There's Java2D for that, no need to duplicate existing functionality.

For its intended purpose, I do think that the existing internal
implementation is actually a good API, because it is geared towards
the problem that it is trying to solve.
Expanding the scope of the existing feature to include any of these
additional things doesn't seem like a sensible idea to me.

I would like to understand whether you disagree with my assessment of
the intended purpose of this feature.
Do you think that there is a "larger picture" that would change the
goals and non-goals?

You seem to imply that extending the existing feature is the wrong
approach, and that there is a better approach which requires more
effort.
Can you share any of insights that lead to this conclusion? I haven't
been able to find this information in JBS.


Am Fr., 4. Juni 2021 um 22:03 Uhr schrieb Kevin Rushforth
:

Characterizing a new feature in terms of exposing internal interfaces
isn't really the right way to think about it. The fact that there might
be internal classes that do something similar to what you propose isn't
really relevant (other than later down the road when it comes time to
discuss the implementation).

Rather you should start with a description of the enhancement you would
like to see: additional image loading capability, in this case. Then a
discussion of the API would be in order.

What's really being asked for here is a feature like Java2D's Image I/O
with pluggable image formats. We spent some time looking at that a few
years ago. Doing this right would be a very large effort. I wouldn't be
in favor of a piecemeal approach without understanding the larger picture.

So I don't support this proposal as stated.

-- Kevin




Re: RFR: 8267551: Support loading images from inline data-URIs [v13]

2021-06-04 Thread Kevin Rushforth
On Wed, 26 May 2021 16:36:24 GMT, Michael Strauß  wrote:

>> This PR adds support for loading images from [inline data 
>> URIs](https://en.wikipedia.org/wiki/Data_URI_scheme), which is also widely 
>> supported by web browsers. This enables developers to package small images 
>> in CSS files, rather than separately deploying the images alongside the CSS 
>> file.
>
> Michael Strauß has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 15 commits:
> 
>  - Merge branch 'master' into feature/image-datauri
>
># Conflicts:
>#  
> modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
>  - Accept regular file paths, change javadoc
>  - Merge branch 'openjdk:master' into feature/image-datauri
>  - Rename DataURI.isValidURI
>  - Reverted a change
>  - Allow leading and trailing whitespace in data URI
>  - Removed test
>  - Changed data URI detection
>  - Merge branch 'master' into feature/image-datauri
>  - Moved test
>  - ... and 5 more: 
> https://git.openjdk.java.net/jfx/compare/7b7050c4...315523c5

I haven't tested this yet, but have done a first pass on the spec changes and 
most of the implementation (I'll finish next week).

Overall, the docs look good. Can you also update the list of supported image 
formats in the `Image` class docs just before line 76?

modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line 
290:

> 288: /**
> 289:  * Load all images present in the specified input. For more details 
> refer to
> 290:  * {@link #loadAll(InputStream, ImageLoadListener, double, double, 
> boolean, float, boolean)}.

`ImageLoadListener` isn't imported, so still needs to be qualified (although we 
don't actually generate any docs since this is an implementation class).

modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line 
333:

> 331: loader = getLoaderBySignature(theStream, listener);
> 332: }
> 333: } catch (Exception e) {

Is this change needed?

modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 47:

> 45: }
> 46: 
> 47: int firstNonWhitespace = 0, length = uri.length();

Why do you need to trim leading spaces? The input URL strings should already be 
trimmed by the caller.

modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 76:

> 74: 
> 75: if (Character.isWhitespace(uri.charAt(0))) {
> 76: uri = uri.trim();

Same question as above.

modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 175:

> 173: 
> 174: public byte[] getData() {
> 175: return data;

This returns a reference to the byte array rather than a copy. Is this what is 
wanted? If so, it would be good to add a comment.

modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 185:

> 183: 
> 184: return originalUri.substring(0, originalUri.length() - 
> originalData.length())
> 185: + originalData.substring(0, 14) + "..." + 
> originalData.substring(originalData.length() - 14);

Why truncate it?

modules/javafx.graphics/src/main/java/com/sun/javafx/util/DataURI.java line 191:

> 189: public boolean equals(Object o) {
> 190: if (this == o) return true;
> 191: if (o == null || getClass() != o.getClass()) return false;

Can this be replaced by `o instanceof DataURI`?

modules/javafx.graphics/src/main/java/javafx/css/converter/URLConverter.java 
line 98:

> 96: }
> 97: 
> 98: private URL resolve(String stylesheetUrl, String resource) {

Why was this change done? It seems unnecessary and possibly unwanted.

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 612:

> 610: /**
> 611:  * Constructs an {@code Image} with content loaded from the specified
> 612:  * image file. The {@code url} parameter can be one of the following:

I would not use the words "image file" in the first sentence. I recommend to 
restore this to "url".

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 624:

> 622:  * 
> 623:  * If a URL uses the "data" scheme, the data must be base64-encoded
> 624:  * and the MIME type must either be empty or a subtype of the

These two paragraphs might be better as part of the class docs (which is where 
we list the supported image formats).

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 638:

> 636: /**
> 637:  * Constructs an {@code Image} with content loaded from the specified
> 638:  * image file. The {@code url} parameter can be one of the following:

Similar comment. Maybe something like this for the first sentence?


Constructs an {@code Image} with content loaded from the specified url using 
the specified parameters.

-

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


Re: RFR: 8267505: {List, Set, Map}PropertyBase::bind should check against identity

2021-06-04 Thread Kevin Rushforth
On Mon, 24 May 2021 11:56:35 GMT, Jose Pereda  wrote:

> ListPropertyBase::bind, SetPropertyBase::bind, MapPropertyBase::bind have a 
> check on whether a different instance of the observable is the same, and this 
> PR changes that to check against identity.
> 
> Tests are included.

Marked as reviewed by kcr (Lead).

-

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


Re: RFR: 8244075: Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is removed from Scene

2021-06-04 Thread Kevin Rushforth
On Mon, 24 May 2021 11:07:12 GMT, Ambarish Rapte  wrote:

> Issue:
> There are several issues related to Accelerator of MenuItem of a ConextMenu 
> set on Control.
> 1. Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is 
> removed from Scene
> 2. Accelerator is not updated correctly when the Control is removed from a 
> Scene and Added to a different Scene
> 3. Accelerator is not removed from Scene when the anchor node is removed from 
> Scene and then it's ContextMenu is set to null
> 
> Fix:
> The accelerator should be added or removed correctly according to the Scene 
> property of the anchor node. 
> The issue#3 in above list is only fixed for Button and fails for other 
> Controls. A test is added for this scenario and I shall report a new issue to 
> address it.
> Added tests that fails before and pass after the fix.

Looks good. I left one comment on the test. When you update it to include the 
bug ID of the follow-on bug, I'll re-review it.

modules/javafx.controls/src/test/java/test/javafx/scene/control/AcceleratorParameterizedTest.java
 line 352:

> 350: }
> 351: 
> 352: @Ignore("Passes only for Button")

Can you file the follow-on bug and then update this comment to use that bug ID?

-

Marked as reviewed by kcr (Lead).

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


Re: RFR: 8263095: Provide a way for a custom control to indicate that its userAgentStyleSheet has changed

2021-06-04 Thread Kevin Rushforth
On Mon, 31 May 2021 13:57:22 GMT, Alessadro Parisi 
 wrote:

> This change allows custom control to change their style dynamically
> When the user-agent stylesheet changes the property automatically calls 
> `NodeHelper.reapplyCSS(Region.this);` to recompute the CSS for the node and 
> its children.
> To make this work the StyleManager class must be fixed too.
> The line `regionUserAgentStylesheet = 
> weakRegionUserAgentStylesheetMap.computeIfAbsent((Region)region, 
> Region::getUserAgentStylesheet);
> ` is problematic because if the region is already present in the map but the 
> user-agent stylesheet changed, the value of `regionUserAgentStylesheet` is 
> not updated and this leads to controls having the wrong style.
> To fix this issue I added this check:
> 
> if (((Region) region).getUserAgentStylesheet() != null && !((Region) 
> region).getUserAgentStylesheet().equals(regionUserAgentStylesheet)) {
> weakRegionUserAgentStylesheetMap.put((Region) region, ((Region) 
> region).getUserAgentStylesheet());
> regionUserAgentStylesheet = ((Region) region).getUserAgentStylesheet();
> }

Moving this to `Draft` to make it clear that it isn't yet ready for review.

-

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


Re: Surface the ImageLoader API

2021-06-04 Thread Kevin Rushforth
Characterizing a new feature in terms of exposing internal interfaces 
isn't really the right way to think about it. The fact that there might 
be internal classes that do something similar to what you propose isn't 
really relevant (other than later down the road when it comes time to 
discuss the implementation).


Rather you should start with a description of the enhancement you would 
like to see: additional image loading capability, in this case. Then a 
discussion of the API would be in order.


What's really being asked for here is a feature like Java2D's Image I/O 
with pluggable image formats. We spent some time looking at that a few 
years ago. Doing this right would be a very large effort. I wouldn't be 
in favor of a piecemeal approach without understanding the larger picture.


So I don't support this proposal as stated.

-- Kevin


On 6/4/2021 9:39 AM, Michael Strauß wrote:

I've been trying to add SVG support to a JavaFX application (which is
the image format I'm encountering most often in app projects), but
that's quite hard because the ImageLoader API is pretty much closed
for extension.

I'm proposing to open up the ImageLoader API to enable third-party
developers to create image loaders for emerging image formats like SVG
or WebP.

There will need to be some minor API and behavioral changes:


1. `ImageLoader.load` requires a new parameter "pixelScale":
current: ImageFrame load(int, int width, int height, boolean, boolean)
new: ImageFrame load(int, int width, int height, double pixelScale,
boolean, boolean)

This parameter is important for vector image loaders like SVG. If an
image loader is invoked with `width==0` and `height==0`, the
implementation should return an image with its natural resolution.
Vector image loaders need to know the requested pixel scale in order
to render the image with the correct pixel density.


2. `ImageFormatDescription.Signature` must be able to accept `null`
values in the byte array:
current: public Signature(byte...) {...}
new: public Signature(Byte...) {...}

A `null` value will match any byte at the corresponding position in
the image file header. This is necessary to match file headers that
contain variable data, like the WebP image header:
byte 0-3: 'R', 'I', 'F', 'F'
byte 4-7: 
byte 8-11: 'W', 'E', 'B', 'P'

Note that in this example, the corresponding signature description would be:
{ 'R', 'I', 'F', 'F', null, null, null, null, 'W', 'E', 'B', 'P' }


3. Enable file extension matching (in addition to signature matching)
with the following semantics:
Any valid `ImageFormatDescription` MUST contain at least one file
extension, and MAY contain any number of signatures and MIME types (or
none at all). When `ImageStorage` chooses an image loader for a file,
it first tries to match the file by signature, and if it doesn't
match, it tries to match by file extension (or, if the source is a
"data" URI, it tries to match by MIME type).

The reason for this is that some image formats like SVG cannot be
meaningfully matched by signature; in this case, the SVG image format
description would not contain a signature description and would always
fall back to matching by file extension or MIME type.


I'd welcome any opinions on this proposal.




Re: RFR: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces [v2]

2021-06-04 Thread Kevin Rushforth
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

Looks fine. I tested it on Windows with / without spaces in `ANT_HOME` and it 
does what I would expect. I sanity tested it on Linux.

> On Ubuntu 20.04 ... The :apps task fails before and after the fix

Maybe something with the version of Java you are using? It runs fine my system, 
and the GitHub actions build is fine.

-

Marked as reviewed by kcr (Lead).

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


Re: RFR: 8267858: Document that title property in WebEngine gets updated asynchronously

2021-06-04 Thread Kevin Rushforth
On Thu, 27 May 2021 17:25:27 GMT, Arun Joseph  wrote:

> The title property is not guaranteed to be updated right after the page is 
> loaded, but gets updated asynchronously.

Looks good.

-

Marked as reviewed by kcr (Lead).

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


Re: RFR: 8264139: Suppress removal warnings for Security Manager methods [v2]

2021-06-04 Thread Kevin Rushforth
> This PR adds the necessary `@SuppressWarnings("removal")` annotations for the 
> recently-integrated security manager deprecation, [JEP 
> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
> 
> There are four commits:
> 
> 1. 678b026 : A patch generated by @wangweij to add annotations to the runtime 
> (`modules/*/src/main/java`) using the same automated tooling that he used as 
> part of the implementation of JEP 411.
> 2. 9698e87 : Same as above for the tests.
> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a 
> static import) was causing a removal warning.
> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added 
> to an entire class, or to a large method where only a small part of the 
> method uses a deprecated method. This was done using similar techniques to 
> the following fixes that Weijun did in openjdk/jdk#4172.
>  
> The first two commits represent the bulk of the changes. Other than scanning 
> to ensure that there are no obvious errors, and testing, they probably don't 
> need the same level of scrutiny as the manual changes do.
> 
> I tested this on all three platforms by doing a build / test with `JDK_HOME` 
> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the 
> build with `gradle -PLINT=removal` and verified that there were removal 
> warnings for the security manager APIs without this fix and none with this 
> fix.
> 
> NOTE: The following files under `modules/javafx.web/src/android` and 
> `modules/javafx.web/src/ios` were not processed by the automated tool. As I 
> have no way to compile them, I chose not to manually fix them either, but 
> doing so would be trivial as a follow-up fix if desired.
> 
> 
> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java

Kevin Rushforth has updated the pull request incrementally with one additional 
commit since the last revision:

  Address review feedback

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/528/files
  - new: https://git.openjdk.java.net/jfx/pull/528/files/4f87d187..84775ff5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=528=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=528=00-01

  Stats: 42 lines in 6 files changed: 6 ins; 25 del; 11 mod
  Patch: https://git.openjdk.java.net/jfx/pull/528.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/528/head:pull/528

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


Re: RFR: 8264139: Suppress removal warnings for Security Manager methods

2021-06-04 Thread Kevin Rushforth
On Fri, 4 Jun 2021 14:37:05 GMT, Weijun Wang  wrote:

>> This PR adds the necessary `@SuppressWarnings("removal")` annotations for 
>> the recently-integrated security manager deprecation, [JEP 
>> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
>> 
>> There are four commits:
>> 
>> 1. 678b026 : A patch generated by @wangweij to add annotations to the 
>> runtime (`modules/*/src/main/java`) using the same automated tooling that he 
>> used as part of the implementation of JEP 411.
>> 2. 9698e87 : Same as above for the tests.
>> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a 
>> static import) was causing a removal warning.
>> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added 
>> to an entire class, or to a large method where only a small part of the 
>> method uses a deprecated method. This was done using similar techniques to 
>> the following fixes that Weijun did in openjdk/jdk#4172.
>>  
>> The first two commits represent the bulk of the changes. Other than scanning 
>> to ensure that there are no obvious errors, and testing, they probably don't 
>> need the same level of scrutiny as the manual changes do.
>> 
>> I tested this on all three platforms by doing a build / test with `JDK_HOME` 
>> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the 
>> build with `gradle -PLINT=removal` and verified that there were removal 
>> warnings for the security manager APIs without this fix and none with this 
>> fix.
>> 
>> NOTE: The following files under `modules/javafx.web/src/android` and 
>> `modules/javafx.web/src/ios` were not processed by the automated tool. As I 
>> have no way to compile them, I chose not to manually fix them either, but 
>> doing so would be trivial as a follow-up fix if desired.
>> 
>> 
>> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
>> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
>> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
>
> modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/ModuleHelper.java line 
> 41:
> 
>> 39: 
>> 40: static {
>> 41: verbose = 
>> AccessController.doPrivileged((PrivilegedAction) () ->
> 
> You can merge this assignment with the declaration on line 38. Or you can 
> keep this so the check of verbose is in the same block with its assignment.

I think I'll keep this one as is to minimize changes.

> modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java line 43:
> 
>> 41: 
>> 42: static {
>> 43: @SuppressWarnings("removal")
> 
> Combine assignment and declaration?

Good idea. This allows the static block to be removed.

> modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/LinuxArch.java 
> line 36:
> 
>> 34: 
>> 35: static {
>> 36: bits = AccessController.doPrivileged((PrivilegedAction) 
>> () -> {
> 
> Combine?

Good idea. This allows the static block to be removed.

> modules/javafx.web/src/main/java/com/sun/webkit/network/HTTP2Loader.java line 
> 415:
> 
>> 413: // Run the HttpClient in the page's access control context
>> 414: @SuppressWarnings("removal")
>> 415: CompletableFuture tmpResponse = 
>> AccessController.doPrivileged((PrivilegedAction>) () 
>> -> {
> 
> Is "var" enough?

Yes, I'll change it.

-

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


Re: RFR: 8264139: Suppress removal warnings for Security Manager methods

2021-06-04 Thread Kevin Rushforth
On Fri, 4 Jun 2021 14:31:49 GMT, Weijun Wang  wrote:

>> This PR adds the necessary `@SuppressWarnings("removal")` annotations for 
>> the recently-integrated security manager deprecation, [JEP 
>> 411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.
>> 
>> There are four commits:
>> 
>> 1. 678b026 : A patch generated by @wangweij to add annotations to the 
>> runtime (`modules/*/src/main/java`) using the same automated tooling that he 
>> used as part of the implementation of JEP 411.
>> 2. 9698e87 : Same as above for the tests.
>> 3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a 
>> static import) was causing a removal warning.
>> 4. 4f87d18 : Manually reduced the scope of the annotation where it was added 
>> to an entire class, or to a large method where only a small part of the 
>> method uses a deprecated method. This was done using similar techniques to 
>> the following fixes that Weijun did in openjdk/jdk#4172.
>>  
>> The first two commits represent the bulk of the changes. Other than scanning 
>> to ensure that there are no obvious errors, and testing, they probably don't 
>> need the same level of scrutiny as the manual changes do.
>> 
>> I tested this on all three platforms by doing a build / test with `JDK_HOME` 
>> set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the 
>> build with `gradle -PLINT=removal` and verified that there were removal 
>> warnings for the security manager APIs without this fix and none with this 
>> fix.
>> 
>> NOTE: The following files under `modules/javafx.web/src/android` and 
>> `modules/javafx.web/src/ios` were not processed by the automated tool. As I 
>> have no way to compile them, I chose not to manually fix them either, but 
>> doing so would be trivial as a follow-up fix if desired.
>> 
>> 
>> modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
>> modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
>> modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
>> modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9969:
> 
>> 9967: accessible = 
>> Application.GetApplication().createAccessible();
>> 9968: accessible.setEventHandler(new Accessible.EventHandler() {
>> 9969: @SuppressWarnings("removal")
> 
> Was this "deprecation" already useless before this change?

Yes. The code to which it applied was removed back in JDK 9, but we forgot to 
remove the annotation.

-

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


Re: RFR: 8268120: Allow hardware cursor to be used on Monocle-EGL platforms [v3]

2021-06-04 Thread Kevin Rushforth
On Fri, 4 Jun 2021 14:17:19 GMT, Johan Vos  wrote:

>> Add EGL cursor implementation (Java + native) and the link to low-level 
>> drivers.
>> Fix for JDK-8268120
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   address reviewer comments

Marked as reviewed by kcr (Lead).

-

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


Re: RFR: 8268120: Allow hardware cursor to be used on Monocle-EGL platforms [v2]

2021-06-04 Thread Kevin Rushforth
On Thu, 3 Jun 2021 11:41:02 GMT, Johan Vos  wrote:

>> Add EGL cursor implementation (Java + native) and the link to low-level 
>> drivers.
>> Fix for JDK-8268120
>
> Johan Vos has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix typo after last-minute inversion (make hwcursor default instead of 
> swcursor)

I don't have any comments aside from those Jose pointed out.

-

Marked as reviewed by kcr (Lead).

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


Re: RFR: 8268120: Allow hardware cursor to be used on Monocle-EGL platforms [v2]

2021-06-04 Thread Kevin Rushforth
On Fri, 4 Jun 2021 12:26:01 GMT, Alexander Scherbatiy  
wrote:

>> Johan Vos has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix typo after last-minute inversion (make hwcursor default instead of 
>> swcursor)
>
> modules/javafx.graphics/src/main/native-glass/monocle/egl/egl_ext.h line 39:
> 
>> 37: 
>> 38: // initialize the EGL system with the specified handle
>> 39: extern jboolean doEglInitialize(void* handle);
> 
> Is it possible to declare arguments as `jlong eglDisplay` here instead of 
> `void* handle` as it is done in other methods?
> As I see nEglInitialize() passes eglDisplay on the java side:
> https://github.com/openjdk/jfx/blob/47700d8ef0175d4b457bb658371d2da4ec0a8181/modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/EGLAcceleratedScreen.java#L48
> but it needs to convert it to a ptr in the native code:
> https://github.com/openjdk/jfx/blob/47700d8ef0175d4b457bb658371d2da4ec0a8181/modules/javafx.graphics/src/main/native-glass/monocle/egl/eglBridge.c#L46

What would be the advantage of changing it? In any case, it seems unrelated to 
this fix.

-

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


Re: RFR: 8267858: Document that title property in WebEngine gets updated asynchronously

2021-06-04 Thread Kevin Rushforth
On Fri, 28 May 2021 06:52:30 GMT, Johan Vos  wrote:

>> The title property is not guaranteed to be updated right after the page is 
>> loaded, but gets updated asynchronously.
>
> modules/javafx.web/src/main/java/javafx/scene/web/WebEngine.java line 441:
> 
>> 439: 
>> 440: /*
>> 441:  * The page title. This gets updated asynchronously.
> 
> Is there any more information or guarantee when the update is expected to be 
> complete, or is it truly asynchronous? I we know that this will happen 
> between the current pulse and the next one, that might be helpful to 
> developers (although I agree waiting for the titleProperty to be changed is 
> the better approach).
> If there is no link beween the webkit thread that gets the title and the 
> quantum renderer, it is really asynchronous and in that case tricks like 
> waiting for a few pulses won't help either -- but it would be good to mention 
> that so that developers don't create workarounds that might work in some 
> cases and fail in others.

I had exactly the same thought about the need to document it more clearly. 
Maybe something like:

"This property will be updated asynchronously some time after the page is 
loaded. Applications should not rely on any particular timing, but should 
listen for changes to this property, or bind to it, to know when it has been 
updated."

Btw, you modified an ordinary comment, not a javadoc comment. The newly added 
sentence needs to go on the javadoc comment, which is on the `titleProperty` 
method a few lines down from here. Be sure to run `gradle javadoc` and look at 
the generated docs.

-

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


RFR: 8264139: Suppress removal warnings for Security Manager methods

2021-06-03 Thread Kevin Rushforth
This PR adds the necessary `@SuppressWarnings("removal")` annotations for the 
recently-integrated security manager deprecation, [JEP 
411](https://openjdk.java.net/jeps/411). See openjdk/jdk#4073.

There are four commits:

1. 678b026 : A patch generated by @wangweij to add annotations to the runtime 
(`modules/*/src/main/java`) using the same automated tooling that he used as 
part of the implementation of JEP 411.
2. 9698e87 : Same as above for the tests.
3. 1c42cf8 : Manually removes a pair of unused imports, one of which (a static 
import) was causing a removal warning.
4. 4f87d18 : Manually reduced the scope of the annotation where it was added to 
an entire class, or to a large method where only a small part of the method 
uses a deprecated method. This was done using similar techniques to the 
following fixes that Weijun did in openjdk/jdk#4172.
 
The first two commits represent the bulk of the changes. Other than scanning to 
ensure that there are no obvious errors, and testing, they probably don't need 
the same level of scrutiny as the manual changes do.

I tested this on all three platforms by doing a build / test with `JDK_HOME` 
set to a local JDK 17 ea build that includes the fix for JEP 411. I ran the 
build with `gradle -PLINT=removal` and verified that there were removal 
warnings for the security manager APIs without this fix and none with this fix.

NOTE: The following files under `modules/javafx.web/src/android` and 
`modules/javafx.web/src/ios` were not processed by the automated tool. As I 
have no way to compile them, I chose not to manually fix them either, but doing 
so would be trivial as a follow-up fix if desired.


modules/javafx.web/src/android/java/com/sun/webkit/Timer.java
modules/javafx.web/src/android/java/com/sun/webkit/WebPage.java
modules/javafx.web/src/android/java/javafx/scene/web/WebEngine.java
modules/javafx.web/src/ios/java/javafx/scene/web/ExportedJavaObject.java
modules/javafx.web/src/ios/java/javafx/scene/web/HTMLEditorSkin.java
modules/javafx.web/src/ios/java/javafx/scene/web/JS2JavaBridge.java
modules/javafx.web/src/ios/java/javafx/scene/web/WebEngine.java

-

Commit messages:
 - Narrow the scope of @SuppressWarnings("removal") annotation
 - Remove unused security manager imports
 - Suppress removal warnings for Security Manager methods in tests
 - 8264139: Suppress removal warnings for Security Manager methods

Changes: https://git.openjdk.java.net/jfx/pull/528/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=528=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264139
  Stats: 586 lines in 194 files changed: 464 ins; 3 del; 119 mod
  Patch: https://git.openjdk.java.net/jfx/pull/528.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/528/head:pull/528

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


Re: RFR: 8268152: gstmpegaudioparse does not provides timestamps for HLS MP3 streams

2021-06-03 Thread Kevin Rushforth
On Thu, 3 Jun 2021 09:37:18 GMT, Alexander Matveev  wrote:

> With recent GStreamer update gstmpegaudioparse no longer provides audio 
> buffers with valid timestamps. This issue is only reproducible with MP3 HTTP 
> Live Stream and was noticed on Linux. I think Windows audio decoder handles 
> such case better and thus MP3 HLS works fine. I did not figure out why it 
> behaves this way and for now fix is reverting gstmpegaudioparse changeset 
> which caused it. Follow up issue will be filed to investigate why javasource 
> and/or hlsprogressbuffer no longer compatible with gstmpegaudioparse and 
> gstmpegaudioparse cannot figure out correct PTS. Also, fixed potential null 
> pointer reference for buffers with invalid PTS.

Looks good. I did a build / test on all three platforms.

-

Marked as reviewed by kcr (Lead).

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


Re: platformWidth

2021-06-03 Thread Kevin Rushforth
The platformWidth / platformHeight can also be different from the width 
/ height on Linux with Hi-DPI scaling, at least in theory, since the 
Screen constructor is called using different parameters for the platform 
values. Mac is different because of how the retina scaling is handled on 
macOS -- even on a retina display, platformWidth/platformHeight == 
width/height and platformScale{X,Y} == 1.


-- Kevin


On 6/3/2021 2:38 PM, John Hendrikx wrote:

Perhaps an example will help from my machine.

I'm running on a 4k screen at 150% scaling. These are the values:

outputScaleX = 1.5
outputScaleY = 1.5
platformScaleX = 1.5
platformScaleY = 1.5
width = 2560
height = 1440
visibleWidth = 2560
visibleHeight = 1400
platformWidth = 3840
platformHeight = 2160

So it seems that platform values report the actual resolution of the 
Screen / Monitor. Probably they're what is being reported by Windows 
and stored there by JavaFX to derive the actual width/height it should 
use while taking scaling into account.


You didn't ask about the difference between visibleHeight and height. 
But just incase, the visibleHeight is 40 pixels smaller than height, 
as this space is taken up by the task bar.


--John


On 01/06/2021 13:27, Johan Vos wrote:

Hi,

com.sun.glass.ui.Screen has 3 width (and height) parameters, each with a
getter:
* width
* visibleWidth
* platformWidth

The latter seems to be used only in the windows build,
via screen.containsPlatformRect and screen.portionIntersectsPlatformRect

I don't really understand what the goal of this platformWidth/Height 
is. It

seems a bit artificial if this is not used on the other platforms, but I
might be missing the bigger purpose of this?

- Johan





Re: RFR: 8263095: Provide a way for a custom control to indicate that its userAgentStyleSheet has changed

2021-06-02 Thread Kevin Rushforth
On Wed, 2 Jun 2021 03:02:26 GMT, Nir Lisker  wrote:

> Don't worry about the CSR for now, someone will create it when the review is 
> near completion. However, since this is an enhancement, you should discuss it 
> first in the mailing list as explained in the README file of the repo.

Exactly. The discussion should happen on openjfx-dev@openjdk.java.net. 
Specifically, the place to start is with the newly proposed API and behavioral 
changes. This is what is important to get general agreement / buy-in on. Any 
newly proposed public methods need to be clearly documented and justified. 
Also, there needs to be a discussion as to whether this change in behavior 
should be done unconditionally, which the current PR proposes to do, or whether 
this behavior should be "opt in".

Since we haven't yet had the discussion, this PR is not yet ready to be 
reviewed. It can be used for illustrative purposes as long as the focus is on 
the public API and behavioral changes and not (yet anyway) on the code.

-

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


Re: Tons of gradle warnings

2021-06-02 Thread Kevin Rushforth
Yeah, both Ambarish and I noticed the odd formatting and the warnings as 
well. I'll file follow-on issues for the warnings.


-- Kevin


On 5/28/2021 3:45 AM, Jeanette Winzenburg wrote:


Hi Experts,

yesterday gradle auto-updated to version 7 (?), since then I get many 
warnings like:


Execution optimizations have been disabled for task 
':graphics:copyClassFilesWin' to ensure correctness due to the 
following reasons:
  - Gradle detected a problem with the following location: 
'C:\Daten\data-for-work\eclipse\gitrep-openjdk\jfx-fork\modules\javafx.graphics\build\module-classes'. 
Reason: Task ':graphics:copyClassFilesWin' uses this output of task 
':graphics:buildModuleWin' without declaring an explicit or implicit 
dependency. This can lead to incorrect results being produced, 
depending on what order the tasks are executed.


Looks like it is safe to ignore them, just want to be sure :)

Another very minor issue: since the same time, there's an issue with 
terminal output in that I see the "executing" messages written out like:


<===--> 58% EXECUTING [54s]> 
:graphics:copyClassFilesWin<===--> 58% EXECUTING 
[55s]<===--> 58% EXECUTING [56s]<===--> 58% EXECUTING 
[57s]<===--> 58% EXECUTING [58s]<===--> 58% EXECUTING 
[59s]<===--> 58% EXECUTING [1m 0s]<===--> 58% 
EXECUTING [1m 1s]<===--> and-so-on-to-the-end


Can live with it, but any idea how to get rid off them?

-- Jeanette





Re: RFR: 8239138: StyleManager should use a BufferedInputStream [v4]

2021-06-01 Thread Kevin Rushforth
On Mon, 31 May 2021 06:37:40 GMT, Ambarish Rapte  wrote:

>> `StyleManager.calculateCheckSum()` uses a raw InputStream as the input to a 
>> `DigestInputStream` and reads one byte at a time. This is slower in 
>> performance and should be changed, either to use `BufferedInputStream` or 
>> read byte buffer of 4096 from the stream or use both.
>> 
>> I have tried three approaches and tested with modena.css which is ~134 KB in 
>> size.
>> Following are the approaches and time in milliseconds taken by the method 
>> StyleManager.calculateCheckSum(), collected from 25 readings,
>> 
>> 1. Use BufferedInputStream and read one byte at a time 
>> [commit#1](https://github.com/openjdk/jfx/commit/6cd7d44d0ce1084c6cdb06a698c7aca127a615ef)
>>  : 
>> - Maximum: 53 ms,  Minimum: 27 ms, Average: 39 ms
>> 2. Use BufferedInputStream and read buffer of 4096 at a time 
>> [commit#1+2](https://github.com/openjdk/jfx/pull/518/files/6e0c621ea62691d5402b2bca5951d1012a5b1b91)
>> - Maximum: 17 ms,  Minimum: 14 ms, Average: 15.58 ms
>> 3. Use raw InputStream(current implementation) and read buffer of 4096 at a 
>> time 
>> [commit#1+2+3](https://github.com/openjdk/jfx/pull/518/files/215e1a183cfb902247f0d48685f7a901cb5fb003),
>>  which also similar to `NativeLibLoader.calculateCheckSum()` and looks 
>> faster than previous two.
>> - Maximum: 16 ms,  Minimum: 13 ms, Average: 14.25 ms
>> 
>> 
>> The time taken by `StyleManager.calculateCheckSum()` with current 
>> implementation is,
>> - Maximum: 61 ms,  Minimum: 38 ms, Average: 50.58 ms
>> 
>> 
>> Both approaches 2 & 3 show good improvement. I would prefer approach#3 as it 
>> is similar to `NativeLibLoader.calculateCheckSum()`.
>> However we can choose approach#2 also.
>> If we choose approach#3 then bug summary should be changed accordingly.
>
> Ambarish Rapte 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 seven additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into SM_BufferedInputStream_8239138
>  - review update
>  - minor: format correction
>  - add test
>  - only buffer[4096], similar to NativeLibLoader.calculateCheckSum()
>  - BufferedInputStream + buffer[4096]
>  - use BufferedInputStream

Marked as reviewed by kcr (Lead).

-

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


Result: New OpenJFX Reviewer: Jeanette Winzenburg

2021-05-28 Thread Kevin Rushforth

Voting for Jeanette Winzenburg [1] to OpenJFX Reviewer [2] is now closed.

Yes: 7
Veto: 0
Abstain: 0

According to the Bylaws definition of Three-Vote Consensus, this is 
sufficient to approve the nomination.


-- Kevin

[1] https://openjdk.java.net/census#fastegal
[2] https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-May/030299.html



Result: New OpenJFX Reviewer: Arun Joseph

2021-05-28 Thread Kevin Rushforth

Voting for Arun Joseph [1] to OpenJFX Reviewer [2] is now closed.

Yes: 6
Veto: 0
Abstain: 0

According to the Bylaws definition of Three-Vote Consensus, this is 
sufficient to approve the nomination.


-- Kevin

[1] https://openjdk.java.net/census#ajoseph
[2] https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-May/030298.html



Re: RFR: 8239138: StyleManager should use a BufferedInputStream [v3]

2021-05-28 Thread Kevin Rushforth
On Thu, 27 May 2021 17:22:35 GMT, Ambarish Rapte  wrote:

>> `StyleManager.calculateCheckSum()` uses a raw InputStream as the input to a 
>> `DigestInputStream` and reads one byte at a time. This is slower in 
>> performance and should be changed, either to use `BufferedInputStream` or 
>> read byte buffer of 4096 from the stream or use both.
>> 
>> I have tried three approaches and tested with modena.css which is ~134 KB in 
>> size.
>> Following are the approaches and time in milliseconds taken by the method 
>> StyleManager.calculateCheckSum(), collected from 25 readings,
>> 
>> 1. Use BufferedInputStream and read one byte at a time 
>> [commit#1](https://github.com/openjdk/jfx/commit/6cd7d44d0ce1084c6cdb06a698c7aca127a615ef)
>>  : 
>> - Maximum: 53 ms,  Minimum: 27 ms, Average: 39 ms
>> 2. Use BufferedInputStream and read buffer of 4096 at a time 
>> [commit#1+2](https://github.com/openjdk/jfx/pull/518/files/6e0c621ea62691d5402b2bca5951d1012a5b1b91)
>> - Maximum: 17 ms,  Minimum: 14 ms, Average: 15.58 ms
>> 3. Use raw InputStream(current implementation) and read buffer of 4096 at a 
>> time 
>> [commit#1+2+3](https://github.com/openjdk/jfx/pull/518/files/215e1a183cfb902247f0d48685f7a901cb5fb003),
>>  which also similar to `NativeLibLoader.calculateCheckSum()` and looks 
>> faster than previous two.
>> - Maximum: 16 ms,  Minimum: 13 ms, Average: 14.25 ms
>> 
>> 
>> The time taken by `StyleManager.calculateCheckSum()` with current 
>> implementation is,
>> - Maximum: 61 ms,  Minimum: 38 ms, Average: 50.58 ms
>> 
>> 
>> Both approaches 2 & 3 show good improvement. I would prefer approach#3 as it 
>> is similar to `NativeLibLoader.calculateCheckSum()`.
>> However we can choose approach#2 also.
>> If we choose approach#3 then bug summary should be changed accordingly.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   review update

Now that PR #523 is integrated, can you merge in the latest changes from master 
to ensure a clean test run?

-

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


Integrated: 8267892: Add .gitattributes to repo

2021-05-28 Thread Kevin Rushforth
On Thu, 27 May 2021 22:14:51 GMT, Kevin Rushforth  wrote:

> This fix adds a `.gitattributes` file to prevent git from replacing line 
> endings if the global `.gitconfig` file on the client system is configured 
> with core.autocrlf = true (which it is by default when using the native Git 
> for Windows).
> 
> This can cause build or test problems with scripts or other files that expect 
> Unix-style line endings. For two recent examples of such problems, see 
> [JDK-8267694](https://bugs.openjdk.java.net/browse/JDK-8267694) and [this 
> comment in PR 
> #518](https://github.com/openjdk/jfx/pull/518#issuecomment-849966053).
> 
> To test this, I set `core.autocrlf = true` in my `$HOME/.gitconfig` and 
> cloned the master branch of the jfx repo into a new local repo. The files all 
> had CRLF line endings. I then cloned the branch with the fix for this PR into 
> a new repo and verified that the line endings are correctly left alone (LF 
> line endings).
> 
> As a second test, here is a failing GHA build of the patch from PR #518 that 
> fails as expected due to this bug:
> 
> https://github.com/kevinrushforth/jfx/actions/runs/883603338
> 
> Here is a GHA build of that same branch, plus the commit to add 
> `.gitattributes`:
> 
> https://github.com/kevinrushforth/jfx/actions/runs/883620592

This pull request has now been integrated.

Changeset: 5e6d4429
Author:Kevin Rushforth 
URL:   
https://git.openjdk.java.net/jfx/commit/5e6d4429159e3fab9ec0aab9720393850d179710
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod

8267892: Add .gitattributes to repo

Reviewed-by: arapte, pbansal

-

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


Re: RFR: 8267819: CoInitialize/CoUninitialize should be called on same thread

2021-05-27 Thread Kevin Rushforth
On Thu, 27 May 2021 04:18:24 GMT, Alexander Matveev  
wrote:

> JDK-8264737 introduced new code for audio device removal/arrival 
> notifications which calls CoInitialize/CoUninitialize on separate threads. 
> CoInitialize/CoUninitialize should be called on same thread, since 
> initialization is per thread. Doing it on separate thread will result in 
> unloading COM libraries on that thread and if it uses COM libraries it might 
> not work correctly. Fixed by calling it on same thread in same way it is done 
> in dshowwrapper.

Looks good, and I think it's cleaner this way too. Two questions:

1. Have you verified that it still works correctly on RDP reconnect?
2. This isn't really related to your fix, since the logic for this part is the 
same before and after, should the rest of the `Init` method be short-circuited 
if `CoInitialize` fails?

-

Marked as reviewed by kcr (Lead).

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


RFR: 8267892: Add .gitattributes to repo

2021-05-27 Thread Kevin Rushforth
This fix adds a `.gitattributes` file to prevent git from replacing line 
endings if the global `.gitconfig` file on the client system is configured with 
core.autocrlf = true (which it is by default when using the native Git for 
Windows).

This can cause build or test problems with scripts or other files that expect 
Unix-style line endings. For two recent examples of such problems, see 
[JDK-8267694](https://bugs.openjdk.java.net/browse/JDK-8267694) and [this 
comment in PR 
#518](https://github.com/openjdk/jfx/pull/518#issuecomment-849966053).

To test this, I set `core.autocrlf = true` in my `$HOME/.gitconfig` and cloned 
the master branch of the jfx repo into a new local repo. The files all had CRLF 
line endings. I then cloned the branch with the fix for this PR into a new repo 
and verified that the line endings are correctly left alone (LF line endings).

As a second test, here is a failing GHA build of the patch from PR #518 that 
fails as expected due to this bug:

https://github.com/kevinrushforth/jfx/actions/runs/883603338

Here is a GHA build of that same branch, plus the commit to add 
`.gitattributes`:

https://github.com/kevinrushforth/jfx/actions/runs/883620592

-

Commit messages:
 - 8267892: Add .gitattributes to repo

Changes: https://git.openjdk.java.net/jfx/pull/523/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=523=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267892
  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/523.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/523/head:pull/523

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


Re: RFR: 8239138: StyleManager should use a BufferedInputStream [v3]

2021-05-27 Thread Kevin Rushforth
On Thu, 27 May 2021 17:22:35 GMT, Ambarish Rapte  wrote:

>> `StyleManager.calculateCheckSum()` uses a raw InputStream as the input to a 
>> `DigestInputStream` and reads one byte at a time. This is slower in 
>> performance and should be changed, either to use `BufferedInputStream` or 
>> read byte buffer of 4096 from the stream or use both.
>> 
>> I have tried three approaches and tested with modena.css which is ~134 KB in 
>> size.
>> Following are the approaches and time in milliseconds taken by the method 
>> StyleManager.calculateCheckSum(), collected from 25 readings,
>> 
>> 1. Use BufferedInputStream and read one byte at a time 
>> [commit#1](https://github.com/openjdk/jfx/commit/6cd7d44d0ce1084c6cdb06a698c7aca127a615ef)
>>  : 
>> - Maximum: 53 ms,  Minimum: 27 ms, Average: 39 ms
>> 2. Use BufferedInputStream and read buffer of 4096 at a time 
>> [commit#1+2](https://github.com/openjdk/jfx/pull/518/files/6e0c621ea62691d5402b2bca5951d1012a5b1b91)
>> - Maximum: 17 ms,  Minimum: 14 ms, Average: 15.58 ms
>> 3. Use raw InputStream(current implementation) and read buffer of 4096 at a 
>> time 
>> [commit#1+2+3](https://github.com/openjdk/jfx/pull/518/files/215e1a183cfb902247f0d48685f7a901cb5fb003),
>>  which also similar to `NativeLibLoader.calculateCheckSum()` and looks 
>> faster than previous two.
>> - Maximum: 16 ms,  Minimum: 13 ms, Average: 14.25 ms
>> 
>> 
>> The time taken by `StyleManager.calculateCheckSum()` with current 
>> implementation is,
>> - Maximum: 61 ms,  Minimum: 38 ms, Average: 50.58 ms
>> 
>> 
>> Both approaches 2 & 3 show good improvement. I would prefer approach#3 as it 
>> is similar to `NativeLibLoader.calculateCheckSum()`.
>> However we can choose approach#2 also.
>> If we choose approach#3 then bug summary should be changed accordingly.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   review update

I filed [JDK-8267892](https://bugs.openjdk.java.net/browse/JDK-8267892) to add 
a `.gitattributes` file. I tested your PR branch with / without the fix, and 
confirmed that it fails without the `gitattributes` that disables `crlf` and 
passes with it.

-

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


Re: RFR: 8239138: StyleManager should use a BufferedInputStream [v3]

2021-05-27 Thread Kevin Rushforth
On Thu, 27 May 2021 17:22:35 GMT, Ambarish Rapte  wrote:

>> `StyleManager.calculateCheckSum()` uses a raw InputStream as the input to a 
>> `DigestInputStream` and reads one byte at a time. This is slower in 
>> performance and should be changed, either to use `BufferedInputStream` or 
>> read byte buffer of 4096 from the stream or use both.
>> 
>> I have tried three approaches and tested with modena.css which is ~134 KB in 
>> size.
>> Following are the approaches and time in milliseconds taken by the method 
>> StyleManager.calculateCheckSum(), collected from 25 readings,
>> 
>> 1. Use BufferedInputStream and read one byte at a time 
>> [commit#1](https://github.com/openjdk/jfx/commit/6cd7d44d0ce1084c6cdb06a698c7aca127a615ef)
>>  : 
>> - Maximum: 53 ms,  Minimum: 27 ms, Average: 39 ms
>> 2. Use BufferedInputStream and read buffer of 4096 at a time 
>> [commit#1+2](https://github.com/openjdk/jfx/pull/518/files/6e0c621ea62691d5402b2bca5951d1012a5b1b91)
>> - Maximum: 17 ms,  Minimum: 14 ms, Average: 15.58 ms
>> 3. Use raw InputStream(current implementation) and read buffer of 4096 at a 
>> time 
>> [commit#1+2+3](https://github.com/openjdk/jfx/pull/518/files/215e1a183cfb902247f0d48685f7a901cb5fb003),
>>  which also similar to `NativeLibLoader.calculateCheckSum()` and looks 
>> faster than previous two.
>> - Maximum: 16 ms,  Minimum: 13 ms, Average: 14.25 ms
>> 
>> 
>> The time taken by `StyleManager.calculateCheckSum()` with current 
>> implementation is,
>> - Maximum: 61 ms,  Minimum: 38 ms, Average: 50.58 ms
>> 
>> 
>> Both approaches 2 & 3 show good improvement. I would prefer approach#3 as it 
>> is similar to `NativeLibLoader.calculateCheckSum()`.
>> However we can choose approach#2 also.
>> If we choose approach#3 then bug summary should be changed accordingly.
>
> Ambarish Rapte has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   review update

The fix and test look fine now. Unfortunately, if you look at the checks for 
this PR, the test fails on the GitHub Actions Windows build:


2021-05-27T17:31:48.5184758Z test.com.sun.javafx.css.StyleManagerTest > 
testCalculateCheckSum FAILED
2021-05-27T17:31:48.5186991Z java.lang.AssertionError: 
2021-05-27T17:31:48.5188560Z at org.junit.Assert.fail(Assert.java:91)
2021-05-27T17:31:48.5190261Z at 
org.junit.Assert.assertTrue(Assert.java:43)
2021-05-27T17:31:48.5192290Z at 
org.junit.Assert.assertTrue(Assert.java:54)
2021-05-27T17:31:48.5195303Z at 
test.com.sun.javafx.css.StyleManagerTest.testCalculateCheckSum(StyleManagerTest.java:1121)
2021-05-27T17:31:49.3450264Z 


This is almost certainly because of DOS line endings courtesy of the native Git 
for Windows where the default configuration is set to `core.autocrlf = true`. 
This causes git to convert all files on checkout to Windows-style line endings. 
I note that others could run into this problem as well -- in fact someone 
recently ran into an WebKit build error caused by `autocrlf` on their system.

I will file a new bug to add a `.gitattributes` to our repo. I note that the 
`jdk` repo already did this prior to the Skara switch. See 
[JDK-8241768](https://bugs.openjdk.java.net/browse/JDK-8241768).

You can either wait for that newly-filed bug to be integrated or else skip this 
test on Windows, referring to that bug. I'd probably recommend waiting.

-

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


Re: RFR: 8239138: StyleManager should use a BufferedInputStream [v2]

2021-05-27 Thread Kevin Rushforth
On Thu, 27 May 2021 02:14:32 GMT, Ambarish Rapte  wrote:

>> `StyleManager.calculateCheckSum()` uses a raw InputStream as the input to a 
>> `DigestInputStream` and reads one byte at a time. This is slower in 
>> performance and should be changed, either to use `BufferedInputStream` or 
>> read byte buffer of 4096 from the stream or use both.
>> 
>> I have tried three approaches and tested with modena.css which is ~134 KB in 
>> size.
>> Following are the approaches and time in milliseconds taken by the method 
>> StyleManager.calculateCheckSum(), collected from 25 readings,
>> 
>> 1. Use BufferedInputStream and read one byte at a time 
>> [commit#1](https://github.com/openjdk/jfx/commit/6cd7d44d0ce1084c6cdb06a698c7aca127a615ef)
>>  : 
>> - Maximum: 53 ms,  Minimum: 27 ms, Average: 39 ms
>> 2. Use BufferedInputStream and read buffer of 4096 at a time 
>> [commit#1+2](https://github.com/openjdk/jfx/pull/518/files/6e0c621ea62691d5402b2bca5951d1012a5b1b91)
>> - Maximum: 17 ms,  Minimum: 14 ms, Average: 15.58 ms
>> 3. Use raw InputStream(current implementation) and read buffer of 4096 at a 
>> time 
>> [commit#1+2+3](https://github.com/openjdk/jfx/pull/518/files/215e1a183cfb902247f0d48685f7a901cb5fb003),
>>  which also similar to `NativeLibLoader.calculateCheckSum()` and looks 
>> faster than previous two.
>> - Maximum: 16 ms,  Minimum: 13 ms, Average: 14.25 ms
>> 
>> 
>> The time taken by `StyleManager.calculateCheckSum()` with current 
>> implementation is,
>> - Maximum: 61 ms,  Minimum: 38 ms, Average: 50.58 ms
>> 
>> 
>> Both approaches 2 & 3 show good improvement. I would prefer approach#3 as it 
>> is similar to `NativeLibLoader.calculateCheckSum()`.
>> However we can choose approach#2 also.
>> If we choose approach#3 then bug summary should be changed accordingly.
>
> Ambarish Rapte has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - minor: format correction
>  - add test

The new test looks like it will work, but maybe could be made a bit clearer?

modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/StyleManagerTest.java
 line 1121:

> 1119: actualChecksum += Integer.toString((b & 0xff) + 0x100, 
> 16).substring(1);
> 1120: }
> 1121: assertEquals(expectedChecksum, actualChecksum);

It might be clearer to use a `byte` array for the expected checksum and use 
`Arrays.equals()` to compare them rather than converting to a `String`. I'm 
scratching my head over the formula in the `Integer.toString` call.

modules/javafx.graphics/src/test/resources/test/com/sun/javafx/css/checksum.css 
line 1:

> 1: 

This file should have a copyright header.

-

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


Re: RFR: 8239138: StyleManager should use a BufferedInputStream

2021-05-25 Thread Kevin Rushforth
On Tue, 25 May 2021 09:04:07 GMT, Ambarish Rapte  wrote:

> `StyleManager.calculateCheckSum()` uses a raw InputStream as the input to a 
> `DigestInputStream` and reads one byte at a time. This is slower in 
> performance and should be changed, either to use `BufferedInputStream` or 
> read byte buffer of 4096 from the stream or use both.
> 
> I have tried three approaches and tested with modena.css which is ~134 KB in 
> size.
> Following are the approaches and time in milliseconds taken by the method 
> StyleManager.calculateCheckSum(), collected from 25 readings,
> 
> 1. Use BufferedInputStream and read one byte at a time 
> [commit#1](https://github.com/openjdk/jfx/commit/6cd7d44d0ce1084c6cdb06a698c7aca127a615ef)
>  : 
> - Maximum: 53 ms,  Minimum: 27 ms, Average: 39 ms
> 2. Use BufferedInputStream and read buffer of 4096 at a time 
> [commit#1+2](https://github.com/openjdk/jfx/pull/518/files/6e0c621ea62691d5402b2bca5951d1012a5b1b91)
> - Maximum: 17 ms,  Minimum: 14 ms, Average: 15.58 ms
> 3. Use raw InputStream(current implementation) and read buffer of 4096 at a 
> time 
> [commit#1+2+3](https://github.com/openjdk/jfx/pull/518/files/215e1a183cfb902247f0d48685f7a901cb5fb003),
>  which also similar to `NativeLibLoader.calculateCheckSum()` and looks faster 
> than previous two.
> - Maximum: 16 ms,  Minimum: 13 ms, Average: 14.25 ms
> 
> 
> The time taken by `StyleManager.calculateCheckSum()` with current 
> implementation is,
> - Maximum: 61 ms,  Minimum: 38 ms, Average: 50.58 ms
> 
> 
> Both approaches 2 & 3 show good improvement. I would prefer approach#3 as it 
> is similar to `NativeLibLoader.calculateCheckSum()`.
> However we can choose approach#2 also.
> If we choose approach#3 then bug summary should be changed accordingly.

I also prefer approach 3, since the body of the try/finally loop is then 
identical to that of `NativeLibLoader::calculateCheckSum`.

Can you add a unit test to validate the checksum method? It should be 
sufficient to do this by taking a file that is > 4096 bytes, but not a multiple 
of 4096, manually running `md5sum` on it to get its checksum that you can then 
use to compare with the computed one.

-

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


  1   2   3   4   5   6   7   8   9   10   >