Re: RFR: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread [v2]
On Tue, 30 Jun 2020 06:05:08 GMT, John Neffenger
wrote:
>> At first glance the fix looks fine (aside from the `assert` statements that
>> need to be removed). It will also need to
>> be tested using the SW pipeline on all platforms.
>
>> It will also need to be tested using the SW pipeline on all platforms.
>
> Thanks for the reminder. I managed to build JavaFX on Windows and macOS
> today, so I'll test this pull request on those
> platforms in addition to Linux desktop and embedded.
I tested this pull request on all of the following platforms:
* JavaFX desktop platforms (*amd64* architecture)
* Windows SDK on Windows 10 Pro Version 2004
* Mac OS X SDK on macOS 10.15.5 (Catalina)
* Linux SDK on Ubuntu 16.04.6 LTS (Xenial Xerus)
* JavaFX embedded platforms (*armhf* architecture)
* armv6hf SDK (Monocle EPD) on Ubuntu 14.04.6 LTS (Trusty Tahr)
* armv6hf SDK (Monocle VNC) on Ubuntu 20.04 LTS (Focal Fossa)
This is a tricky timing problem on most platforms, so I used rather intrusive
techniques to catch the error and verify
it was fixed. The bug is easily visible only on the Monocle VNC platform.
### Desktop
I tested the desktop platforms as follows:
1. I added `assert` statements and calls to `Thread.sleep` as shown by [this
commit](https://github.com/jgneff/javafx-graphics/commit/ffc639e4) to catch the
error. It looks like a lot of changes,
but they simply call `sleep` to change the timing of the threads so that the
JavaFX Application Thread is unable to
keep up with the QuantumRenderer. The `assert` statements catch the error when
the rendering thread starts looking for
an unused buffer.
All platforms printed many `InvalidMarkException` errors as the buffer
position was modified while in use on the JavaFX
application thread. The Linux and Windows platforms printed, "Exception in
thread 'JavaFX Application Thread'
java.nio.InvalidMarkException," while the macOS platform printed,
"Exception in thread 'AppKit Thread'
java.nio.InvalidMarkException."
2. I applied the fix to call `getBuffer` instead of `getPixels` in
`QueuedPixelSource`.
3. After the fix, no errors were detected by the assertions on any of the
platforms.
### Embedded
I tested the embedded platforms as follows:
1. I added only `assert` statements to the `HeadlessScreen` and `EPDScreen`
classes, as shown below. Both platforms
printed many `InvalidMarkException` errors as the buffer position was modified
while in use on the JavaFX application
thread.
2. I applied the fix to call `getBuffer` instead of `getPixels` in
`QueuedPixelSource`.
3. After the fix, no errors were detected by the assertions on either platform.
`HeadlessScreen` and `EPDScreen`
@Override
public synchronized void uploadPixels(Buffer b, int x, int y,
int width, int height, float alpha) {
+assert b.mark() == b;
pixels.composePixels(b, x, y, width, height, alpha);
+assert b.reset() == b;
}
For the Monocle VNC platform, you can also simply connect and watch the bug
corrupt the frames sent to the VNC client,
as shown in my prior comment on this pull request.
### Other results
I found some unexpected results, listed below.
* JavaFX on Linux does not limit its frame rate to 60 Hz when using the
hardware pipeline, reaching over 350 frames per
second.
* JavaFX on macOS ignores the system property `-Djavafx.animation.pulse=8` and
runs at 60 Hz, even though it prints the
message "Setting PULSE_DURATION to 8 hz."
* JavaFX on macOS prints the error shown below when `Platform.exit` is called
to end the application. The error also
occurs on JavaFX 14.0.1 and 15-ea+6. The error does not occur when the window
is closed manually using the mouse.
Java has been detached already, but someone is still trying to use it at
-[GlassViewDelegate dealloc]:
/Users/john/src/jfx/modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m:204
0 libglass.dylib 0x00010eb6c05d -[GlassViewDelegate dealloc] + 221
1 libglass.dylib 0x00010eb71af6 -[GlassView3D dealloc] + 246
2 libobjc.A.dylib 0x7fff66937054
_ZN19AutoreleasePoolPage12releaseUntilEPP11objc_object + 134
3 libobjc.A.dylib 0x7fff6691bdba objc_autoreleasePoolPop + 175
4 CoreFoundation 0x7fff2dad23c5
__CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 23
5 CoreFoundation 0x7fff2dad22f7 __CFRunLoopDoObservers + 457
6 CoreFoundation 0x7fff2dad1895 __CFRunLoopRun + 874
7 CoreFoundation 0x7fff2dad0ece CFRunLoopRunSpecific + 462
8 libjli.dylib 0x00010be945c1 CreateExecutionEnvironment + 399
9 libjli.dylib 0x00010be90752 JLI_Launch + 1354
10 java 0x00010be83ca1 main + 375
11 libdyld.dylib0x7fff67acdcc9 start + 1
12 ??? 0x000b 0x0 + 11
### Test scripts
Below are the scripts I used to run the tests. The scripts include the system
property
Re: RFR: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread
On 6/25/20 10:56 PM, John Neffenger wrote: Fixes [JDK-8201567](https://bugs.openjdk.java.net/browse/JDK-8201567). - Commit messages: - 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread Changes: https://git.openjdk.java.net/jfx/pull/255/files Webrev: https://webrevs.openjdk.java.net/jfx/255/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8201567 Stats: 35 lines in 5 files changed: 29 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/255.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/255/head:pull/255 PR: https://git.openjdk.java.net/jfx/pull/255 Does this fix the years old Linux JavaFX buffer reset bug?
Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException [v6]
On Tue, 16 Jun 2020 09:03:35 GMT, Robert Lichtenberger wrote: >> This PR fixes JDK-8176270 by clamping the end index of the selected text to >> the length of the text. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8176270: Adding ChangeListener to TextField.selectedTextProperty causes > StringOutOfBoundsException > > Move replaceSelectionAtEndWithListener test to general test class. > Add a more general test for selection/text properties and replace/undo/redo > operations. Changing from using bindings to using listeners seems reasonable as long as there can't be a memory leak as a result (it should be fine). As I note inline below, I think the clamping is still wrong. Maybe you can't ever hit a case where it matters now, but it should still be fixed. modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java line 186: > 185: int length = txt.length(); > 186: if (end > start + length) end = length; > 187: if (start > length-1) start = end = 0; As I mentioned in [this comment in PR #73](https://github.com/openjdk/jfx/pull/73#discussion_r376528686), I think this test is wrong. The value of `start` has no impact on whether `end` is out of range. So... shouldn't this simply be `if (end > length) end = length;` ? - PR: https://git.openjdk.java.net/jfx/pull/138
Re: RFR: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread [v2]
> Fixes [JDK-8201567](https://bugs.openjdk.java.net/browse/JDK-8201567). John Neffenger has updated the pull request incrementally with one additional commit since the last revision: Remove assert statements - Changes: - all: https://git.openjdk.java.net/jfx/pull/255/files - new: https://git.openjdk.java.net/jfx/pull/255/files/47ad942a..d408506a Webrevs: - full: https://webrevs.openjdk.java.net/jfx/255/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/255/webrev.00-01 Stats: 21 lines in 3 files changed: 5 ins; 14 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/255.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/255/head:pull/255 PR: https://git.openjdk.java.net/jfx/pull/255
Re: RFR: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread
On Sun, 28 Jun 2020 02:35:16 GMT, John Neffenger
wrote:
>> modules/javafx.graphics/src/main/java/com/sun/glass/ui/Pixels.java line 194:
>>
>>> 193: public final Buffer getBuffer() {
>>> 194: assert this.bytes != null || this.ints != null;
>>> 195: return this.bytes != null ? this.bytes : this.ints;
>>
>> We should not use `assert` statements in the JavaFX runtime. If this is a
>> condition that needs to be checked, then use
>> an `if` test and throw an appropriate `RuntimeException` or an
>> `InternalError`.
>
> Thanks, Kevin. I saw `assert` used elsewhere (`EventLoop` in the same
> package, for example), and thought it was
> okay—even preferable—for these "should never occur" checks. Is that a general
> policy not to use `asserts` anywhere in
> JavaFX? Should I also remove the asserts I added to `HeadlessScreen` and
> `EPDScreen` in lieu of a unit test case?
I don't know whether it is documented, but we did at one point have a policy to
avoid them and removed a few elsewhere
in glass (which is where most of the usage was). The main reason is that when
running applications that want to use
assertion checking themselves it's too easy for a stray assert in a library to
cause problems if we don't test for it.
In case it really is an invariant, and nothing that could be provoked by API
calls (which I think this case is), then
checking explicitly seems best.
I wouldn't recommend removing any existing `assert` statements, but we could
file a follow-up issue to remove them
-
PR: https://git.openjdk.java.net/jfx/pull/255
Re: RFR: 8248317: Change JavaFX release version to 16
On Tue, 30 Jun 2020 15:30:31 GMT, Kevin Rushforth wrote: > Bump the version number of JavaFX to 16. I will integrate this immediately > after forking the `jfx15` stabilization > branch. Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/260
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v14]
On Tue, 30 Jun 2020 09:44:33 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:
>
> Prevent attempt to render tiles with a 0 sized dimension.
The code looks good to me too, suggested minor change.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1638:
> 1637: renderWholeImage(x, y, w, h, rf, pImage);
> 1638: }
> 1639: params.platformImage = pImage;
I tried to write this code using for loop to make it little easy for reader to
understand. This is just a suggestion, I
leave it to you to whether replace it or not,
int mTileWidth = computeTileSize(w, maxTextureSize);
int mTileHeight = computeTileSize(h, maxTextureSize);
IntBuffer buffer = IntBuffer.allocate(mTileWidth * mTileHeight);
int mTileXOffset = 0;
int mTileYOffset = 0;
for (mTileXOffset = 0; (mTileXOffset + mTileWidth) <= w; mTileXOffset
+= mTileWidth) {
for (mTileYOffset = 0; (mTileYOffset + mTileHeight) <= h;
mTileYOffset += mTileHeight) {
renderTile(x, mTileXOffset, y, mTileYOffset,
mTileWidth, mTileHeight,
buffer, rf, tileRttCache, pImage);
}
}
int rTileXOffset = mTileXOffset;
int rTileWidth = w - rTileXOffset;
if (rTileWidth > 0) {
for (int rTileYOffset = 0; (rTileYOffset + mTileHeight) <= h;
rTileYOffset += mTileHeight) {
renderTile(x, rTileXOffset, y, rTileYOffset,
rTileWidth, mTileHeight,
buffer, rf, tileRttCache, pImage);
}
}
int bTileYOffset = mTileYOffset;
int bTileHeight = h - bTileYOffset;
if (bTileHeight > 0) {
for (int bTileXOffset = 0; (bTileXOffset + mTileWidth) <= w;
bTileXOffset += mTileWidth) {
renderTile(x, bTileXOffset, y, bTileYOffset,
mTileWidth, bTileHeight,
buffer, rf, tileRttCache, pImage);
}
}
if (rTileWidth > 0 && bTileHeight > 0) {
renderTile(x, rTileXOffset, y, bTileYOffset, rTileWidth,
bTileHeight,
buffer, rf, tileRttCache, pImage);
}
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1564:
> 1563: if (h > maxTextureSize || w > maxTextureSize) {
> 1564: // The requested size for the screenshot is too
> big to fit a single texture,
> 1565: // so we need to take several snapshot tiles
> and merge them into pImage
Very minor: screenshot should be changed to snapshot.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1635:
> 1634: else {
> 1635: // The requested size for the screenshot fits
> max texture size,
> 1636: // so we can directly render it in the target
> image.
Very minor: screenshot should be changed to snapshot.
-
PR: https://git.openjdk.java.net/jfx/pull/112
Integrated: 8247947: Build DirectShow Samples (Base Classes) from source checked into repo
On Thu, 25 Jun 2020 23:19:05 GMT, Alexander Matveev wrote: > - Added DirectShow baseclasses to repository. > - Dependency on Windows SDK 7.1 DirectShow baseclasses was removed. This pull request has now been integrated. Changeset: 62f8cee7 Author:Alexander Matveev URL: https://git.openjdk.java.net/jfx/commit/62f8cee7 Stats: 38685 lines in 73 files changed: 8 ins; 38674 del; 3 mod 8247947: Build DirectShow Samples (Base Classes) from source checked into repo Reviewed-by: kcr, arapte - PR: https://git.openjdk.java.net/jfx/pull/254
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v14]
On Tue, 30 Jun 2020 09:44:33 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:
>
> Prevent attempt to render tiles with a 0 sized dimension.
I've finished my testing on 4 platforms: Windows 10, macOS 10.15, Ubuntu 16.10
(physical), Ubuntu 20.04 (VM with
forceGPU set to true). All looks good.
The code looks good too with a couple questions below.
tests/system/src/test/java/test/javafx/scene/Snapshot2Test.java line 31:
> 30: import java.util.concurrent.ThreadLocalRandom;
> 31: import java.util.stream.IntStream;
> 32:
These are now unused imports.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1569:
> 1568: }
> 1569: // Find out if it is possible to divide up the
> image in tiles of the same size
> 1570: int tileWidth = computeTileSize(w,
> maxTextureSize);
Very minor: this comment seems like a hold-over from when the method was
computing an optimum size. Now the caller
doesn't know about "tiles of the same size".
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java
line 1557:
> 1556: // A temp QuantumImage used only as a RTT cache for
> rendering tiles.
> 1557: var tileRttCache = new
> QuantumImage((com.sun.prism.Image) null);
> 1558: try {
In the case of a snapshot that isn't tiled, you will end up creating and
disposing a dummy `QuantumImage` that is never
used. Maybe it would be worth initializing it to null here, constructing the
object in the `if (h > maxTextureSize ||
w > maxTextureSize)` block, and checking for null before disposing in the
finally block?
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
On Wed, 17 Jun 2020 07:32:46 GMT, Prasanta Sadhukhan wrote: >> Oliver Schmidtmer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Change to Math.ceil and add test > > In 2D, we normally use sun.java2d.pipe.Region.clipRound as it also checks for > -ve/+ve max INTEGER but I guess that is > internal class to FX so it's ok to use Math.round. Approval pending test > creation. While both might work, as long as there is no mixed usage of round and ceil, Math.ceil seems to be better. I'm not sure if the timed waiting for the resizes is the best way for ensuring that the buffer is resized to the new bounds, so I'm open for suggestions. To me at least it seems to create a reproducible sheared output after the second resize in the test case and not anymore after changing the calculations to Math.ceil. - PR: https://git.openjdk.java.net/jfx/pull/246
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
> In edge cases where monitor scaling of 1.25 or 1.75 is active, Math.ceil and > Math.round produce different results and > EmbeddedScene#getPixels in JFXPanel#paintComponent causes an off-by-one error > on the line width and therefore sheared > rendering. The changes were already proposed by the submitter in JBS-8220484. > > OCA is signed and send. Oliver Schmidtmer has updated the pull request incrementally with one additional commit since the last revision: Change to Math.ceil and add test - Changes: - all: https://git.openjdk.java.net/jfx/pull/246/files - new: https://git.openjdk.java.net/jfx/pull/246/files/5eda49bf..994ca03c Webrevs: - full: https://webrevs.openjdk.java.net/jfx/246/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/246/webrev.00-01 Stats: 161 lines in 3 files changed: 155 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/246.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/246/head:pull/246 PR: https://git.openjdk.java.net/jfx/pull/246
Re: RFR: 8248317: Change JavaFX release version to 16
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 Tue, 30 Jun 2020 15:30:31 GMT, Kevin Rushforth wrote: > Bump the version number of JavaFX to 16. I will integrate this immediately > after forking the `jfx15` stabilization > branch. @arapte please review - PR: https://git.openjdk.java.net/jfx/pull/260
RFR: 8248317: Change JavaFX release version to 16
Bump the version number of JavaFX to 16. I will integrate this immediately after forking the `jfx15` stabilization branch. - Commit messages: - 8248317: Change JavaFX release version to 16 Changes: https://git.openjdk.java.net/jfx/pull/260/files Webrev: https://webrevs.openjdk.java.net/jfx/260/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8248317 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/260.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/260/head:pull/260 PR: https://git.openjdk.java.net/jfx/pull/260
Re: RFR: 8247947: Build DirectShow Samples (Base Classes) from source checked into repo
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, 25 Jun 2020 23:19:05 GMT, Alexander Matveev wrote: > - Added DirectShow baseclasses to repository. > - Dependency on Windows SDK 7.1 DirectShow baseclasses was removed. Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/254
RFR: 8248551: [TestBug] Ignore two failing FXML unit tests which use Nashorn script engine
Issue : https://bugs.openjdk.java.net/browse/JDK-8248551 Fix : Added a check whether 'Nashorn' script engine is available before running the identified tests, otherwise tests are ignored. Testing : 1) With JDK-14, these tests pass- but they log a warning (same as existing behavior without this PR) 2) With JDK-15-ea, these tests get ignored. - Commit messages: - check for Nashorn engine Changes: https://git.openjdk.java.net/jfx/pull/259/files Webrev: https://webrevs.openjdk.java.net/jfx/259/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8248551 Stats: 19 lines in 1 file changed: 18 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/259.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/259/head:pull/259 PR: https://git.openjdk.java.net/jfx/pull/259
Integrated: 8248551: [TestBug] Ignore two failing FXML unit tests which use Nashorn script engine
On Tue, 30 Jun 2020 11:11:12 GMT, Ajit Ghaisas wrote: > Issue : https://bugs.openjdk.java.net/browse/JDK-8248551 > Fix : Added a check whether 'Nashorn' script engine is available before > running the identified tests, otherwise tests > are ignored. > Testing : > 1) With JDK-14, these tests pass- but they log a warning (same as existing > behavior without this PR) > 2) With JDK-15-ea, these tests get ignored. This pull request has now been integrated. Changeset: 527cc2b0 Author:Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/527cc2b0 Stats: 19 lines in 1 file changed: 0 ins; 18 del; 1 mod 8248551: [TestBug] Ignore two failing FXML unit tests which use Nashorn script engine Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/259
Re: RFR: 8248551: [TestBug] Ignore two failing FXML unit tests which use Nashorn script engine
On Tue, 30 Jun 2020 11:11:12 GMT, Ajit Ghaisas wrote: > Issue : https://bugs.openjdk.java.net/browse/JDK-8248551 > Fix : Added a check whether 'Nashorn' script engine is available before > running the identified tests, otherwise tests > are ignored. > Testing : > 1) With JDK-14, these tests pass- but they log a warning (same as existing > behavior without this PR) > 2) With JDK-15-ea, these tests get ignored. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/259
Integrated: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts
On Mon, 20 Apr 2020 12:45:30 GMT, Rony G. Flatscher wrote: > This PR adds a "compile" process instruction to FXML files with the optional > PI data "true" (default) and "false". The > PI data is turned into a boolean value using "Boolean.parseBoolean(String)". > This makes it possible to inject the compile PI everywhere in a FXML file and > turn on and off compilation of scripts if > the scripting engine implements the javax.script.Compilable interface. The PR > adds the ability for a fallback in case > compilation of scripts fails, in which case a warning gets issued about this > fact and evaluation of the script will be > done without compilation. Because of the fallback scripts get compiled with > this version by default. > - > ### Progress > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > - [ ] Change must be properly reviewed > > ### Issue > * [JDK-8238080](https://bugs.openjdk.java.net/browse/JDK-8238080): > FXMLLoader: if script engines implement >javax.script.Compilable compile scripts > > > ### Download > `$ git fetch https://git.openjdk.java.net/jfx pull/192/head:pull/192` > `$ git checkout pull/192` This pull request has now been integrated. Changeset: 45c9854c Author:Rony G. Flatscher Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/45c9854c Stats: 2553 lines in 29 files changed: 2 ins; 2523 del; 28 mod 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts Reviewed-by: kcr, aghaisas - PR: https://git.openjdk.java.net/jfx/pull/192
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v14]
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. -- > 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: Prevent attempt to render tiles with a 0 sized dimension. - Changes: - all: https://git.openjdk.java.net/jfx/pull/112/files - new: https://git.openjdk.java.net/jfx/pull/112/files/efccc4d8..c0f7d14f Webrevs: - full: https://webrevs.openjdk.java.net/jfx/112/webrev.13 - incr: https://webrevs.openjdk.java.net/jfx/112/webrev.12-13 Stats: 27 lines in 1 file changed: 12 ins; 8 del; 7 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
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]
On Mon, 29 Jun 2020 22:46:26 GMT, Kevin Rushforth wrote: >> 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. I've pushed the changes discussed above, and verified that the tests now pass on a Ubuntu 20.04 VM with `prism.forceGpu=true` (and that they indeed failed prior to those changes). - PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238954: Improve performance of tiled snapshot rendering [v13]
On Mon, 29 Jun 2020 21:32:33 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.
>
> 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)`
You're right: the comments and variable names are swapped. Will fix it (and
obviously add the extra condition that
prevents attempts to render 0 width/height tiles). Thanks.
-
PR: https://git.openjdk.java.net/jfx/pull/112
Re: RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v6]
Hi Ajit, Kevin looked into it already yesterday. There was some problem at github at the time I submitted the /integrate comment, which merely needs to be reissued by me. Having been on the road I was not able to do it yesterday, will be first thing after arriving at the office today. Also, thank you very much for your review efforts! Best regards —-rony Rony G. Flatscher (mobil/e) > Am 29.06.2020 um 07:56 schrieb Ajit Ghaisas : > > On Sat, 27 Jun 2020 14:07:24 GMT, Rony G. Flatscher > wrote: > >>> This PR adds a "compile" process instruction to FXML files with the >>> optional PI data "true" (default) and "false". The >>> PI data is turned into a boolean value using "Boolean.parseBoolean(String)". >>> This makes it possible to inject the compile PI everywhere in a FXML file >>> and turn on and off compilation of scripts if >>> the scripting engine implements the javax.script.Compilable interface. The >>> PR adds the ability for a fallback in case >>> compilation of scripts fails, in which case a warning gets issued about >>> this fact and evaluation of the script will be >>> done without compilation. Because of the fallback scripts get compiled with >>> this version by default. >>> - >>> ### Progress >>> - [x] Change must not contain extraneous whitespace >>> - [x] Commit message must refer to an issue >>> - [ ] Change must be properly reviewed >>> >>> ### Issue >>> * [JDK-8238080](https://bugs.openjdk.java.net/browse/JDK-8238080): >>> FXMLLoader: if script engines implement >>> javax.script.Compilable compile scripts >>> >>> >>> ### Download >>> `$ git fetch https://git.openjdk.java.net/jfx pull/192/head:pull/192` >>> `$ git checkout pull/192` >> >> Rony G. Flatscher has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Incorporating Kevin's review comments (overlooked some places). > > Marked as reviewed by aghaisas (Reviewer). > > - > > PR: https://git.openjdk.java.net/jfx/pull/192
Re: RFR: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread
On Sat, 27 Jun 2020 15:37:52 GMT, Kevin Rushforth wrote: > It will also need to be tested using the SW pipeline on all platforms. Thanks for the reminder. I managed to build JavaFX on Windows and macOS today, so I'll test this pull request on those platforms in addition to Linux desktop and embedded. - PR: https://git.openjdk.java.net/jfx/pull/255
Re: RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v6]
On Sat, 27 Jun 2020 14:07:24 GMT, Rony G. Flatscher wrote: >> This PR adds a "compile" process instruction to FXML files with the optional >> PI data "true" (default) and "false". The >> PI data is turned into a boolean value using "Boolean.parseBoolean(String)". >> This makes it possible to inject the compile PI everywhere in a FXML file >> and turn on and off compilation of scripts if >> the scripting engine implements the javax.script.Compilable interface. The >> PR adds the ability for a fallback in case >> compilation of scripts fails, in which case a warning gets issued about this >> fact and evaluation of the script will be >> done without compilation. Because of the fallback scripts get compiled with >> this version by default. >> - >> ### Progress >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> - [ ] Change must be properly reviewed >> >> ### Issue >> * [JDK-8238080](https://bugs.openjdk.java.net/browse/JDK-8238080): >> FXMLLoader: if script engines implement >>javax.script.Compilable compile scripts >> >> >> ### Download >> `$ git fetch https://git.openjdk.java.net/jfx pull/192/head:pull/192` >> `$ git checkout pull/192` > > Rony G. Flatscher has updated the pull request incrementally with one > additional commit since the last revision: > > Incorporating Kevin's review comments (overlooked some places). Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/192
Re: RFR: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts [v6]
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 Sat, 27 Jun 2020 14:41:29 GMT, Kevin Rushforth wrote: >> Rony G. Flatscher has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Incorporating Kevin's review comments (overlooked some places). > > Looks good. @kevinrushforth, I see this is not integrated yet due to jcheck failure. No details about failure can be seen (At least I am unable to see any reported failures). Can you please have a look? - PR: https://git.openjdk.java.net/jfx/pull/192
