[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2
From: Kevin Rushforth <kevin.rushforth () oracle ! com>
Date: 2018-06-29 20:52:07
Message-ID: 646800cc-0ffd-3fe8-5cd0-67d6cc59eddb () oracle ! com
[Download RAW message or body]
I'm giving a +1 on the implementation changes. I scanned the webrev and
didn't see anything out of place. I compared the diffs of the FX Marlin
0.9.2 with the Java2D 0.9.1 changeset, and there were a few more diffs
than I might have expoected, but nothing jumped out of me as a problem.
Also, I've tested it pretty well on all three platforms.
The overall +1 is pending the fixes needed for the test: at least the
copyright header and shortening or disabling the test.
-- Kevin
On 6/29/2018 11:25 AM, Kevin Rushforth wrote:
> 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@oracle.com <mailto:kevin.rushforth@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
>>>
>>>
>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic