Bug 53512 – [CG] getBBox() on a SVGPathElement with curves incorrectly includes control points

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

[Alexis Deveria]

Created attachment 80784 [details]
Shows a path with a rectangle generated on top of it with the calculated BBox values

Running getBBox() on paths with cubic bezier curves results in a box that is too big. It appears the curve's control points are included, which is of course incorrect. Non-Webkit browsers get this right.

[Dirk Schulze]

(In reply to comment #0)
> Created an attachment (id=80784) [details]
> Shows a path with a rectangle generated on top of it with the calculated BBox values
> 
> Running getBBox() on paths with cubic bezier curves results in a box that is too big. It appears the curve's control points are included, which is of course incorrect. Non-Webkit browsers get this right.

I doubt that this is a problem in WebKit. I guess CG doesn't get it right. Bounding boxes on paths are always calculated by the graphic engines. Adapting bug title.

[Tim Horton]

Created attachment 102313 [details]
pure-CG non-reproducing case

I haven't taken enough of a look yet, but attached is a pure-CG example that gets the bounding box for exactly the same shape perfectly correct.

So, it could still be CG, if we're getting it in some weird state, but someone will have to dig deeper.

[Tim Horton]

<rdar://problem/9861154>

[Nikolas Zimmermann]

(In reply to comment #2)
> Created an attachment (id=102313) [details]
> pure-CG non-reproducing case
> 
> I haven't taken enough of a look yet, but attached is a pure-CG example that gets the bounding box for exactly the same shape perfectly correct.
> 
> So, it could still be CG, if we're getting it in some weird state, but someone will have to dig deeper.

Great catch! I traced down what we're passing to CG, by breaking on SVGPathBuilder::moveTo/lineTo/curveToCubic.

Let's compare your testcase and what we feed to CG:

1.
<code>
    CGMutablePathRef path = CGPathCreateMutable();
    CGPathMoveToPoint(path, NULL, 131, 210);
</code>

WebCore::Path::moveTo (this=0x21bd315c, point=@0x21b5bbe8) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/PathCG.cpp:191
191	    CGPathMoveToPoint(m_path, 0, point.x(), point.y());
(gdb) p point
$9 = (const 'WebCore::FloatPoint' &) @0x21b5bbe8: {
  m_x = 131, 
  m_y = 210
}

That's the same.

2.
<code>
    CGPathAddCurveToPoint(path, NULL, 152, 173, 207, 125, 240, 177);
</code>

WebCore::Path::addBezierCurveTo (this=0x21bd315c, cp1=@0xbfffbd08, cp2=@0xbfffbd00, p=@0x21b5bbe8) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/PathCG.cpp:206
206	    CGPathAddCurveToPoint(m_path, 0, cp1.x(), cp1.y(), cp2.x(), cp2.y(), p.x(), p.y());
(gdb) p cp1
$10 = (const 'WebCore::FloatPoint' &) @0xbfffbd08: {
  m_x = 152, 
  m_y = 173
}
(gdb) p cp2
$11 = (const 'WebCore::FloatPoint' &) @0xbfffbd00: {
  m_x = 207, 
  m_y = 125
}
(gdb) p p
$12 = (const 'WebCore::FloatPoint' &) @0x21b5bbe8: {
  m_x = 240, 
  m_y = 177
}

That's the same.

3.
<code>
    CGPathAddCurveToPoint(path, NULL, 273, 229, 211, 293, 193, 257);
</code>

WebCore::Path::addBezierCurveTo (this=0x21bd315c, cp1=@0xbfffbd08, cp2=@0xbfffbd00, p=@0x21b5bbe8) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/PathCG.cpp:206
206	    CGPathAddCurveToPoint(m_path, 0, cp1.x(), cp1.y(), cp2.x(), cp2.y(), p.x(), p.y());
(gdb) p cp1
$13 = (const 'WebCore::FloatPoint' &) @0xbfffbd08: {
  m_x = 273, 
  m_y = 229
}
(gdb) p cp2
$14 = (const 'WebCore::FloatPoint' &) @0xbfffbd00: {
  m_x = 211, 
  m_y = 293
}
(gdb) p p
$15 = (const 'WebCore::FloatPoint' &) @0x21b5bbe8: {
  m_x = 193, 
  m_y = 257
}

That's the same.

4.
<code>
    CGPathAddCurveToPoint(path, NULL, 175, 221, 110, 247, 131, 210);
</code>

WebCore::Path::addBezierCurveTo (this=0x21bd315c, cp1=@0xbfffbd08, cp2=@0xbfffbd00, p=@0x21b5bbe8) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/PathCG.cpp:206
206	    CGPathAddCurveToPoint(m_path, 0, cp1.x(), cp1.y(), cp2.x(), cp2.y(), p.x(), p.y());
(gdb) p cp1
$16 = (const 'WebCore::FloatPoint' &) @0xbfffbd08: {
  m_x = 175, 
  m_y = 221
}
(gdb) p cp2
$17 = (const 'WebCore::FloatPoint' &) @0xbfffbd00: {
  m_x = 110, 
  m_y = 247
}
(gdb) p p
$18 = (const 'WebCore::FloatPoint' &) @0x21b5bbe8: {
  m_x = 131, 
  m_y = 210
}

That's the same.

5.
<code>
    CGPathCloseSubpath(path);
</code>

WebCore::Path::closeSubpath (this=0x21bd315c) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/PathCG.cpp:216
216	    CGPathCloseSubpath(m_path);

Also the same!
So we're constructing the path in an identical way.

Okay, where's the problem?
Stepping down from my last breakpoint SVGPathBuilder::closePath shows the answer:

Breakpoint 4, WebCore::SVGPathBuilder::closePath (this=0x108ca590) at SVGPathBuilder.cpp:66
66	    ASSERT(m_path);
(gdb) s
67	    m_path->closeSubpath();
(gdb) fin
Run till exit from #0  WebCore::SVGPathBuilder::closePath (this=0x108ca590) at SVGPathBuilder.cpp:67
WebCore::SVGPathParser::parseClosePathSegment (this=0x108c49c0) at SVGPathParser.cpp:48
48	}
(gdb) fin
Run till exit from #0  WebCore::SVGPathParser::parseClosePathSegment (this=0x108c49c0) at SVGPathParser.cpp:48
WebCore::SVGPathParser::parsePathDataFromSource (this=0x108c49c0, pathParsingMode=WebCore::NormalizedParsing) at SVGPathParser.cpp:331
331	            break;
(gdb) fin
Run till exit from #0  WebCore::SVGPathParser::parsePathDataFromSource (this=0x108c49c0, pathParsingMode=WebCore::NormalizedParsing) at SVGPathParser.cpp:331
0x09b421ca in WebCore::SVGPathParserFactory::buildPathFromByteStream (this=0x108c4990, stream=0x108c4050, result=@0x108c4d9c) at SVGPathParserFactory.cpp:172
172	    bool ok = parser->parsePathDataFromSource(NormalizedParsing);
Value returned is $59 = true
(gdb) p (CGRect)CGPathGetPathBoundingBox(result.m_path)
$60 = {
  origin = {
    x = 126.869362, 
    y = 154.158096
  }, 
  size = {
    width = 122.646141, 
    height = 113.340134
  }
}
(gdb) 

All fine here at this point!

(gdb) fin
Run till exit from #0  0x09b421ca in WebCore::SVGPathParserFactory::buildPathFromByteStream (this=0x11a2f140, stream=0x11a361a0, result=@0x11a3fc6c) at SVGPathParserFactory.cpp:172
WebCore::SVGPathElement::toPathData (this=0x11a54f10, path=@0x11a3fc6c) at SVGPathElement.cpp:334
334	}
Value returned is $70 = true
(gdb) fin
Run till exit from #0  WebCore::SVGPathElement::toPathData (this=0x11a54f10, path=@0x11a3fc6c) at SVGPathElement.cpp:334
WebCore::RenderSVGPath::layout (this=0x11a3fc4c) at RenderSVGPath.cpp:120
120	        m_needsPathUpdate = false;
(gdb) pQuit
(gdb) p (CGRect)CGPathGetPathBoundingBox(m_path.m_path)
$71 = {
  origin = {
    x = 126.869362, 
    y = 154.158096
  }, 
  size = {
    width = 122.646141, 
    height = 113.340134
  }
}
(gdb) 

Still fine.

Stepping on...

359	    m_fillBoundingBox = m_path.boundingRect();
Value returned is $74 = {m_location = {m_x = 110, m_y = 125}, m_size = {m_width = 163, m_height = 168}}


FloatRect Path::boundingRect() const
{
    return CGPathGetBoundingBox(m_path);
}

Funny bug, eh? :-) The value magically is truncated.


I've stepped through WebCore::SVGPathParserFactory::buildPathFromByteStream (this=0x209b8200, stream=0x209b7440, result=@0x21bd315c) at SVGPathParserFactory.cpp:174
and it turns out that
(gdb) p (CGRect)CGPathGetPathBoundingBox(result.m_path)
$23 = {
  origin = {
    x = 126.869362, 
    y = 154.158096
  }, 
  size = {
    width = 122.646141, 
    height = 113.340134
  }


Your program gives:
x=126.869362, y=154.158096, w=122.646141, h=113.340134

At this point your CG demo program and WebCore still agree!

Breaking on the next call of Path::boundingRect(), gives:
0x09a2a92d in WebCore::RenderSVGPath::updateCachedBoundaries (this=0x21bd313c) at RenderSVGPath.cpp:359
359	    m_fillBoundingBox = m_path.boundingRect();
Value returned is $25 = {m_location = {m_x = 110, m_y = 125}, m_size = {m_width = 163, m_height = 168}}

[Nikolas Zimmermann]

Okay I found the inconsistency, any theories? :-)

WebCore::Path::boundingRect (this=0x11a38aec) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/PathCG.cpp:166
166	    return CGPathGetBoundingBox(m_path);
(gdb) p (CGRect)CGPathGetPathBoundingBox(m_path)
$91 = {
  origin = {
    x = 126.869362, 
    y = 154.158096
  }, 
  size = {
    width = 122.646141, 
    height = 113.340134
  }
}
(gdb) s
WebCore::FloatRect::FloatRect (this=0xbfffbd70, r=@0xbfffbd40) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/FloatRectCG.cpp:36
36	FloatRect::FloatRect(const CGRect& r) : m_location(r.origin), m_size(r.size)
(gdb) p r
$92 = (const CGRect &) @0xbfffbd40: {
  origin = {
    x = 110, 
    y = 125
  }, 
  size = {
    width = 163, 
    height = 168
  }
}

[Matt Arsenault]

Created attachment 102561 [details]
Screenshot of problem in OS X and Linux

Also looks like it happens on Chromium/Linux

[Tim Horton]

Thanks, Matt. Changed the title correspondingly.

[Tim Horton]

(In reply to comment #7)
> Thanks, Matt. Changed the title correspondingly.

So, I don't know why it happens on Chromium/Linux, but the reason it happens on OS X is clear:

My example program used CGPathGetPathBoundingBox, which doesn't include the control points, but WebKit uses CGPathGetBoundingBox, which *does*. The reason Niko's seeing confusing things in GDB is that CGPathGetBoundingBox must call CGPathGetPathBoundingBox.

I just made a build using CGPathGetPathBounding box and it perfectly bounds just the shape.

However, I don't think we can use CGPathGetPathBoundingBox, since it's only available on 10.6 and above.

What's the correct way to fix this?

[Rob Buis]

Hi Tim,

(In reply to comment #8)
> (In reply to comment #7)
> > Thanks, Matt. Changed the title correspondingly.
> 
> So, I don't know why it happens on Chromium/Linux, but the reason it happens on OS X is clear:
> 
> My example program used CGPathGetPathBoundingBox, which doesn't include the control points, but WebKit uses CGPathGetBoundingBox, which *does*. The reason Niko's seeing confusing things in GDB is that CGPathGetBoundingBox must call CGPathGetPathBoundingBox.
> 
> I just made a build using CGPathGetPathBounding box and it perfectly bounds just the shape.
> 
> However, I don't think we can use CGPathGetPathBoundingBox, since it's only available on 10.6 and above.
> 
> What's the correct way to fix this?

Can you just ifdef the use of CGPathGetPathBoundingBox so at least it is fixed for >= 10.6?
Cheers,

Rob.

[Dirk Schulze]

(In reply to comment #9)
> Hi Tim,
> 
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Thanks, Matt. Changed the title correspondingly.
> > 
> > So, I don't know why it happens on Chromium/Linux, but the reason it happens on OS X is clear:
> > 
> > My example program used CGPathGetPathBoundingBox, which doesn't include the control points, but WebKit uses CGPathGetBoundingBox, which *does*. The reason Niko's seeing confusing things in GDB is that CGPathGetBoundingBox must call CGPathGetPathBoundingBox.
> > 
> > I just made a build using CGPathGetPathBounding box and it perfectly bounds just the shape.
> > 
> > However, I don't think we can use CGPathGetPathBoundingBox, since it's only available on 10.6 and above.
> > 
> > What's the correct way to fix this?
> 
> Can you just ifdef the use of CGPathGetPathBoundingBox so at least it is fixed for >= 10.6?
> Cheers,
> 
> Rob.

I agree with Rob, we did this in the past, so I don't see problems. The compiler flag for 10.5 was TIGER IIRC

[Tim Horton]

(In reply to comment #10)
> > 
> > Can you just ifdef the use of CGPathGetPathBoundingBox so at least it is fixed for >= 10.6?
> > Cheers,
> > 
> > Rob.
> 
> I agree with Rob, we did this in the past, so I don't see problems. The compiler flag for 10.5 was TIGER IIRC

Oh, of course. I'll post a patch sometime today.

[Tim Horton]

Created attachment 103298 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103298 [details]
Patch

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

New failing tests:
svg/custom/getBBox-path.svg

[Tim Horton]

Cloned to https://bugs.webkit.org/show_bug.cgi?id=65939 for the Chromium case.

[Tim Horton]

Created attachment 103393 [details]
Patch plus Chromium expectations

[Dirk Schulze]

Comment on attachment 103393 [details]
Patch plus Chromium expectations

Soory, took a while :P The pack might be outdated, please land it manually.

[Tim Horton]

(In reply to comment #16)
> (From update of attachment 103393 [details])
> Soory, took a while :P The pack might be outdated, please land it manually.

Landed in r94116 to fix the CG case.

[Simon Fraser (smfr)]

Caused bug 67318?

[Tim Horton]

Need to investigate the test failures mentioned in bug 67318.

[Tim Horton]

This morning, Andreas was talking about the possibility of using fast (rough, including control points, etc.) bounding-box computation for repaint and other things, and just using the slower but more accurate method for getBBox calls. So that should potentially also be considered before we re-commit.

[Tim Horton]

Created attachment 106813 [details]
new patch, with split boundingRect

Not quite ready for commit yet (the TODO in SVGPathElement).

[Tim Horton]

Created attachment 106937 [details]
patch + caching bbox

[WebKit Review Bot]

Attachment 106937 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/svg/SVGPathElement.cpp:377:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 106938 [details]
style fix

[Adam Barth]

Comment on attachment 103393 [details]
Patch plus Chromium expectations

Cleared Dirk Schulze's review+ from obsolete attachment 103393 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Oliver Hunt]

Comment on attachment 106938 [details]
style fix

r+

[WebKit Review Bot]

Comment on attachment 106938 [details]
style fix

Rejecting attachment 106938 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ore/platform/graphics/cg/GraphicsContextCG.cpp
patching file Source/WebCore/platform/graphics/cg/PathCG.cpp
patching file Source/WebCore/rendering/RenderObject.h
Hunk #1 succeeded at 421 (offset 44 lines).
patching file Source/WebCore/rendering/svg/RenderSVGPath.cpp
patching file Source/WebCore/svg/SVGPathElement.cpp
patching file Source/WebCore/svg/SVGPathElement.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10259356

[Tim Horton]

(In reply to comment #27)
> (From update of attachment 106938 [details])
> Rejecting attachment 106938 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> ore/platform/graphics/cg/GraphicsContextCG.cpp
> patching file Source/WebCore/platform/graphics/cg/PathCG.cpp
> patching file Source/WebCore/rendering/RenderObject.h
> Hunk #1 succeeded at 421 (offset 44 lines).
> patching file Source/WebCore/rendering/svg/RenderSVGPath.cpp
> patching file Source/WebCore/svg/SVGPathElement.cpp
> patching file Source/WebCore/svg/SVGPathElement.h
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 1
> 
> Full output: http://queues.webkit.org/results/10259356

Landed manually to update patch 99460.

[Nicholas Kyriakides]

Created attachment 188969 [details]
Bounding box of path is rendered incorrectly. Its dimensions are captured using the getBbox function and they are too big.