New renderer for SVGCircleElement

[Philip Rogers]

We need a new renderer for SVGCircleElement! Ideally, one that will not be slow.

Patch forthcoming.

[Philip Rogers]

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

[Philip Rogers]

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

[Renata Hodovan]

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.

[Philip Rogers]

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.

[Philip Rogers]

Created attachment 131038 [details]
Update per reviewer comments and add faster CG codepath

[Philip Rogers]

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.

[Philip Rogers]

Created attachment 131087 [details]
Patch with expectations!

[Philip Rogers]

Created attachment 131108 [details]
Updated patch to head so it should apply cleanly

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Nikolas Zimmermann]

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.

[Philip Rogers]

Created attachment 132285 [details]
Updated patch per reviewer comments

[Philip Rogers]

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

[Nikolas Zimmermann]

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.

[WebKit Review Bot]

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.

[Philippe Normand]

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

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[WebKit Review Bot]

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

[Philip Rogers]

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.

[Philip Rogers]

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.

[Philip Rogers]

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.

[Philip Rogers]

Created attachment 134144 [details]
Forgot to update ChangeLog in previous patch

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Gustavo Noronha (kov)]

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

[Early Warning System Bot]

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

[Nikolas Zimmermann]

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.

[Philip Rogers]

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.

[Eric Seidel (no email)]

This ia a great patch!  I'm a bit confused why RenderSVGElipse needs to be so complicated...

[Eric Seidel (no email)]

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?

[Philip Rogers]

Created attachment 134633 [details]
Update per reviewer comments and update text expectations

[Philip Rogers]

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.

[Eric Seidel (no email)]

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?

[Gustavo Noronha (kov)]

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

[Early Warning System Bot]

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

[Philip Rogers]

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

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Philip Rogers]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Philip Rogers]

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.

[Eric Seidel (no email)]

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?

[Build Bot]

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

[Philip Rogers]

Created attachment 134673 [details]
Updating patch for bot happiness

[Philip Rogers]

Created attachment 134690 [details]
Updating patch for bot happiness (v2)

[Stephen Chenney]

Committed r112667: <http://trac.webkit.org/changeset/112667>

[Nikolas Zimmermann]

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?

[WebKit Review Bot]

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

[Philip Rogers]

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

[Philip Rogers]

(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

[Philip Rogers]

Created attachment 134979 [details]
Fix complex stroke regression

[Eric Seidel (no email)]

Comment on attachment 134979 [details]
Fix complex stroke regression

Thanks!

[WebKit Review Bot]

Comment on attachment 134979 [details]
Fix complex stroke regression

Clearing flags on attachment: 134979

Committed r112806: <http://trac.webkit.org/changeset/112806>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Qt results updated in 
 - http://trac.webkit.org/changeset/112671
 - http://trac.webkit.org/changeset/112712