Bug 76177 - SVG group getBBox returns 0,0,0,0 for a group of perpendicular paths
Summary: SVG group getBBox returns 0,0,0,0 for a group of perpendicular paths
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-12 07:35 PST by Philip Rogers
Modified: 2012-01-18 00:35 PST (History)
8 users (show)

See Also:


Attachments
Testcase (795 bytes, text/html)
2012-01-12 07:35 PST, Philip Rogers
no flags Details
Fix getBBox for perpendicular paths (6.20 KB, patch)
2012-01-12 07:41 PST, Philip Rogers
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix getBBox for perpendicular paths (6.91 KB, patch)
2012-01-14 08:38 PST, Philip Rogers
no flags Details | Formatted Diff | Diff
Fix getBBox for perpendicular paths (6.87 KB, patch)
2012-01-17 13:39 PST, Philip Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2012-01-12 07:35:50 PST
Created attachment 122240 [details]
Testcase

A group of perpendicular paths returns a bounding box of 0,0,0,0 (see attached testcase).

(bug was originally reported at: http://code.google.com/p/chromium/issues/detail?id=109805)
Comment 1 Philip Rogers 2012-01-12 07:41:28 PST
Created attachment 122241 [details]
Fix getBBox for perpendicular paths
Comment 2 Philip Rogers 2012-01-12 07:46:15 PST
This changes platform/graphics and is not SVG specific so I've added darin@apple.com
Comment 3 Darin Adler 2012-01-12 11:03:24 PST
Comment on attachment 122241 [details]
Fix getBBox for perpendicular paths

View in context: https://bugs.webkit.org/attachment.cgi?id=122241&action=review

> Source/WebCore/platform/graphics/FloatRect.cpp:114
> +    float left = min(x(), other.x());
> +    float top = min(y(), other.y());
> +    float right = max(maxX(), other.maxX());
> +    float bottom = max(maxY(), other.maxY());

The "l, t, r, b" or "left, top, right, bottom" naming is obsolete for IntRect and FloatRect. In new code we should name these things like x, y, maxX, and maxY.

> Source/WebCore/platform/graphics/FloatRect.cpp:119
>  void FloatRect::uniteIfNonZero(const FloatRect& other)

Not new to this patch:

It would be clearer if the uniteIfNonZero function used the isZero function rather than having a hand-written distinct implementation of “non-zero”. I am concerned that this function’s definition of 0 is !x, while FloatSize’s definition of 0 is fabs(x) < epsilon. It looks like Levi made that change to FloatSize in July and I am not sure why it’s not needed here as well.
Comment 4 Levi Weintraub 2012-01-12 11:09:20 PST
(In reply to comment #3)
> It would be clearer if the uniteIfNonZero function used the isZero function rather than having a hand-written distinct implementation of “non-zero”. I am concerned that this function’s definition of 0 is !x, while FloatSize’s definition of 0 is fabs(x) < epsilon. It looks like Levi made that change to FloatSize in July and I am not sure why it’s not needed here as well.

It was definitely an oversight that this still uses zero instead of considering epsilon, let's correct that now. Of course, epsilon is not perfect either...
Comment 5 Philip Rogers 2012-01-12 11:12:53 PST
(In reply to comment #4)
> (In reply to comment #3)
> > It would be clearer if the uniteIfNonZero function used the isZero function rather than having a hand-written distinct implementation of “non-zero”. I am concerned that this function’s definition of 0 is !x, while FloatSize’s definition of 0 is fabs(x) < epsilon. It looks like Levi made that change to FloatSize in July and I am not sure why it’s not needed here as well.
> 
> It was definitely an oversight that this still uses zero instead of considering epsilon, let's correct that now. Of course, epsilon is not perfect either...

I agree and can do this. Should I do that in this review, or as a followup patch?
Comment 6 Darin Adler 2012-01-12 11:13:34 PST
(In reply to comment #5)
> I agree and can do this. Should I do that in this review, or as a followup patch?

Seems good to do it separately. Might even be worth spending a short time on little research to see if we can find any case where we know it affects behavior.
Comment 7 WebKit Review Bot 2012-01-12 13:42:42 PST
Comment on attachment 122241 [details]
Fix getBBox for perpendicular paths

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

New failing tests:
svg/batik/filters/filterRegions.svg
svg/clip-path/deep-nested-clip-in-mask-panning.svg
svg/filters/filterRes.svg
svg/dynamic-updates/SVGFEBlendElement-dom-in-attr.html
svg/W3C-SVG-1.1/filters-morph-01-f.svg
svg/filters/filter-refresh.svg
svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html
svg/dynamic-updates/SVGFEBlendElement-dom-mode-attr.html
svg/dynamic-updates/SVGFEBlendElement-svgdom-in-prop.html
svg/filters/feConvolveFilter-y-bounds.svg
svg/dynamic-updates/SVGFEBlendElement-svgdom-in2-prop.html
svg/W3C-SVG-1.1/filters-blend-01-b.svg
svg/animations/animate-mpath-insert.html
svg/clip-path/deep-nested-clip-in-mask.svg
svg/dynamic-updates/SVGFEBlendElement-svgdom-mode-prop.html
Comment 8 Philip Rogers 2012-01-12 14:00:08 PST
(In reply to comment #7)
> (From update of attachment 122241 [details])
> Attachment 122241 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11227184
> 
> New failing tests:
> svg/batik/filters/filterRegions.svg
> svg/clip-path/deep-nested-clip-in-mask-panning.svg
> svg/filters/filterRes.svg
> svg/dynamic-updates/SVGFEBlendElement-dom-in-attr.html
> svg/W3C-SVG-1.1/filters-morph-01-f.svg
> svg/filters/filter-refresh.svg
> svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html
> svg/dynamic-updates/SVGFEBlendElement-dom-mode-attr.html
> svg/dynamic-updates/SVGFEBlendElement-svgdom-in-prop.html
> svg/filters/feConvolveFilter-y-bounds.svg
> svg/dynamic-updates/SVGFEBlendElement-svgdom-in2-prop.html
> svg/W3C-SVG-1.1/filters-blend-01-b.svg
> svg/animations/animate-mpath-insert.html
> svg/clip-path/deep-nested-clip-in-mask.svg
> svg/dynamic-updates/SVGFEBlendElement-svgdom-mode-prop.html

I think a couple of these are real failures (failed on my machine w/o patch, pass with it). Sorry for the churn guys, I'll have a new patch up soon.
Comment 9 Philip Rogers 2012-01-14 08:38:31 PST
Created attachment 122548 [details]
Fix getBBox for perpendicular paths

This patch contains two fixes for failures exposed in my previous patch:
1) We need to special-case the first bounding box in SVGRendererSupport::computeContainerBoundingBoxes. What's happening is we start with a bounding box of 0,0,0,0 and we add the union of all our child bounding boxes. The problem is that the resulting bounding box will always start at x=0, y=0, due to the initial bounding box of 0,0,0,0, so we need to special-case the first child and set the initial bounding box to our first child's bounding box.

2) My previous patch had a mistake where I failed to transform a child's bounding box using transform.mapRect().
Comment 10 Levi Weintraub 2012-01-17 11:18:15 PST
Comment on attachment 122548 [details]
Fix getBBox for perpendicular paths

View in context: https://bugs.webkit.org/attachment.cgi?id=122548&action=review

LGTM, but need someone else to sign off on it :)

> Source/WebCore/platform/graphics/FloatRect.cpp:114
> +    float left = min(x(), other.x());
> +    float top = min(y(), other.y());
> +    float right = max(maxX(), other.maxX());
> +    float bottom = max(maxY(), other.maxY());

As Darin pointed out, we should avoid using these directional names in favor of maxX and maxY to not confuse things in vertical text.
Comment 11 Philip Rogers 2012-01-17 13:39:23 PST
Created attachment 122806 [details]
Fix getBBox for perpendicular paths

Small rename: l,t,r,b -> minX, minY, maxX, maxY per a Darin's comment.
Comment 12 Darin Adler 2012-01-17 17:48:33 PST
Comment on attachment 122806 [details]
Fix getBBox for perpendicular paths

View in context: https://bugs.webkit.org/attachment.cgi?id=122806&action=review

Does the test cover the “united with 0,0,0,0” issue?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:194
> +        if (isFirstChild)
> +            isFirstChild = false;

No need for the if here. An unconditional assignment does the same thing.
Comment 13 Philip Rogers 2012-01-17 18:07:15 PST
Thank you for the second review.

(In reply to comment #12)
> (From update of attachment 122806 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122806&action=review
> 
> Does the test cover the “united with 0,0,0,0” issue?

It does. The test draws two perpendicular lines starting at (5,5) and extending to (5,105) and (105,5). Including (0,0) would have resulted in a bounding box of size 105*105, but starting at the first child results in the expected 100*100 bounding box.

> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:194
> > +        if (isFirstChild)
> > +            isFirstChild = false;
> 
> No need for the if here. An unconditional assignment does the same thing.

I have a followup patch for the !width() issue and I'll tack on a fix for this unnecessary check.
Comment 14 WebKit Review Bot 2012-01-17 19:18:40 PST
Comment on attachment 122806 [details]
Fix getBBox for perpendicular paths

Clearing flags on attachment: 122806

Committed r105231: <http://trac.webkit.org/changeset/105231>
Comment 15 WebKit Review Bot 2012-01-17 19:18:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Nikolas Zimmermann 2012-01-18 00:35:37 PST
Just wanted to leave a note: great findings Philip! Thanks for fixing quickly!