Re: RFR: 8268219: hlsprogressbuffer should provide PTS after GStreamer update
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]
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 sha
Re: RFR: 8267418: IntelliJ build and test of JavaFX does not work [v4]
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
Re: RFR: 8267418: IntelliJ build and test of JavaFX does not work [v4]
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 >From the gradle documentation: [Skipping Javadocs and >sources](https://docs.gradle.org/current/userguide/dependency_verification.html#sec:skipping-javadocs) The verification of auto downloaded *-sources.jar files can be skipped using following change. This way we can avoid deleting the `verification-metadata.xml` file. diff --git a/gradle/verification-metadata.xml b/gradle/verification-metadata.xml index abacd0b05b..0a3d33726d 100644 --- a/gradle/verification-metadata.xml +++ b/gradle/verification-metadata.xml @@ -3,6 +3,9 @@ true false + + + I tested this change in addition to the PR changes, dependency verification does not fail with this change but fails without. Can you please verify and test this change preferably on a clean repo by creating a new IntelliJ project. - PR: https://git.openjdk.java.net/jfx/pull/506
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v24]
On Tue, 15 Jun 2021 10:02:02 GMT, Ambarish Rapte wrote: > I will try with a full clean build and update. I executed the sample with a clean build and also with a fresh clone of repo. The issue still reproduces. > Did the tests that fail with this patch all run on the integrated graphics? > Kevin's Mac behaved correctly with this patch. Can you get a dedicated GPU so > test this with? Yes, I have a machine with integrated GPU: Mac Mojave 10.14.6, Integrated GPU: Intel Iris Graphics 6100 1536 MB. The issue seems to be specific to this machine. I don't have another mac with discrete GPU. - PR: https://git.openjdk.java.net/jfx/pull/334
Integrated: 8264139: Suppress removal warnings for Security Manager methods
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
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v24]
On Tue, 15 Jun 2021 08:57:18 GMT, Nir Lisker wrote: > > > Looks like artifacts. I don't know if a mistake in the code can produce such > results. I will try with a full clean build and update. - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8268642: Expand the javafx.beans.property.Property documentation
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") I will review this. - PR: https://git.openjdk.java.net/jfx/pull/533
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v24]
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 Looks like artifacts. I don't know if a mistake in the code can produce such results. Did the tests that fail with this patch all run on the integrated graphics? Kevin's Mac behaved correctly with this patch. Can you get a dedicated GPU so test this with? I tested with a dedicated GPU on both Win10 and Ubuntu 20 so make sure that both pipelines work. I don't have any more environments I can test on. - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8264139: Suppress removal warnings for Security Manager methods [v2]
On Fri, 4 Jun 2021 16:39:26 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
>
> Kevin Rushforth has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review feedback
Looks good to me.
-
Marked as reviewed by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/528
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v15]
On Fri, 11 Jun 2021 11:38:33 GMT, Ambarish Rapte wrote: >> Hello Nir, could you please merge this branch with latest master. > >> I think so too. Did you test on Win? Does @arapte want to test these >> functions too? > > Yes, I shall test too, and get back to the review... > @arapte If you run the test app without the spotlight patch, but with the > attenuation patch, does it work better? Here is what I tried, 1. Run existing AttenLightingSample without this PR changes. The sample works as expected. 2. Run existing AttenLightingSample with this PR. The sample does NOT work as expected. 2.1 If any one light is turned on then the output is same as the screenshot I shared in previous comment. 2.2 If two or three lights(including red light) are turned on then the red light does not illuminate correctly. (Observe the following screenshot, there are some yellow colored pixels in red light color)  So looks like, the issue occurs only with the changes in this PR. - PR: https://git.openjdk.java.net/jfx/pull/334
