JDK-8204621: Upgrade MarlinFX to 0.9.2
bourges.laurent at gmail.com
Mon Jun 25 16:01:10 UTC 2018
Here are my comments below:
2018-06-16 1:47 GMT+02:00 Kevin Rushforth <kevin.rushforth at oracle.com>:
> I tested this on all three platforms and the updated rasterizer looks good.
> I spot checked the code changes, but didn't get time to do a complete
> review yet. I was mostly looking for diffs between the Java2D version which
> was already reviewed, and this one.
> I do have a couple comments on the new ClipShapeTest (which looks like a
> nice accuracy test, btw).
> 1. The test runs for way too long (about 20x too long) to include in our
> normal test runs. By default the entire class file (all three tests) needs
> to take < 5 minutes and 2 minutes would be better. I measured the time on 4
> machines that I have and found that if you cut the number of iterations
> down from 5000 to 250 it will be just about the right run time. Then you
> can set the timeout to 120 seconds (the slowest test on the slowest of my
> machines took about 48 seconds, so a 2 minute timeout should be plenty).
I agree this test is very long but it is the only mean I found to test all
possible stroke combinations and test enough shapes (5000) to detect bugs.
I wondered if using mask directly (via ShapeUtils.getMaskData()) would
become faster but it will never run below the 2 minutes threshold in total.
Finally I think this test should be manually run only if Marlin renderer is
How to do that ? use @Ignore or specific tags ?
> 2. Can you explain the reason for setting the following?
> 206 // disable static clipping setting:
> 207 System.setProperty("prism.marlin.clip", "false");
> 208 System.setProperty("prism.marlin.clip.runtime.enable",
> 210 // enable subdivider:
> 211 System.setProperty("prism.marlin.clip.subdivider", "true");
> 213 // disable min length check: always subdivide curves at clip
> 214 System.setProperty("prism.marlin.clip.subdivider.minLength", "-1");
> 216 // If any curve, increase curve accuracy:
> 217 // curve length max error:
> 218 System.setProperty("prism.marlin.curve_len_err", "1e-4");
> 220 // cubic min/max error:
> 221 System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
> 222 System.setProperty("prism.marlin.cubic_inc_d1", "1e-4"); //
> or disabled ~ 1e-6
> 224 // quad max error:
> 225 System.setProperty("prism.marlin.quad_dec_d2", "5e-4");
> It seems better to test with the default parameters (i.e., it makes a
> better regression test that way).
I deliberately set all these Marlin clip (runtime + always subdivider) /
curve quality settings (quads / cubics thresholds) to be sure of the
concrete Marlin setup as quality thresholds are sensitive to such values.
The ClipShapeTest is dedicated to test the clipper (+ subdivider) part of
the Marlin renderer.
> 3. Related to that, I think you should eliminate the following (I don't
> recommend running functional tests with this set), although since you don't
> do any animation, it probably doesn't matter.
> 227 System.setProperty("javafx.animation.fullspeed", "true"); //
> full speed
I will remove it and see if the overall test is not slower.
Is Platform.runLater() impacted by such setting (latency of FX thread ->
Prism rendering thread ?) ?
> On 6/8/2018 7:28 AM, Laurent Bourgès wrote:
>> Please review this large patch to upgrade MarlinFX to 0.9.2 in OpenJFX11:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8198885
>> webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.0/ <
>> PR: https://github.com/javafxports/openjdk-jfx/pull/96 (CI OK)
>> This patch is almost identical to Marlin(2D) patch, see:
>> I added the ClipShapeTest (ported to jfx) that compares shape clipping
>> (within threshold) and it works (within large timeouts):
>> gradle -PFULL_TEST=true :system:test --tests
More information about the openjfx-dev