This change adds support for the multiply blend mode

This change adds support for the multiply blend mode

[reed1, 2013-02-01 20:11:29]

Looks clean.

The impl in SkXfermode (of multiply_proc) looks faster than what was in
SkBlendImageFilter. Lets be sure (1) its correct, (2) we may need a build-flag
around the change (at least the different impl) so we can stage rebaselining
existing results (I'm assuming we may get slightly different results)...

Adding stephen, as the author of SkBlendImageFilter.

https://codereview.appspot.com/7221086/diff/1/src/effects/SkBlendImageFilter.cpp
File src/effects/SkBlendImageFilter.cpp (right):

https://codereview.appspot.com/7221086/diff/1/src/effects/SkBlendImageFilter....
src/effects/SkBlendImageFilter.cpp:39: 
Need to delete this local function.

[RikC, 2013-02-01 20:13:56]

On 2013/02/01 20:11:29, reed1 wrote:
> Looks clean.
> 
> The impl in SkXfermode (of multiply_proc) looks faster than what was in
> SkBlendImageFilter. Lets be sure (1) its correct, (2) we may need a build-flag
> around the change (at least the different impl) so we can stage rebaselining
> existing results (I'm assuming we may get slightly different results)...
> 
> Adding stephen, as the author of SkBlendImageFilter.
> 
How can I set a build flag?

[reed1, 2013-02-01 20:18:16]

something like

static void multiply_proc(...) {
#ifdef SK_IGNORE_MULTIPLY_XFERMODE_OPT
    ... put the current code from SkBlendImageFilter here
#else
    ... put your new code here
#endif
}

That way skia (which won't define that flag) will immediately begin testing and
benchmarking the new code. Chrome can stage this by #defining the flag
initially, build its rebaselined images, and then remove their definition.

[Stephen White, 2013-02-01 20:24:42]

On 2013/02/01 20:13:56, RikC wrote:
> On 2013/02/01 20:11:29, reed1 wrote:
> > Looks clean.
> > 
> > The impl in SkXfermode (of multiply_proc) looks faster than what was in
> > SkBlendImageFilter. Lets be sure (1) its correct, (2) we may need a
build-flag
> > around the change (at least the different impl) so we can stage rebaselining
> > existing results (I'm assuming we may get slightly different results)...
> > 
> > Adding stephen, as the author of SkBlendImageFilter.
> > 
> How can I set a build flag?

Thanks for taking this on.

Are the results of the "blend" GM the same?

You can test that via something like this:

# revert your patch
mkdir foo
ninja -C out/Release gm   # or whatever build system you're using
out/Release/gm -w foo --match blend

# then, apply your patch, rebuild, and
out/Release/gm -r foo --match blend

Hopefully they're only slightly different.  If so, add a compile-time flag,
something like

#ifndef SK_USE_OLD_MULTIPLY_BLENDMODE
// new stuff
#else
// old stuff
#endif

Then put something in the patch description reminding us to #define
SK_USE_OLD_MULTIPLY_BLENDMODE in Chrome until we've rebaselined the WebKit
tests.

[RikC, 2013-02-01 21:04:28]

> 
> Then put something in the patch description reminding us to #define
> SK_USE_OLD_MULTIPLY_BLENDMODE in Chrome until we've rebaselined the WebKit
> tests.

Done.
Also, updated the patch description

[Stephen White, 2013-02-01 21:10:17]

On 2013/02/01 21:04:28, RikC wrote:
> > 
> > Then put something in the patch description reminding us to #define
> > SK_USE_OLD_MULTIPLY_BLENDMODE in Chrome until we've rebaselined the WebKit
> > tests.
> 
> Done.
> Also, updated the patch description

LGTM

[Stephen White, 2013-02-01 21:12:07]

On 2013/02/01 21:10:17, Stephen White wrote:
> On 2013/02/01 21:04:28, RikC wrote:
> > > 
> > > Then put something in the patch description reminding us to #define
> > > SK_USE_OLD_MULTIPLY_BLENDMODE in Chrome until we've rebaselined the WebKit
> > > tests.
> > 
> > Done.
> > Also, updated the patch description
> 
> LGTM

Hmm.  Perhaps we should also augment the xfermodes GM's to exercise this new
mode as well, instead of leaving it up to the blend GM.

[Stephen White, 2013-02-01 21:12:50]

On 2013/02/01 21:12:07, Stephen White wrote:
> On 2013/02/01 21:10:17, Stephen White wrote:
> > On 2013/02/01 21:04:28, RikC wrote:
> > > > 
> > > > Then put something in the patch description reminding us to #define
> > > > SK_USE_OLD_MULTIPLY_BLENDMODE in Chrome until we've rebaselined the
WebKit
> > > > tests.
> > > 
> > > Done.
> > > Also, updated the patch description
> > 
> > LGTM
> 
> Hmm.  Perhaps we should also augment the xfermodes GM's to exercise this new
> mode as well, instead of leaving it up to the blend GM.

(That can be done in a different patch, if necessary.)

[RikC, 2013-02-01 21:19:55]

> > 
> > Hmm.  Perhaps we should also augment the xfermodes GM's to exercise this new
> > mode as well, instead of leaving it up to the blend GM.
> 
> (That can be done in a different patch, if necessary.)

I can do that in a new patch. What file is it that tests the blend modes? 
I found XfermodeTest.cpp and it should already test the new one...

[Stephen White, 2013-02-01 21:27:48]

On 2013/02/01 21:19:55, RikC wrote:
> > > 
> > > Hmm.  Perhaps we should also augment the xfermodes GM's to exercise this
new
> > > mode as well, instead of leaving it up to the blend GM.
> > 
> > (That can be done in a different patch, if necessary.)
> 
> I can do that in a new patch. What file is it that tests the blend modes? 
> I found XfermodeTest.cpp and it should already test the new one...

I was thinking of gm/xfermodes.cpp.

[reed1, 2013-02-01 21:34:45]

+1 for adding kMultiply to the gm/xfermodes

[RikC, 2013-02-01 21:37:17]

On 2013/02/01 21:34:45, reed1 wrote:
> +1 for adding kMultiply to the gm/xfermodes

Done

[Stephen White, 2013-02-01 21:37:46]

On 2013/02/01 21:37:17, RikC wrote:
> On 2013/02/01 21:34:45, reed1 wrote:
> > +1 for adding kMultiply to the gm/xfermodes
> 
> Done

New patch LGTM.

[RikC, 2013-02-01 23:33:06]

> New patch LGTM

Can you commit it?

[reed1, 2013-02-04 17:09:19]

I will commit this.

[reed1, 2013-02-04 20:16:14]

landed in 7553