GPU implementation of color matrix filter

Implement the color matrix filter in Ganesh.  Also, fix and enable the color matrix test slide.

This was basically implemented in the same places where the blending-based color filter was being done.  The shader simply does a mat4 matrix multiply and a vec4 add.

[Stephen White, 2012-01-03 14:53:26]

Remove debugging output, fix and enable test

[Stephen White, 2012-01-03 15:03:42]

On 2012/01/03 14:53:26, Stephen White wrote:
> Remove debugging output, fix and enable test

OK, this is ready for review.  Some issues that might be improved:

- could reduce GrPaint::fColorMatrix from 20 to 16 elements, by re-using
GrPaint::fColorFilterColor for the constant column
- SkColorMatrixFilter:  instead of converting back from SkFixed to SkScalar, the
original passed-in SkScalars could be stored in the SkColorMatrixFilter.  This
would mean more work or more memory usage for the raster path, though.
- the SkBool8 in GrGLProgram::ProgramDesc could be collapsed with the other
SkBool8s, or converted to bitfields

[bsalomon, 2012-01-03 15:36:22]

Overall LGTM.

Can we do a bench run to see if adding fields is affecting performance? I'd
particularly check chrome_scrollGmail on a high repeat count on mac or linux
(win is inconsistent).

http://codereview.appspot.com/5489107/diff/2002/src/effects/SkColorMatrixFilt...
File src/effects/SkColorMatrixFilter.cpp (right):

http://codereview.appspot.com/5489107/diff/2002/src/effects/SkColorMatrixFilt...
src/effects/SkColorMatrixFilter.cpp:338: int unshift = 16 - fState.fShift;
Would it make sense to just store the originals in setup() to avoid this hairy
code and int->float conversions?

http://codereview.appspot.com/5489107/diff/2002/src/gpu/GrGLProgram.cpp
File src/gpu/GrGLProgram.cpp (right):

http://codereview.appspot.com/5489107/diff/2002/src/gpu/GrGLProgram.cpp#newco...
src/gpu/GrGLProgram.cpp:833: } else if (fProgramDesc.fColorMatrixEnabled) {
So we're saying the the color matrix is only invoked if the filter mode is dst?
I know that in the skia pipeline the color matrix is a color filter but in our
program desc they look orthogonal.

Perhaps we should add a special mode value that means matrix (rather than the
bool) or apply them serially.

I'd prefer the latter since it seems cleaner and we could find another use for
the color matrix aside from implementing SkColorMatrixFilter.

http://codereview.appspot.com/5489107/diff/2002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5489107/diff/2002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:444: SkScalar matrix[20];
wrong indent?

[Stephen White, 2012-01-03 19:42:21]

On 2012/01/03 15:36:22, bsalomon wrote:
> Overall LGTM.
> 
> Can we do a bench run to see if adding fields is affecting performance? I'd
> particularly check chrome_scrollGmail on a high repeat count on mac or linux
> (win is inconsistent).

Sure, done.  (Let me know if I did this incorrectly..)

Before:
senorblanco-linux:~/src/skia2> out/Release/bench -repeat 100 -match chrome
-config NULLGPU
skia bench: alpha=0xFF antialias=1 filter=0 rotate=0 scale=0 clip=0
dither=default strokeWidth=none scalar=float system=UNIX
SkFontHost::GetFileName unimplemented
running bench [1254 1160]           chrome_scrollGmail  NULLGPU: cmsecs = 199.14
gmsecs =   0.00
purging 100K from font cache [1 entries]

After:
senorblanco-linux:~/src/skia1> out/Release/bench -repeat 100 -match chrome
-config NULLGPU
skia bench: alpha=0xFF antialias=1 filter=0 rotate=0 scale=0 clip=0
dither=default strokeWidth=none scalar=float system=UNIX
SkFontHost::GetFileName unimplemented
running bench [1254 1160]           chrome_scrollGmail  NULLGPU: cmsecs = 197.44
gmsecs =   0.00
purging 100K from font cache [1 entries]

http://codereview.appspot.com/5489107/diff/2002/src/effects/SkColorMatrixFilt...
> File src/effects/SkColorMatrixFilter.cpp (right):
> 
>
http://codereview.appspot.com/5489107/diff/2002/src/effects/SkColorMatrixFilt...
> src/effects/SkColorMatrixFilter.cpp:338: int unshift = 16 - fState.fShift;
> Would it make sense to just store the originals in setup() to avoid this hairy
> code and int->float conversions?

Yeah, I suggested that it one of my comments.  It would make the filter a little
bigger to keep both versions (80 bytes), but if we're ok with that, I can do it.
 I could also keep only the SkScalar version, and have the raster path do a
lazy-conversion and allocate its own struct, but that's a bit more involved and
probably only worth doing if we care about those 80 bytes.

http://codereview.appspot.com/5489107/diff/2002/src/gpu/GrGLProgram.cpp
> File src/gpu/GrGLProgram.cpp (right):
> 
>
http://codereview.appspot.com/5489107/diff/2002/src/gpu/GrGLProgram.cpp#newco...
> src/gpu/GrGLProgram.cpp:833: } else if (fProgramDesc.fColorMatrixEnabled) {
> So we're saying the the color matrix is only invoked if the filter mode is
dst?
> I know that in the skia pipeline the color matrix is a color filter but in our
> program desc they look orthogonal.
> 
> Perhaps we should add a special mode value that means matrix (rather than the
> bool) or apply them serially.
> 
> I'd prefer the latter since it seems cleaner and we could find another use for
> the color matrix aside from implementing SkColorMatrixFilter.

Done.  I renamed the local var so as not to conflict with the color filter's
one.

http://codereview.appspot.com/5489107/diff/2002/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
http://codereview.appspot.com/5489107/diff/2002/src/gpu/SkGpuDevice.cpp#newco...
> src/gpu/SkGpuDevice.cpp:444: SkScalar matrix[20];
> wrong indent?

Fixed.

Thanks for the review!

[Stephen White, 2012-01-03 19:44:22]

Fixes from review comments

[reed1, 2012-01-03 19:56:09]

Lets keep the conversion code (from funny fixed -> float) in for now, knowing
that if we see performance or precision bugs later, we can always eat the extra
80 bytes.

[bsalomon, 2012-01-03 20:13:48]

http://codereview.appspot.com/5489107/diff/9001/src/gpu/GrGLProgram.cpp
File src/gpu/GrGLProgram.cpp (right):

http://codereview.appspot.com/5489107/diff/9001/src/gpu/GrGLProgram.cpp#newco...
src/gpu/GrGLProgram.cpp:812: if (SkXfermode::kZero_Coeff == uniformCoeff &&
I think there is a subtle bug now. This code assumes it is the last step in
color computation before coverage computation. So if it sees that the coeffs are
(effectively) zero and zero it writes zeros directly to the FS color output
we'll skip the coverage computation. We could check that the color matrix is
disabled as part of the if condition here.

[Stephen White, 2012-01-03 20:32:10]

http://codereview.appspot.com/5489107/diff/9001/src/gpu/GrGLProgram.cpp
File src/gpu/GrGLProgram.cpp (right):

http://codereview.appspot.com/5489107/diff/9001/src/gpu/GrGLProgram.cpp#newco...
src/gpu/GrGLProgram.cpp:812: if (SkXfermode::kZero_Coeff == uniformCoeff &&
Ooooh, good catch!  Thanks!

[bsalomon, 2012-01-03 20:50:24]

On 2012/01/03 20:32:10, Stephen White wrote:
> http://codereview.appspot.com/5489107/diff/9001/src/gpu/GrGLProgram.cpp
> File src/gpu/GrGLProgram.cpp (right):
> 
>
http://codereview.appspot.com/5489107/diff/9001/src/gpu/GrGLProgram.cpp#newco...
> src/gpu/GrGLProgram.cpp:812: if (SkXfermode::kZero_Coeff == uniformCoeff &&
> Ooooh, good catch!  Thanks!

LGTM

[bsalomon, 2012-01-03 21:43:49]

http://codereview.appspot.com/5489107/diff/1009/src/gpu/GrGLProgram.cpp
File src/gpu/GrGLProgram.cpp (right):

http://codereview.appspot.com/5489107/diff/1009/src/gpu/GrGLProgram.cpp#newco...
src/gpu/GrGLProgram.cpp:376: fsCode->appendf("%s = %s * %s + %s;\n", outputVar,
COL_MATRIX_UNI_NAME, inColor, COL_MATRIX_VEC_UNI_NAME);
After looking at Issue 5504117 I now think this needs to unpremul the input and
then premul the output to be consistent with the raster code.

[Stephen White, 2012-01-03 21:52:51]

On 2012/01/03 21:43:49, bsalomon wrote:
> http://codereview.appspot.com/5489107/diff/1009/src/gpu/GrGLProgram.cpp
> File src/gpu/GrGLProgram.cpp (right):
> 
>
http://codereview.appspot.com/5489107/diff/1009/src/gpu/GrGLProgram.cpp#newco...
> src/gpu/GrGLProgram.cpp:376: fsCode->appendf("%s = %s * %s + %s;\n",
outputVar,
> COL_MATRIX_UNI_NAME, inColor, COL_MATRIX_VEC_UNI_NAME);
> After looking at Issue 5504117 I now think this needs to unpremul the input
and
> then premul the output to be consistent with the raster code.

Yep.  I just got lazy 'cos the GM slide has no src alpha, so the results were
all correct.  :)

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

On 2012/01/03 20:50:24, bsalomon wrote:
> On 2012/01/03 20:32:10, Stephen White wrote:
> > http://codereview.appspot.com/5489107/diff/9001/src/gpu/GrGLProgram.cpp
> > File src/gpu/GrGLProgram.cpp (right):
> > 
> >
>
http://codereview.appspot.com/5489107/diff/9001/src/gpu/GrGLProgram.cpp#newco...
> > src/gpu/GrGLProgram.cpp:812: if (SkXfermode::kZero_Coeff == uniformCoeff &&
> > Ooooh, good catch!  Thanks!
> 
> LGTM

Landed as r2948.  Closing.