Bug 73021 – Divide by zero for zero-length arcs

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Stephen Chenney]

Created attachment 116356 [details]
Test case

The code in SVGPathParser::decomposeArcToCubic performs a divide by zero when the start and endpoint of the arc are the same. This value happens to result in nothing being drawn, but it is poor behavior when linecaps are defined. Specifically, for consistency with other paths and continuity in animation, non-butt linecaps should be drawn for zero-length arcs. See http://lists.w3.org/Archives/Public/www-svg/2011Nov/0129.html

I propose catching the zero length arc case and converting it to a zero length line. This has the advantage of matching behavior for the zero arc radius case.

[Stephen Chenney]

Created attachment 116367 [details]
Patch

Do not submit this patch - needs mac expectations

[WebKit Review Bot]

Comment on attachment 116367 [details]
Patch

Attachment 116367 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10608399

New failing tests:
svg/as-background-image/svg-as-background-4.html

[Stephen Chenney]

Created attachment 116398 [details]
Patch

This patch is ready for commit.

[Stephen Chenney]

Comment on attachment 116398 [details]
Patch

This has problems - need to fix.

[Stephen Chenney]

Created attachment 116413 [details]
Patch

This patch is really ready for commit.

[Alexey Proskuryakov]

This test looks like it should have cross-platform expectations. What makes them different?

[Stephen Chenney]

The platforms differ by one pixel (or so) in the radius of the circle drawn for linecaps in CoreGraphics when compared to skia. The expectations are set up so that anything using CoreGraphcis catches the mac expectations, while chromium skia on mac catches the chromium-mac expectations and linux and windows catch win expectations.

I have verified linux, mac, chromium-mac-skia and chromium-mac-cg.

[Alexey Proskuryakov]

Thank you for the explanation. Should this be considered a bug in either implementation?

[Stephen Chenney]

The bug manifested in all the platforms I tested the file on (before any fix). That was Safari, Chrome on Mac and Linux, and the Linux desktop file previewer (which I think is the gtk port). The code that is modified by the patch is common to all platforms, so I presume it affects all platforms.

I suspect that expectations will be required for the Qt port, but I have no way of generating those. The Qt gardener should be able to update expectations once the patch is in.

[Stephen Chenney]

Comment on attachment 116413 [details]
Patch

It occurs to me that I need to test an arc that has the same start and end point but the large sweep (i.e a circle). This should not be replaced with a line.

[Alexey Proskuryakov]

Re comment 9 - my question was whether the one pixel difference between CG and Skia is a bug in either of those, or if it's something we don't intend to be compatible.

[Stephen Chenney]

Comment on attachment 116413 [details]
Patch

On second thought, an arc with the same starting and ending point is ill-defined as a circle, because it is impossible to come up with a center. So this patch stands.

[Stephen Chenney]

(In reply to comment #11)
> Re comment 9 - my question was whether the one pixel difference between CG and Skia is a bug in either of those, or if it's something we don't intend to be compatible.

The platforms are free to make different assumptions and different rounding errors and different anti-aliasing behavior. So we do not force them to all be the same. It's just not practical.

[Nikolas Zimmermann]

Comment on attachment 116413 [details]
Patch

Looks good to me! r=me.

[Stephen Chenney]

Comment on attachment 116413 [details]
Patch

Reverting commit-queue while expectations are sorted out

[Stephen Chenney]

Created attachment 117664 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117664 [details]
Patch

Clearing flags on attachment: 117664

Committed r101895: <http://trac.webkit.org/changeset/101895>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.