Bug 76177 – SVG group getBBox returns 0,0,0,0 for a group of perpendicular paths

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

[Philip Rogers]

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)

[Philip Rogers]

Created attachment 122241 [details]
Fix getBBox for perpendicular paths

[Philip Rogers]

This changes platform/graphics and is not SVG specific so I've added [e-mail]

[Darin Adler]

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.

[Levi Weintraub]

(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...

[Philip Rogers]

(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?

[Darin Adler]

(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.

[WebKit Review Bot]

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

[Philip Rogers]

(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.

[Philip Rogers]

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().

[Levi Weintraub]

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.

[Philip Rogers]

Created attachment 122806 [details]
Fix getBBox for perpendicular paths

Small rename: l,t,r,b -> minX, minY, maxX, maxY per a Darin's comment.

[Darin Adler]

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.

[Philip Rogers]

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.

[WebKit Review Bot]

Comment on attachment 122806 [details]
Fix getBBox for perpendicular paths

Clearing flags on attachment: 122806

Committed r105231: <http://trac.webkit.org/changeset/105231>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

Just wanted to leave a note: great findings Philip! Thanks for fixing quickly!