[skia] Switch FEColorMatrix filter to use skia's SkColorMatrixFilter
Created attachment 121171 [details] Patch
Comment on attachment 121171 [details] Patch Attachment 121171 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11107030
Comment on attachment 121171 [details] Patch Attachment 121171 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11115008
Comment on attachment 121171 [details] Patch Attachment 121171 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11128003
Created attachment 121201 [details] Patch
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
Created attachment 121283 [details] Patch
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.
(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.
Created attachment 121450 [details] Patch
(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?
(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.
(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.
Created attachment 121499 [details] Patch
(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.
(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.
Created attachment 121620 [details] Patch
(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 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 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 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.
(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.
Committed r104566: <http://trac.webkit.org/changeset/104566>