Implement morphology (erode and dilate) effects

Erode and dilate effects, CPU and GPU implementations.

The GPU implementation behaves like a filtering mode, and re-uses the kernelWidth parameter from the convolution shader.  The convolution was also refactored to remove imageIncrement from GrContext, replace it with a FilterDirection (also used by the morphology), and push imageIncrement computation down to GrGpuGLShaders.

On the CPU side, SkMorphologyImageFilter is an abstract base class, with SkErodeImageFilter and SkDilateImageFilter derived from it.  The software implementation is fairly straightforward, with a two-pass separable process like the GPU side.

[reed1, 2012-02-15 21:02:55]

raster side seems fine, deferring to brian for GPU aspects

[bsalomon, 2012-02-16 15:57:13]

High level comments:

1) I think we should push the logic of how to apply the filters into GrContext
(e.g. GrContext::convolveTexture(), GrContext::erodeTexture(), ...)

2) Can we remove imageIncrement as something seen by users of GrContext? Instead
have a way of signaling on GrSamplerState that the filter is applied in X or in
Y and have the flush in GrGpuGLShaders compute (1/w, 0) or (0, 1/h).

3) Is it easy to share the imageIncrement uniform with the 4x4 downsampler's
similar uniform?

[Stephen White, 2012-03-01 22:37:14]

On 2012/02/16 15:57:13, bsalomon wrote:
> High level comments:

All good points!
> 
> 1) I think we should push the logic of how to apply the filters into GrContext
> (e.g. GrContext::convolveTexture(), GrContext::erodeTexture(), ...)

Will do.

> 
> 2) Can we remove imageIncrement as something seen by users of GrContext?
Instead
> have a way of signaling on GrSamplerState that the filter is applied in X or
in
> Y and have the flush in GrGpuGLShaders compute (1/w, 0) or (0, 1/h).

Done in the next draft.

> 3) Is it easy to share the imageIncrement uniform with the 4x4 downsampler's
> similar uniform?

Not easily, since we either need to zero out one component of it (which might
confuse other users of that value), or bake the direction into the shader, which
will mean the convolution grows from 25 compiled variants to 50.

[Stephen White, 2012-03-01 22:45:06]

So in this version, I've removed the imageIncrement from GrSamplerState, and
added a FilterDirection instead.  Since this makes the convolveInX/convolveInY
pretty trivial, I removed them.

I've also made SkMorphologyImageFilter into an abstract base class, and derived
SkDilateImageFilter and SkErodeImageFilter from it.  I think this looks nicer
from an API point of view, and avoids that ugly bool that "asAMorphology()" had.

I'm going to work on refactoring gaussianBlur() and applyMorphology() into
GrContext, but perhaps that should wait for another patch?

[reed1, 2012-03-02 13:18:06]

http://codereview.appspot.com/5656067/diff/6001/src/effects/SkMorphologyImage...
File src/effects/SkMorphologyImageFilter.cpp (right):

http://codereview.appspot.com/5656067/diff/6001/src/effects/SkMorphologyImage...
src/effects/SkMorphologyImageFilter.cpp:85: 
can we combine these two, by just passing in the src and dst stride as a
parameter? I can't tell for sure, but they look pretty similar.

same question for the erode functions.

[bsalomon, 2012-03-02 14:15:24]

I like this version.

On 2012/03/01 22:45:06, Stephen White wrote:
> So in this version, I've removed the imageIncrement from GrSamplerState, and
> added a FilterDirection instead.  Since this makes the convolveInX/convolveInY
> pretty trivial, I removed them.
> 
> I've also made SkMorphologyImageFilter into an abstract base class, and
derived
> SkDilateImageFilter and SkErodeImageFilter from it.  I think this looks nicer
> from an API point of view, and avoids that ugly bool that "asAMorphology()"
had.
> 
> I'm going to work on refactoring gaussianBlur() and applyMorphology() into
> GrContext, but perhaps that should wait for another patch?

I think that's OK.

The GPU part LGTM.

[bsalomon, 2012-03-02 14:15:30]

http://codereview.appspot.com/5656067/diff/6001/include/gpu/GrSamplerState.h
File include/gpu/GrSamplerState.h (right):

http://codereview.appspot.com/5656067/diff/6001/include/gpu/GrSamplerState.h#...
include/gpu/GrSamplerState.h:242: FilterDirection     fFilterDirection : 8;
ubernit: can we tab out the other shorter guys to line up with fFilterDirection?

[Stephen White, 2012-03-02 17:34:23]

http://codereview.appspot.com/5656067/diff/6001/include/gpu/GrSamplerState.h
File include/gpu/GrSamplerState.h (right):

http://codereview.appspot.com/5656067/diff/6001/include/gpu/GrSamplerState.h#...
include/gpu/GrSamplerState.h:242: FilterDirection     fFilterDirection : 8;
On 2012/03/02 14:15:30, bsalomon wrote:
> ubernit: can we tab out the other shorter guys to line up with
fFilterDirection?

Done.

http://codereview.appspot.com/5656067/diff/6001/src/effects/SkMorphologyImage...
File src/effects/SkMorphologyImageFilter.cpp (right):

http://codereview.appspot.com/5656067/diff/6001/src/effects/SkMorphologyImage...
src/effects/SkMorphologyImageFilter.cpp:85: 
On 2012/03/02 13:18:06, reed1 wrote:
> can we combine these two, by just passing in the src and dst stride as a
> parameter? I can't tell for sure, but they look pretty similar.
> 
> same question for the erode functions.

We could, except that the range of the inner loop depends on the current x or y,
and width or height, depending.  It could be done by "lying" to the function
about width and height (so x and y are swapped), passing in both X and Y
strides.  I'll give that a shot.

[reed1, 2012-03-02 20:21:10]

nice code sharing.

Perhaps we could make the '<' or '>' (the only diff between erode and dilate?) a
template parameter.... just kidding.

lgtm

[Stephen White, 2012-03-02 20:26:46]

On 2012/03/02 20:21:10, reed1 wrote:
> nice code sharing.
> 
> Perhaps we could make the '<' or '>' (the only diff between erode and dilate?)
a
> template parameter.... just kidding.

Actually it's a good idea.  It just seemed like there wasn't a lot of
template-heavy code in Skia, so I was avoiding it.

[reed1, 2012-03-02 20:33:51]

We have a lot of attempted code sharing (e.g. bitmapshaderproc), but in that
case, we use #defines and multiple includes (not as pretty as templates).

Your call.

[Stephen White, 2012-03-02 22:28:23]

Landed as r3310.  Closing.

[Stephen White, 2012-03-02 22:28:23]

Landed as r3310.  Closing.