[OpenJDK 2D-Dev] [10] Marlin2D upgrade 0.7.5

Fri May 5 05:02:03 UTC 2017

Hi Jim,

Would you have time to look at this 2nd webrev ?

Laurent

Le 26 avr. 2017 11:32 PM, "Laurent Bourgès" <bourges.laurent at gmail.com> a
écrit :

> Hi Jim,
>
> Here is an updated webrev:
> http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.1/
>
> My comments below explaining changes:
>
> 2017-04-26 1:47 GMT+02:00 Jim Graham <james.graham at oracle.com>:
>
>> Hi Laurent,
>>
>> - I found some fp constants with no "d" or "f" modifier.  I sent a grep
>> output in a separate email...
>>
>
>     I fixed all true matches except in comments.
>
>
>>
>> - Why does FloatMath.java copy the constants from sun.misc rather than
>> refer to them?
>>   (An alternative is a static import or having local copies that copy by
>>    source-based reference (foo_bar = Foo.bar) rather than copying by
>>    human-cut-and-paste)
>>
>
>     This is needed by MarlinFX as sun.misc.FloatConsts is a class
> inaccessible from javafx.graphics module.
>     I prefer copying these constants (source code) to make Marlin2D /
> MarlinFX consistent and avoid exposing the FloatConsts class to modules.
>
>
>>
>> - Should FloatMath be renamed to FPMath or IEEEMath since it now also
>> handles doubles?
>>
>
>     Agreed and I would prefer FPMath, but it can be done later (52 matches)
>
>
>>
>> - MarlinCache - What was the reason to eliminate the mean crossings
>> calculations from the decision on whether or not to use RLE?
>>
>
>     I changed my mind as this calculation only disables 'RLE' and block
> flag optimization early: it is just a shortcut to avoid testing the real
> number of crossing per pixel row i.e. it only save few tests per row. To
> compute the mean crossings, I had to accumulate the edge height so it costs
> 1 add per edge that represents an overhead for lots of edges. Finally I
> prefer having simplified a bit the code.
>
>
>>
>> - MarlinCache line 548 - why the simpler logic here?
>>
>
>     This is related to modified Renderer calls to copyAARow() to better
> handle half-open intervals:
>         before: [maxX + 2] => now: [maxX + 1]
>         // +1 because alpha [pix_minX; pix_maxX[
>         copyAARow(_alpha, lastY, minX, maxX + 1, useBlkFlags);
>
>
>>
>> - MarlinProperties, getLog2_XY - 0 to 4 is 1 to 16 subpixel locations in
>> X or Y but you list the subpixel limits as 1 to 256.  Also, it looks like
>> the real limits are 0 to 8 which really does work out to 1 to 256.  So, I
>> think the "4" in the comment is incorrect and should be "8"?
>>
>
>     Fixed getSubPixel_Log2_X()
>
>
>>
>> - MarlinProperties - TileSize vs TileWidth.  Is there a reason you
>> haven't created a TileHeight property?  I could see a couple of ways of
>> going here:
>>    - TileSize means height and TileWidth is new which is just odd naming
>>      In this case, I'd make the default for TileWidth be the value for
>> TileSize
>>      otherwise setting tilesize used to set both W&H, but now it only
>> sets H?
>>    - TileSize is legacy and new values are TileWidth and TileHeight
>>      Both default to TileSize if not specified
>> In either case, I would think that TileWidth should default to TileSize?
>>
>
>     Fixed, I adopted the first solution and getTileWidth_Log2() uses
> getTileSize_Log2() to get the default value (W=H)
>
>
>>
>> - MarlinProperties - Inc/Dec - constants don't end with d or f.
>>
>
>     Fixed.
>
>
>>
>> - MarlinProperties - getFloat() takes doubles for def/min/max?
>>
>
>     Fixed.
>
>
>>
>> - MarlinTileGenerator - lines 67,69 - null is the default value for fields
>>
>
>     I prefer to be explicit (netbeans reports a warning): left unchanged.
>
>
>>
>> - MarlinTileGenerator,MarlinRenderer - all of the methods called on rdrF
>> and rdrD have the same signature.  Why not have MarlinRenderer include
>> those methods and then you just need to store a single MarlinRenderer field
>> and be able to manipulate either type...?
>>
>
>     I agree. I tried but benchamrks proved that interface calls method
> were slower than monomorphic calls so I adopted this bimorphic call
> optimization where only 1 type is really used at runtime.
>
>
>>
>> - MarlinTileGenerator.TH_AA_ALPHA_00/FF - seem misnamed, really they are
>> low and high thresholds for filling.  Maybe LO and HI?
>>
>
>     Renamed as TH_AA_ALPHA_FILL_EMPTY & TH_AA_ALPHA_FILL_FULL
>
>
>>
>> - MarlinTileGenerator, line 416,443,508 - Isn't that "skip" instead of
>> "fill"?
>>
>
>     Fixed.
>
>
>>
>> - MarlinTileGenerator, line 420,554,687 - Isn't cx >= aax0?  And isn't x1
>> >= aax0? ???
>>
>
>     Fixed comment:
>     // now: cx >= x0 and cx >= aax0
>
>
>>
>> - MarlinTileGenerator, line 491 - spacing on byte cast
>>
>
>     Fixed.
>
>
>>
>> - MarlinTilegenerator, line 655 - "Clear" or "Fill"?
>>
>
>     Fixed.
>
>
>>
>> - Renderer, line 85,114 - maybe add a line saying that is the result of
>> <name> test?  Did we put that test into the repo anywhere?
>>
>
>     Added comment: 'TestNonAARasterization: cubics/quads' (not in repo,
> only in JBS)
>
>
>>
>> - Renderer, line 1318,1365 - that was fallout from the half-open stuff at
>> 1391, right?
>>
>
>     Exactly, I fixed handling half-open intervals in MarlinFX so it is a
> backport.
>
>
>>
>> - Stroker, line 112 - "* 8"?  Or perhaps "* 6 + 2" since the endpoints
>> are shared?
>>
>
>     Yes endpoints are shared so I fixed the capacity
>
>
>>
>> - Stroker, line 347,376 - it appears to still use m[off],m[off+1] instead
>> of 0,1...?
>>
>
>     Fixed comments.
>
>
>>
>> - Stroker, line 377 - safecomputeMiter -> safeComputeMiter?  Also, this
>> looks to be some sort of "compute something for an offset for quads"
>> method, at least in the way it is used, even if it does look similar to the
>> computeMiter method.
>>
>
>     Renamed the method; it was backported from openpisces (so I did not
> really study the approach)
>
>
>>
>> - Stroker, line 841 - "winden" => "widen"
>>
>
>     Fixed.
>
>
>>
>> - Stroker, lines 839,847,856,866 - there is no p4
>>
>
>     Already mentioned: see https://bugs.openjdk.java.net/
> browse/JDK-8170029
>     To be fixed later
>
>
>>
>> I'm assuming that the D*.java files were all generated from the regular
>> files using a sed script?  I skipped those (for now)...
>>
>
> I also fixed D* files.
>
> Laurent
>
>
>> On 4/22/17 4:28 AM, Laurent Bourgès wrote:
>>
>>> Hi,
>>>
>>> I am pleased to have successfully upgraded my machine with OpenJDK10 and
>>> merged my Marlin 0.7.5 release (dec 2016) with
>>> its Java2d variant.
>>>
>>> Please review this Marlin2D upgrade to Marlin 0.7.5:
>>>
>>> JBS: no bug yet for OpenJDK 10
>>> webrev: http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.0/
>>>
>>> Changes:
>>> - Improved MarlinTileGenerator to efficiently handle asymetric tiles (W
>>> x H) with almost full / empty fills
>>> - Added Marlin Double-precision pipeline
>>> - MarlinFX backports (Dasher, Stroker changes from OpenPisces)
>>> - dead code & few comment removals in Strokers
>>> - fixed all floating-point number literals to be x.0f or x.0d to
>>> simplify the conversion between float & double variants
>>>
>>> As this patch is huge, do you want me to provide webrevs against Float
>>> vs Double pipelines or against OpenJFX10 ?
>>>
>>> PS: I forgot to upgrade the copyright headers to 2017, but will do later
>>>
>>> Cheers,
>>> Laurent
>>>
>>
>
>
> --
> --
> Laurent Bourgès
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170505/d3e07e68/attachment.html>