Actually, there is one functional difference between this implementation
and the Ductus pipeline...
The similar method in the Ductus pipeline is operating entirely on
device space coordinates after all transformation (which is why it can
have normalization built-in).
The method you modified is sometimes called on transformed device space
coordinates, but in the case of a draw operation it can be called on
user space coordinates if the transform is non-uniform.
Since this change will solve the issue for fills and uniform-scaled
draws(), it handles the 90% case, but if you use a non-uniform scale of
more than 2x, then it will still allow overflow in the final rendered
path if a coordinate is near UPPER_BND...
...jim
On 3/10/16 4:47 PM, Jim Graham wrote:
I can confirm that this is the same solution provided for the Ductus
code so it will bring us to par with the former rasterizer.
I would also check what happens when you try to render and
createStrokedPath() for a path that is so full of NaN values that it
never gets any segments out, and/or if it only manages a moveto - just
make sure that we render nothing rather than get an exception because
something in the code isn't set up for that. I'm guessing that this may
already be covered by a test case that ensures that we don't blow up
with g2d.fill/draw(new Path2D.Double()); but it couldn't hurt to add a
quick test.
Finally, I'll mention that I was never very comfortable with the way
that code was written in the Ductus pipeline. It does the trick and I
don't see it passing through any NaN or infinite values, but it just
seems like it might be overly complicated and it's hard to make sure
that it is doing something "sensible" in the face of a partially
constructed secondary sub-path (i.e. pathClosed handling). For one
thing, I'm >80% sure that skip and subpathStarted could be folded into
the same boolean, and/or that our output in the case where
skip/subpathStarted disagree is what we want it to be, but this is all
beyond the scope of this fix.
I'd say add another test for completely bogus path(s) and it's good to
go, but at least keep it in the back of your mind that this code might
not represent the best solution to the problem...
...jim
On 3/10/16 7:09 AM, Laurent Bourgès wrote:
Hi,
Please review this bug fix for the Marlin renderer to properly ignore
point coordinates with NaN/Infinity values:
bug: https://bugs.openjdk.java.net/browse/JDK-8144938
webrev: http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144938.0/
Changes:
- MarlinRenderingEngine.pathToLoop(): added bound checks for point
coordinates by portingDuctusRenderingEngine#feedConsumer()
- CrashNaNTest: check output image to detect incorrect rendering
- minor: unused import in AAShapePipe + Version incremented
Regards,
Laurent
