[11] JDK-8204621: Upgrade MarlinFX to 0.9.2

Fri Jun 29 18:25:40 UTC 2018

One more thing about the test. All of the OpenJFX unit tests should have 
GPL v2 + Classpath Exception (this differs from the JDK).

-- Kevin

On 6/29/2018 10:23 AM, Kevin Rushforth wrote:
>
> I'll plan to review the code today if possible. This will need one 
> more reviewer, so maybe Phil can also review it, since he reviewed the 
> Java2D patch?
>
> As for my comments on the test:
>
>> Finally I think this test should be manually run only if Marlin 
>> renderer is modified.
>> How to do that ? use @Ignore or specific tags ?
>
> As a slight variation of this: How about running a small number (say, 
> 200 or 250) by default, but adding a flag to run more? Alternatively, 
> you could use a flag to enable it, but since you would need to do 
> something extra (provide a flag or modify the test) to run it, we 
> might as well get at least some testing all the time. Unless you 
> really think there is no value in doing this.
>
>> 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.
>
> As a best practice, tests should generally be run using the same 
> settings as are used in production. Other than to verify how it 
> behaves when you change these settings, I don't see the value in 
> testing the system running in a mode that no application will ever 
> see. I may be missing some point here.
>
> -- Kevin
>
>
> On 6/25/2018 9:01 AM, Laurent Bourgès wrote:
>> Kevin,
>>
>> Here are my comments below:
>>
>> 2018-06-16 1:47 GMT+02:00 Kevin Rushforth <kevin.rushforth at oracle.com 
>> <mailto: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
>>         <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/>
>> <http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.0/
>> <http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.0/>>
>>         PR: https://github.com/javafxports/openjdk-jfx/pull/96
>> <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
>>         <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
>>
>>
>>
>