Bug ID: JDK-8170140 Rendering anomaly with MarlinFX renderer

JDK 9
9Fixed

Changeset: cd2b6db2b2cd Author: lbourges Date: 2016-11-30 17:06 -0800 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/cd2b6db2b2cd 8170140: Rendering anomaly with MarlinFX renderer Reviewed-by: flar, kcr
01-12-2016
+1 I'm good - sorted imports or not - I'll leave it to Kevin to sort them to his satisfaction. ;)
30-11-2016
Certainly. I'll wait for Jim to confirm the .3 patch, fix the imports, and then push it later today.
30-11-2016
Could you fix imports in the test and push the patch as you both agree ?
30-11-2016
I also verified the test on Linux (Ubuntu 14.04) and the new test passes with your fix and fails without it. The test looks fine to me. I might suggest sorting the import statements (I think just the javafx.beans imports), but not a requirement.
30-11-2016
Thanks Jim for the test fix, I only tested on my linux laptop and was looking for such render scale property (javafx newbie). Here is the updated webrev incorporating your changes to the QPathTest class: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170140.3/ The test now fails without the fix and passes with it on ubuntu 16.4.
29-11-2016
I inserted the following 3 lines into the test case (I chose to put them right after the myApp.stage.show() line) and they normalized the rendering to 2.0 on my retina MBP (and should always render with a render scale of 2.0): DoubleProperty rscale = new SimpleDoubleProperty(SCALE); myApp.stage.renderScaleXProperty().bind(rscale); myApp.stage.renderScaleYProperty().bind(rscale); I removed all other uses of SCALE (the scale properties on the path and the scaling of the width/height of the scene) and just used those lines to scale the rendering. The test now fails without the fix and passes with it on my retina MBP...
28-11-2016
With respect to the fix itself, the fix looks good to me and it solves the problem with the test case attached to this report. Unfortunately the test in the patch passes whether or not I have the fix applied when I run it on my retina MBP. The reason for this is that the test renders the shape at a scale of 2.0, but the window already has a scale of 2.0 due to the retina screen and thus the final rendering is at 4.0 where the bug does not exhibit. If I set the SCALE to 1.0 the it reproduces on my MBP, but it would probably not reproduce on a non-HiDPI screen. The best way around this would be to set the render scale instead. In order to make sure that the system does not override the render scale due to a system DPI message you would have to bind the property to something that is hard-coded at 2.0. I want Kevin to look at the test case as well, since he is more familiar with all of the i's that need dotting and the t's that need crossing for unit tests. In particular, noting that system properties are being set in the test.
28-11-2016
Jim, you have understood the problem and the fix perfectly with all its subtle details. Thanks for your help.
28-11-2016
Let me make sure I understand the fix fully. The last interesting shape crossing is in the block associated with ato. blkE is set to the block number following the block associated with ato. (It's an exclusive final block.) blkE-1 is the last block that has "interesting alpha deltas" in that there are potential edge crossings in there which are inside the renderable portion of the shape. You need to be sure to process block blkE-1. blkE might have some alpha deltas in it - most commonly when the shape ends on the last pixel which happens to be at the block boundary, the final in-to-out crossing will cause a flag to be set at blkE. Also, clipping might cause us to accumulate some alphas deltas there as well. But, blkE does not need to be evaluated, it is only there because we sometimes process an edge crossing beyond the right edge of the clip region. Blocks are processed lazily. We find a block flag that is set, then find the following block flag that isn't set and then process all blocks between them. As a result, alpha deltas are only accumulated when we find an unset block flag. If the area of interest ends with a sequence of "all block flags set" then we never find a zero block flag to tell us to process all of those blocks and accumulate the deltas. If blkE has an alpha delta in it, its block flag will be set and thus we will end on a lazily-unprocessed set of blocks. The fix is to set the block flag for blkE to 0 since it doesn't have any interesting deltas in it and it may be needed to trigger the delta processing in block blkE-1. So, the entire fix is that one line that sets the block flag to zero. But, this fix also pointed out that we post-fill the last alpha we found in all cases, but it may be a zero so you added a test around that post-fill to make sure we weren't filling a bunch of zeroed values with more zeroes...? The tests for curAlpha aren't strictly necessary, but improve the optimization of the block flags in the wake of fixing this bug.
28-11-2016
Jim, you were right ! I managed reproducing the case you mentioned (q letter overlapping the clip's right boundary) and I agree the final loop must be rewritten. Here is the updated webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170140.2/
28-11-2016
My concern is that the right edge of the rendering span could be defined by the right edge of the shape in which case it is true that curAlpha will have resolved to 0, but it could also be defined by the right edge of the clip and so the last alpha value rendered could be an unresolved non-0 alpha value. You assume curAlpha will be 0 if you've reached the end of the alpha array, but it could also be non-zero if we reached the end of the array because further fillable areas of the shape were clipped out. A do-while will execute one more alpha sample fill before it realizes that it was already at the end of the range it should have been filling...
24-11-2016
Here is the updated webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170140.1/ The only change consist in using Utils.TIMEOUT (=5s) to let enough time to start the application in the QPathTest. About loops: the main loop makes the running sum = curAlpha but also fills (large) spans in between (copied from the RLE approach in Marlin2D) so there is also a (possible) remainder if curAlpha != 0. I added a new test to avoid filling 0 that is already handled by the final loop = right-side zero fill. Happy thanksgiving, Laurent
24-11-2016
In the junit test, I can increase the launch timeout (now 1s) that is probably to short. Will look at other tests to get an appropriate timeout value. Could you indicate more clearly which loop(s) should be rewritten ? Maybe we should discuss on the review thread.
24-11-2016
The patch does seem to fix the test case. I am wondering why you use a do/while loop in the code that fills the final alpha level rather than a while loop. How can you be sure that the remainder is non-empty?
23-11-2016
The unit test fails to launch for me. I get: test.com.sun.marlin.QPathTest > classMethod FAILED junit.framework.AssertionFailedError: Timeout waiting for Application to launch at test.com.sun.marlin.QPathTest.setupOnce(QPathTest.java:121)
23-11-2016
Fix understood and first webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170140.0/ Webrev thread: http://mail.openjdk.java.net/pipermail/openjfx-dev/2016-November/019971.html
23-11-2016
I can confirm that the NoRLE flag works around it (reproduces with false, but not with true).
23-11-2016
Thanks Jim, I successfully reproduced the problem: it seems related to the block flag optimization. Workaround: use prism.marlin.forceNoRLE=false to fix the issue (note: the property name is wrong as it disables the block flag optimization not RLE that is not applicable to marlinFX) javafx -Dglass.gtk.uiScale=2.0 -Dprism.marlinrasterizer=true -Dprism.verbose=true -Dprism.marlin.log=true -Dprism.marlin.doChecks=true -Dprism.marlin.forceNoRLE=true QPathTest Will diagnose the bug asap...
23-11-2016
To test DPI scaling on Windows you can use -Dglass.win.uiScale=2.0 On Linux you can use -Dglass.gtk.uiScale=2.0 (upper/lowercase is important) I tried it on Windows and Ubuntu 14.04 and it reproduced with 2.0 and 3.0, but not 1.0 nor a variety of non-integer scales.
22-11-2016
Running with the logging parameters above shows a number of errors of the form: WARNING: Invalid value at: 157 = 32 from: 157 to: 193
22-11-2016
Attaching a standalone case that demonstrates the problem on Mac, Windows and Ubuntu 14.04 all with the ES2 and/or D3D pipelines (ES2 tested on all 3, D3D only tested on Windows). It does not reproduce with the SW pipeline, though.
22-11-2016
Jim, I tried on my ubuntu 16.04 with GDK_SCALE=2 but did not see any artefact, except some tearing (VSYNC issue with nvidia cards). Could you add a screenshot showing the problem and test with checks enabled: -Dprism.marlin.log=true -Dprism.marlin.doChecks=true It will detect any issue related to partial array cleanup. Finally is there any jvm setting to test dpi scaling issue ?
22-11-2016
On Windows 10 I wasn't able to reproduce the issue with the "q" character, but the dot at the top of the "j" character was showing similar rendering anomalies. It is worth noting that these are running on a retina MBP with the MacOS install running at the default 2.0 rendering scale and the Win10 install running with a 175% scale. Running the Win10 version with a forced 2.0 scale showed no anomalies for any of the lower case characters, even the "q" that was causing problems under MacOS, so the issue is very depending on scaling and possibly the way that the platforms produce the character outlines.
21-11-2016