Summary: | New renderer for SVGCircleElement | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||||||||||||||||||||||||||||||||
Component: | SVG | Assignee: | Philip Rogers <pdr> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, eric, gustavo, ossy, pnormand, rakuco, rhodovan.u-szeged, schenney, webkit.review.bot, xan.lopez, zimmermann | ||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||
Bug Blocks: | 65236 | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Philip Rogers
2012-03-06 08:13:52 PST
Created attachment 130388 [details]
Draft: new renderer for SVGCircleElement
This patch is just a draft so I didn't update the layout tests (but almost all SVG related tests need it, because DRT will dump RenderSVGCircle instead of RenderSVGPath if the SVG graphics contains a circle object).
Created attachment 130391 [details]
Draft: new renderer for SVGCircleElement
Removed unrelated change that found its way in, and added a proper ChangeLog entry so Niko will be more likely to review :)
Comment on attachment 130391 [details] Draft: new renderer for SVGCircleElement View in context: https://bugs.webkit.org/attachment.cgi?id=130391&action=review Just two short questions... > Source/WebCore/rendering/svg/RenderSVGCircle.cpp:98 > + context->fillEllipse(m_boundingBox); Shouldn't you apply the fallback mechanism here if isPaintingFallback() is true? > Source/WebCore/rendering/svg/RenderSVGCircle.cpp:103 > + context->strokeEllipse(m_boundingBox); The same here. A quick note: this patch is missing the real solution to the performance issue and is just scaffolding at the moment. I think there are two possible routes for this patch: 1) Call through to fast platform fillEllipse/strokeEllipse implementations. 2) Transform a unit circle path instead of creating/copying paths. Created attachment 131038 [details]
Update per reviewer comments and add faster CG codepath
Some results on themainblue benchmark: http://themaninblue.com/experiment/AnimationBenchmark/svg/?particles=3000 On CG I'm getting roughly 10% speed increase with this patch (18fps vs 20fps). Similarly, on a benchmark I made that stresses this codepath: http://philbit.com/circleBench.html (Click the bottom link until it says "SVG with circle"). CG without patch: 60ms/frame, CG with patch: 30ms/frame I've talked with the Skia team and we need to add a fastpath there before we will see a speed increase on Skia, and this patch is currently a wash on Skia. I'm following up with them at the moment, but I think this patch can be landed as a first-pass. I expect to see similar speed improvements on Skia. Created attachment 131087 [details]
Patch with expectations!
Created attachment 131108 [details]
Updated patch to head so it should apply cleanly
Comment on attachment 131108 [details] Updated patch to head so it should apply cleanly Attachment 131108 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11894833 Comment on attachment 131108 [details] Updated patch to head so it should apply cleanly Attachment 131108 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11916014 Comment on attachment 131108 [details] Updated patch to head so it should apply cleanly Attachment 131108 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11906830 Comment on attachment 131108 [details] Updated patch to head so it should apply cleanly View in context: https://bugs.webkit.org/attachment.cgi?id=131108&action=review r- for the build problems across the EWS, and some general issues: > Source/WebCore/ChangeLog:8 > + This patch introduces a special renderer for SVGCircleElements to avoid I'm wondering why you didn't do this for SVGEllipseElement, and special case circles in the RenderSVGEllipse renderer? Platforms could still use their fast paths as soon as we call gc->fill/strokeEllipse. > Source/WebCore/platform/graphics/GraphicsContext.cpp:782 > +#if !USE(CG) > +void GraphicsContext::fillEllipse(const FloatRect& ellipse) > +{ > + if (paintingDisabled()) I don't like this pattern. We should rather use: void GraphicsContext::fillEllipse(const FloatRect& ellipse) { platformFillEllipse(ellipse); } If a platform wants to optimize ellipse, they put && !USE(MYPLATFORM) here, and then have to supply GraphicsContext::platformFillEllipse, in their platform implementation. That can then still use fillEllipseAsPath() when it encounters cases where it can't use the fast path. Sharing the fallback path through all platforms is important! #if !USE(CG) // append && !USE(MYPLATFORM) here, if you want to optimize ellipses for your platform... void GraphicsContext::platformFillEllipse(const FloatRect& ellipse) { fillEllipseAsPath(ellipse); } #endif void GraphicsContext::fillEllipseAsPath(const FloatRect& ellipse) // Make this a private method of GraphicsContext. { Path path; path.addEllipse(...); fillPath(path); } Seems cleaner, no? > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1647 > + // FIXME: Implement faster fill without falling back to path rendering. This patch makes me think CGContextFillEllipseInRect will never support gradients: https://bugs.webkit.org/attachment.cgi?id=95657&action=prettypatch According to CGContext reference its only fills with solid color. So this FIXME belongs in CG rather then here :-) > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1651 > + Path path; > + path.addEllipse(ellipse); > + fillPath(path); > + return; This fallback path should be shared cross-platform, that's why I'm proposing the new design above. > Source/WebCore/rendering/svg/RenderSVGCircle.cpp:40 > +: RenderSVGShape(node) Indentation, 4 spaces please. > Source/WebCore/rendering/svg/RenderSVGCircle.cpp:57 > + SVGCircleElement* circle = static_cast<SVGCircleElement*>(node()); > + ASSERT(circle); I'd generalize this for both circle & ellipse elements. Introduce a calculateRadiiAndCentroid() method, that either takes these values from a circle or ellipse element, you can check via hasTagName. > Source/WebCore/rendering/svg/RenderSVGCircle.cpp:127 > + float distanceSquared = (point - m_centroid).diagonalLengthSquared(); > + return distanceSquared <= (m_radius * m_radius); That's faster than "return (point - m_centroid).diagonalLength() <= radius". If this is not hot in profiles, I'd revert this. Assuming we'l never have negative radii, this is an equivalent comparison. > LayoutTests/ChangeLog:7 > + You could add some more context here. Created attachment 132285 [details]
Updated patch per reviewer comments
(In reply to comment #12) > (From update of attachment 131108 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131108&action=review > > r- for the build problems across the EWS, and some general issues: > > > Source/WebCore/ChangeLog:8 > > + This patch introduces a special renderer for SVGCircleElements to avoid > > I'm wondering why you didn't do this for SVGEllipseElement, and special case circles in the RenderSVGEllipse renderer? > Platforms could still use their fast paths as soon as we call gc->fill/strokeEllipse. Done. > > > Source/WebCore/platform/graphics/GraphicsContext.cpp:782 > > +#if !USE(CG) > > +void GraphicsContext::fillEllipse(const FloatRect& ellipse) > > +{ > > + if (paintingDisabled()) > > I don't like this pattern. We should rather use: > void GraphicsContext::fillEllipse(const FloatRect& ellipse) > { > platformFillEllipse(ellipse); > } > > If a platform wants to optimize ellipse, they put && !USE(MYPLATFORM) here, and then have to supply GraphicsContext::platformFillEllipse, in their platform implementation. That can then still use fillEllipseAsPath() when it encounters cases where it can't use the fast path. Sharing the fallback path through all platforms is important! > > #if !USE(CG) // append && !USE(MYPLATFORM) here, if you want to optimize ellipses for your platform... > void GraphicsContext::platformFillEllipse(const FloatRect& ellipse) { fillEllipseAsPath(ellipse); } > #endif > > void GraphicsContext::fillEllipseAsPath(const FloatRect& ellipse) // Make this a private method of GraphicsContext. > { > Path path; > path.addEllipse(...); > fillPath(path); > } > > Seems cleaner, no? Agreed! I went with this approach in the newest patch. > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1647 > > + // FIXME: Implement faster fill without falling back to path rendering. > > This patch makes me think CGContextFillEllipseInRect will never support gradients: > https://bugs.webkit.org/attachment.cgi?id=95657&action=prettypatch > > According to CGContext reference its only fills with solid color. So this FIXME belongs in CG rather then here :-) Done. > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1651 > > + Path path; > > + path.addEllipse(ellipse); > > + fillPath(path); > > + return; > > This fallback path should be shared cross-platform, that's why I'm proposing the new design above. > > > Source/WebCore/rendering/svg/RenderSVGCircle.cpp:40 > > +: RenderSVGShape(node) > > Indentation, 4 spaces please. Done. > > > Source/WebCore/rendering/svg/RenderSVGCircle.cpp:57 > > + SVGCircleElement* circle = static_cast<SVGCircleElement*>(node()); > > + ASSERT(circle); > > I'd generalize this for both circle & ellipse elements. > Introduce a calculateRadiiAndCentroid() method, that either takes these values from a circle or ellipse element, you can check via hasTagName. Done. > > > Source/WebCore/rendering/svg/RenderSVGCircle.cpp:127 > > + float distanceSquared = (point - m_centroid).diagonalLengthSquared(); > > + return distanceSquared <= (m_radius * m_radius); > > That's faster than "return (point - m_centroid).diagonalLength() <= radius". If this is not hot in profiles, I'd revert this. > Assuming we'l never have negative radii, this is an equivalent comparison. The square root calculation is actually pretty slow which is why I did that. I've reworked this due to having to do the ellipse code, but there still may be room for improvement. > > > LayoutTests/ChangeLog:7 > > + > > You could add some more context here. Because the tests are really hard to update (took me 2 hours to generate that expectation file), I've removed them until the code review is complete. Do you mind reviewing this new patch, and then we'll deal with the test expectations before an official r+. Comment on attachment 132285 [details] Updated patch per reviewer comments View in context: https://bugs.webkit.org/attachment.cgi?id=132285&action=review Looks much nicer! Next round of comments: > Source/WebCore/WebCore.gypi:5682 > + 'rendering/svg/RenderSVGEllipse.h', > + 'rendering/svg/RenderSVGEllipse.cpp' cpp before h. Please swap those lines. > Source/WebCore/platform/graphics/GraphicsContext.cpp:813 > +#endif > + > +#if !USE(CG) // append && !USE(MYPLATFORM) here to optimize stroking ellipses for your platform. I'd merge those ifs, its unlikely a platform only wants to optimize stroking ellipses, but not filling. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:64 > + if (style()->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) { > + RenderSVGShape::createShape(); > + setIsPaintingFallback(true); > + return; > + } Isn't this copied from RenderSVGRect? If so how about moving this whole if statement into a bool REnderSVGShape::fallbackToPathPaintingIfNeeded() method or something with similar naming. if (fallbackToPathPaintingIfNeeded()) return; > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:66 > + calculateRadiiAndCentroid(); AndCenter? > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:76 > + float halfStrokeWidth = this->strokeWidth() / 2; > + m_outerStrokeRect.inflate(halfStrokeWidth); Just use m_outerStrokeRect.inflate(strokeWidth() / 2) here. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:90 > + m_centroid = FloatPoint(circle->cx().value(lengthContext), circle->cy().value(lengthContext)); > + } else { Early exit here, to avoid the } else { branch. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:117 > + if (!isPaintingFallback()) { I'd swap the order, use the early return case for the complex code path. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:126 > + if (!isPaintingFallback()) { Ditto. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:136 > +{ > + float halfStrokeWidth = this->strokeWidth() / 2; > + this-> is not needed. Newline can be removed as well > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:137 > + FloatPoint d = FloatPoint(m_centroid.x() - point.x(), m_centroid.y() - point.y()); diameter? > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:146 > + float x_rXOuter = d.x() / (m_radiusX + halfStrokeWidth); // x / (rX + sw/2) > + float y_rYOuter = d.y() / (m_radiusY + halfStrokeWidth); // y / (rY + sw/2) > + > + return (x_rXInner * x_rXInner + y_rYInner * y_rYInner >= 1) && (x_rXOuter * x_rXOuter + y_rYOuter * y_rYOuter <= 1); > +} I'd avoid the pre computation of all these variables here. First check if the outer case is violated, then early exit. If not, check the inner ellipse, etc.. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:153 > + FloatPoint d = FloatPoint(m_centroid.x() - point.x(), m_centroid.y() - point.y()); diameter? > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:158 > + return (x_rX * x_rX + y_rY * y_rY <= 1); Braces not needed. > Source/WebCore/rendering/svg/RenderSVGEllipse.h:31 > +#include "RenderSVGPath.h" s/Path/Shape/ ? > Source/WebCore/rendering/svg/RenderSVGEllipse.h:58 > + FloatPoint m_centroid; m_center sounds better, no? > Source/WebCore/rendering/svg/RenderSVGEllipse.h:60 > + float m_radiusX; > + float m_radiusY; Why not use a FloatSize here? FloatSize m_radii. Attachment 132285 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:140: x_rXInner is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:141: y_rYInner is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:142: x_rXOuter is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:143: y_rYOuter is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:156: x_rX is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:157: y_rY is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 6 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 132285 [details] Updated patch per reviewer comments Attachment 132285 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12134442 Comment on attachment 132285 [details] Updated patch per reviewer comments Attachment 132285 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12128486 Comment on attachment 132285 [details] Updated patch per reviewer comments Attachment 132285 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12131463 Comment on attachment 132285 [details] Updated patch per reviewer comments Attachment 132285 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12129485 Comment on attachment 132285 [details] Updated patch per reviewer comments Attachment 132285 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12134456 Created attachment 133693 [details]
Update per reviewer comments
Note: this patch does not contain the large update of test expectations, which I will add before landing.
Accidentally did an EWS run, I apologize for the spam failures. Comments in-line. (In reply to comment #15) > (From update of attachment 132285 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132285&action=review > > Looks much nicer! Next round of comments: > > > Source/WebCore/WebCore.gypi:5682 > > + 'rendering/svg/RenderSVGEllipse.h', > > + 'rendering/svg/RenderSVGEllipse.cpp' > > cpp before h. Please swap those lines. Done. > > > Source/WebCore/platform/graphics/GraphicsContext.cpp:813 > > +#endif > > + > > +#if !USE(CG) // append && !USE(MYPLATFORM) here to optimize stroking ellipses for your platform. > > I'd merge those ifs, its unlikely a platform only wants to optimize stroking ellipses, but not filling. Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:64 > > + if (style()->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) { > > + RenderSVGShape::createShape(); > > + setIsPaintingFallback(true); > > + return; > > + } > > Isn't this copied from RenderSVGRect? If so how about moving this whole if statement into a bool REnderSVGShape::fallbackToPathPaintingIfNeeded() method or something with similar naming. Unfortunately RenderSVGRect has some different conditions for this which make is hard to unify them (e.g., rects fall back for rounded corners, circles don't). > if (fallbackToPathPaintingIfNeeded()) > return; > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:66 > > + calculateRadiiAndCentroid(); > > AndCenter? Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:76 > > + float halfStrokeWidth = this->strokeWidth() / 2; > > + m_outerStrokeRect.inflate(halfStrokeWidth); > > Just use m_outerStrokeRect.inflate(strokeWidth() / 2) here. Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:90 > > + m_centroid = FloatPoint(circle->cx().value(lengthContext), circle->cy().value(lengthContext)); > > + } else { > > Early exit here, to avoid the } else { branch. Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:117 > > + if (!isPaintingFallback()) { > > I'd swap the order, use the early return case for the complex code path. Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:126 > > + if (!isPaintingFallback()) { > > Ditto. Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:136 > > +{ > > + float halfStrokeWidth = this->strokeWidth() / 2; > > + > > this-> is not needed. Newline can be removed as well Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:137 > > + FloatPoint d = FloatPoint(m_centroid.x() - point.x(), m_centroid.y() - point.y()); > > diameter? Was supposed to mean "difference" but it was a terrible name and idea anyway. Changed to "center" which makes more sense in context. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:146 > > + float x_rXOuter = d.x() / (m_radiusX + halfStrokeWidth); // x / (rX + sw/2) > > + float y_rYOuter = d.y() / (m_radiusY + halfStrokeWidth); // y / (rY + sw/2) > > + > > + return (x_rXInner * x_rXInner + y_rYInner * y_rYInner >= 1) && (x_rXOuter * x_rXOuter + y_rYOuter * y_rYOuter <= 1); > > +} > > I'd avoid the pre computation of all these variables here. First check if the outer case is violated, then early exit. If not, check the inner ellipse, etc.. Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:153 > > + FloatPoint d = FloatPoint(m_centroid.x() - point.x(), m_centroid.y() - point.y()); > > diameter? > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:158 > > + return (x_rX * x_rX + y_rY * y_rY <= 1); > > Braces not needed. Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.h:31 > > +#include "RenderSVGPath.h" > > s/Path/Shape/ ? Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.h:58 > > + FloatPoint m_centroid; > > m_center sounds better, no? Absolutely! Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.h:60 > > + float m_radiusX; > > + float m_radiusY; > > Why not use a FloatSize here? FloatSize m_radii. Done. Created attachment 134142 [details] Added deeper hooks into Skia for stroking and filling ellipses The Skia team asked that I go ahead and fill out the Skia side all the way to Skia's drawOval() call. In the very near future (http://code.google.com/p/skia/issues/detail?id=544), drawOval will be updated in Skia to be even faster (right now it just falls back to path rendering code). Note: I think this patch is ready to land except for the missing test expectations. Created attachment 134144 [details]
Forgot to update ChangeLog in previous patch
Comment on attachment 134144 [details] Forgot to update ChangeLog in previous patch Attachment 134144 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12148660 Comment on attachment 134144 [details] Forgot to update ChangeLog in previous patch Attachment 134144 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12145778 Comment on attachment 134144 [details] Forgot to update ChangeLog in previous patch Attachment 134144 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12147741 Comment on attachment 134144 [details] Forgot to update ChangeLog in previous patch Attachment 134144 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12142746 Comment on attachment 134144 [details] Forgot to update ChangeLog in previous patch View in context: https://bugs.webkit.org/attachment.cgi?id=134144&action=review Looks great, nothing holds that patch back IMHO, please try to get the builds fixed, then I can r+ it. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:36 > +#include <wtf/Platform.h> This seems unnecessary. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:59 > + if (style()->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) { I'd still like to see this specific check centralized, but we can refactor this later, once this is in. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:81 > + ASSERT(circle); This assert makes no sense, you have to ASSERT(node()) before. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:91 > + ASSERT(ellipse); Ditto. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:154 > + // This works by checking if the point satisfies the ellipse equation missing trailing period. > Source/WebCore/svg/SVGEllipseElement.h:53 > + RenderObject* createRenderer(RenderArena*, RenderStyle*); virtual, and OVERRIDE. Created attachment 134399 [details]
Patch for landing containing updated test expectations
In this patch I've addressed Niko's last comments, updated the test expectations for mac, and added a slew of updates in chromium/test_expectations.txt. I talked with Schenney about how this file is becoming unmanageable (4000+ lines!) and we have decided to do a fixit next week to rectify that situation.
This ia a great patch! I'm a bit confused why RenderSVGElipse needs to be so complicated... Comment on attachment 134399 [details] Patch for landing containing updated test expectations View in context: https://bugs.webkit.org/attachment.cgi?id=134399&action=review > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:81 > + ASSERT(circle); This and the elipse assert below could be combined into a node() assert at the top of this function. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:91 > + SVGEllipseElement* ellipse = static_cast<SVGEllipseElement*>(node()); > + ASSERT(ellipse); Seems we should assert the tagname before we do a possibly bad cast. :) > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:100 > + if (isPaintingFallback()) Confused by what isPaintingFallback() does? Must be new-ish. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:130 > +bool RenderSVGEllipse::shapeDependentStrokeContains(const FloatPoint& point) const I see. We do this manually to avoid path-based hit testing? > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:147 > +bool RenderSVGEllipse::shapeDependentFillContains(const FloatPoint& point, const WindRule fillRule) const Are you really OK ignoring wind-rule here? I guess there is no way to specify an ellipse that would intersect itself? > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:150 > + if (isPaintingFallback()) > + return RenderSVGShape::shapeDependentFillContains(point, fillRule); Why don't we need this check in the stroke version? > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:152 > + FloatPoint center = FloatPoint(m_center.x() - point.x(), m_center.y() - point.y()); I'm not sure what "center" means here? > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:158 > + return xrX * xrX + yrY * yrY <= 1; Parens might help. Should 1 be 1.0? > Source/WebCore/rendering/svg/RenderSVGEllipse.h:46 > + virtual bool isEmpty() const { return hasPath() ? RenderSVGShape::isEmpty() : m_boundingBox.isEmpty(); }; This must be used by RenderSVGShape for optimizations? Created attachment 134633 [details]
Update per reviewer comments and update text expectations
Thanks for the review! RenderSVGElipse does seem a bit complicated but it allows us to move the SVG rendering code closer to the graphics contexts which leads to an increase in performance. This results a performance increase today for CG, and the Skia team will be accelerating circle / ellipses in the very near future. Further comments are in-line. (In reply to comment #33) > (From update of attachment 134399 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134399&action=review > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:81 > > + ASSERT(circle); > > This and the elipse assert below could be combined into a node() assert at the top of this function. Good catch, done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:91 > > + SVGEllipseElement* ellipse = static_cast<SVGEllipseElement*>(node()); > > + ASSERT(ellipse); > > Seems we should assert the tagname before we do a possibly bad cast. :) > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:100 > > + if (isPaintingFallback()) > > Confused by what isPaintingFallback() does? Must be new-ish. The SVG spec allows for some special vector effects that would clutter up this code (and the spec hints at more coming in the future). The only one today is the non-scaling stroke, and in this case we fall back to path rendering (in RenderSVGShape) for simplicity. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:130 > > +bool RenderSVGEllipse::shapeDependentStrokeContains(const FloatPoint& point) const > > I see. We do this manually to avoid path-based hit testing? This is correct. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:147 > > +bool RenderSVGEllipse::shapeDependentFillContains(const FloatPoint& point, const WindRule fillRule) const > > Are you really OK ignoring wind-rule here? I guess there is no way to specify an ellipse that would intersect itself? This is correct; we can safely ignore the winding rule. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:150 > > + if (isPaintingFallback()) > > + return RenderSVGShape::shapeDependentFillContains(point, fillRule); > > Why don't we need this check in the stroke version? Good catch--we do need it. I added this check and added a test so this mistake won't happen again (svg/hittest/svg-ellipse-non-scale-stroke.xhtml). Unfortunately, in the process of doing this I found that hit testing is just completely broken for non-scaling strokes, so I filed this and marked my new test as failing: https://bugs.webkit.org/show_bug.cgi?id=82628 > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:152 > > + FloatPoint center = FloatPoint(m_center.x() - point.x(), m_center.y() - point.y()); > > I'm not sure what "center" means here? When performing a hit test using the closed-form ellipse equation, we calculate the center of an ellipse as the difference between the real ellipse and the point. I chose center so that it would be more clear to someone referencing the ellipse equation. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:158 > > + return xrX * xrX + yrY * yrY <= 1; > > Parens might help. Should 1 be 1.0? Done. > > > Source/WebCore/rendering/svg/RenderSVGEllipse.h:46 > > + virtual bool isEmpty() const { return hasPath() ? RenderSVGShape::isEmpty() : m_boundingBox.isEmpty(); }; > > This must be used by RenderSVGShape for optimizations? RenderSVGShape calls isEmpty and when we fall back to RenderSVGShape (in the case of non-scaling strokes), we don't setup the bounding box. Normally, we don't fall back and in this normal case we need this call to return whether our bounding box is empty. Comment on attachment 134633 [details] Update per reviewer comments and update text expectations View in context: https://bugs.webkit.org/attachment.cgi?id=134633&action=review Excepting the odd test_expectations.txt commenting, this LGTM. > LayoutTests/platform/chromium/test_expectations.txt:1293 > +// BUGWK53378 WIN : svg/clip-path/deep-nested-clip-in-mask-panning.svg = IMAGE+TEXT > +// BUGWK53378 MAC : svg/clip-path/deep-nested-clip-in-mask-panning.svg = IMAGE > +// BUGWK53378 WIN : svg/clip-path/deep-nested-clip-in-mask.svg = IMAGE+TEXT > +// BUGWK53378 MAC : svg/clip-path/deep-nested-clip-in-mask.svg = IMAGE Why the commented out expectations? Why not just remove them? Comment on attachment 134633 [details] Update per reviewer comments and update text expectations Attachment 134633 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12184522 Comment on attachment 134633 [details] Update per reviewer comments and update text expectations Attachment 134633 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12202430 (In reply to comment #36) > (From update of attachment 134633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134633&action=review > > Excepting the odd test_expectations.txt commenting, this LGTM. > > > LayoutTests/platform/chromium/test_expectations.txt:1293 > > +// BUGWK53378 WIN : svg/clip-path/deep-nested-clip-in-mask-panning.svg = IMAGE+TEXT > > +// BUGWK53378 MAC : svg/clip-path/deep-nested-clip-in-mask-panning.svg = IMAGE > > +// BUGWK53378 WIN : svg/clip-path/deep-nested-clip-in-mask.svg = IMAGE+TEXT > > +// BUGWK53378 MAC : svg/clip-path/deep-nested-clip-in-mask.svg = IMAGE > > Why the commented out expectations? Why not just remove them? At 4000+ lines, test_expectations is a mess right now. Schenney, Fmalita, and I have a fixit for improving this situation (primarily fixing SVG's tests) next week. The cases you pointed out are situations where we are already failing for some reason (e.g., IMAGE), and my patch makes us fail for TEXT (due to RenderSVGShape -> RenderSVGEllipse). Once we rebaseline the test on the Chromium side, we will have the correct text but we we will need to return the text_expectations to say the test is failing for IMAGE. I left these in (along with the comments in the entry for my bug) to help make this process easier. Comment on attachment 134633 [details] Update per reviewer comments and update text expectations Attachment 134633 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12204412 Comment on attachment 134633 [details] Update per reviewer comments and update text expectations Attachment 134633 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12193483 (In reply to comment #41) > (From update of attachment 134633 [details]) > Attachment 134633 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/12193483 I have a patch forthcoming to fix these build issues. I also talked with the #WebKit sheriff in prep for landing this and he pointed out that there's a better way to update test_expectations.txt, so I'll fix that in the next patch too. Comment on attachment 134633 [details] Update per reviewer comments and update text expectations Attachment 134633 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12202472 New failing tests: http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml Created attachment 134662 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 134663 [details]
Updated all platform build files due to new files
The previous patch failed to build on the bots because I didn't update all the platform project files. This patch should fix that issue.
I've also updated Chromium's test_expectations to not update every failing test, per dpranke's suggestion in #WebKit.
@Eric, do you mind one more look, just in case? My plan is to let this pass on EWS and land tomorrow morning.
Comment on attachment 134663 [details] Updated all platform build files due to new files View in context: https://bugs.webkit.org/attachment.cgi?id=134663&action=review LGTM. > Source/WebCore/GNUmakefile.list.am:3940 > + Source/WebCore/rendering/svg/RenderSVGEllipse.cpp \ > + Source/WebCore/rendering/svg/RenderSVGEllipse.h \ Wrong indent? Comment on attachment 134663 [details] Updated all platform build files due to new files Attachment 134663 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12193512 Created attachment 134673 [details]
Updating patch for bot happiness
Created attachment 134690 [details]
Updating patch for bot happiness (v2)
Committed r112667: <http://trac.webkit.org/changeset/112667> svg/custom/gradient-stroke-width.svg svg/custom/transform-with-shadow-and-gradient.svg seem to be broken now. The stroke is black instead of filled with a complex stroke as expected. Can you look into it Philip? Comment on attachment 134690 [details] Updating patch for bot happiness (v2) Rejecting attachment 134690 [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: sts/svg/hittest/svg-ellipse-non-scale-stroke-expected.txt patching file LayoutTests/svg/hittest/svg-ellipse-non-scale-stroke.xhtml patching file LayoutTests/svg/hittest/svg-ellipse.xhtml patching file LayoutTests/svg/hixie/links/001-expected.txt Hunk #1 FAILED at 3. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/svg/hixie/links/001-expected.txt.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12265816 (In reply to comment #51) > svg/custom/gradient-stroke-width.svg > svg/custom/transform-with-shadow-and-gradient.svg > seem to be broken now. The stroke is black instead of filled with a complex stroke as expected. Can you look into it Philip? I'm on it--looking into it now. (In reply to comment #51) > svg/custom/gradient-stroke-width.svg > svg/custom/transform-with-shadow-and-gradient.svg > seem to be broken now. The stroke is black instead of filled with a complex stroke as expected. Can you look into it Philip? I confirmed this change broke these tests; it is caused by a simple mistake in GraphicsContextCG :( I'll have a patch up as soon as my local build finishes (30min). The second test (transform-with-shadow-and-gradient.svg) is failing on linux/Skia but it appears to have been failing for a while (since before this change). I've filed a separate bug to track it: https://bugs.webkit.org/show_bug.cgi?id=82836 Created attachment 134979 [details]
Fix complex stroke regression
Comment on attachment 134979 [details]
Fix complex stroke regression
Thanks!
Comment on attachment 134979 [details] Fix complex stroke regression Clearing flags on attachment: 134979 Committed r112806: <http://trac.webkit.org/changeset/112806> All reviewed patches have been landed. Closing bug. Qt results updated in - http://trac.webkit.org/changeset/112671 - http://trac.webkit.org/changeset/112712 |