736031 - getBBox returns incorrect results with empty containers

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: testcase

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Jonathan Watt [: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.

Comment 3

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/2205ed382ab0

Comment 4

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2205ed382ab0

Comment 5

[Jonathan Watt [:jwatt]]

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?

Comment 6

[Jonathan Watt [:jwatt]]

Err, I mean:

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

Comment 7

[Robert Longson [:longsonr]]

Attachment: patch 2

Comment 8

[Jonathan Watt [: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.

Comment 9

[Jonathan Watt [:jwatt]]

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);

Comment 10

[Jonathan Watt [:jwatt]]

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.

Comment 11

[Robert Longson [:longsonr]]

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

Comment 12

[Robert Longson [:longsonr]]

Attachment: patch

Comment 13

[Robert Longson [:longsonr]]

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

Comment 14

[Robert Longson [:longsonr]]

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

Comment 15

[Robert Longson [:longsonr]]

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

Comment 16

[Daniel Holbert [:dholbert]]

I backed out bug 647914 on aurora (firefox12) & beta (firefox13), so those are now [unaffected] by this bug here.  Setting status flags accordingly.

Comment 17

[Jonathan Watt [:jwatt]]

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

Comment 18

[Jonathan Watt [:jwatt]]

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?

Comment 19

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 20

[Jonathan Watt [: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.

Comment 21

[Jonathan Watt [:jwatt]]

Oh, and for the comment documenting SVGBBox, can you make it:

/**
 * ...
 */

instead of:

/*
 *
 */

so that it gets picked up by doxygen properly?

Comment 22

[Robert Longson [:longsonr]]

Attachment: final review comments addressed

Comment 23

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/01fd11649cc8

Comment 24

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/01fd11649cc8

Comment 25

[Paul Silaghi, QA [:pauly]]

The square from the testcase looks OK on FF 14b9 on Win 7, Ubuntu 12.04 and Mac OS X 10.6. Verified fixed.