Add isSingleComponent bool to getConstantColorComponent

Add isSingleComponent bool to getConstantColorComponent

Initial step to allowing effects to use/output 1 or 4 color/coverage components. This cl doesn't change any current logic and all effects still assume they are working with 4 components.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/3b8af078281a5a20f951b9fd84f38d92b8f6217b

Committed: https://skia.googlesource.com/skia/+/1a8ecdfb73a15de600d5779b75d7c4b61863c50b

[egdaniel, 2014-09-29 14:37:48]

egdaniel@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@chromium.org

[joshua.litt, 2014-09-29 15:22:08]

lgtm with nit

https://codereview.chromium.org/608253002/diff/1/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.chromium.org/608253002/diff/1/src/gpu/GrDrawState.cpp#newc...
src/gpu/GrDrawState.cpp:799: processor->getConstantColorComponents(&coverage,
&coverageComponentFlags, &isSingleComponent);
Newline

[bsalomon, 2014-09-29 15:28:04]

Don't all the the overrides need to set it to false (at least for now)? It seems
like they all are just passing it through which means if it was true for a
processor that does compute different r,g,b,a values then it's output value for
isSingleComponent will be wrong.

https://codereview.chromium.org/608253002/diff/1/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/608253002/diff/1/include/gpu/GrProcessor.h#ne...
include/gpu/GrProcessor.h:47: * be all 1's or all 0's. If it is all 1's then all
color components must be equal.
all 1's isn't quit right (since this is a uint32_t).

"...then the flag value for r,g,b, and a must all be the same. If the flags are
set then ..."?

[egdaniel, 2014-09-29 15:50:20]

Set isSingleComponent to false in all the current effects

https://codereview.chromium.org/608253002/diff/1/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/608253002/diff/1/include/gpu/GrProcessor.h#ne...
include/gpu/GrProcessor.h:47: * be all 1's or all 0's. If it is all 1's then all
color components must be equal.
On 2014/09/29 15:28:04, bsalomon wrote:
> all 1's isn't quit right (since this is a uint32_t).
> 
> "...then the flag value for r,g,b, and a must all be the same. If the flags
are
> set then ..."?

Done.

https://codereview.chromium.org/608253002/diff/1/src/gpu/GrDrawState.cpp
File src/gpu/GrDrawState.cpp (right):

https://codereview.chromium.org/608253002/diff/1/src/gpu/GrDrawState.cpp#newc...
src/gpu/GrDrawState.cpp:799: processor->getConstantColorComponents(&coverage,
&coverageComponentFlags, &isSingleComponent);
On 2014/09/29 15:22:08, joshua.litt wrote:
> Newline

Done.

[reed1, 2014-09-29 15:58:31]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-09-29 15:58:31]

Have you considered, since you're making a change to a virtual (with LOTS of
subclasses) changing to a struct of some kind instead of more flat parameters?
This might ease future refactoring (since there's no signature change)...

[egdaniel, 2014-09-29 21:03:21]

Updated

[bsalomon, 2014-09-30 00:41:01]

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:40: struct InvarientOutput{
"Invariant" (gulp)

There might be some more useful helpers for modifying this here:

e.g.:

void mulByUnknownOpaqueColor() {
    if (this->isOpaque()) {
        fValidFlags = kA_GrCCF;
    } else {
        fValidFlags = 0;
    }
    fIsSingleComponent = false;
}

void mulByUnknownColor() {
    // really any mulBy should have this "if"
    if (kA_GrCC && 0 == GrColorUnpackAlpha(fColor)) {
        fValidFlags = kRGBA_GrCCF;
        fColor = 0;
        fIsSingleComponent = true;
    } else {
        fValidFlags = 0;
        fIsSingleComponent = false;
    }
}

void mulByUnknownAlpha() {
   // fIsSingleComponent stays the same
   fValidFlags = 0;
}

void mulByKnownColor(GrColor color)

(I didn't think these through all that thoroughly)

There are probably very few effects that wouldn't be able to share a common
implementation using helpers.

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:42: uint32_t validFlags;
fColor, fValidFlags, fIsSingleComponent

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:45: bool hasOpaqueAlpha() const {
isOpaque?

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:50: return (validFlags == kRGBA_GrColorComponentFlags
&&
isSolidWhite?

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:72: void computeInvarientOutput(InvarientOutput*
invarientOutput) const {
Because the param is inout it feels weird to call it the param invariantOutput. 
"inout"? ...

https://codereview.chromium.org/608253002/diff/80001/src/gpu/GrProcessor.cpp
File src/gpu/GrProcessor.cpp (right):

https://codereview.chromium.org/608253002/diff/80001/src/gpu/GrProcessor.cpp#...
src/gpu/GrProcessor.cpp:98: }
else {
  SkASSERT(!kRGBA || !<all components the same>);
}
?

[egdaniel, 2014-10-02 14:58:33]

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor.h
File include/gpu/GrProcessor.h (right):

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:40: struct InvarientOutput{
On 2014/09/30 00:41:01, bsalomon wrote:
> "Invariant" (gulp)

Fixed speeling

> 
> There might be some more useful helpers for modifying this here:
> 
> e.g.:
> 
> void mulByUnknownOpaqueColor() {
>     if (this->isOpaque()) {
>         fValidFlags = kA_GrCCF;
>     } else {
>         fValidFlags = 0;
>     }
>     fIsSingleComponent = false;
> }
> 
> void mulByUnknownColor() {
>     // really any mulBy should have this "if"
>     if (kA_GrCC && 0 == GrColorUnpackAlpha(fColor)) {
>         fValidFlags = kRGBA_GrCCF;
>         fColor = 0;
>         fIsSingleComponent = true;
>     } else {
>         fValidFlags = 0;
>         fIsSingleComponent = false;
>     }
> }
> 
> void mulByUnknownAlpha() {
>    // fIsSingleComponent stays the same
>    fValidFlags = 0;
> }
> 
> void mulByKnownColor(GrColor color)
> 
> (I didn't think these through all that thoroughly)
> 
> There are probably very few effects that wouldn't be able to share a common
> implementation using helpers.

Will work on adding helpers in follow up cl(s)

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:42: uint32_t validFlags;
On 2014/09/30 00:41:01, bsalomon wrote:
> fColor, fValidFlags, fIsSingleComponent

Done.

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:45: bool hasOpaqueAlpha() const {
On 2014/09/30 00:41:01, bsalomon wrote:
> isOpaque?

Done.

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:50: return (validFlags == kRGBA_GrColorComponentFlags
&&
On 2014/09/30 00:41:01, bsalomon wrote:
> isSolidWhite?

Done.

https://codereview.chromium.org/608253002/diff/80001/include/gpu/GrProcessor....
include/gpu/GrProcessor.h:72: void computeInvarientOutput(InvarientOutput*
invarientOutput) const {
On 2014/09/30 00:41:01, bsalomon wrote:
> Because the param is inout it feels weird to call it the param
invariantOutput. 
> "inout"? ...

Done.

https://codereview.chromium.org/608253002/diff/80001/src/gpu/GrProcessor.cpp
File src/gpu/GrProcessor.cpp (right):

https://codereview.chromium.org/608253002/diff/80001/src/gpu/GrProcessor.cpp#...
src/gpu/GrProcessor.cpp:98: }
On 2014/09/30 00:41:01, bsalomon wrote:
> else {
>   SkASSERT(!kRGBA || !<all components the same>);
> }
> ?

Once we have better helper functions that will set the correct single/four
component bool, I will add this assert in

[bsalomon, 2014-10-02 15:16:25]

lgtm

[egdaniel, 2014-10-02 16:48:32]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-10-02 16:48:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608253002/100001

[commit-bot: I haz the power, 2014-10-02 16:57:53]

Committed patchset #6 (id:100001) as 3b8af078281a5a20f951b9fd84f38d92b8f6217b

[robertphillips, 2014-10-02 19:11:21]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/617853003/ by robertphillips@google.com.

The reason for reverting is: Changing some GMs.

[egdaniel, 2014-10-02 20:48:22]

Fix for the cmp gm failures

[bsalomon, 2014-10-02 21:05:07]

lgtm

[egdaniel, 2014-10-03 13:16:25]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-10-03 13:17:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/608253002/120001

[commit-bot: I haz the power, 2014-10-03 13:24:19]

Committed patchset #7 (id:120001) as 1a8ecdfb73a15de600d5779b75d7c4b61863c50b