Bug 81104 - REGRESSION: getBBox returns incorrect results with empty containers
Summary: REGRESSION: getBBox returns incorrect results with empty containers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philip Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-14 07:36 PDT by Stephen Chenney
Modified: 2012-03-26 07:37 PDT (History)
8 users (show)

See Also:


Attachments
Layout test (1.02 KB, text/html)
2012-03-14 07:36 PDT, Stephen Chenney
no flags Details
Ref test (398 bytes, text/html)
2012-03-14 07:36 PDT, Stephen Chenney
no flags Details
First pass (8.80 KB, patch)
2012-03-22 10:37 PDT, Philip Rogers
zimmermann: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Asking reviewers what they think about a different approach (13.44 KB, patch)
2012-03-24 17:47 PDT, Philip Rogers
zimmermann: review+
Details | Formatted Diff | Diff
Update per reviewer comments (13.43 KB, patch)
2012-03-25 11:50 PDT, 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 Stephen Chenney 2012-03-14 07:36:00 PDT
Created attachment 131841 [details]
Layout test

When getBBox is called on a container that contains an empty container and some other content, it returns a bounding box that includes the origin, when in fact it should just include the non-empty container's bound.

Attached is a potential layout test and the expected result as a ref-test.

http://code.google.com/p/chromium/issues/detail?id=114590
Comment 1 Stephen Chenney 2012-03-14 07:36:19 PDT
Created attachment 131842 [details]
Ref test
Comment 2 Dirk Schulze 2012-03-14 09:32:12 PDT
Why do you need a ref test here at all? A pure JS test with DRT is the better choice. Nevertheless, I didn't understand what you expect.

Currently we get (100,100,100,100) as BBox for the <g> with the name 'container'.

Other browsers:
Opera:  (100,100,100,100)
Firefox:  (0,0,200,200)
IE: (100,100,100,100)

So it seems to me that most browsers ignore the empty g. I would mark this bug as invalid, because I would ignore the g as well. It has not content, therefore no bounding box.

Also, please use <!DOCTYPE html> and omit ns declarations on uploading html files. Makes it easier to debug it on IE :)
Comment 3 Stephen Chenney 2012-03-14 10:02:12 PDT
What did you test on? The Chrome bug says:

Issue not reproducible in 17.0.963.79 (Official Build 125985). Issue reproducible with 18.0.1025.11 (Official Build 121089) and 19.0.1068.0 (Official Build 126342).

I don't really care how we test it. I just did what was fastest to get up.
Comment 4 Dirk Schulze 2012-03-14 10:42:14 PDT
I indeed did not test trunk. Didn't realized that it is a regression (or progression). Now WebKit gives back (0,0,200,200).

Can you please write a mail to www-svg if and how empty SVGGElements influence the bounding box of the parent?

Thanks.
Comment 5 Stephen Chenney 2012-03-14 11:06:14 PDT
I don't think it's necessary to ask the WG. The spec is clear and there is only one rect inside the group.

http://www.w3.org/TR/SVG/types.html#__svg__SVGLocatable__getBBox
SVGRect getBBox()
Returns the tight bounding box in current user space (i.e., after application of the ‘transform’ attribute, if any) on the geometry of all contained graphics elements, exclusive of stroking, clipping, masking and filter effects). Note that getBBox must return the actual bounding box at the time the method was called, even in case the element has not yet been rendered.
Returns
An SVGRect object that defines the bounding box.

http://www.w3.org/TR/SVG/intro.html#Definitions
graphics element:
One of the element types that can cause graphics to be drawn onto the target canvas. Specifically: ‘circle’, ‘ellipse’, ‘image’, ‘line’, ‘path’, ‘polygon’, ‘polyline’, ‘rect’, ‘text’ and ‘use’.
Comment 6 Dirk Schulze 2012-03-14 11:13:00 PDT
(In reply to comment #5)
> I don't think it's necessary to ask the WG. The spec is clear and there is only one rect inside the group.
> 
> http://www.w3.org/TR/SVG/types.html#__svg__SVGLocatable__getBBox
> SVGRect getBBox()
> Returns the tight bounding box in current user space (i.e., after application of the ‘transform’ attribute, if any) on the geometry of all contained graphics elements, exclusive of stroking, clipping, masking and filter effects). Note that getBBox must return the actual bounding box at the time the method was called, even in case the element has not yet been rendered.
> Returns
> An SVGRect object that defines the bounding box.
> 
> http://www.w3.org/TR/SVG/intro.html#Definitions
> graphics element:
> One of the element types that can cause graphics to be drawn onto the target canvas. Specifically: ‘circle’, ‘ellipse’, ‘image’, ‘line’, ‘path’, ‘polygon’, ‘polyline’, ‘rect’, ‘text’ and ‘use’.

Ok, than this bug is a regression. Adding Rob, because I think he changed the definition for empty on another place. Maybe this influences this bug?
Comment 7 Stephen Chenney 2012-03-14 11:19:33 PDT
A big part of the problem is that WebKit and SVG does not define the notion of an "invalid" as opposed to "zero-sized" bounding box. It wouldn't surprise me if the empty <g> element is returning (0, 0, 0, 0) as the bound and the code in its parent just combines that with the rect's bound.

This is problematic because I also read the spec to mean that a path with a single move(0,0) in it should contribute to the bounding box, even though its bound is also (0,0,0,0).
Comment 8 Dirk Schulze 2012-03-14 11:20:48 PDT
(In reply to comment #7)
> A big part of the problem is that WebKit and SVG does not define the notion of an "invalid" as opposed to "zero-sized" bounding box. It wouldn't surprise me if the empty <g> element is returning (0, 0, 0, 0) as the bound and the code in its parent just combines that with the rect's bound.
> 
> This is problematic because I also read the spec to mean that a path with a single move(0,0) in it should contribute to the bounding box, even though its bound is also (0,0,0,0).

That seems to be indeed the case.
Comment 9 Robert Longson 2012-03-16 06:51:33 PDT
FWIW Firefox:  (0,0,200,200) was a trunk regression which is unlikely to make it to a release. Trunk now returns (100,100,100,100)

Firefox also won't run the Layout test file because it uses document.getElementsByName and SVG elements don't have a name property.
Comment 10 Nikolas Zimmermann 2012-03-19 08:17:58 PDT
(In reply to comment #7)
> A big part of the problem is that WebKit and SVG does not define the notion of an "invalid" as opposed to "zero-sized" bounding box. It wouldn't surprise me if the empty <g> element is returning (0, 0, 0, 0) as the bound and the code in its parent just combines that with the rect's bound.
> 
> This is problematic because I also read the spec to mean that a path with a single move(0,0) in it should contribute to the bounding box, even though its bound is also (0,0,0,0).

How about introducing a static FloatRect::invalid(), and use that whenever we want to indicate error state?
Comment 11 Dirk Schulze 2012-03-19 09:46:24 PDT
(In reply to comment #10)
> (In reply to comment #7)
> > A big part of the problem is that WebKit and SVG does not define the notion of an "invalid" as opposed to "zero-sized" bounding box. It wouldn't surprise me if the empty <g> element is returning (0, 0, 0, 0) as the bound and the code in its parent just combines that with the rect's bound.
> > 
> > This is problematic because I also read the spec to mean that a path with a single move(0,0) in it should contribute to the bounding box, even though its bound is also (0,0,0,0).
> 
> How about introducing a static FloatRect::invalid(), and use that whenever we want to indicate error state?

I would definitely vote for that! But do other WebKit devs see the same usage?
Comment 12 Stephen Chenney 2012-03-19 10:30:47 PDT
Adding an invalid FloatRect object would have impact throughout WebKit. There is a lot of code that treats (0, 0, 0, 0) rectangles specially. If we decide to push for a special invalid rect object we would need to look at every instance of checks for a 0,0,0,0 rectangle and decide which are looking to see if a rectangle is "empty" and which are for a rectangle that is undefined or unset.

Note that Core Graphics uses (0, 0, -1, -1) for "null" rectangle and this has bitten us in the past. See bug 65796, comment #65 and later.
Comment 13 Philip Rogers 2012-03-19 10:34:00 PDT
(In reply to comment #12)
> Adding an invalid FloatRect object would have impact throughout WebKit. There is a lot of code that treats (0, 0, 0, 0) rectangles specially. If we decide to push for a special invalid rect object we would need to look at every instance of checks for a 0,0,0,0 rectangle and decide which are looking to see if a rectangle is "empty" and which are for a rectangle that is undefined or unset.
> 
> Note that Core Graphics uses (0, 0, -1, -1) for "null" rectangle and this has bitten us in the past. See bug 65796, comment #65 and later.

I talked with Niko for a bit about this and a magic value of max_val - 1 could be fairly low-impact. I'm not a huge fan of magic constants, but I'm even less of a fan of an extra boolean hanging off FloatRect.
Comment 14 Philip Rogers 2012-03-22 10:37:04 PDT
Created attachment 133293 [details]
First pass
Comment 15 Dirk Schulze 2012-03-22 10:56:55 PDT
Comment on attachment 133293 [details]
First pass

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

> Source/WebCore/platform/graphics/FloatRect.h:101
> +    static bool isValid(FloatRect &rect) { return rect.width() >= 0 && rect.height() >= 0; }
> +    static FloatRect invalid() { return FloatRect(0, 0, -1, -1); }

Are you sure that no code in WebCore is using negative with or height for FloatRect in internal code or in Canvas? Even if they would not call isValid, it would still be strange.
Comment 16 Philip Rogers 2012-03-22 11:01:27 PDT
(In reply to comment #15)
> (From update of attachment 133293 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133293&action=review
> 
> > Source/WebCore/platform/graphics/FloatRect.h:101
> > +    static bool isValid(FloatRect &rect) { return rect.width() >= 0 && rect.height() >= 0; }
> > +    static FloatRect invalid() { return FloatRect(0, 0, -1, -1); }
> 
> Are you sure that no code in WebCore is using negative with or height for FloatRect in internal code or in Canvas? Even if they would not call isValid, it would still be strange.

I am not sure which is why I added the extra comment and made it static. Another option would be to hide this in SVGRenderSupport, but I think that's kind-of dirty. Suggestions welcome, as I'm out of ideas on this one :/
Comment 17 Dirk Schulze 2012-03-22 11:05:26 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 133293 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133293&action=review
> > 
> > > Source/WebCore/platform/graphics/FloatRect.h:101
> > > +    static bool isValid(FloatRect &rect) { return rect.width() >= 0 && rect.height() >= 0; }
> > > +    static FloatRect invalid() { return FloatRect(0, 0, -1, -1); }
> > 
> > Are you sure that no code in WebCore is using negative with or height for FloatRect in internal code or in Canvas? Even if they would not call isValid, it would still be strange.
> 
> I am not sure which is why I added the extra comment and made it static. Another option would be to hide this in SVGRenderSupport, but I think that's kind-of dirty. Suggestions welcome, as I'm out of ideas on this one :/

The position for the static FloatRect::invalid is fine. I am more wondering about (0,0,-1,-1). Didn't you want to use max_val - 1 or sth. like that for invalid rects? Might make more sense. (nan,nan,nan,nan) would definitely be a invalid rect, but might be a bit dangerous ;)
Comment 18 WebKit Review Bot 2012-03-22 11:50:22 PDT
Comment on attachment 133293 [details]
First pass

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

New failing tests:
svg/custom/getBBox-perpendicular-path.svg
svg/filters/filter-placement-issue.svg
svg/clip-path/clipper-placement-issue.svg
svg/dynamic-updates/SVGFEBlendElement-dom-in-attr.html
svg/filters/feGaussianBlur.svg
svg/filters/filter-refresh.svg
svg/dynamic-updates/SVGFEBlendElement-dom-in2-attr.html
svg/dynamic-updates/SVGFEBlendElement-dom-mode-attr.html
svg/filters/filterRes.svg
svg/dynamic-updates/SVGFEBlendElement-svgdom-in-prop.html
svg/dynamic-updates/SVGFEBlendElement-svgdom-in2-prop.html
svg/W3C-SVG-1.1/filters-blend-01-b.svg
svg/repaint/filter-child-repaint.svg
svg/W3C-SVG-1.1/filters-gauss-01-b.svg
svg/dynamic-updates/SVGFEBlendElement-svgdom-mode-prop.html
Comment 19 Nikolas Zimmermann 2012-03-23 02:16:46 PDT
Comment on attachment 133293 [details]
First pass

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

Patch looks good, still some issues to consider though:

> Source/WebCore/platform/graphics/FloatRect.h:99
> +    // We define an invalid FloatRect as having negative width or height.

This is heavy, what I have in mind, is passing around a reference to an internal static FloatRect, like:
static const FloatRect& invalidFloatRect()
{
    static const float kInvalidFloat = std::numeric_limits<float>::max() - 1;
    DEFINE_STATIC_LOCAL(FloatRect, invalidRect, (kInvalidFloat, kInvalidFloat, kInvalidFloat, kInvalidFloat));
    return invalidRect;
}

FloatRect defaultBoundingBoxForContainer = invalidFloatRect();
You could make invalidFloatRect() a free-function, no need for a class-static, but if you prefer it, you can keep it there.
You can also save the isValid() method completely, and use: someRect == invalidRect() to compare.

IMHO we should also use special values like flt_max-1, to distinguish from negative values for the size, even if that could be used for that. To avoid clashes, or problems like Dirk mentioned in his review, its safer to not reuse negative values for the sake of determining the validity of a FloatRect.

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:153
>      m_strokeBoundingBox = FloatRect();

The stroke bounding box should suffer from the same problem no?
I could imagine it might make sense to keep the repaint bbox as-is, but I might be wrong (didn't investigate)

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:105
> +                    isObjectBoundingBoxValid = false;

= true, no? :-) That should explain possible pixel test failures..

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:118
> +                    isObjectBoundingBoxValid = false;

Ditto.

I'm aware this is just a first-stab, but code like this should really be shared, in a static inline helper method.
Comment 20 Philip Rogers 2012-03-24 17:47:16 PDT
Created attachment 133663 [details]
Asking reviewers what they think about a different approach

I found that introducing an invalid objectBoundingBox is actually very noisy and expensive: a lot of code, e.g., hit testing, ends up calling objectBoundingBox() and needs to have guards added to check for the invalid state. Furthermore, in most cases an empty bounding box (0,0,0,0) ends up being the desired fallback anyway.

I've attached a speculative approach that goes a different direction: adding a flag to just RenderSVGRoot and RenderSVGContainer for whether the contained object bounding box is invalid. What do you think?
Comment 21 Nikolas Zimmermann 2012-03-25 05:07:09 PDT
Comment on attachment 133663 [details]
Asking reviewers what they think about a different approach

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

That approach is much less intrusive, and I think its the best mid-term solution. If we ever encounter non-SVG code suffering from similar problems, we can always come back and introduce FloatRect::invalid().
I'll r+ this as-is, if someone dislikes this concept, feel free to r- with reasons :-)

> Source/WebCore/ChangeLog:20
> +        Reviewed by NOBODY (OOPS!).

This line should be on top.

> Source/WebCore/rendering/svg/RenderSVGContainer.h:45
> +    bool isObjectBoundingBoxValid() { return m_objectBoundingBoxValid; }

Make it const.

> Source/WebCore/rendering/svg/RenderSVGContainer.h:80
> +    bool m_objectBoundingBoxValid;

You could move this down and mark as : 1, as its done for m_needsBoundariesUpdate, which in theory saves space.

> Source/WebCore/rendering/svg/RenderSVGRoot.h:107
> +    bool m_objectBoundingBoxValid;

This can also be moved down and marked as :1.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:88
> +// Update an object's bounding box taking into account the validity of each object bounding box.

typo: objects

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:92
> +    if (otherValid) {

Early exit if !otherValid.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94
> +        if (objectBoundingBoxValid)
> +            objectBoundingBox.uniteEvenIfEmpty(otherBoundingBox);

Ditto. Early exit instead of using an else branch.
Comment 22 Philip Rogers 2012-03-25 11:50:34 PDT
Created attachment 133685 [details]
Update per reviewer comments
Comment 23 Philip Rogers 2012-03-25 11:58:48 PDT
Thanks again for the quick review. It's not every project that one can get quality reviews in just a few hours over the weekend :) Replies in-line.

(In reply to comment #21)
> (From update of attachment 133663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133663&action=review
> 
> That approach is much less intrusive, and I think its the best mid-term solution. If we ever encounter non-SVG code suffering from similar problems, we can always come back and introduce FloatRect::invalid().
Agreed. Another option would be to introduce something like InvalidatableFloatRect that extends FloatRect but has a boolean for validity. This could just fall back to normal FloatRect codepaths anywhere validity wasn't checked.

> I'll r+ this as-is, if someone dislikes this concept, feel free to r- with reasons :-)
> 

I'll hold off on committing until Monday so Dirk and Stephen have a chance to comment.

> > Source/WebCore/ChangeLog:20
> > +        Reviewed by NOBODY (OOPS!).
> 
> This line should be on top.

Done.

> 
> > Source/WebCore/rendering/svg/RenderSVGContainer.h:45
> > +    bool isObjectBoundingBoxValid() { return m_objectBoundingBoxValid; }
> 
> Make it const.

Done.

> 
> > Source/WebCore/rendering/svg/RenderSVGContainer.h:80
> > +    bool m_objectBoundingBoxValid;
> 
> You could move this down and mark as : 1, as its done for m_needsBoundariesUpdate, which in theory saves space.

I don't think this is possible because we need to pass m_needsBoundariesUpdate by reference and I don't think that's possible if it's packed in a bitfield.

> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.h:107
> > +    bool m_objectBoundingBoxValid;
> 
> This can also be moved down and marked as :1.
> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:88
> > +// Update an object's bounding box taking into account the validity of each object bounding box.
> 
> typo: objects

I think this is correct but it's hard to parse since it's ambiguous whether "object bounding box" is a noun. I just reworded the sentence to say "Update a bounding box taking into account the validity of the other bounding box."

> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:92
> > +    if (otherValid) {
> 
> Early exit if !otherValid.

Done.

> 
> > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94
> > +        if (objectBoundingBoxValid)
> > +            objectBoundingBox.uniteEvenIfEmpty(otherBoundingBox);
> 
> Ditto. Early exit instead of using an else branch.

Done.
Comment 24 WebKit Review Bot 2012-03-26 07:37:16 PDT
Comment on attachment 133685 [details]
Update per reviewer comments

Clearing flags on attachment: 133685

Committed r112091: <http://trac.webkit.org/changeset/112091>
Comment 25 WebKit Review Bot 2012-03-26 07:37:22 PDT
All reviewed patches have been landed.  Closing bug.