Add constant color GrFP.

Add constant color GrFP.

Committed: https://skia.googlesource.com/skia/+/dfbbec436cbcacc3270d4b28357c8393e67d6494

Committed: https://skia.googlesource.com/skia/+/c9c3e62b4ef6ec288549a0ad1d252986d7f0889d

[egdaniel, 2015-03-04 15:17:02]

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

[egdaniel, 2015-03-04 15:17:03]

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

https://codereview.chromium.org/978713002/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:134: this->updateIsSingleComponent();
at least with what is in the updateISC function now, I don't think this sets
isSingleComponent correctly.

Think of the case where beforehand we have:
fValidFlags = 0, fIsSingleComponent = true;

If we multiply by a color here where all the components are not equal, we should
end up with something like:
fValidFlags = 0, fIsSingleComponent = false.

In the current thing I think we would end up,
fValidFlags = 0, fIsSingleComponent = true;

https://codereview.chromium.org/978713002/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:264: void updateIsSingleComponent() {
In general I am not a big fan of this function or at least with its given name.
I don't want someone to get confused that calling updateIsSingleComponent will
always update it correctly. This function seems to be written to cover one
specific case.

[bsalomon, 2015-03-04 19:43:36]

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

https://codereview.chromium.org/978713002/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:134: this->updateIsSingleComponent();
On 2015/03/04 15:17:03, egdaniel wrote:
> at least with what is in the updateISC function now, I don't think this sets
> isSingleComponent correctly.
> 
> Think of the case where beforehand we have:
> fValidFlags = 0, fIsSingleComponent = true;
> 
> If we multiply by a color here where all the components are not equal, we
should
> end up with something like:
> fValidFlags = 0, fIsSingleComponent = false.
> 
> In the current thing I think we would end up,
> fValidFlags = 0, fIsSingleComponent = true;

Done.

[egdaniel, 2015-03-04 20:22:34]

https://codereview.chromium.org/978713002/diff/20001/include/gpu/GrInvariantO...
File include/gpu/GrInvariantOutput.h (right):

https://codereview.chromium.org/978713002/diff/20001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:130: } else if (0 == color) {
we definitely don't need the 0 check here since it would get caught above

https://codereview.chromium.org/978713002/diff/20001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:156: } else if (0 == color) {
same as above.

https://codereview.chromium.org/978713002/diff/20001/include/gpu/effects/GrCo...
File include/gpu/effects/GrColorProcessor.h (right):

https://codereview.chromium.org/978713002/diff/20001/include/gpu/effects/GrCo...
include/gpu/effects/GrColorProcessor.h:16: * This is a simple
GrFragmentProcessor that simply outputs a constant color. It may do one of the
maybe "This is a simple GrFragmentProcessor that simply outputs a simple
constant color simply."?

https://codereview.chromium.org/978713002/diff/20001/include/gpu/effects/GrCo...
include/gpu/effects/GrColorProcessor.h:17: * following with its input color:
ignore it, multiply it by the constant color, multiply its alpha
, or multiply

https://codereview.chromium.org/978713002/diff/20001/include/gpu/effects/GrCo...
include/gpu/effects/GrColorProcessor.h:20: class GrColorProcessor : public
GrFragmentProcessor {
Can we call this GrContColorProcessor, or something not GrColorProcessor. In
some crazy future world if we do ever decide to pull apart color and coverage
processors, I would rather save GrColorProcessor to be some base class name.

https://codereview.chromium.org/978713002/diff/20001/src/gpu/effects/GrColorP...
File src/gpu/effects/GrColorProcessor.cpp (right):

https://codereview.chromium.org/978713002/diff/20001/src/gpu/effects/GrColorP...
src/gpu/effects/GrColorProcessor.cpp:43: // We use the value 0 as an uninit
sentinel, so we will always upload that value even if it
update comment to say grcolor_illegal

https://codereview.chromium.org/978713002/diff/20001/src/gpu/effects/GrColorP...
src/gpu/effects/GrColorProcessor.cpp:69:
inout->setToOther(kRGBA_GrColorComponentFlags, fColor,
GrInvariantOutput::kWill_ReadInput);
do you think we should update setToOther to set fIsSingleComponent to true if
fColor is all equal and valid? Currently it always sets it to false. Currently
there is a debug code only function colorComponentsAllEqual that would check
this. Just need to move out of debug only. Other option would be to pass it in
as a param whether it is single component or not.

This thought is for if someone wanted a coverage stage that just set a constant
single component color.

[bsalomon, 2015-03-31 19:33:42]

ptal

https://codereview.chromium.org/978713002/diff/20001/include/gpu/GrInvariantO...
File include/gpu/GrInvariantOutput.h (right):

https://codereview.chromium.org/978713002/diff/20001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:130: } else if (0 == color) {
On 2015/03/04 20:22:34, egdaniel wrote:
> we definitely don't need the 0 check here since it would get caught above

Done.

https://codereview.chromium.org/978713002/diff/20001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:156: } else if (0 == color) {
On 2015/03/04 20:22:34, egdaniel wrote:
> same as above.

Done.

https://codereview.chromium.org/978713002/diff/20001/include/gpu/effects/GrCo...
File include/gpu/effects/GrColorProcessor.h (right):

https://codereview.chromium.org/978713002/diff/20001/include/gpu/effects/GrCo...
include/gpu/effects/GrColorProcessor.h:16: * This is a simple
GrFragmentProcessor that simply outputs a constant color. It may do one of the
On 2015/03/04 20:22:34, egdaniel wrote:
> maybe "This is a simple GrFragmentProcessor that simply outputs a simple
> constant color simply."?

Done. :)

https://codereview.chromium.org/978713002/diff/20001/include/gpu/effects/GrCo...
include/gpu/effects/GrColorProcessor.h:17: * following with its input color:
ignore it, multiply it by the constant color, multiply its alpha
On 2015/03/04 20:22:34, egdaniel wrote:
> , or multiply

Done.

https://codereview.chromium.org/978713002/diff/20001/include/gpu/effects/GrCo...
include/gpu/effects/GrColorProcessor.h:20: class GrColorProcessor : public
GrFragmentProcessor {
On 2015/03/04 20:22:34, egdaniel wrote:
> Can we call this GrContColorProcessor, or something not GrColorProcessor. In
> some crazy future world if we do ever decide to pull apart color and coverage
> processors, I would rather save GrColorProcessor to be some base class name.

Done.

https://codereview.chromium.org/978713002/diff/20001/src/gpu/effects/GrColorP...
File src/gpu/effects/GrColorProcessor.cpp (right):

https://codereview.chromium.org/978713002/diff/20001/src/gpu/effects/GrColorP...
src/gpu/effects/GrColorProcessor.cpp:43: // We use the value 0 as an uninit
sentinel, so we will always upload that value even if it
On 2015/03/04 20:22:34, egdaniel wrote:
> update comment to say grcolor_illegal

Done.

https://codereview.chromium.org/978713002/diff/20001/src/gpu/effects/GrColorP...
src/gpu/effects/GrColorProcessor.cpp:69:
inout->setToOther(kRGBA_GrColorComponentFlags, fColor,
GrInvariantOutput::kWill_ReadInput);
On 2015/03/04 20:22:34, egdaniel wrote:
> do you think we should update setToOther to set fIsSingleComponent to true if
> fColor is all equal and valid? Currently it always sets it to false. Currently
> there is a debug code only function colorComponentsAllEqual that would check
> this. Just need to move out of debug only. Other option would be to pass it in
> as a param whether it is single component or not.
> 
> This thought is for if someone wanted a coverage stage that just set a
constant
> single component color.

Done.

[egdaniel, 2015-03-31 21:02:55]

https://codereview.chromium.org/978713002/diff/100001/include/gpu/GrInvariant...
File include/gpu/GrInvariantOutput.h (right):

https://codereview.chromium.org/978713002/diff/100001/include/gpu/GrInvariant...
include/gpu/GrInvariantOutput.h:207: GrColor a = GrColorUnpackA(fColor);
Maybe we should create some shared helper that can also used by
colorComponentsAllEqual() below

https://codereview.chromium.org/978713002/diff/100001/include/gpu/GrInvariant...
include/gpu/GrInvariantOutput.h:217: SkASSERT(!(kR_GrColorComponentFlag &
fValidFlags) || !GrColorUnpackR(fColor));
are there asserts really needed? seems like we are do the checkout of premul for
some client. But even if it is in an invalid premul state we would just be
forcing it to the correct one anyways.

Or we could make a shared helper with validPremulColor() below.

https://codereview.chromium.org/978713002/diff/100001/include/gpu/GrInvariant...
include/gpu/GrInvariantOutput.h:220: fColor = 0;
just call internalSetToTransparentBlacK()?

https://codereview.chromium.org/978713002/diff/100001/src/gpu/effects/GrConst...
File src/gpu/effects/GrConstColorProcessor.cpp (right):

https://codereview.chromium.org/978713002/diff/100001/src/gpu/effects/GrConst...
src/gpu/effects/GrConstColorProcessor.cpp:75: if (kIgnore_InputMode == fMode) {
did you mean kRGB_InputMode?

[bsalomon, 2015-04-01 19:54:27]

Addressed comments and added test.

https://codereview.chromium.org/978713002/diff/100001/include/gpu/GrInvariant...
File include/gpu/GrInvariantOutput.h (right):

https://codereview.chromium.org/978713002/diff/100001/include/gpu/GrInvariant...
include/gpu/GrInvariantOutput.h:207: GrColor a = GrColorUnpackA(fColor);
On 2015/03/31 21:02:54, egdaniel wrote:
> Maybe we should create some shared helper that can also used by
> colorComponentsAllEqual() below

Done.

https://codereview.chromium.org/978713002/diff/100001/include/gpu/GrInvariant...
include/gpu/GrInvariantOutput.h:217: SkASSERT(!(kR_GrColorComponentFlag &
fValidFlags) || !GrColorUnpackR(fColor));
On 2015/03/31 21:02:54, egdaniel wrote:
> are there asserts really needed? seems like we are do the checkout of premul
for
> some client. But even if it is in an invalid premul state we would just be
> forcing it to the correct one anyways.
> 
> Or we could make a shared helper with validPremulColor() below.

I think if we're going to assume that we're always in premul state, then we
should assert. I added lots of 'em.

https://codereview.chromium.org/978713002/diff/100001/src/gpu/effects/GrConst...
File src/gpu/effects/GrConstColorProcessor.cpp (right):

https://codereview.chromium.org/978713002/diff/100001/src/gpu/effects/GrConst...
src/gpu/effects/GrConstColorProcessor.cpp:75: if (kIgnore_InputMode == fMode) {
On 2015/03/31 21:02:54, egdaniel wrote:
> did you mean kRGB_InputMode?

Done.

[egdaniel, 2015-04-01 20:22:36]

lgtm with one minor thing.

So in ProcOptInfo, every time (in debug) we call
processor->computeInvariantOutput(inOut*), we call validate afterwards which
checks that it is a valid premul color. Do we still need to be checking that
with your change? All the data members are private so they can only be updated
through helper functions. Or maybe change all the Asserts you added to
validates, and then remove the one from procOpt? I have really little opinion
either way.

[bsalomon, 2015-04-01 21:49:08]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-04-01 21:49:08]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/978713002/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2015-04-01 21:49:34]

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

[commit-bot: I haz the power, 2015-04-01 21:55:01]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/dfbbec436cbcacc3270d4b28357c8393e67d6494

[bsalomon, 2015-04-02 15:33:32]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1055023002/ by bsalomon@google.com.

The reason for reverting is: Revert while investigating assertions..

[bsalomon, 2015-04-02 18:06:28]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-04-02 18:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/978713002/#ps200001
(title: "fix mulAlphaByKnownFourComponents
")

[commit-bot: I haz the power, 2015-04-02 18:06:55]

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

[commit-bot: I haz the power, 2015-04-02 18:12:14]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/c9c3e62b4ef6ec288549a0ad1d252986d7f0889d