Added Skia turbulence filter to Webkit.

Added Skia turbulence filter to Webkit.

The FETurbulence class was modified to include hookups into Skia's PerlinNoise shader (which should be numerically equivalent to Blink's). Note that a temporary patch was added to support SVG declared filters within CSS (which was not working before this cl), so that we query the size when it has not yet been set. This issue is already present in other filters and is logged as issue 231504.

Tests: effect-reference.html and effect-reference-hw.html were each modified to add an feTurbulence case.

R=senorblanco@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149212

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152369

[Stephen White, 2013-04-15 17:56:28]

Please add a test case to LayoutTests/css3/filters/effect-reference.html and
effect-reference-hw.html

https://codereview.chromium.org/14263013/diff/1/Source/WebCore/platform/graph...
File Source/WebCore/platform/graphics/filters/skia/FETurbulenceSkia.cpp (right):

https://codereview.chromium.org/14263013/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/filters/skia/FETurbulenceSkia.cpp:38: bool
FETurbulence::platformApplySkia()
Now that we're in a post-blink world, you might as well put the contents of this
file directly in FETurbulence.cpp.

https://codereview.chromium.org/14263013/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/filters/skia/FETurbulenceSkia.cpp:48: const
float thisBaseFrequencyX = baseFrequencyX();
I guess it's personal style, but I never bother making POD type locals const --
just let the compiler sort it out.  In fact, it doesn't look like these
variables save you much of anything over just calling the accessors directly in
Create*Noise() below.

https://codereview.chromium.org/14263013/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/filters/skia/FETurbulenceSkia.cpp:54: const
SkIRect rect = SkIRect::MakeXYWH(
Looks like a big chunk of this function could be refactored with
createImageFilter() below.  Perhaps createNoiseShader?

https://codereview.chromium.org/14263013/diff/1/Source/WebCore/platform/graph...
Source/WebCore/platform/graphics/filters/skia/FETurbulenceSkia.cpp:79:
SkIntToScalar(filterRegion.x()), SkIntToScalar(filterRegion.y()),
I'm pretty sure the filterRegion is not initialized yet at this point in
createImageFilter().  But adding a test case to effect-reference.html and
effect-reference-hw.html will tell you.  For now, you may have to create a new
SkShaderImageFilter which creates its output the same size as its input.

[sugoi1, 2013-04-17 22:02:44]

I made the changes, but note that this version :
1 ) Requires this Skia cl about SkRectShaderImageFilter :
https://codereview.chromium.org/14093008/
2 ) Suffers from the scaling issue mentioned here :
https://code.google.com/p/chromium/issues/detail?id=231504

[sugoi1, 2013-04-23 15:31:03]

FYI, the required changes to SkRectShaderImageFilter have been committed and are
now in the chromium codebase.

[Stephen White, 2013-04-23 15:48:36]

https://codereview.chromium.org/14263013/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/filters/FETurbulence.cpp (right):

https://codereview.chromium.org/14263013/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/filters/FETurbulence.cpp:371: noiseSize.isEmpty()
? absolutePaintRect().size() : noiseSize);
Please add a FIXME here, since this is just a temporary step until
filterPrimitiveSubregion() is properly set for reference filters.

https://codereview.chromium.org/14263013/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/filters/FETurbulence.cpp:413: numOctaves(),
seed(), stitchTiles() ? &size : 0) :
I think you'll need SkFloatToScalar() on all the scalar parameters.

https://codereview.chromium.org/14263013/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/filters/FETurbulence.cpp:430: filterRegion.x(),
filterRegion.y(), filterRegion.width(), filterRegion.height());
Nit:  there's an SkIRect cast operator on IntRect; I think you can just cast it.

[sugoi1, 2013-04-23 20:05:48]

https://codereview.chromium.org/14263013/diff/4001/Source/core/platform/graph...
File Source/core/platform/graphics/filters/FETurbulence.cpp (right):

https://codereview.chromium.org/14263013/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/filters/FETurbulence.cpp:371: noiseSize.isEmpty()
? absolutePaintRect().size() : noiseSize);
On 2013/04/23 15:48:36, Stephen White wrote:
> Please add a FIXME here, since this is just a temporary step until
> filterPrimitiveSubregion() is properly set for reference filters.

Done.

https://codereview.chromium.org/14263013/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/filters/FETurbulence.cpp:413: numOctaves(),
seed(), stitchTiles() ? &size : 0) :
On 2013/04/23 15:48:36, Stephen White wrote:
> I think you'll need SkFloatToScalar() on all the scalar parameters.

Done.

https://codereview.chromium.org/14263013/diff/4001/Source/core/platform/graph...
Source/core/platform/graphics/filters/FETurbulence.cpp:430: filterRegion.x(),
filterRegion.y(), filterRegion.width(), filterRegion.height());
On 2013/04/23 15:48:36, Stephen White wrote:
> Nit:  there's an SkIRect cast operator on IntRect; I think you can just cast
it.

Done.

[Stephen White, 2013-04-23 20:22:56]

LGTM

[Stephen White, 2013-04-23 20:23:49]

On 2013/04/23 20:22:56, Stephen White wrote:
> LGTM

Whoops -- spoke too soon.  You'll need to add a line to TestExpectations until
the tests are rebaselined.

[commit-bot: I haz the power, 2013-04-24 19:09:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/14263013/16001

[commit-bot: I haz the power, 2013-04-24 20:10:49]

Change committed as 149034

[ojan, 2013-04-24 20:28:35]

This patch broke the mac debug compile. I'm rolling out.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Builder%2...

[pdr, 2013-04-24 22:13:24]

I don't think this change description was detailed enough. Please use better
commit messages so we remember what was going on when we come back to this code
a year from now.

[sugoi1, 2013-04-26 15:01:17]

Since it was reverted an now has to be re-committed, I added a better
description for this cl.

[commit-bot: I haz the power, 2013-04-26 15:13:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/14263013/27001

[commit-bot: I haz the power, 2013-04-26 15:45:26]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-04-26 15:47:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/14263013/27001

[commit-bot: I haz the power, 2013-04-26 15:50:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Justin Novosad, 2013-04-26 17:17:42]

Committed patchset #5 manually as r149212.

[ojan, 2013-04-26 22:02:59]

This test crashes on Mac/Linux debug. I assume it's hitting an assert.

Rolling back.

See
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

[sugoi1, 2013-04-29 13:50:59]

On 2013/04/26 22:02:59, ojan wrote:
> This test crashes on Mac/Linux debug. I assume it's hitting an assert.
> 
> Rolling back.
> 
> See
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

Hi Ojan, I'd like to fix the issue, but I see no failures in the link you
provided.

[sugoi1, 2013-04-29 14:15:18]

On 2013/04/29 13:50:59, sugoi1 wrote:
> On 2013/04/26 22:02:59, ojan wrote:
> > This test crashes on Mac/Linux debug. I assume it's hitting an assert.
> > 
> > Rolling back.
> > 
> > See
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...
> 
> Hi Ojan, I'd like to fix the issue, but I see no failures in the link you
provided.

Never mind, I see it now, I don't know why I was originally seeing a page with
"all tests passed" on it...

[ojan, 2013-04-29 17:22:38]

On 2013/04/29 14:15:18, sugoi1 wrote:
> On 2013/04/29 13:50:59, sugoi1 wrote:
> > On 2013/04/26 22:02:59, ojan wrote:
> > > This test crashes on Mac/Linux debug. I assume it's hitting an assert.
> > > 
> > > Rolling back.
> > > 
> > > See
> > >
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...
> > 
> > Hi Ojan, I'd like to fix the issue, but I see no failures in the link you
> provided.
> 
> Never mind, I see it now, I don't know why I was originally seeing a page with
> "all tests passed" on it...

Unfortunately, there was some data lost on the dashboard over the weekend.  I'm
not quite sure how to get back to what the failure was. :(

[sugoi, 2013-06-13 17:23:23]

Updated and ran trybots

[Stephen White, 2013-06-13 17:28:05]

https://codereview.chromium.org/14263013/diff/49001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FETurbulence.cpp (right):

https://codereview.chromium.org/14263013/diff/49001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FETurbulence.cpp:370: //         it's
currently not being set when an SVG filter is used in CSS, we have to use
BTW, I think filterPrimitiveSubregion() should now be correctly set for the CSS
filters case. You could give it a try if you like, or do that in a future patch.

https://codereview.chromium.org/14263013/diff/49001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FETurbulence.h (right):

https://codereview.chromium.org/14263013/diff/49001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FETurbulence.h:119: virtual bool
platformApplySkia();
The base class version of this function was renamed to applySkia(), so this
probably isn't getting called. It should also be marked OVERRIDE, which would
catch this error.

[sugoi, 2013-06-13 17:46:33]

https://codereview.chromium.org/14263013/diff/49001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FETurbulence.cpp (right):

https://codereview.chromium.org/14263013/diff/49001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FETurbulence.cpp:370: //         it's
currently not being set when an SVG filter is used in CSS, we have to use
On 2013/06/13 17:28:06, Stephen White wrote:
> BTW, I think filterPrimitiveSubregion() should now be correctly set for the
CSS
> filters case. You could give it a try if you like, or do that in a future
patch.

Done.

https://codereview.chromium.org/14263013/diff/49001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FETurbulence.h (right):

https://codereview.chromium.org/14263013/diff/49001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FETurbulence.h:119: virtual bool
platformApplySkia();
On 2013/06/13 17:28:06, Stephen White wrote:
> The base class version of this function was renamed to applySkia(), so this
> probably isn't getting called. It should also be marked OVERRIDE, which would
> catch this error.

Done.

[Stephen White, 2013-06-13 17:50:50]

LGTM

[commit-bot: I haz the power, 2013-06-13 17:51:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/14263013/58001

[commit-bot: I haz the power, 2013-06-13 19:02:50]

Change committed as 152369