Hi Jim, Would you have time to look at this 2nd webrev ?
Laurent Le 26 avr. 2017 11:32 PM, "Laurent Bourgès" <bourges.laur...@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.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 >
