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

Laurent Bourgès bourges.laurent at gmail.com
Wed Apr 26 21:32:42 UTC 2017


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/20170426/41be9f39/attachment.html>


More information about the 2d-dev mailing list