Closed Bug 736031 Opened 8 years ago Closed 8 years ago

getBBox returns incorrect results with empty containers

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla14
Tracking Status
firefox11 --- unaffected
firefox12 + unaffected
firefox13 + unaffected
firefox14 + verified
firefox-esr10 --- unaffected

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

(Keywords: regression, testcase)

Attachments

(3 files, 3 obsolete files)

Attached file testcase
No description provided.
Keywords: regression
Keywords: testcase
Depends on: 647914
Attached patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #606160 - Flags: review?(jwatt)
Comment on attachment 606160 [details] [diff] [review]
patch

Review of attachment 606160 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/test/bbox-helper.svg
@@ +13,5 @@
>    <g id="h">
>      <circle cx="200" cy="50" r="5"/>
>      <path d="M 200,100 L 300,100"/>
>    </g>
> +  <g id="e">

Please add this here:

  <!-- empty container should not affect parent's bbox -->

::: layout/svg/base/src/nsSVGContainerFrame.cpp
@@ +262,5 @@
>      nsISVGChildFrame* svgKid = do_QueryFrame(kid);
>      if (svgKid) {
>        gfxMatrix transform = aToBBoxUserspace;
>        nsIContent *content = kid->GetContent();
> +      if (content->IsSVG()) {

Ah, yes, that would be rather a redundant check.
Attachment #606160 - Flags: review?(jwatt) → review+
Flags: in-testsuite+
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/2205ed382ab0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 647914
No longer depends on: 647914
Hmm, change:

+  <g id="e">
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+  </g>

to:

+  <g id="e">
+    <g/>
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+  </g>

and the test will fail, right?
Err, I mean:

+  <g id="e">
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+    <g/>
+  </g>
Attached patch patch 2 (obsolete) — Splinter Review
Attachment #606988 - Flags: review?(jwatt)
This raises the question of whether or not a zero length path that still draws a circle/square due to linecap should contribute to the bbox or not. It seems that if the bbox is supposed to roughly be the bounds of the objects that paint something, then it should. (I think it should.) If so, then it would be better to also check that x and y are zero too, something like this:

  if (childBBox == gfxRect(0,0,0,0)) {
    // Assume the child is an empty container, and ignore it. There's a tiny chance
    // that someone someday will have a zero length path with a linecap that they
    // want included in the bbox that happens to be positioned exactly at
    // x=0,y=0, but that's going to be a very rare case.
    continue;
  }

Also please have the test do the following to more thoroughly test this:

+  <g id="e">
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+    <g/>
+    <circle cx="100" cy="100" r="5"/>
+    <g/>
+  </g>

And please add a test to make sure that zero length path with a linecap _does_ contribute.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
In fact the existing comment could do with some rewording, and breaking at 80 chars. If you rename |firstChild| to |firstChildWithBBox|, the code should look like this I think:

      gfxRect childBBox = svgKid->GetBBoxContribution(transform, aFlags);

      if (childBBox == gfxRect(0,0,0,0)) {
        // Assume the child is an empty container, and ignore it. There's a
        // tiny chance that someone someday will want a zero length path with a
        // linecap that happens to be positioned exactly at x=0,y=0 to be
        // included in a bbox, but that's going to be very rare since zero
        // length paths are themselves rare.
        continue;
      }
      // We need to include zero width/height vertical/horizontal lines, so we
      // have to use UnionEdges below, but that means that we must special case
      // the first bbox so that we don't include the initial gfxRect(0,0,0,0)
      // value of bboxUnion.
      if (firstChildWithBBox) {
        bboxUnion = childBBox;
        firstChildWithBBox = false;
        continue;
      }
      bboxUnion = bboxUnion.UnionEdges(childBBox);
Actually, I guess that second comment should change to something more like:

      // We need to include zero width/height vertical/horizontal lines, and
      // zero length paths that might have linecaps, so we have to use
      // UnionEdges below. That means that we must special case the first bbox
      // so that we don't include the initial gfxRect(0,0,0,0) value of
      // bboxUnion.
(In reply to Jonathan Watt [:jwatt] from comment #8)
> This raises the question of whether or not a zero length path that still
> draws a circle/square due to linecap should contribute to the bbox or not.
> It seems that if the bbox is supposed to roughly be the bounds of the
> objects that paint something, then it should. (I think it should.) If so,
> then it would be better to also check that x and y are zero too, something
> like this:
> 

I decided to fix zero lengh path bounds properly in another bug.
Attached patch patch (obsolete) — Splinter Review
Attachment #606988 - Attachment is obsolete: true
Attachment #606988 - Flags: review?(jwatt)
Attachment #607225 - Flags: review?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #8)
> This raises the question of whether or not a zero length path that still
> draws a circle/square due to linecap should contribute to the bbox or not.
> It seems that if the bbox is supposed to roughly be the bounds of the
> objects that paint something, then it should. (I think it should.) If so,
> then it would be better to also check that x and y are zero too, something
> like this:
> 
>   if (childBBox == gfxRect(0,0,0,0)) {
>     // Assume the child is an empty container, and ignore it. There's a tiny
> chance
>     // that someone someday will have a zero length path with a linecap that
> they
>     // want included in the bbox that happens to be positioned exactly at
>     // x=0,y=0, but that's going to be a very rare case.
>     continue;
>   }
> 

Zero length subpaths don't have zero height and width unless they have no stroke in which case they aren't painted and don't count. So with or without my multiple zero length subpath bugfix we don't need to account for them here.
(In reply to Robert Longson from comment #13)
> Zero length subpaths don't have zero height and width unless they have no
> stroke in which case they aren't painted and don't count. So with or without
> my multiple zero length subpath bugfix we don't need to account for them
> here.

Of course this is getBBox so stroke doesn't count. So the question is does the fill of a zero length subpath count towards getBBox and I'd argue it doesn't on its own as only the stroke makes it visible.
(In reply to Jonathan Watt [:jwatt] from comment #9)
> 
>       gfxRect childBBox = svgKid->GetBBoxContribution(transform, aFlags);
> 
>       if (childBBox == gfxRect(0,0,0,0)) {
>         // Assume the child is an empty container, and ignore it. There's a
>         // tiny chance that someone someday will want a zero length path
> with a
>         // linecap that happens to be positioned exactly at x=0,y=0 to be
>         // included in a bbox, but that's going to be very rare since zero
>         // length paths are themselves rare.

At the moment and with/without my other patch in the other bug you are reviewing all zero length paths return (0,0,0,0) wherever they are located as we only count such paths when there is a stroke and we're not ignoring the stroke here.
I backed out bug 647914 on aurora (firefox12) & beta (firefox13), so those are now [unaffected] by this bug here.  Setting status flags accordingly.
(In reply to Robert Longson from comment #14)
> Of course this is getBBox so stroke doesn't count. So the question is does
> the fill of a zero length subpath count towards getBBox and I'd argue it
> doesn't on its own as only the stroke makes it visible.

Hmm. getBBox is supposed to give the user a rough approximation of the bounds of the area painted by a given part of the graphic. The only reason it's based only on path bounds is because the spec authors (correctly) worried about implementation performance if it had to return the exact paint bounds, taking stroke, dashing, clipping, masking, filtering, markers etc. into account. So while the line of what implementations should take into account was drawn at "just the path bounds", it is still meant to be API to get an approximation of the paint bounds. Given that, I definitely think that zero size objects and zero length [sub]paths should contribute to getBBox if they result in paint being laid down due to stroke.
I'm kind of thinking that it's a mistake that GetBBoxContribution returns a gfxRect. Seems like it should probably return an instance of something like the following so that we can distinguish between "zero size" and "no contribution":

struct SVGBBox {
  gfxRect bbox;
  bool hasBBox;
}

Or something like that. What do you think?
Attached patch address review comments (obsolete) — Splinter Review
Attachment #607225 - Attachment is obsolete: true
Attachment #607225 - Flags: review?(jwatt)
Attachment #615055 - Flags: review?(jwatt)
Comment on attachment 615055 [details] [diff] [review]
address review comments

Review of attachment 615055 [details] [diff] [review]:
-----------------------------------------------------------------

I'm much happier with this, thanks a lot Robert! r=me, with a few comments.

::: content/svg/content/test/bbox-helper.svg
@@ +17,5 @@
>    <g id="e">
>      <!-- empty container should not affect parent's bbox -->
>      <g/>
>      <circle cx="100" cy="100" r="5"/>
> +    <g/>

Can you add an extra:

    <circle cx="100" cy="100" r="5"/>
    <g/>

Just so that there's an empty container between two shapes with valid bbox too?

::: layout/svg/base/src/nsSVGMarkerFrame.cpp
@@ +182,5 @@
>                                            PRUint32 aFlags,
>                                            nsSVGPathGeometryFrame *aMarkedFrame,
>                                            const nsSVGMark *aMark,
>                                            float aStrokeWidth)
>  {

To get NRVO you could declare |SVGBBox bbox;| here, and return it at the early return paints. Up to you.

@@ +215,1 @@
>    bool firstChild = true;

firstChild can die.

::: layout/svg/base/src/nsSVGPathGeometryFrame.cpp
@@ +265,3 @@
>  nsSVGPathGeometryFrame::GetBBoxContribution(const gfxMatrix &aToBBoxUserspace,
>                                              PRUint32 aFlags)
>  {

Again, you could put |SVGBBox bbox;| here and return that from the early return points.

@@ +266,5 @@
>                                              PRUint32 aFlags)
>  {
>    if (aToBBoxUserspace.IsSingular()) {
>      // XXX ReportToConsole
>      return gfxRect(0.0, 0.0, 0.0, 0.0);

In fact, you want to return an explicit SVGBBox here so that this doesn't contribute.

::: layout/svg/base/src/nsSVGUtils.cpp
@@ -1544,3 @@
>    }
> -  NS_ASSERTION(bbox.Width() >= 0.0 && bbox.Height() >= 0.0, "Invalid bbox!");
> -  return bbox;

I think the idea here was NRVO, but not a big deal.

::: layout/svg/base/src/nsSVGUtils.h
@@ +152,5 @@
> +  SVGBBox() 
> +    : mIsEmpty(true) {}
> +
> +  SVGBBox(const gfxRect& aRect) 
> +    : mBBox(aRect), mIsEmpty(false) {}

Some folks might prefer that this be |explicit|, but I think it's okay in this case.
Attachment #615055 - Flags: review?(jwatt) → review+
Oh, and for the comment documenting SVGBBox, can you make it:

/**
 * ...
 */

instead of:

/*
 *
 */

so that it gets picked up by doxygen properly?
Attachment #615055 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/01fd11649cc8
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
The square from the testcase looks OK on FF 14b9 on Win 7, Ubuntu 12.04 and Mac OS X 10.6. Verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.