Implement SkXfermode image filter

Implement SkXfermode image filter. This required changing the signature of SkXfermode::asNewEffectOrCoeffs(), to add an optional background texture.

For the raster path, we do a straightforward 2-pass method: draw background, then composite the foreground over it.

For the GPU path, if the xfermode can be expressed as an effect, we build an effect with the background texture incorporated, then do a single-pass draw fetching both foreground and background textures, and compositing to the result. If the xfermode is expressed as src/dst coefficients, we do a 2-pass draw as in the raster path and use fixed-function blending.

R=bsalomon@google.com, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9373

[Stephen White, 2013-05-30 16:46:02]

PTAL. There's a bit of cargo-culting around the GrTextureAccess and
GrGLEffectMatrix that I'm not sure about.

I also noticed that XferEffect::onIsEqual() isn't comparing its mode against the
other. I left it like that, but it seems wrong?

[reed1, 2013-05-30 16:56:32]

https://codereview.chromium.org/16125008/diff/5001/include/effects/SkXfermode...
File include/effects/SkXfermodeImageFilter.h (right):

https://codereview.chromium.org/16125008/diff/5001/include/effects/SkXfermode...
include/effects/SkXfermodeImageFilter.h:16: class SK_API SkXfermodeImageFilter :
public SkImageFilter {
/**
 * This filter takes an xfermode, and uses it to combine the foreground and
background images...
 */

https://codereview.chromium.org/16125008/diff/5001/include/effects/SkXfermode...
include/effects/SkXfermodeImageFilter.h:20: ~SkXfermodeImageFilter();
nit:
virtual ~SkXfermodeImageFilter();

https://codereview.chromium.org/16125008/diff/5001/src/effects/SkXfermodeImag...
File src/effects/SkXfermodeImageFilter.cpp (right):

https://codereview.chromium.org/16125008/diff/5001/src/effects/SkXfermodeImag...
src/effects/SkXfermodeImageFilter.cpp:25: SkSafeRef(fMode);
What happens if mode is NULL? Is that ever useful? Should we change to a Factory
for this subclass, that returns NULL if it doesn't like its input, rather than a
constructor that has to handle this sort of case?

[Stephen White, 2013-05-30 17:24:19]

https://codereview.chromium.org/16125008/diff/5001/include/effects/SkXfermode...
File include/effects/SkXfermodeImageFilter.h (right):

https://codereview.chromium.org/16125008/diff/5001/include/effects/SkXfermode...
include/effects/SkXfermodeImageFilter.h:16: class SK_API SkXfermodeImageFilter :
public SkImageFilter {
On 2013/05/30 16:56:32, reed1 wrote:
> /**
>  * This filter takes an xfermode, and uses it to combine the foreground and
> background images...
>  */

Done.

https://codereview.chromium.org/16125008/diff/5001/include/effects/SkXfermode...
include/effects/SkXfermodeImageFilter.h:20: ~SkXfermodeImageFilter();
On 2013/05/30 16:56:32, reed1 wrote:
> nit:
> virtual ~SkXfermodeImageFilter();

Done.

https://codereview.chromium.org/16125008/diff/5001/src/effects/SkXfermodeImag...
File src/effects/SkXfermodeImageFilter.cpp (right):

https://codereview.chromium.org/16125008/diff/5001/src/effects/SkXfermodeImag...
src/effects/SkXfermodeImageFilter.cpp:25: SkSafeRef(fMode);
On 2013/05/30 16:56:32, reed1 wrote:
> What happens if mode is NULL? Is that ever useful? Should we change to a
Factory
> for this subclass, that returns NULL if it doesn't like its input, rather than
a
> constructor that has to handle this sort of case?

I don't think it's particularly useful, but it's not harmful (AsNewEffect.. will
handle it correctly). A factory seems like overkill in this case.

[bsalomon, 2013-05-30 18:00:00]

Mostly LGTM. Thanks for catching the bug in isequal!

https://codereview.chromium.org/16125008/diff/11001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/16125008/diff/11001/include/core/SkXfermode.h...
include/core/SkXfermode.h:201: ignored.
Can we add a comment that background may be NULL in which case the effect should
access the dst color in the fragment shader?

https://codereview.chromium.org/16125008/diff/11001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/16125008/diff/11001/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:1409: const XferEffect& s =
CastEffect<XferEffect>(other);
you're correct that mode should be compared here to (this began life as the
DarkenEffect and I forgot to update this function).

[Stephen White, 2013-05-30 18:45:49]

https://codereview.chromium.org/16125008/diff/11001/include/core/SkXfermode.h
File include/core/SkXfermode.h (right):

https://codereview.chromium.org/16125008/diff/11001/include/core/SkXfermode.h...
include/core/SkXfermode.h:201: ignored.
On 2013/05/30 18:00:00, bsalomon wrote:
> Can we add a comment that background may be NULL in which case the effect
should
> access the dst color in the fragment shader?

Done.

https://codereview.chromium.org/16125008/diff/11001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/16125008/diff/11001/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:1409: const XferEffect& s =
CastEffect<XferEffect>(other);
On 2013/05/30 18:00:00, bsalomon wrote:
> you're correct that mode should be compared here to (this began life as the
> DarkenEffect and I forgot to update this function).

Done.

[bsalomon, 2013-05-30 18:48:35]

On 2013/05/30 18:45:49, Stephen White wrote:
> https://codereview.chromium.org/16125008/diff/11001/include/core/SkXfermode.h
> File include/core/SkXfermode.h (right):
> 
>
https://codereview.chromium.org/16125008/diff/11001/include/core/SkXfermode.h...
> include/core/SkXfermode.h:201: ignored.
> On 2013/05/30 18:00:00, bsalomon wrote:
> > Can we add a comment that background may be NULL in which case the effect
> should
> > access the dst color in the fragment shader?
> 
> Done.
> 
> https://codereview.chromium.org/16125008/diff/11001/src/core/SkXfermode.cpp
> File src/core/SkXfermode.cpp (right):
> 
>
https://codereview.chromium.org/16125008/diff/11001/src/core/SkXfermode.cpp#n...
> src/core/SkXfermode.cpp:1409: const XferEffect& s =
> CastEffect<XferEffect>(other);
> On 2013/05/30 18:00:00, bsalomon wrote:
> > you're correct that mode should be compared here to (this began life as the
> > DarkenEffect and I forgot to update this function).
> 
> Done.

lgtm

[reed1, 2013-05-30 18:59:44]

lgtm

[Stephen White, 2013-05-30 20:05:09]

Of course, I forgot to test the SkArithmeticMode as an SkXfermodeImageFilter,
which was the whole point of this exercise!  (Thankfully the Windows build
caught my omission with SK_OVERRIDE.)

This patch implements background support on the GPU side of SkArithmeticMode.
Since I don't fancy rebaselining the arithmode test yet again, I've just made it
alternate between arithmode-as-xfermode and arithmode-as-xfermode-image-filter.

PTAL.  Thanks!

[bsalomon, 2013-05-30 20:39:44]

On 2013/05/30 20:05:09, Stephen White wrote:
> Of course, I forgot to test the SkArithmeticMode as an SkXfermodeImageFilter,
> which was the whole point of this exercise!  (Thankfully the Windows build
> caught my omission with SK_OVERRIDE.)
> 
> This patch implements background support on the GPU side of SkArithmeticMode.
> Since I don't fancy rebaselining the arithmode test yet again, I've just made
it
> alternate between arithmode-as-xfermode and
arithmode-as-xfermode-image-filter.
> 
> PTAL.  Thanks!

lgtm

[reed1, 2013-05-30 20:42:44]

I don't really understand the concern about rebaselining and gms.

1. is there a fix to just arithmeticmode here? if so, lets land that (I can take
care of rebaslining in the morning if you like)
2. if we want a gm that exercises xfermimagefilter, lets write a new one for
that

https://codereview.chromium.org/16125008/diff/2003/gm/arithmode.cpp
File gm/arithmode.cpp (right):

https://codereview.chromium.org/16125008/diff/2003/gm/arithmode.cpp#newcode121
gm/arithmode.cpp:121: SkXfermode* xfer = SkArithmeticMode::Create(0, one, one,
0);
I think this xfer leaks, and it makes us leak the outer-xfer as well.

[reed1, 2013-05-30 20:46:17]

to be clearer, I'm fine if we land a new GM (exercising xfermodeimagefilter
well) in a separate CL

[Stephen White, 2013-05-30 20:50:26]

https://codereview.chromium.org/16125008/diff/2003/gm/arithmode.cpp
File gm/arithmode.cpp (right):

https://codereview.chromium.org/16125008/diff/2003/gm/arithmode.cpp#newcode121
gm/arithmode.cpp:121: SkXfermode* xfer = SkArithmeticMode::Create(0, one, one,
0);
On 2013/05/30 20:42:44, reed1 wrote:
> I think this xfer leaks, and it makes us leak the outer-xfer as well.

Good point. This whole whole section never gets exercised, actually, since k - K
modulus 2 is always zero. Doh! So much for being clever.

I'm going to add a new case to the xfermodeimagefilter GM instead, which has no
baselines yet so it won't turn the bots red.

[Stephen White, 2013-05-30 20:52:22]

On 2013/05/30 20:42:44, reed1 wrote:
> I don't really understand the concern about rebaselining and gms.
> 
> 1. is there a fix to just arithmeticmode here? if so, lets land that (I can
take
> care of rebaslining in the morning if you like)

No, the fix requires the SkXfermode changes, since the signature to
asNewEffectOrCoeff changed. So it really is an all-or-nothing kind of deal.

> 2. if we want a gm that exercises xfermimagefilter, lets write a new one for
> that

There is one in this CL.  I'll just add a new case for the arithmode, instead of
doing it the other way around.

> https://codereview.chromium.org/16125008/diff/2003/gm/arithmode.cpp
> File gm/arithmode.cpp (right):
> 
> https://codereview.chromium.org/16125008/diff/2003/gm/arithmode.cpp#newcode121
> gm/arithmode.cpp:121: SkXfermode* xfer = SkArithmeticMode::Create(0, one, one,
> 0);
> I think this xfer leaks, and it makes us leak the outer-xfer as well.

[Stephen White, 2013-05-31 17:49:19]

Committed patchset #8 manually as r9373 (presubmit successful).