Bug 73021 - Divide by zero for zero-length arcs
Summary: Divide by zero for zero-length arcs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen Chenney
URL:
Keywords:
Depends on:
Blocks: 71820 73677
  Show dependency treegraph
 
Reported: 2011-11-23 07:33 PST by Stephen Chenney
Modified: 2011-12-02 19:12 PST (History)
4 users (show)

See Also:


Attachments
Test case (1003 bytes, image/svg+xml)
2011-11-23 07:33 PST, Stephen Chenney
no flags Details
Patch (12.66 KB, patch)
2011-11-23 08:41 PST, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (41.28 KB, patch)
2011-11-23 12:30 PST, Stephen Chenney
schenney: review-
schenney: commit-queue-
Details | Formatted Diff | Diff
Patch (41.16 KB, patch)
2011-11-23 13:35 PST, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (24.94 KB, patch)
2011-12-02 11:59 PST, Stephen Chenney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen Chenney 2011-11-23 07:33:35 PST
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.
Comment 1 Stephen Chenney 2011-11-23 08:41:15 PST
Created attachment 116367 [details]
Patch

Do not submit this patch - needs mac expectations
Comment 2 WebKit Review Bot 2011-11-23 09:26:43 PST
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
Comment 3 Stephen Chenney 2011-11-23 12:30:56 PST
Created attachment 116398 [details]
Patch

This patch is ready for commit.
Comment 4 Stephen Chenney 2011-11-23 12:46:56 PST
Comment on attachment 116398 [details]
Patch

This has problems - need to fix.
Comment 5 Stephen Chenney 2011-11-23 13:35:38 PST
Created attachment 116413 [details]
Patch

This patch is really ready for commit.
Comment 6 Alexey Proskuryakov 2011-11-23 14:25:02 PST
This test looks like it should have cross-platform expectations. What makes them different?
Comment 7 Stephen Chenney 2011-11-23 14:30:56 PST
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.
Comment 8 Alexey Proskuryakov 2011-11-23 14:50:10 PST
Thank you for the explanation. Should this be considered a bug in either implementation?
Comment 9 Stephen Chenney 2011-11-23 14:57:14 PST
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.
Comment 10 Stephen Chenney 2011-11-28 17:10:18 PST
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.
Comment 11 Alexey Proskuryakov 2011-11-28 17:19:25 PST
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.
Comment 12 Stephen Chenney 2011-11-29 07:12:15 PST
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.
Comment 13 Stephen Chenney 2011-11-29 07:13:36 PST
(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.
Comment 14 Nikolas Zimmermann 2011-12-02 11:08:35 PST
Comment on attachment 116413 [details]
Patch

Looks good to me! r=me.
Comment 15 Stephen Chenney 2011-12-02 11:33:17 PST
Comment on attachment 116413 [details]
Patch

Reverting commit-queue while expectations are sorted out
Comment 16 Stephen Chenney 2011-12-02 11:59:06 PST
Created attachment 117664 [details]
Patch
Comment 17 WebKit Review Bot 2011-12-02 19:12:49 PST
Comment on attachment 117664 [details]
Patch

Clearing flags on attachment: 117664

Committed r101895: <http://trac.webkit.org/changeset/101895>
Comment 18 WebKit Review Bot 2011-12-02 19:12:56 PST
All reviewed patches have been landed.  Closing bug.