Implementation of the displacement effect (both CPU and GPU)

Implementation of the displacement effect (both CPU and GPU)

TEST=Added new GM called "displacement"
Committed: https://code.google.com/p/skia/source/detail?r=7182

[sugoi1, 2013-01-11 19:15:41]

Ok, I tested with the new webkit implementation of displacement that's in review
right now, so this implementation seems to produce the proper result compared to
the new implementation.

[Stephen White, 2013-01-11 20:24:50]

https://codereview.appspot.com/7058075/diff/8001/gm/displacement.cpp
File gm/displacement.cpp (right):

https://codereview.appspot.com/7058075/diff/8001/gm/displacement.cpp#newcode2
gm/displacement.cpp:2: * Copyright 2012 Google Inc.
Nit:  2013

https://codereview.appspot.com/7058075/diff/8001/include/effects/SkDisplaceme...
File include/effects/SkDisplacementMapEffect.h (right):

https://codereview.appspot.com/7058075/diff/8001/include/effects/SkDisplaceme...
include/effects/SkDisplacementMapEffect.h:2: * Copyright 2012 The Android Open
Source Project
Nit: 2013

https://codereview.appspot.com/7058075/diff/8001/include/effects/SkDisplaceme...
include/effects/SkDisplacementMapEffect.h:25:
SkDisplacementMapEffect(ChannelSelectorType xChannelSelector,
ChannelSelectorType yChannelSelector, SkScalar scale, SkImageFilter* color,
SkImageFilter* displacement);
As discussed, one parameter here should default to NULL, and use the src input
passed to filterImage() if NULL.

https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
src/effects/SkDisplacementMapEffect.cpp:2: * Copyright 2012 The Android Open
Source Project
Nit:  2013

https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
src/effects/SkDisplacementMapEffect.cpp:160: SkBitmap color, displacement = src;
If you do switch the colorInput to be default-NULL, you'll need to move this
assignment to color = src.

https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
src/effects/SkDisplacementMapEffect.cpp:174: dst->allocPixels();
Should check for failed allocation here:

if (!dst->getPixels()) { 
  return false;
} 

(I realize some of the other image filters have this problem too.)

https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
src/effects/SkDisplacementMapEffect.cpp:372: code->appendf("\t\t%s.rgb = (%s.a <
1e-6) ? vec3(0.0) : clamp(%s.rgb / %s.a, 0.0, 1.0);",
1e-6 seems kinda arbitrary, but ok.  BTW, you'll probably want to make sure this
shader compiles on OSX (its parser is pickier than other platforms).

[bsalomon, 2013-01-11 20:53:13]

https://codereview.appspot.com/7058075/diff/8001/include/effects/SkDisplaceme...
File include/effects/SkDisplacementMapEffect.h (right):

https://codereview.appspot.com/7058075/diff/8001/include/effects/SkDisplaceme...
include/effects/SkDisplacementMapEffect.h:2: * Copyright 2012 The Android Open
Source Project
On 2013/01/11 20:24:50, Stephen White wrote:
> Nit: 2013


and AOSP->"Google Inc."

https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
src/effects/SkDisplacementMapEffect.cpp:2: * Copyright 2012 The Android Open
Source Project
On 2013/01/11 20:24:50, Stephen White wrote:
> Nit:  2013

AOSP->"Google Inc."

https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
src/effects/SkDisplacementMapEffect.cpp:294: GrContext::AutoRenderTarget
art(context, dst->asRenderTarget());
Stephen, do we need to save off the clip, RT, and matrix anymore? I thought we
made the caller do it to prevent the filter's state changes from leaking out.

[bsalomon, 2013-01-11 20:54:16]

We should add it to the shader unit test (see GrEffectUnitTest.h)

[Stephen White, 2013-01-11 21:15:55]

On 2013/01/11 20:53:13, bsalomon wrote:
>
https://codereview.appspot.com/7058075/diff/8001/include/effects/SkDisplaceme...
> File include/effects/SkDisplacementMapEffect.h (right):
> 
>
https://codereview.appspot.com/7058075/diff/8001/include/effects/SkDisplaceme...
> include/effects/SkDisplacementMapEffect.h:2: * Copyright 2012 The Android Open
> Source Project
> On 2013/01/11 20:24:50, Stephen White wrote:
> > Nit: 2013
> 
> 
> and AOSP->"Google Inc."
> 
>
https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
> File src/effects/SkDisplacementMapEffect.cpp (right):
> 
>
https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
> src/effects/SkDisplacementMapEffect.cpp:2: * Copyright 2012 The Android Open
> Source Project
> On 2013/01/11 20:24:50, Stephen White wrote:
> > Nit:  2013
> 
> AOSP->"Google Inc."
> 
>
https://codereview.appspot.com/7058075/diff/8001/src/effects/SkDisplacementMa...
> src/effects/SkDisplacementMapEffect.cpp:294: GrContext::AutoRenderTarget
> art(context, dst->asRenderTarget());
> Stephen, do we need to save off the clip, RT, and matrix anymore? I thought we
> made the caller do it to prevent the filter's state changes from leaking out.

You're right; there's an AutoWideOpenIdentityDraw in the caller that should do
it for us.  We should probably go through and clean up the existing filters.

[sugoi1, 2013-01-14 20:30:22]

I added the unit test for GrDisplacementMapEffect, although I'm not sure
how/when it gets run, so please explain, if you know how this works :)

[Stephen White, 2013-01-14 20:38:45]

https://codereview.appspot.com/7058075/diff/15001/gm/displacement.cpp
File gm/displacement.cpp (right):

https://codereview.appspot.com/7058075/diff/15001/gm/displacement.cpp#newcode79
gm/displacement.cpp:79: canvas->drawSprite(*color, 0, 0, &paint);
I'm afraid we'll have to change all these drawSprite()s to drawBitmap() with a
clip, in order to avoid breaking Justin's scaled SkPicture playback  (see
https://code.google.com/p/skia/source/detail?r=7152).

[bsalomon, 2013-01-14 20:42:09]

https://codereview.appspot.com/7058075/diff/15001/src/effects/SkDisplacementM...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.appspot.com/7058075/diff/15001/src/effects/SkDisplacementM...
src/effects/SkDisplacementMapEffect.cpp:134:
SkDisplacementMapEffect::SkDisplacementMapEffect(ChannelSelectorType
xChannelSelector, ChannelSelectorType yChannelSelector, SkScalar scale,
SkImageFilter* displacement, SkImageFilter* color)
Can we wrap the params?

Also usual style for the init list is

: fFirst(first)
, fSecond(second)
...
, fLast(last) {

https://codereview.appspot.com/7058075/diff/15001/src/effects/SkDisplacementM...
src/effects/SkDisplacementMapEffect.cpp:354: // not of constant color when a
displacement effect is applied.
nice comment

[sugoi1, 2013-01-14 20:42:26]

https://codereview.appspot.com/7058075/diff/15001/gm/displacement.cpp
File gm/displacement.cpp (right):

https://codereview.appspot.com/7058075/diff/15001/gm/displacement.cpp#newcode79
gm/displacement.cpp:79: canvas->drawSprite(*color, 0, 0, &paint);
Done, I just replaced drawSprite by drawBitmap, they have the same signature for
this specific case.

[bsalomon, 2013-01-14 20:42:51]

On 2013/01/14 20:42:09, bsalomon wrote:
>
https://codereview.appspot.com/7058075/diff/15001/src/effects/SkDisplacementM...
> File src/effects/SkDisplacementMapEffect.cpp (right):
> 
>
https://codereview.appspot.com/7058075/diff/15001/src/effects/SkDisplacementM...
> src/effects/SkDisplacementMapEffect.cpp:134:
> SkDisplacementMapEffect::SkDisplacementMapEffect(ChannelSelectorType
> xChannelSelector, ChannelSelectorType yChannelSelector, SkScalar scale,
> SkImageFilter* displacement, SkImageFilter* color)
> Can we wrap the params?
> 
> Also usual style for the init list is
> 
> : fFirst(first)
> , fSecond(second)
> ...
> , fLast(last) {
> 
>
https://codereview.appspot.com/7058075/diff/15001/src/effects/SkDisplacementM...
> src/effects/SkDisplacementMapEffect.cpp:354: // not of constant color when a
> displacement effect is applied.
> nice comment

Oh and otherwise lgtm.

[sugoi1, 2013-01-15 14:38:40]

Implemented the "inverse Y" math in the displacement shader in case one or both
textures is in the directX style inverse Y coordinate system.

[Stephen White, 2013-01-15 15:17:03]

https://codereview.appspot.com/7058075/diff/17003/gm/displacement.cpp
File gm/displacement.cpp (right):

https://codereview.appspot.com/7058075/diff/17003/gm/displacement.cpp#newcode82
gm/displacement.cpp:82: SkAutoTUnref<SkImageFilter>
displ(SkNEW_ARGS(SkBitmapSource, (fBitmap/*fCheckerboard*/)));
Nit:  please remove commented-out code.

https://codereview.appspot.com/7058075/diff/17003/src/effects/SkDisplacementM...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.appspot.com/7058075/diff/17003/src/effects/SkDisplacementM...
src/effects/SkDisplacementMapEffect.cpp:437: code->appendf("\t\tvec2 %s =
vec2(%s.x, 0.5*(1.0-%s) + %s*%s.y);",
I don't think you need to y-flip the coordinates passed by the system (they're
correct as-is), only any offsets you apply to them (as you did, correctly, in
the cCoords case below).  You should be able to use the original coordinates in
this case.

https://codereview.appspot.com/7058075/diff/17003/src/effects/SkDisplacementM...
src/effects/SkDisplacementMapEffect.cpp:447: code->appendf("\t\tvec2 %s = %s +
vec2(%s, %s*%s)*(%s.",
As discussed, you could bake scale * (1, sign) into a single vec2 uniform, and
save an upload.

[bsalomon, 2013-01-15 15:21:20]

https://codereview.appspot.com/7058075/diff/17003/src/effects/SkDisplacementM...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.appspot.com/7058075/diff/17003/src/effects/SkDisplacementM...
src/effects/SkDisplacementMapEffect.cpp:437: code->appendf("\t\tvec2 %s =
vec2(%s.x, 0.5*(1.0-%s) + %s*%s.y);",
On 2013/01/15 15:17:04, Stephen White wrote:
> I don't think you need to y-flip the coordinates passed by the system (they're
> correct as-is), only any offsets you apply to them (as you did, correctly, in
> the cCoords case below).  You should be able to use the original coordinates
in
> this case.

The coords computed using GrGLEffectMatrix already account for the texture's
orientation.

Aside: We have a number of effects that use "imageIncrement" in addition to a
matrix. Maybe we should consider a helper class, either in addition to the
matrix helper or as an expanded version of the matrix helper.

[sugoi1, 2013-01-15 15:24:44]

Ok, I fixed the inverse Y implementation.

[Stephen White, 2013-01-15 15:32:30]

On 2013/01/15 15:24:44, sugoi1 wrote:
> Ok, I fixed the inverse Y implementation.

LGTM.  Thanks for the changes.