[OpenJDK 2D-Dev] RFR 8144938: Handle properly coordinate overflow in Marlin Renderer

[Jim Graham]

The new tests look good.

There is one logic error in the new version of the degenerate coordinate 
filter.

It is valid to have a PATH_CLOSE immediately followed by a <geom>To call 
and the subpath will start at the previous close/moveto point.  For example:

moveto 40, 40
lineto  0,  0
lineto 80,  0
closepath
lineto 80, 80
lineto  0, 80
closepath

should draw an hourglass, but with your fix it will lose the lower half 
because the lineto after the close will be turned into a moveto (and the 
second subpath then becomes empty.

I think it should be fine just getting rid of the subpath=false in the 
close case.

It is arguable, though, if we have closepath followed by a NaN if 
perhaps that NaN should break the subpath from the previous close, but I 
don't think that is in keeping with the current philosophy of simply 
pretending that bad coordinates don't exist.  Essentially, the close of 
a previously valid path gives us a valid start to the new subpath 
regardless if it immediately feeds into more bad coordinates - it still 
started its run on a valid point.  So, I'd argue that simply passing 
along the prior value of the subpath flag would be appropriate, whether 
it is true or false (and, obviously, also closing the path in the 
consumer if it is true)...

			...jim

On 3/15/2016 6:46 AM, Laurent Bourgès wrote:
> Dear Jim,
>
> Here is a new webrev:
> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144938.2/
>
> Changes:
> - pathToLoop() rewritten to simplify the logic (skip and pathClosed
> flags removed)
> - added several createStrokedShape() tests in CrashNaNTest class
>
> I hope my changes to the filtering algorithm are correct:
> I advocate not understanding why it was so complicated (3 flags => 1)
> but maybe I am wrong and I missed several corner cases...
>
>
> My comments below:
>
> 2016-03-15 0:34 GMT+01:00 Jim Graham <james.graham at oracle.com
> <mailto:james.graham at oracle.com>>:
>
>     Hi Laurent,
>
>     Did you consider adding a test with a completely degenerate path to
>     make sure that we don't have any state errors if all of the segments
>     are eaten by the "finite path" filter?
>
>
> Fixed and it produces empty paths (only pathDone):
>
> I wonder if it is time to remove the following lines 317-322 in
> MarlinRenderingEngine.strokeTo():
>                  // Every path needs an initial moveTo and a pathDone.
> If these
>                  // are not there this causes a SIGSEGV in libawt.so (at
> the time
>                  // of writing of this comment (September 16, 2010)).
> Actually,
>                  // I am not sure if the moveTo is necessary to avoid
> the SIGSEGV
>                  // but the pathDone is definitely needed.
>                  pc2d.moveTo(0f, 0f);
>
> For me, it is useless as only pc2d.pathDone() is mandatory.
>
>
>     I like your approach.  It is worth noting that:
>
>     - non-uniform scales are not common
>     - your "always overflow-filter path at device resolution" fixes the
>     issues with measuring overflow in user space that I pointed out in
>     my prior email and only does so at a likely small expense in terms
>     of non-uniform scale performance.  Common cases will see no change
>     (other than the fact that you have new code in the path feeder).
>
>
> Thanks.
> It has an important consequence for me:
> it's possible to introduce a new stage in the pipeline (before
> inverseDeltaTransformConsumer) that implements path clipping in
> device-space ONLY compatible with the rectangular bounding box.
>
>
>     With respect to the optimization that I gave a rough outline of.  It
>     is true that:
>
>     - it should help eliminate all of the inverse transforms in the
>     strokeTo code
>     - the math is incomplete and would need some work
>     - it only targets the non-uniform case which may not be a high priority
>     - it would probably get rid of the entire TransformingPathConsumer
>     module, but that module is not complex or trouble-prone and seems to
>     be working fine
>
>
> I totally agree.
>
> I think improving Marlin's Stroker to better optimize square caps /
> mitter joins or adding path clipping would provide more benefits as it
> would minimize the number of point / edges used later by the Renderer stage.
>
> Thanks for your comments,
> Laurent