Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]
On Mon, 29 Jun 2020 21:50:50 GMT, Kevin Rushforth wrote: >> Frederic Thevenet has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fill test image with a bilinear gradient instead of random noise. > > I think I found the problem in the tiling logic that leads to the macOS > failures. You need to check that the remainder > width or height is > 0. Also, it looks like you have the "B" and "R" loops > backwards, which is a bit confusing. Actually, it also fails the same way on Ubuntu 20.04 as it does on macOS if I force HW acceleration on Ubuntu. It's off by default when running in a VM, so is using the SW pipeline, which doesn't have a hard limit (so that could explain the heap problem you were facing). Ensuring that the tile size is > 0 fixes it on Linux and Mac. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]
On Mon, 29 Jun 2020 16:27:19 GMT, Frederic Thevenet
wrote:
>> Issue JDK-8088198, where an exception would be thrown when trying to capture
>> a snapshot whose final dimensions would be
>> larger than the running platform's maximum supported texture size, was
>> addressed in openjfx14. The fix, based around
>> the idea of capturing as many tiles of the maximum possible size and
>> re-compositing the final snapshot out of these, is
>> currently only attempted after the original, non-tiled, strategy has already
>> failed. This was decided to avoid any risk
>> of regressions, either in terms of performances and correctness, while still
>> offering some relief to the original
>> issue. This follow-on issue aims to propose a fix to the original issue,
>> that is able to correctly decide on the best
>> snapshot strategy (tiled or not) to adopt before applying it and ensure best
>> performances possible when tiling is
>> necessary while still introducing no regressions compared to the original
>> solution.
>
> Frederic Thevenet has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fill test image with a bilinear gradient instead of random noise.
I think I found the problem in the tiling logic that leads to the macOS
failures. You need to check that the remainder
width or height is > 0. Also, it looks like you have the "B" and "R" loops
backwards, which is a bit confusing.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1612:
> 1611: int bTileHeight = tileHeight;
> 1612: while (bTileHeight == tileHeight && bOffset <
> h) {
> 1613: renderTile(x, xOffset, y, bOffset,
> mTileWidth, bTileHeight, buffer, rf, tileRttCache,
> pImage);
It looks like the "B" and the "R" loops are reversed. This isn't causing any
actual problems, but is confusing since it
doesn't match the comment or the diagram above. The comment for this block says
"B" tiles, but actually, it is the "R"
tiles in the diagram that this is looping over. At the end of the main loop,
`mTileWIdth` and `mTileHeight` will be set
to the size of the corner tile. Given this, the tiles of `mTileWidth` X
`tileHeight` will be the right hand column.
Once you fix this, you will need to surround this `while` loop in a check for
`if (mTileWidth > 0)`
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1620:
> 1619: int rTileWidth = tileWidth;
> 1620: while (rTileWidth == tileWidth && rOffset < w) {
> 1621: renderTile(x, rOffset, y, yOffset,
> rTileWidth, mTileHeight, buffer, rf, tileRttCache,
> pImage);
Similarly, you will need to surround this `while` loop in a check for `if
(mTileHeight > 0)`
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1626:
> 1625: // Render corner "C" tile if needed
> 1626: if (bOffset > 0 && rOffset > 0) {
> 1627: renderTile(x, rOffset, y, bOffset,
> rTileWidth, bTileHeight, buffer, rf, tileRttCache,
> pImage);
I might recommend to also add a check for `mTileWidth > 0 && mTileHeight > 0`
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]
On Mon, 29 Jun 2020 16:41:53 GMT, Frederic Thevenet wrote: >> I have changed the test setup to fill up the scene with a gradient instead >> of random noise; it should be as able to >> catch misalignment, while not looking like something's gone horribly wrong. > > Also, while running the tests again on a ubuntu 20.04 VM, I encountered 2 > occurences where one of the tests failed, out > of ~8 runs in total. In one case, the error where similar to yours: >> Task :systemTests:test > > test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameSizeImm[3] FAILED > java.lang.IllegalArgumentException: Unrecognized image loader: null > at > javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278) > at > javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53) > at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1342) > at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136) > at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214) > at > test.javafx.scene.Snapshot2Test.lambda$doTestTiledSnapshotImm$12(Snapshot2Test.java:375) > > 148 tests completed, 1 failed > While in the other, it pointed to an Out Of Memory Error: > > Task :systemTests:test > > test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameHeightDefer[3] > FAILED > java.lang.OutOfMemoryError: Java heap space > at java.base/java.nio.HeapByteBuffer.(HeapByteBuffer.java:63) > at java.base/java.nio.ByteBuffer.allocate(ByteBuffer.java:351) > at > javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.createPlatformImage(QuantumToolkit.java:1426) > at javafx.graphics/javafx.scene.image.Image.(Image.java:740) > at > javafx.graphics/javafx.scene.image.WritableImage.(WritableImage.java:77) > at > test.javafx.scene.Snapshot2Test.doTestTiledSnapshotDefer(Snapshot2Test.java:377) > at > test.javafx.scene.Snapshot2Test.testSnapshot2x2TilesSameHeightDefer(Snapshot2Test.java:489) > > 148 tests completed, 1 failed > > Again, in my environment, it only failed one test, and not all the time, but > this could be the same root cause. Two > ways we could find out would be by either increasing the max heap size for > the test runner or use a smaller image and > force the prism.maxTextureSize property to something less than 4096 to > trigger tiling anyway. Unfortunately, I could > not find out how to achieve either of these (simply passing the properties to > the JVM running the Gradle wrapper seem > to have no effect). It runs fine for me on Ubuntu 20.04 (VM) and Windows 10. I also see the failures on macOS. This isn't related to heap size. The failures are consistent, and hit virtually all of the tests whose dimensions are larger than 4096. Also, I modified the test to `@Ignore` all but one of the failing tests, and that test still fails. I instrumented the code, and from one of the failing tests I see this: renderToImage: need to tile image, size = 18000 x 90 renderTile: 4096 x 90 renderTile: 4096 x 90 renderTile: 4096 x 90 renderTile: 4096 x 90 renderTile: 1616 x 90 renderTile: 4096 x 0 This last tile is an illegal tile size (so you're probably just getting lucky that it isn't causing problems on Windows or Linux). I would check the tiling logic. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8201568: zForce touchscreen input device fails when closed and immediately reopened
The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.
--
On Mon, 29 Jun 2020 11:26:50 GMT, Johan Vos wrote:
>> Fixes [JDK-8201568](https://bugs.openjdk.java.net/browse/JDK-8201568).
>
> The rationale makes sense (open/open/close) instead of (open/close/open)
Based on my notes below, I believe this change is safe for any Linux input
device driver because the driver shouldn't
receive the intermediate *open* and *close* calls at all.
Nonetheless, it would be reassuring if someone could try [this
change](https://github.com/openjdk/jfx/pull/258/files)
just once on a mainstream device, such as the Raspberry Pi with a touchscreen
LCD panel. I only have six obscure ARM
devices and a headless QEMU ARM virtual machine for testing. @johanvos or
@dellgreen - Is that something you could
test? If you think it's overkill and not worth it, that's fine, too.
Notes
The Linux kernel documentation about [opening and
closing](https://www.kernel.org/doc/html/latest/input/input-programming.html#dev-open-and-dev-close)
input devices
states:
> Note that input core keeps track of number of users for the device and makes
> sure that dev->open() is called only when
> the first user connects to the device and that dev->close() is called when
> the very last user disconnects. Calls to
> both callbacks are serialized.
Also, the Linux Programmer's Manual for the *close* system call (`man 2 close`)
states:
> If *fd* is the last file descriptor referring to the underlying open file
> description (see **open**(2)), the resources
> associated with the open file description are freed.
Running a JavaFX program with `strace -f -e trace=open,close -o strace.log`
shows the one *open* for the *event0*
keyboard, and the *open, open, close* for the *event1* touchscreen:
5847 open("/dev/input/event0", O_RDONLY) = 11
...
5847 open("/dev/input/event1", O_RDONLY) = 12
5847 open("/dev/input/event1", O_RDONLY) = 13
5847 close(13) = 0
Both devices are actually closed by the kernel when the JavaFX program ends.
-
PR: https://git.openjdk.java.net/jfx/pull/258
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]
On Mon, 29 Jun 2020 16:16:58 GMT, Frederic Thevenet wrote: >> Thanks for your review. >> >> I don't have access to a Mac so I can't check that directly. >> The tests pass on both my Windows 10 and Ubuntu 20.04 environments and the >> stack isn't very helpful (it simply >> indicates the renderImage method returned null). >> Running the test with the unpatched version of the >> `QuantumToolkit::renderToImage` method would typically result in >> such a stack, but there are many other possible reasons. >> With regard to the choice of random noise for the test image, my idea was to >> be able to catch any misalignment caused >> by tiling in the final snapshot: using a single color or a simple pattern >> would not necessarily help catching such >> errors. Using a complex image could do the trick, and avoid the broken >> aspect you mentioned, but it would need to be >> very large (>4096x4096), and I was not sure it would be wise to add such a >> large binary resource to the repo. So I >> settled for random noise, since it was the simplest way to generate an image >> which guaranties any misalignment would be >> caught by comparing pixels 1 to 1. > > I have changed the test setup to fill up the scene with a gradient instead of > random noise; it should be as able to > catch misalignment, while not looking like something's gone horribly wrong. Also, while running the tests again on a ubuntu 20.04 VM, I encountered 2 occurences where one of the tests failed, out of ~8 runs in total. In one case, the error where similar to yours: > Task :systemTests:test test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameSizeImm[3] FAILED java.lang.IllegalArgumentException: Unrecognized image loader: null at javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278) at javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53) at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1342) at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136) at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214) at test.javafx.scene.Snapshot2Test.lambda$doTestTiledSnapshotImm$12(Snapshot2Test.java:375) 148 tests completed, 1 failed While in the other, it pointed to an Out Of Memory Error: Task :systemTests:test test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameHeightDefer[3] FAILED java.lang.OutOfMemoryError: Java heap space at java.base/java.nio.HeapByteBuffer.(HeapByteBuffer.java:63) at java.base/java.nio.ByteBuffer.allocate(ByteBuffer.java:351) at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.createPlatformImage(QuantumToolkit.java:1426) at javafx.graphics/javafx.scene.image.Image.(Image.java:740) at javafx.graphics/javafx.scene.image.WritableImage.(WritableImage.java:77) at test.javafx.scene.Snapshot2Test.doTestTiledSnapshotDefer(Snapshot2Test.java:377) at test.javafx.scene.Snapshot2Test.testSnapshot2x2TilesSameHeightDefer(Snapshot2Test.java:489) 148 tests completed, 1 failed Again, in my environment, it only failed one test, and not all the time, but this could be the same root cause. Two ways we could find out would be by either increasing the max heap size for the test runner or use a smaller image and force the prism.maxTextureSize property to something less than 4096 to trigger tiling anyway. Unfortunately, I could not find out how to achieve either of these (simply passing the properties to the JVM running the Gradle wrapper seem to have no effect). - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]
On Mon, 29 Jun 2020 14:35:00 GMT, Frederic Thevenet wrote: >> I observed that the added tests are failing on mac machine(Mojave 10.14.6), >> but they do pass on windows10. Can you >> please verify ? Timeout and Unrecognized Image loader errors from the log, >> test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameWidthImm[0] FAILED >> java.lang.IllegalArgumentException: Unrecognized image loader: null >> at >> javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278) >> at >> javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53) >> at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1342) >> at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136) >> at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214) >> at >> test.javafx.scene.Snapshot2Test.lambda$doTestTiledSnapshotImm$12(Snapshot2Test.java:364) >> >> test.javafx.scene.Snapshot2Test > testSnapshot2x1TilesSameSizeDefer[0] FAILED >> junit.framework.AssertionFailedError: Timeout waiting for snapshot >> callback >> at >> test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:157) >> at >> test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:183) >> at >> test.javafx.scene.Snapshot2Test.doTestTiledSnapshotDefer(Snapshot2Test.java:378) >> at >> test.javafx.scene.Snapshot2Test.testSnapshot2x1TilesSameSizeDefer(Snapshot2Test.java:459) >> >> I would suggest to fill the image with a single or preferably some pattern >> of multiple recognizable colors instead of >> noise. The noise gives a broken feel. It might confuse the verifications of >> any future fixes in this area. A well >> defined color would be nice. > > Thanks for your review. > > I don't have access to a Mac so I can't check that directly. > The tests pass on both my Windows 10 and Ubuntu 20.04 environments and the > stack isn't very helpful (it simply > indicates the renderImage method returned null). > Running the test with the unpatched version of the > `QuantumToolkit::renderToImage` method would typically result in > such a stack, but there are many other possible reasons. > With regard to the choice of random noise for the test image, my idea was to > be able to catch any misalignment caused > by tiling in the final snapshot: using a single color or a simple pattern > would not necessarily help catching such > errors. Using a complex image could do the trick, and avoid the broken aspect > you mentioned, but it would need to be > very large (>4096x4096), and I was not sure it would be wise to add such a > large binary resource to the repo. So I > settled for random noise, since it was the simplest way to generate an image > which guaranties any misalignment would be > caught by comparing pixels 1 to 1. I have changed the test setup to fill up the scene with a gradient instead of random noise; it should be as able to catch misalignment, while not looking like something's gone horribly wrong. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]
> Issue JDK-8088198, where an exception would be thrown when trying to capture > a snapshot whose final dimensions would be > larger than the running platform's maximum supported texture size, was > addressed in openjfx14. The fix, based around > the idea of capturing as many tiles of the maximum possible size and > re-compositing the final snapshot out of these, is > currently only attempted after the original, non-tiled, strategy has already > failed. This was decided to avoid any risk > of regressions, either in terms of performances and correctness, while still > offering some relief to the original > issue. This follow-on issue aims to propose a fix to the original issue, > that is able to correctly decide on the best > snapshot strategy (tiled or not) to adopt before applying it and ensure best > performances possible when tiling is > necessary while still introducing no regressions compared to the original > solution. Frederic Thevenet has updated the pull request incrementally with one additional commit since the last revision: Fill test image with a bilinear gradient instead of random noise. - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/a01aaa20..efccc4d8 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.12 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.11-12 Stats: 16 lines in 1 file changed: 11 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/112.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/112/head:pull/112 PR: https://git.openjdk.java.net/jfx/pull/112
Integrated: 8240264: iOS: Unnecessary logging on every pulse when GL context changes
On Wed, 8 Apr 2020 08:37:27 GMT, Jose Pereda wrote: > This PR comments out too verbose console logging, as it has been done with > some other fprintf calls. > As a follow up of this PR, a custom directive that enables this output if > needed can be considered. This pull request has now been integrated. Changeset: 15f97ed4 Author:Jose Pereda URL: https://git.openjdk.java.net/jfx/commit/15f97ed4 Stats: 28 lines in 3 files changed: 0 ins; 26 del; 2 mod 8240264: iOS: Unnecessary logging on every pulse when GL context changes Reviewed-by: kcr, jvos - PR: https://git.openjdk.java.net/jfx/pull/165
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]
On Mon, 29 Jun 2020 14:02:11 GMT, Ambarish Rapte wrote: >> I went ahead and wrote a bunch of tests that: >> 1. Setup a scene to display an `ImageView` of a selected dimensions chosen >> to trigger tiling in different ways when >> taking snapshots. 2. Fill up the image with noise. >> 3. Take a snapshot and do a pixel-wise comparison with the original image. >> >> I've added the new tests to the existing `Snapshot2Test.java`. > > I observed that the added tests are failing on mac machine(Mojave 10.14.6), > but they do pass on windows10. Can you > please verify ? Timeout and Unrecognized Image loader errors from the log, > test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameWidthImm[0] FAILED > java.lang.IllegalArgumentException: Unrecognized image loader: null > at > javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278) > at > javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53) > at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1342) > at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136) > at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214) > at > test.javafx.scene.Snapshot2Test.lambda$doTestTiledSnapshotImm$12(Snapshot2Test.java:364) > > test.javafx.scene.Snapshot2Test > testSnapshot2x1TilesSameSizeDefer[0] FAILED > junit.framework.AssertionFailedError: Timeout waiting for snapshot > callback > at > test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:157) > at > test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:183) > at > test.javafx.scene.Snapshot2Test.doTestTiledSnapshotDefer(Snapshot2Test.java:378) > at > test.javafx.scene.Snapshot2Test.testSnapshot2x1TilesSameSizeDefer(Snapshot2Test.java:459) > > I would suggest to fill the image with a single or preferably some pattern of > multiple recognizable colors instead of > noise. The noise gives a broken feel. It might confuse the verifications of > any future fixes in this area. A well > defined color would be nice. Thanks for your review. I don't have access to a Mac so I can't check that directly. The tests pass on both my Windows 10 and Ubuntu 20.04 environments and the stack isn't very helpful (it simply indicates the renderImage method returned null). Running the test with the unpatched version of the `QuantumToolkit::renderToImage` method would typically result in such a stack, but there are many other possible reasons. With regard to the choice of random noise for the test image, my idea was to be able to catch any misalignment caused by tiling in the final snapshot: using a single color or a simple pattern would not necessarily help catching such errors. Using a complex image could do the trick, and avoid the broken aspect you mentioned, but it would need to be very large (>4096x4096), and I was not sure it would be wise to add such a large binary resource to the repo. So I settled for random noise, since it was the simplest way to generate an image which guaranties any misalignment would be caught by comparing pixels 1 to 1. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v5]
On Wed, 17 Jun 2020 11:33:27 GMT, Frederic Thevenet wrote: >>> [...] I'd like to see some additional system tests that cover all of the >>> cases of X and Y fitting/not-fitting exactly >>> (and if you stick with your current approach, X or Y as the inner loop). >> >> What kind of tests do you have in mind? More specifically do you mean simply >> adding tests that expand on the existing >> `doTestSnapshotScaleNodeDefer`and `doTestSnapshotScaleNodeImm` (which >> basically just prove that taking a snapshot >> returns a non-null image of the expected size)? Or do you think we need to >> include a test that proves the snapshot >> produced by tiling is entirely faithful to the original, pixel-wise? > > I went ahead and wrote a bunch of tests that: > 1. Setup a scene to display an `ImageView` of a selected dimensions chosen to > trigger tiling in different ways when > taking snapshots. 2. Fill up the image with noise. > 3. Take a snapshot and do a pixel-wise comparison with the original image. > > I've added the new tests to the existing `Snapshot2Test.java`. I observed that the added tests are failing on mac machine(Mojave 10.14.6), but they do pass on windows10. Can you please verify ? Timeout and NPE exception from the log, test.javafx.scene.Snapshot2Test > testSnapshot2x2TilesSameWidthImm[0] FAILED java.lang.IllegalArgumentException: Unrecognized image loader: null at javafx.graphics/javafx.scene.image.WritableImage.loadTkImage(WritableImage.java:278) at javafx.graphics/javafx.scene.image.WritableImage$1.loadTkImage(WritableImage.java:53) at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1342) at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136) at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214) at test.javafx.scene.Snapshot2Test.lambda$doTestTiledSnapshotImm$12(Snapshot2Test.java:364) test.javafx.scene.Snapshot2Test > testSnapshot2x1TilesSameSizeDefer[0] FAILED junit.framework.AssertionFailedError: Timeout waiting for snapshot callback at test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:157) at test.javafx.scene.SnapshotCommon.runDeferredSnapshotWait(SnapshotCommon.java:183) at test.javafx.scene.Snapshot2Test.doTestTiledSnapshotDefer(Snapshot2Test.java:378) at test.javafx.scene.Snapshot2Test.testSnapshot2x1TilesSameSizeDefer(Snapshot2Test.java:459) I would suggest to fill the image with a single or preferably some pattern of multiple recognizable colors instead of noise. The noise gives a broken feel. It might confuse the verifications of any future fixes in this area. A well defined color would be nice. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8240264: iOS: Unnecessary logging on every pulse when GL context changes [v3]
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Sun, 28 Jun 2020 18:34:01 GMT, Jose Pereda wrote: >> This PR comments out too verbose console logging, as it has been done with >> some other fprintf calls. >> As a follow up of this PR, a custom directive that enables this output if >> needed can be considered. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Restore initial printout Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/165
Re: RFR: 8238954: Improve performance of tiled snapshot rendering
Hi Ambarish, Thanks for your review. I don't have access to a Mac so I can't check that directly. The tests pass on both my Windows 10 and Ubuntu 20.04 environments and the stack isn't very helpful (it simply indicates the renderImage method returned null). There could be many reasons for that, but this is what would happen when running the test with the unpatched version of the QuantumToolkit::renderToImage method. Maybe double-check the pull applied cleanly on your clone? With regard to the choice of random noise for the test image, my idea was to be able to catch any misalignment caused by tiling in the final snapshot: using a single color or a simple pattern would not necessarily help catching such errors. Using a complex image could do the trick, and avoid the broken aspect you mentioned, but it would need to be very large (>4096x4096), and I was not sure it would be wise to add such a large binary resource to the repo. So I settled for random noise, since it was the simplest way to generate an image with 100% guaranties any misalignment would be caught by comparing pixels 1 to 1. PS: I cc'ing the mailing list, so that the skara bot will hopefully pick it up and add it to the PR, so we can resume the discussion there once Github is up again. -- Fred. - Mail original - De: "Ambarish Rapte" À: "Kevin Rushforth" , "thevenet fred" Envoyé: Lundi 29 Juin 2020 13:12:47 Objet: RE: RFR: 8238954: Improve performance of tiled snapshot rendering Hi, Looks like github is down, Unable to do a fresh login OR post a comment on the PR from already logged in session. Here is my comment about the tests, I observed that the added tests are failing on mac machine(Mojave 10.14.6), but they do pass on windows10. Can you please verify ? Attached mac test execution log. I would suggest to fill the image with a single or preferably some pattern of multiple recognizable colors instead of noise. The noise gives a broken feel. It might confuse the verifications of any future fixes in this area. A well defined color would be nice. Regards, Ambarish -Original Message- From: Kevin Rushforth Sent: Sunday, June 28, 2020 1:27 AM To: thevenet.f...@free.fr Cc: Ambarish Rapte Subject: Re: RFR: 8238954: Improve performance of tiled snapshot rendering No worries. I hope to finish the review on Monday or Tuesday. I ran a full test on Windows today and it looks good. I have some more testing to do and then I will sanity test it on other platforms. I took a quick look at the code changes since last time, and didn't spot any issues. I'll do a more careful review Monday. The new tests look good, but I haven't looked carefully at the code yet. -- Kevin On 6/26/2020 8:28 AM, thevenet.f...@free.fr wrote: >> I didn't copy openjfx-dev, so there is no way my reply could have >> been forwarded to either the list or the PR (else it would have been). > Yes, of course: I realized that immediately *after* I pressed the send > button... > >> I don't see any duplication of your message. There is exactly one >> reply to the list, which was forwarded to the PR (as expected). The >> Skara bot will forward comments made in the GitHub PR as a reply to >> the RFR email and vice-versa. >> >> If you want to make a comment on the mailing list related to your PR, >> but don't want that to show up in the PR, the easiest way is to start >> a new message thread rather than doing a reply. > Indeed, there's no need to overthink it. > > Thank you, and my apologies for being a little dense today ;-) > > -- Fred > > >> -- Kevin >> >> >> On 6/26/2020 7:57 AM, thevenet.f...@free.fr wrote: >>> Hi Kevin, thank you for that. >>> >>> As an unrelated side question: my original email to the list got >>> picked up by a bot and pushed to the PR, and then pushed back onto >>> the list,adding even more noise. >>> Meanwhile your reply didn't; why is that? >>> >>> Do you know where I can find the rules used by the gh bot to pick-up >>> messages from the list, so I can avoid tripping one of them >>> unintentionally? >>> >>> Thanks. >>> >>> -- Fred >>> >>> >>> >>> - Mail original - >>> De: "Kevin Rushforth" >>> À: "thevenet fred" >>> Cc: "Ambarish Rapte" >>> Envoyé: Vendredi 26 Juin 2020 15:39:56 >>> Objet: Re: RFR: 8238954: Improve performance of tiled snapshot >>> rendering >>> >>> Thanks for the reminder. Ambarish and I spoke yesterday and both of >>> us will review it. >>> >>> -- Kevin >>> >>> >>> On 6/26/2020 2:13 AM, thevenet.f...@free.fr wrote: Hi everyone, I think this is now ready for another (and hopefully final) round of reviews. Thanks. -- Fred - Mail original - De: "Frederic Thevenet" À: "openjfx-dev" Envoyé: Vendredi 28 Février 2020 18:58:00 Objet: RFR: 8238954: Improve performance of tiled snapshot rendering Issue JDK-8088198, where an exception would be thrown when trying to
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Sun, 28 Jun 2020 01:56:26 GMT, John Neffenger wrote: >>> OK, that seems fine then. I'll take a closer look and then finish my review. >> >> Actually, I think you may be right, though. Sorry for replying before >> looking into it. I now think the >> `ScheduledThreadPoolExecutor` should be shut down, but let me look into it a >> bit more this afternoon before your final >> review. Thanks! The new `ScheduledThreadPoolExecutor` is ~complicated~ >> flexible! > > I think the code in the `_stop` method is correct after all. > > The `MonocleTimer` class is written to allow for multiple calls to the pair > of `_start` and `_stop` methods (even > though I don't think that ever happens), and the static > `ScheduledThreadPoolExecutor`, named `scheduler`, is created > only once and reused on subsequent calls. Changing the `_stop` method to > call `task.cancel(true)` still leaves the > timer thread running, which prevents the JavaFX application from exiting when > the timer thread is a user thread. > Furthermore, whether it's a user or daemon thread, if the call to > `task.cancel(true)` happens to run exactly when the > periodic task is *in progress*, the `timerRunnable` lambda in > `QuantumToolkit` prints the stack trace when it catches > the `InterruptedException`. java.lang.InterruptedException: > at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit >.lambda$runToolkit$12(QuantumToolkit.java:345) > at java.base/java.util.concurrent.Executors$RunnableAdapter >.call(Executors.java:515) > at java.base/java.util.concurrent.FutureTask >.runAndReset(FutureTask.java:305) > at > java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask >.run(ScheduledThreadPoolExecutor.java:305) > at java.base/java.util.concurrent.ThreadPoolExecutor >.runWorker(ThreadPoolExecutor.java:1130) > at java.base/java.util.concurrent.ThreadPoolExecutor$Worker >.run(ThreadPoolExecutor.java:630) > at java.base/java.lang.Thread >.run(Thread.java:832) > > So the call to `task.cancel(false)` is correct. > > Changing the `_stop` method to shut down the `scheduler` will terminate the > associated thread, regardless of its daemon > status, but a subsequent call to `_start` will throw a > `RejectedExecutionException` when trying to schedule the timer > task: java.util.concurrent.RejectedExecutionException: > Task > java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask@b1fe89 > [Not completed, task = > java.util.concurrent.Executors$RunnableAdapter@1f85c96 > [Wrapped task = > com.sun.javafx.tk.quantum.QuantumToolkit$$Lambda$111/0x34563828@141859b]] > rejected from java.util.concurrent.ScheduledThreadPoolExecutor@55f462 > [Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed > tasks = 0] > at java.base/java.util.concurrent.ThreadPoolExecutor$AbortPolicy >.rejectedExecution(ThreadPoolExecutor.java:2057) > at java.base/java.util.concurrent.ThreadPoolExecutor >.reject(ThreadPoolExecutor.java:827) > at java.base/java.util.concurrent.ScheduledThreadPoolExecutor >.delayedExecute(ScheduledThreadPoolExecutor.java:340) > at java.base/java.util.concurrent.ScheduledThreadPoolExecutor >.scheduleAtFixedRate(ScheduledThreadPoolExecutor.java:632) > at javafx.graphics/com.sun.glass.ui.monocle.MonocleTimer >._start(MonocleTimer.java:64) > at javafx.graphics/com.sun.glass.ui.Timer >.start(Timer.java:96) > at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit >.runToolkit(QuantumToolkit.java:384) > at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit >.lambda$startup$10(QuantumToolkit.java:280) > at javafx.graphics/com.sun.glass.ui.Application >.lambda$run$1(Application.java:153) > at javafx.graphics/com.sun.glass.ui.monocle.RunnableProcessor >.runLoop(RunnableProcessor.java:92) > at javafx.graphics/com.sun.glass.ui.monocle.RunnableProcessor >.run(RunnableProcessor.java:51) > at java.base/java.lang.Thread >.run(Thread.java:832) > > So if we want `MonocleTimer` to reuse a single `ScheduledThreadPoolExecutor` > object, I think the only way to make sure > that its timer thread exits when the application exits is to set its daemon > status to `true`. While the PR should indeed fix the original issue, I'm unsure about the behavior of multiple invocations of start/stop rather than using the (nop) pause method. However, it seems this behavior is similar on other platforms, so I assume it is by design. - PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8201570: Get two bytes for the Linux input event type, not four
On Sat, 27 Jun 2020 00:12:41 GMT, John Neffenger wrote: > Fixes [JDK-8201570](https://bugs.openjdk.java.net/browse/JDK-8201570). This fixes a bug that currently didn't cause any harm (https://bugs.openjdk.java.net/browse/JDK-8201570?focusedCommentId=14172176) , but it's better to fix it now. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/257
Re: RFR: 8201568: zForce touchscreen input device fails when closed and immediately reopened
On Sat, 27 Jun 2020 00:21:06 GMT, John Neffenger wrote: > Fixes [JDK-8201568](https://bugs.openjdk.java.net/browse/JDK-8201568). The rationale makes sense (open/open/close) instead of (open/close/open) - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/258
Re: RFR: 8244212: Optionally download media and webkit libraries from latest openjfx EA build
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Thu, 30 Apr 2020 18:56:40 GMT, Jesper Skov wrote: > I have tested on Linux (Fedora 31) only. > It works as intended (with one test failure due to 15-ea+4 being too old now). > > UPDATE_STUB_CACHE suggests there is some magic available to help manage the > stub cache. > But as I could not find anything about it, that section in the documentation > may need some love. Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/202
