[11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Laurent Bourgès]

Kevin,

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
modified.
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",
> "true");
>  209
>  210         // enable subdivider:
>  211         System.setProperty("prism.marlin.clip.subdivider", "true");
>  212
>  213         // disable min length check: always subdivide curves at clip
> edges
>  214 System.setProperty("prism.marlin.clip.subdivider.minLength", "-1");
>  215
>  216         // If any curve, increase curve accuracy:
>  217         // curve length max error:
>  218         System.setProperty("prism.marlin.curve_len_err", "1e-4");
>  219
>  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
>  223
>  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 ?) ?

Laurent



>
> On 6/8/2018 7:28 AM, Laurent Bourgès wrote:
>
>> Hi,
>>
>> 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/ <
>> http://cr.openjdk.java.net/%7Elbourges/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:
>> https://bugs.openjdk.java.net/browse/JDK-8198885
>>
>> 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
>> test.com.sun.marlin.ClipShapeTest
>>
>> Regards,
>> Laurent
>>
>
>