Bug 53512

Summary: [CG] getBBox() on a SVGPathElement with curves incorrectly includes control points
Product: WebKit Reporter: Alexis Deveria <adeveria>
Component: SVGAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, jeffschiller, kling, krit, rwlbuis, simon.fraser, thorton, webkit.review.bot, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 67280, 67323, 71766    
Bug Blocks:    
Attachments:
Description Flags
Shows a path with a rectangle generated on top of it with the calculated BBox values
none
pure-CG non-reproducing case
none
Screenshot of problem in OS X and Linux
none
Patch
none
Patch plus Chromium expectations
none
new patch, with split boundingRect
none
patch + caching bbox
none
style fix
oliver: review+, webkit.review.bot: commit-queue-
Bounding box of path is rendered incorrectly. Its dimensions are captured using the getBbox function and they are too big. none

Description Alexis Deveria 2011-02-01 10:43:38 PST
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.
Comment 1 Dirk Schulze 2011-05-22 06:25:33 PDT
(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.
Comment 2 Tim Horton 2011-07-28 17:00:09 PDT
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.
Comment 3 Tim Horton 2011-07-28 17:02:51 PDT
<rdar://problem/9861154>
Comment 4 Nikolas Zimmermann 2011-07-28 17:53:21 PDT
(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}}
Comment 5 Nikolas Zimmermann 2011-07-28 18:00:30 PDT
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
  }
}
Comment 6 Matt Arsenault 2011-08-01 14:48:00 PDT
Created attachment 102561 [details]
Screenshot of problem in OS X and Linux

Also looks like it happens on Chromium/Linux
Comment 7 Tim Horton 2011-08-01 14:51:31 PDT
Thanks, Matt. Changed the title correspondingly.
Comment 8 Tim Horton 2011-08-05 16:12:04 PDT
(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?
Comment 9 Rob Buis 2011-08-05 17:50:38 PDT
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.
Comment 10 Dirk Schulze 2011-08-05 23:26:52 PDT
(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
Comment 11 Tim Horton 2011-08-08 10:49:44 PDT
(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.
Comment 12 Tim Horton 2011-08-08 15:03:37 PDT
Created attachment 103298 [details]
Patch
Comment 13 WebKit Review Bot 2011-08-08 15:25:47 PDT
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
Comment 14 Tim Horton 2011-08-09 13:48:20 PDT
Cloned to https://bugs.webkit.org/show_bug.cgi?id=65939 for the Chromium case.
Comment 15 Tim Horton 2011-08-09 14:01:58 PDT
Created attachment 103393 [details]
Patch plus Chromium expectations
Comment 16 Dirk Schulze 2011-08-30 12:38:15 PDT
Comment on attachment 103393 [details]
Patch plus Chromium expectations

Soory, took a while :P The pack might be outdated, please land it manually.
Comment 17 Tim Horton 2011-08-30 15:11:16 PDT
(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.
Comment 18 Simon Fraser (smfr) 2011-08-31 14:08:46 PDT
Caused bug 67318?
Comment 19 Tim Horton 2011-08-31 14:38:17 PDT
Need to investigate the test failures mentioned in bug 67318.
Comment 20 Tim Horton 2011-08-31 16:47:45 PDT
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.
Comment 21 Tim Horton 2011-09-08 16:43:31 PDT
Created attachment 106813 [details]
new patch, with split boundingRect

Not quite ready for commit yet (the TODO in SVGPathElement).
Comment 22 Tim Horton 2011-09-09 16:32:54 PDT
Created attachment 106937 [details]
patch + caching bbox
Comment 23 WebKit Review Bot 2011-09-09 16:37:31 PDT
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.
Comment 24 Tim Horton 2011-09-09 16:43:22 PDT
Created attachment 106938 [details]
style fix
Comment 25 Adam Barth 2011-10-15 01:52:17 PDT
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.
Comment 26 Oliver Hunt 2011-11-02 14:35:38 PDT
Comment on attachment 106938 [details]
style fix

r+
Comment 27 WebKit Review Bot 2011-11-02 14:39:32 PDT
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
Comment 28 Tim Horton 2011-11-07 12:40:54 PST
(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.
Comment 29 Nicholas Kyriakides 2013-02-18 17:47:37 PST
Created attachment 188969 [details]
Bounding box of path is rendered incorrectly. Its dimensions are captured using the getBbox function and they are too big.