Re: RFR: 8267314: Loading some animated GIFs fails with ArrayIndexOutOfBoundsException: Index 4096 out of bounds for length 4096 [v2]
On Fri, 21 May 2021 19:24:19 GMT, Jose Pereda wrote: >> This PR limits the `tableIndex` value, used by the LZWDecoder algorithm in >> `GIFImageLoader2`, to avoid a potential AIOOB exception that happens on some >> animated GIFs, to the maximum size of the tables used (4096). >> >> In some occasions loading an animated GIF like the one used in the included >> test, doesn't throw such exception, because we `allow partially loaded >> animated images` in `ImageStorage`, but only a few frames are loaded. >> >> In theory, greater values of such index would operate over completely full >> tables, so there is no need to add new values in this case, and therefore, >> there is no risk in limiting the value to 4096. >> >> This PR will prevent the exception and all the frames should load. The >> included test passes now (and fails loading only 10 frames out of 44 without >> the proposed fix). > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Modify test to avoid online resources @jperedadnr Since you're looking at this already, can you check if https://bugs.openjdk.java.net/browse/JDK-8209560 is related? - PR: https://git.openjdk.java.net/jfx/pull/513
Re: RFR: 8267551: Support loading images from inline data-URIs [v9]
> 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: Reverted a change - Changes: - all: https://git.openjdk.java.net/jfx/pull/508/files - new: https://git.openjdk.java.net/jfx/pull/508/files/d707615e..30211a4c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=508=08 - incr: https://webrevs.openjdk.java.net/?repo=jfx=508=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v8]
> 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 two additional commits since the last revision: - Allow leading and trailing whitespace in data URI - Removed test - Changes: - all: https://git.openjdk.java.net/jfx/pull/508/files - new: https://git.openjdk.java.net/jfx/pull/508/files/907bc782..d707615e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=508=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx=508=06-07 Stats: 35 lines in 3 files changed: 27 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Integrated: 8210199: [linux / macOS] fileChooser can't handle emojis
On Wed, 21 Apr 2021 10:42:04 GMT, Jose Pereda wrote: > Both `GlassDialogs.m` for macOS and `GlassCommonDialogs.c` for Linux use UTF8 > encoding for the file names selected via native FileChooser, and this will > fail if there are emojis in the file name. > > This PR uses the same approach as in > [JDK-8258381](https://bugs.openjdk.java.net/browse/JDK-8258381) for macOS, > using UTF16 encoding. > > For Linux, the Java `String(byte[] bytes)` constructor with default charset > is used instead, preventing the need of using UTF8 encoding. This pull request has now been integrated. Changeset: aebac072 Author:Jose Pereda URL: https://git.openjdk.java.net/jfx/commit/aebac072b1919e68f7732de929dc085d00c62e92 Stats: 15 lines in 2 files changed: 12 ins; 0 del; 3 mod 8210199: [linux / macOS] fileChooser can't handle emojis Reviewed-by: pbansal, kcr - PR: https://git.openjdk.java.net/jfx/pull/471
Re: RFR: 8267551: Support loading images from inline data-URIs [v7]
> 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: Changed data URI detection - Changes: - all: https://git.openjdk.java.net/jfx/pull/508/files - new: https://git.openjdk.java.net/jfx/pull/508/files/4302e9f1..907bc782 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=508=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=508=05-06 Stats: 65 lines in 6 files changed: 12 ins; 25 del; 28 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267551: Support loading images from inline data-URIs [v6]
> 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 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 feature/image-datauri - Moved test - Added javadoc - Added javadoc - Added javadoc - Added javadoc - Added data-URI loading support for images - Changes: - all: https://git.openjdk.java.net/jfx/pull/508/files - new: https://git.openjdk.java.net/jfx/pull/508/files/3a77ac0b..4302e9f1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=508=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=508=04-05 Stats: 683 lines in 52 files changed: 400 ins; 98 del; 185 mod Patch: https://git.openjdk.java.net/jfx/pull/508.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/508/head:pull/508 PR: https://git.openjdk.java.net/jfx/pull/508
Re: RFR: 8267314: Loading some animated GIFs fails with ArrayIndexOutOfBoundsException: Index 4096 out of bounds for length 4096 [v2]
On Fri, 21 May 2021 19:24:19 GMT, Jose Pereda wrote: >> This PR limits the `tableIndex` value, used by the LZWDecoder algorithm in >> `GIFImageLoader2`, to avoid a potential AIOOB exception that happens on some >> animated GIFs, to the maximum size of the tables used (4096). >> >> In some occasions loading an animated GIF like the one used in the included >> test, doesn't throw such exception, because we `allow partially loaded >> animated images` in `ImageStorage`, but only a few frames are loaded. >> >> In theory, greater values of such index would operate over completely full >> tables, so there is no need to add new values in this case, and therefore, >> there is no risk in limiting the value to 4096. >> >> This PR will prevent the exception and all the frames should load. The >> included test passes now (and fails loading only 10 frames out of 44 without >> the proposed fix). > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Modify test to avoid online resources Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/513
Re: RFR: 8210199: [linux / macOS] fileChooser can't handle emojis
On Wed, 21 Apr 2021 10:42:04 GMT, Jose Pereda wrote: > Both `GlassDialogs.m` for macOS and `GlassCommonDialogs.c` for Linux use UTF8 > encoding for the file names selected via native FileChooser, and this will > fail if there are emojis in the file name. > > This PR uses the same approach as in > [JDK-8258381](https://bugs.openjdk.java.net/browse/JDK-8258381) for macOS, > using UTF16 encoding. > > For Linux, the Java `String(byte[] bytes)` constructor with default charset > is used instead, preventing the need of using UTF8 encoding. I filed [JDK-8267572](https://bugs.openjdk.java.net/browse/JDK-8267572) to track the hang on Linux. - PR: https://git.openjdk.java.net/jfx/pull/471
Re: RFR: 8210199: [linux / macOS] fileChooser can't handle emojis
On Wed, 21 Apr 2021 10:42:04 GMT, Jose Pereda wrote: > Both `GlassDialogs.m` for macOS and `GlassCommonDialogs.c` for Linux use UTF8 > encoding for the file names selected via native FileChooser, and this will > fail if there are emojis in the file name. > > This PR uses the same approach as in > [JDK-8258381](https://bugs.openjdk.java.net/browse/JDK-8258381) for macOS, > using UTF16 encoding. > > For Linux, the Java `String(byte[] bytes)` constructor with default charset > is used instead, preventing the need of using UTF8 encoding. I can reproduce the problem Pankaj reported on Linux, and can confirm that it is unrelated to FileChooser. It looks like some sort of deadlock that happens when calling `java.awt.Desktop::open` from the event handler. I'll file a separate bug for that. I modified the test program to call `Files.readString(file.toPath())` instead, and can confirm that the fix works on Linux as well as on macOS. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/471
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v14]
On Thu, 15 Apr 2021 02:21:50 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? >> >> - [ ] 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: > > Combined rotation and direction No, but I can do that. - PR: https://git.openjdk.java.net/jfx/pull/334
Integrated: 8264127: ListCell editing status is true, when index changes while editing
On Wed, 24 Mar 2021 14:58:40 GMT, Florian Kirmaier wrote: > Fixing ListCell editing status is true, when index changes while editing. This pull request has now been integrated. Changeset: 240d28ff Author:Florian Kirmaier Committer: Jeanette Winzenburg URL: https://git.openjdk.java.net/jfx/commit/240d28ff4c73777f57fdf88250cceb601e5c18c1 Stats: 151 lines in 2 files changed: 137 ins; 1 del; 13 mod 8264127: ListCell editing status is true, when index changes while editing Reviewed-by: fastegal, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/441
