Add GrEffect::updateKnownColorComponents(). It is used to determine whether the output of an effect…

Add GrEffect::updateKnownColorComponents(). It is used to determine whether the output of an effect has a constant output value for r,g,b, or a.
Committed: https://code.google.com/p/skia/source/detail?r=7144

[bsalomon, 2013-01-09 19:09:41]

Currently I just use new virtual to reimplement srcAlphaWillBeOne() but the idea
is to use it in at least two other places:

1) GrDrawState::isOpaque() (to be written)

2) In buildProgram() to omit stages that don't contribute to the computed color
or coverage value.

[robertphillips, 2013-01-09 19:36:31]

LGTM + some nits & a question

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkColorMatrixFil...
File src/effects/SkColorMatrixFilter.cpp (right):

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkColorMatrixFil...
src/effects/SkColorMatrixFilter.cpp:340: 
OVERRIDE?

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkColorMatrixFil...
src/effects/SkColorMatrixFilter.cpp:360: if (0 != fMatrix.fMat[15 + i]) {
// if any relevant component of the color to be passed through the matrix is
unknown we can't know the final result

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkColorMatrixFil...
src/effects/SkColorMatrixFilter.cpp:360: if (0 != fMatrix.fMat[15 + i]) {
Define 15 to be something like kAlphaRowOffset (or something)? 19 to
AlphaTranslateOffset?

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkLightingImageF...
File src/effects/SkLightingImageFilter.cpp (right):

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:314: 
virtual?

https://codereview.appspot.com/7064057/diff/4001/src/gpu/effects/GrSingleText...
File src/gpu/effects/GrSingleTextureEffect.cpp (right):

https://codereview.appspot.com/7064057/diff/4001/src/gpu/effects/GrSingleText...
src/gpu/effects/GrSingleTextureEffect.cpp:105:
GrPixelConfigIsOpaque(fTextureAccess.getTexture()->config())) {
Shouldn't we also check if the texture is known to be opaque?

[bsalomon, 2013-01-09 19:43:00]

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkColorMatrixFil...
File src/effects/SkColorMatrixFilter.cpp (right):

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkColorMatrixFil...
src/effects/SkColorMatrixFilter.cpp:340: 
On 2013/01/09 19:36:31, robertphillips wrote:
> OVERRIDE?

Done.

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkColorMatrixFil...
src/effects/SkColorMatrixFilter.cpp:360: if (0 != fMatrix.fMat[15 + i]) {
On 2013/01/09 19:36:31, robertphillips wrote:
> // if any relevant component of the color to be passed through the matrix is
> unknown we can't know the final result

Done.

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkColorMatrixFil...
src/effects/SkColorMatrixFilter.cpp:360: if (0 != fMatrix.fMat[15 + i]) {
On 2013/01/09 19:36:31, robertphillips wrote:
> // if any relevant component of the color to be passed through the matrix is
> unknown we can't know the final result

Done.

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkLightingImageF...
File src/effects/SkLightingImageFilter.cpp (right):

https://codereview.appspot.com/7064057/diff/4001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:314: 
On 2013/01/09 19:36:31, robertphillips wrote:
> virtual?

Done.

https://codereview.appspot.com/7064057/diff/4001/src/gpu/effects/GrSingleText...
File src/gpu/effects/GrSingleTextureEffect.cpp (right):

https://codereview.appspot.com/7064057/diff/4001/src/gpu/effects/GrSingleText...
src/gpu/effects/GrSingleTextureEffect.cpp:105:
GrPixelConfigIsOpaque(fTextureAccess.getTexture()->config())) {
On 2013/01/09 19:36:31, robertphillips wrote:
> Shouldn't we also check if the texture is known to be opaque?

I think in a future CL I will add a knownOpaque flag to the
GrSingleTextureEffect cons. We can pass along SkBitmap's isOpaque() result when
available.

[bsalomon, 2013-01-10 19:18:25]

Stephen, did you want to look at this one? I implemented the new virtual for
several of the effects you originally wrote.

[Stephen White, 2013-01-11 16:09:58]

https://codereview.appspot.com/7064057/diff/11001/include/gpu/GrEffect.h
File include/gpu/GrEffect.h (right):

https://codereview.appspot.com/7064057/diff/11001/include/gpu/GrEffect.h#newc...
include/gpu/GrEffect.h:60: virtual void updateKnownColorComponents(GrColor*
color, uint32_t* validFlags) const = 0;
As discussed, I'd prefer the name getContantColorComponents().  Update sounds
like it's updating something in the effect itself, and "known" is not clear to
me.

[bsalomon, 2013-01-11 16:31:22]

The latest patchset (5) has the renaming to getConstantColorComponents and a
very minor comment change in SkTableColorFilter.cpp. I sync'ed up to ToT so
sorry for the extra noise in the delta from patchset 4. But other than the
renaming and comment change the diff is the same as patchset 4.

[Stephen White, 2013-01-11 20:44:45]

LGTM with nits (sorry if you get this twice; rietveld is giving me grief.)

https://codereview.appspot.com/7064057/diff/15001/src/effects/SkColorMatrixFi...
File src/effects/SkColorMatrixFilter.cpp (right):

https://codereview.appspot.com/7064057/diff/15001/src/effects/SkColorMatrixFi...
src/effects/SkColorMatrixFilter.cpp:343: // We only bother to check whether the
alpha channel will be known. If SkColorMatrix had
Nit:  known -> constant

https://codereview.appspot.com/7064057/diff/15001/src/effects/SkColorMatrixFi...
src/effects/SkColorMatrixFilter.cpp:365: // If any relevant component of the
color to be passed through the matrix is unknown we
Nit:  non-constant

https://codereview.appspot.com/7064057/diff/15001/src/effects/SkColorMatrixFi...
src/effects/SkColorMatrixFilter.cpp:373: outputA +=
fMatrix.fMat[kAlphaRowStartIdx + i] * component;
I was going to ask if there's a possibility for overflow here, but if there is,
it'd overflow in the normal processing path as well, I'm guessing.