<div dir="auto">Hi Jim,<div dir="auto"><br><div dir="auto">Would you have time to look at this 2nd webrev ?</div><div dir="auto"><br></div><div dir="auto">Laurent</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">Le 26 avr. 2017 11:32 PM, "Laurent Bourgès" <<a href="mailto:bourges.laurent@gmail.com">bourges.laurent@gmail.com</a>> a écrit :<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>Hi Jim,<br><br></div>Here is an updated webrev:<br><a href="http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.1/" target="_blank">http://cr.openjdk.java.net/~<wbr>lbourges/marlin/Marlin-075.1/</a><br><br></div>My comments below explaining changes:<br><div><div><div><div><div class="gmail_extra"><br><div class="gmail_quote">2017-04-26 1:47 GMT+02:00 Jim Graham <span dir="ltr"><<a href="mailto:james.graham@oracle.com" target="_blank">james.graham@oracle.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Laurent,<br>
<br>
- I found some fp constants with no "d" or "f" modifier.  I sent a grep output in a separate email...<br></blockquote><div><br>    I fixed all true matches except in comments.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Why does FloatMath.java copy the constants from sun.misc rather than refer to them?<br>
  (An alternative is a static import or having local copies that copy by<br>
   source-based reference (foo_bar = Foo.bar) rather than copying by<br>
   human-cut-and-paste)<br></blockquote><div><br>    This is needed by MarlinFX as sun.misc.FloatConsts is a class inaccessible from javafx.graphics module.<br>    I prefer copying these constants (source code) to make Marlin2D / MarlinFX consistent and avoid exposing the FloatConsts class to modules.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Should FloatMath be renamed to FPMath or IEEEMath since it now also handles doubles?<br></blockquote><div><br>    Agreed and I would prefer FPMath, but it can be done later (52 matches)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinCache - What was the reason to eliminate the mean crossings calculations from the decision on whether or not to use RLE?<br></blockquote><div><br>    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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinCache line 548 - why the simpler logic here?<br></blockquote><div><br>    This is related to modified Renderer calls to copyAARow() to better handle half-open intervals:<br>        before: [maxX + 2] => now: [maxX + 1]<br>        // +1 because alpha [pix_minX; pix_maxX[<br>        copyAARow(_alpha, lastY, minX, maxX + 1, useBlkFlags);<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- 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"?<br></blockquote><div><br>    Fixed getSubPixel_Log2_X() <br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- 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:<br>
   - TileSize means height and TileWidth is new which is just odd naming<br>
     In this case, I'd make the default for TileWidth be the value for TileSize<br>
     otherwise setting tilesize used to set both W&H, but now it only sets H?<br>
   - TileSize is legacy and new values are TileWidth and TileHeight<br>
     Both default to TileSize if not specified<br>
In either case, I would think that TileWidth should default to TileSize?<br></blockquote><div><br>    Fixed, I adopted the first solution and getTileWidth_Log2() uses getTileSize_Log2() to get the default value (W=H)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinProperties - Inc/Dec - constants don't end with d or f.<br></blockquote><div><br>    Fixed.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinProperties - getFloat() takes doubles for def/min/max?<br></blockquote><div><br>    Fixed.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinTileGenerator - lines 67,69 - null is the default value for fields<br></blockquote><div><br>    I prefer to be explicit (netbeans reports a warning): left unchanged.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinTileGenerator,MarlinRend<wbr>erer - 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...?<br></blockquote><div><br>    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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinTileGenerator.TH_AA_ALPH<wbr>A_00/FF - seem misnamed, really they are low and high thresholds for filling.  Maybe LO and HI?<br></blockquote><div><br>    Renamed as TH_AA_ALPHA_FILL_EMPTY & TH_AA_ALPHA_FILL_FULL<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinTileGenerator, line 416,443,508 - Isn't that "skip" instead of "fill"?<br></blockquote><div><br>    Fixed.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinTileGenerator, line 420,554,687 - Isn't cx >= aax0?  And isn't x1 >= aax0? ???<br></blockquote><div><br>    Fixed comment: <br>    // now: cx >= x0 and cx >= aax0<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinTileGenerator, line 491 - spacing on byte cast<br></blockquote><div><br>    Fixed.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- MarlinTilegenerator, line 655 - "Clear" or "Fill"?<br></blockquote><div><br>    Fixed.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- 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?<br></blockquote><div><br>    Added comment: 'TestNonAARasterization: cubics/quads' (not in repo, only in JBS)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Renderer, line 1318,1365 - that was fallout from the half-open stuff at 1391, right?<br></blockquote><div><br>    Exactly, I fixed handling half-open intervals in MarlinFX so it is a backport.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Stroker, line 112 - "* 8"?  Or perhaps "* 6 + 2" since the endpoints are shared?<br></blockquote><div><br>    Yes endpoints are shared so I fixed the capacity<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Stroker, line 347,376 - it appears to still use m[off],m[off+1] instead of 0,1...?<br></blockquote><div><br>    Fixed comments.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- 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.<br></blockquote><div><br>    Renamed the method; it was backported from openpisces (so I did not really study the approach)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Stroker, line 841 - "winden" => "widen"<br></blockquote><div><br>    Fixed.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Stroker, lines 839,847,856,866 - there is no p4<br></blockquote><div><br>    Already mentioned: see <a href="https://bugs.openjdk.java.net/browse/JDK-8170029" target="_blank">https://bugs.openjdk.java.net/<wbr>browse/JDK-8170029</a><br>    To be fixed later<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I'm assuming that the D*.java files were all generated from the regular files using a sed script?  I skipped those (for now)...<span class="m_-1314918688989111123gmail-im m_-1314918688989111123gmail-HOEnZb"><br></span></blockquote><div><br></div><div>I also fixed D* files.<br><br></div><div>Laurent<br><span class="m_-1314918688989111123gmail-im m_-1314918688989111123gmail-HOEnZb"></span><br><span class="m_-1314918688989111123gmail-im m_-1314918688989111123gmail-HOEnZb"></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-1314918688989111123gmail-im m_-1314918688989111123gmail-HOEnZb">
<br>
On 4/22/17 4:28 AM, Laurent Bourgès wrote:<br>
</span><div class="m_-1314918688989111123gmail-HOEnZb"><div class="m_-1314918688989111123gmail-h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hi,<br>
<br>
I am pleased to have successfully upgraded my machine with OpenJDK10 and merged my Marlin 0.7.5 release (dec 2016) with<br>
its Java2d variant.<br>
<br>
Please review this Marlin2D upgrade to Marlin 0.7.5:<br>
<br>
JBS: no bug yet for OpenJDK 10<br>
webrev: <a href="http://cr.openjdk.java.net/%7Elbourges/marlin/Marlin-075.0/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~lb<wbr>ourges/marlin/Marlin-075.0/</a><br>
<br>
Changes:<br>
- Improved MarlinTileGenerator to efficiently handle asymetric tiles (W x H) with almost full / empty fills<br>
- Added Marlin Double-precision pipeline<br>
- MarlinFX backports (Dasher, Stroker changes from OpenPisces)<br>
- dead code & few comment removals in Strokers<br>
- fixed all floating-point number literals to be x.0f or x.0d to simplify the conversion between float & double variants<br>
<br>
As this patch is huge, do you want me to provide webrevs against Float vs Double pipelines or against OpenJFX10 ?<br>
<br>
PS: I forgot to upgrade the copyright headers to 2017, but will do later<br>
<br>
Cheers,<br>
Laurent<br>
</blockquote>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div class="m_-1314918688989111123gmail_signature">-- <br>Laurent Bourgès</div>
</div></div></div></div></div></div>
</blockquote></div></div>