Bug 75582 - [skia] Switch FEColorMatrix filter to use skia's SkColorMatrixFilter
Summary: [skia] Switch FEColorMatrix filter to use skia's SkColorMatrixFilter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-04 15:13 PST by Stephen White
Modified: 2012-01-10 07:01 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.07 KB, patch)
2012-01-04 15:14 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2012-01-04 18:33 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (9.89 KB, patch)
2012-01-05 08:30 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (9.14 KB, patch)
2012-01-06 11:20 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (13.53 KB, patch)
2012-01-06 14:41 PST, Stephen White
krit: review+
krit: commit-queue-
Details | Formatted Diff | Diff
Patch (11.63 KB, patch)
2012-01-08 22:12 PST, Stephen White
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2012-01-04 15:13:24 PST
[skia] Switch FEColorMatrix filter to use skia's SkColorMatrixFilter
Comment 1 Stephen White 2012-01-04 15:14:03 PST
Created attachment 121171 [details]
Patch
Comment 2 Early Warning System Bot 2012-01-04 17:00:24 PST
Comment on attachment 121171 [details]
Patch

Attachment 121171 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11107030
Comment 3 Gustavo Noronha (kov) 2012-01-04 17:05:25 PST
Comment on attachment 121171 [details]
Patch

Attachment 121171 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11115008
Comment 4 Gyuyoung Kim 2012-01-04 17:09:30 PST
Comment on attachment 121171 [details]
Patch

Attachment 121171 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11128003
Comment 5 Stephen White 2012-01-04 18:33:11 PST
Created attachment 121201 [details]
Patch
Comment 6 WebKit Review Bot 2012-01-05 08:29:04 PST
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
Comment 7 Stephen White 2012-01-05 08:30:30 PST
Created attachment 121283 [details]
Patch
Comment 8 Stephen White 2012-01-06 07:28:02 PST
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.
Comment 9 Kenneth Russell 2012-01-06 10:05:29 PST
(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.
Comment 10 Stephen White 2012-01-06 11:20:47 PST
Created attachment 121450 [details]
Patch
Comment 11 Nikolas Zimmermann 2012-01-06 11:42:13 PST
(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?
Comment 12 Stephen White 2012-01-06 11:49:47 PST
(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.
Comment 13 Nikolas Zimmermann 2012-01-06 11:51:30 PST
(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.
Comment 14 Stephen White 2012-01-06 14:41:25 PST
Created attachment 121499 [details]
Patch
Comment 15 Stephen White 2012-01-06 14:45:32 PST
(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.
Comment 16 Dirk Schulze 2012-01-08 17:35:57 PST
(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.
Comment 17 Stephen White 2012-01-08 22:12:40 PST
Created attachment 121620 [details]
Patch
Comment 18 Stephen White 2012-01-09 06:39:54 PST
(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.
Comment 19 Eric Seidel (no email) 2012-01-09 13:34:17 PST
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).
Comment 20 Stephen White 2012-01-09 14:23:32 PST
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.
Comment 21 Dirk Schulze 2012-01-09 23:33:16 PST
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.
Comment 22 Stephen White 2012-01-10 06:59:49 PST
(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.
Comment 23 Stephen White 2012-01-10 07:01:33 PST
Committed r104566: <http://trac.webkit.org/changeset/104566>