Summary: | [CG] getBBox() on a SVGPathElement with curves incorrectly includes control points | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexis Deveria <adeveria> | ||||||||||||||||||||
Component: | SVG | Assignee: | 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: |
|
(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. 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.
(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}} 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 } } Created attachment 102561 [details]
Screenshot of problem in OS X and Linux
Also looks like it happens on Chromium/Linux
Thanks, Matt. Changed the title correspondingly. (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? 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. (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 (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. Created attachment 103298 [details]
Patch
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 Cloned to https://bugs.webkit.org/show_bug.cgi?id=65939 for the Chromium case. Created attachment 103393 [details]
Patch plus Chromium expectations
Comment on attachment 103393 [details]
Patch plus Chromium expectations
Soory, took a while :P The pack might be outdated, please land it manually.
(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. Caused bug 67318? Need to investigate the test failures mentioned in bug 67318. 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. Created attachment 106813 [details]
new patch, with split boundingRect
Not quite ready for commit yet (the TODO in SVGPathElement).
Created attachment 106937 [details]
patch + caching bbox
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.
Created attachment 106938 [details]
style fix
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 on attachment 106938 [details]
style fix
r+
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 (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. Created attachment 188969 [details]
Bounding box of path is rendered incorrectly. Its dimensions are captured using the getBbox function and they are too big.
|
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.