Bug 75582 – [skia] Switch FEColorMatrix filter to use skia's SkColorMatrixFilter

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

[Stephen White]

[skia] Switch FEColorMatrix filter to use skia's SkColorMatrixFilter

[Stephen White]

Created attachment 121171 [details]
Patch

[Early Warning System Bot]

Comment on attachment 121171 [details]
Patch

Attachment 121171 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11107030

[Gustavo Noronha (kov)]

Comment on attachment 121171 [details]
Patch

Attachment 121171 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11115008

[Gyuyoung Kim]

Comment on attachment 121171 [details]
Patch

Attachment 121171 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11128003

[Stephen White]

Created attachment 121201 [details]
Patch

[WebKit Review Bot]

Comment on attachment 121201 [details]
Patch

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

New failing tests:
svg/filters/feColorMatrix-saturate.svg
svg/filters/feColorMatrix-offset.svg
svg/dynamic-updates/SVGFEColorMatrixElement-svgdom-in-prop.html
svg/W3C-SVG-1.1/filters-color-01-b.svg
svg/dynamic-updates/SVGFEColorMatrixElement-dom-in-attr.html
svg/dynamic-updates/SVGFEColorMatrixElement-dom-type-attr.html
svg/dynamic-updates/SVGFEColorMatrixElement-dom-values-attr.html

[Stephen White]

Created attachment 121283 [details]
Patch

[Stephen White]

Note that this could probably be handled a bit more cleanly.  Here are some other options:

1)  Make a platformApplySkia() virtual on FilterEffect, which FilterEffect::apply() would call for USE(SKIA).  The base class implementation could call platformApplySoftware(), so that effects which are not yet ported would still work.

2)  Rename FEColorMatrix::platformApplySkia() to platformApplySoftware(), and #ifdef out the standard implementation entirely for USE(SKIA).  This would be similar to GraphicsContext and friends.

#1 might be a bit more clean, design-wise, but #2 would reduce code size, since only a single version of each function would be compiled in.  Note that (unlike the NEON case?), there's no need for more than one implementation to be available at runtime.

[Kenneth Russell]

(In reply to comment #8)
> Note that this could probably be handled a bit more cleanly.  Here are some other options:
> 
> 1)  Make a platformApplySkia() virtual on FilterEffect, which FilterEffect::apply() would call for USE(SKIA).  The base class implementation could call platformApplySoftware(), so that effects which are not yet ported would still work.
> 
> 2)  Rename FEColorMatrix::platformApplySkia() to platformApplySoftware(), and #ifdef out the standard implementation entirely for USE(SKIA).  This would be similar to GraphicsContext and friends.
> 
> #1 might be a bit more clean, design-wise, but #2 would reduce code size, since only a single version of each function would be compiled in.  Note that (unlike the NEON case?), there's no need for more than one implementation to be available at runtime.

I'd suggest going with #2 for parity with how GraphicsContext is currently implemented. The fact that the Skia #ifdef leaks into the header is a little gross.

[Stephen White]

Created attachment 121450 [details]
Patch

[Nikolas Zimmermann]

(In reply to comment #9)
> (In reply to comment #8)
> > Note that this could probably be handled a bit more cleanly.  Here are some other options:
> > 
> > 1)  Make a platformApplySkia() virtual on FilterEffect, which FilterEffect::apply() would call for USE(SKIA).  The base class implementation could call platformApplySoftware(), so that effects which are not yet ported would still work.
> > 
> > 2)  Rename FEColorMatrix::platformApplySkia() to platformApplySoftware(), and #ifdef out the standard implementation entirely for USE(SKIA).  This would be similar to GraphicsContext and friends.
> > 
> > #1 might be a bit more clean, design-wise, but #2 would reduce code size, since only a single version of each function would be compiled in.  Note that (unlike the NEON case?), there's no need for more than one implementation to be available at runtime.
> 
> I'd suggest going with #2 for parity with how GraphicsContext is currently implemented. The fact that the Skia #ifdef leaks into the header is a little gross.

I'd rather go with 1) to avoid the same design problems as GC has (complex ifdefs, etc.).
What's the benefit to using SkColorMatrixFilter btw?
The ChangeLog doesn't say this, and I can't remember. Can it be accelerated out-of-the-box with Skia?

[Stephen White]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Note that this could probably be handled a bit more cleanly.  Here are some other options:
> > > 
> > > 1)  Make a platformApplySkia() virtual on FilterEffect, which FilterEffect::apply() would call for USE(SKIA).  The base class implementation could call platformApplySoftware(), so that effects which are not yet ported would still work.
> > > 
> > > 2)  Rename FEColorMatrix::platformApplySkia() to platformApplySoftware(), and #ifdef out the standard implementation entirely for USE(SKIA).  This would be similar to GraphicsContext and friends.
> > > 
> > > #1 might be a bit more clean, design-wise, but #2 would reduce code size, since only a single version of each function would be compiled in.  Note that (unlike the NEON case?), there's no need for more than one implementation to be available at runtime.
> > 
> > I'd suggest going with #2 for parity with how GraphicsContext is currently implemented. The fact that the Skia #ifdef leaks into the header is a little gross.
> 
> I'd rather go with 1) to avoid the same design problems as GC has (complex ifdefs, etc.).

OK, I've uploaded a patch with #2 as Ken suggested.  If you'd still prefer #1, I'll go that way instead.  I'm leaning towards #2, due to the code size issue.

> What's the benefit to using SkColorMatrixFilter btw?
> The ChangeLog doesn't say this, and I can't remember. Can it be accelerated out-of-the-box with Skia?

As of http://code.google.com/p/skia/source/detail?r=2948, it can.

[Nikolas Zimmermann]

(In reply to comment #12)
> OK, I've uploaded a patch with #2 as Ken suggested.  If you'd still prefer #1, I'll go that way instead.  I'm leaning towards #2, due to the code size issue.

I think it was agreed to go with #1) before, let's wait for Dirk/Zoltan to comment, who are behind the original design. CC'ing Zoltan.

[Stephen White]

Created attachment 121499 [details]
Patch

[Stephen White]

(In reply to comment #14)
> Created an attachment (id=121499) [details]
> Patch

This is an implementation of option #2, so we can choose.  Note that since FEGaussianBlur already had a platformApplySkia() function, I also switched it over to the new signature, and removed the skia-specific code from FEGaussianBlur::platformApplySoftware().  This means we will be using the Skia path for both software and hardware processing.  This doesn't require any new baselines since the implementations produce the same pixels.

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #12)
> > OK, I've uploaded a patch with #2 as Ken suggested.  If you'd still prefer #1, I'll go that way instead.  I'm leaning towards #2, due to the code size issue.
> 
> I think it was agreed to go with #1) before, let's wait for Dirk/Zoltan to comment, who are behind the original design. CC'ing Zoltan.

I would also use option #1.

In general I think it is a good idea to use Skia specific code instead of software rendering.

[Stephen White]

Created attachment 121620 [details]
Patch

[Stephen White]

(In reply to comment #17)
> Created an attachment (id=121620) [details]
> Patch

This is a version of option #2 with the FEGaussianBlur change as well, so the two patches are functionally equivalent.  I'll mark the previous attachment (which implements #1) for review as well, since that seems to be the consensus.

[Eric Seidel (no email)]

Comment on attachment 121499 [details]
Patch

Cleared review? from obsolete attachment 121499 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Stephen White]

Comment on attachment 121620 [details]
Patch

OK, well apparently eseidel's scripts don't like two patches marked for r+, so I'll just mark the second-last patch (option #1) for review.

[Dirk Schulze]

Comment on attachment 121499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121499&action=review

Looks a lot better for me. r=me with little changes before landing.

> Source/WebCore/platform/graphics/filters/skia/FEColorMatrixSkia.cpp:60
> +    matrix[0] =  0.213f + cosHue * 0.787f - sinHue * 0.213f;
> +    matrix[1] =  0.715f - cosHue * 0.715f - sinHue * 0.715f;
> +    matrix[2] =  0.072f - cosHue * 0.072f + sinHue * 0.928f;

Why are there multiple spaces after the equal sign?

> Source/WebCore/platform/graphics/filters/skia/FEColorMatrixSkia.cpp:64
> +    matrix[5] =  0.213f - cosHue * 0.213f + sinHue * 0.143f;
> +    matrix[6] =  0.715f + cosHue * 0.285f + sinHue * 0.140f;
> +    matrix[7] =  0.072f - cosHue * 0.072f - sinHue * 0.283f;

ditto.

[Stephen White]

(In reply to comment #21)
> (From update of attachment 121499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121499&action=review
> 
> Looks a lot better for me. r=me with little changes before landing.
> 
> > Source/WebCore/platform/graphics/filters/skia/FEColorMatrixSkia.cpp:60
> > +    matrix[0] =  0.213f + cosHue * 0.787f - sinHue * 0.213f;
> > +    matrix[1] =  0.715f - cosHue * 0.715f - sinHue * 0.715f;
> > +    matrix[2] =  0.072f - cosHue * 0.072f + sinHue * 0.928f;
> 
> Why are there multiple spaces after the equal sign?
> 
> > Source/WebCore/platform/graphics/filters/skia/FEColorMatrixSkia.cpp:64
> > +    matrix[5] =  0.213f - cosHue * 0.213f + sinHue * 0.143f;
> > +    matrix[6] =  0.715f + cosHue * 0.285f + sinHue * 0.140f;
> > +    matrix[7] =  0.072f - cosHue * 0.072f - sinHue * 0.283f;
> 
> ditto.

I think I was trying to line up the matrix elements.  Will remove on landing.

[Stephen White]

Committed r104566: <http://trac.webkit.org/changeset/104566>