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.gra...@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
