| Summary: | REGRESSION: getBBox returns incorrect results with empty containers | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Stephen Chenney <schenney> | ||||||||||||
| Component: | SVG | Assignee: | Philip Rogers <pdr> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | dglazkov, krit, longsonr, pdr, rwlbuis, schenney, webkit.review.bot, zimmermann | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Stephen Chenney
2012-03-14 07:36:00 PDT
Created attachment 131842 [details]
Ref test
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 :) 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. 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. 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’. (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? 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). (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. 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. (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? (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? 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. (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. Created attachment 133293 [details]
First pass
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. (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 :/ (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 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 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. 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 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. Created attachment 133685 [details]
Update per reviewer comments
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 on attachment 133685 [details] Update per reviewer comments Clearing flags on attachment: 133685 Committed r112091: <http://trac.webkit.org/changeset/112091> All reviewed patches have been landed. Closing bug. |