Remove GP from drawstate, revision of invariant output for GP

Remove GP from drawstate, revision of invariant output for GP

BUG=skia:

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

Committed: https://skia.googlesource.com/skia/+/56995b5cc00c9c83bd5fcf86bca9a67e939a96cb

[joshualitt, 2014-12-09 20:22:16]

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

[joshualitt, 2014-12-09 20:22:17]

In this CL I have removed GP from drawstate, and now pass it explicitly into all
draw calls.  GP still lives on optstate.

I have also cleaned up the invariant calculations a bit with the addition of the
InitInvariantOutput struct.  There is still a ton of cleanup to do after this.

[bsalomon, 2014-12-09 20:39:50]

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

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:21: fValidFlags = kRGBA_GrColorComponentFlags;
fSingleComponent = (true if all the values the same);

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:25: fColor |= (fColor >> GrColor_SHIFT_A) |
alpha;
fColor = (alpha << SHIFT_A);

right?

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:26: fValidFlags |= kA_GrColorComponentFlag;
= not |=, right?

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:27: }
fSingleComponent = true;

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:30: fValidFlags &= ~kA_GrColorComponentFlag;
fValidFlags = 0;
fSingleComponent = true;

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:34: fValidFlags = 0;
fSingleComponent = false;

Should we make the name of this explicitly 4 channel? e.g. setUnknownComponents
?

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:39: fValidFlags |= kA_GrColorComponentFlag;
fSingleComponent = false

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
include/gpu/GrInvariantOutput.h:42: void setIsSingleComponent() {
fIsSingleComponent = true; }
Do we need this two helpers? seems always implied by the above helpers and the
fields are all public.

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

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrXferProcessor....
include/gpu/GrXferProcessor.h:98: void computeInvariantOutput(GrInvariantOutput*
inout) const;
Do we need this at all?

https://codereview.chromium.org/791743003/diff/1/include/gpu/GrXferProcessor....
include/gpu/GrXferProcessor.h:111: * Subclass implements this to support
getConstantColorComponents(...).
comment is wrong?

[joshualitt, 2014-12-10 15:00:28]

On 2014/12/09 20:39:50, bsalomon wrote:
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutput.h
> File include/gpu/GrInvariantOutput.h (right):
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
> include/gpu/GrInvariantOutput.h:21: fValidFlags = kRGBA_GrColorComponentFlags;
> fSingleComponent = (true if all the values the same);
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
> include/gpu/GrInvariantOutput.h:25: fColor |= (fColor >> GrColor_SHIFT_A) |
> alpha;
> fColor = (alpha << SHIFT_A);
> 
> right?
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
> include/gpu/GrInvariantOutput.h:26: fValidFlags |= kA_GrColorComponentFlag;
> = not |=, right?
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
> include/gpu/GrInvariantOutput.h:27: }
> fSingleComponent = true;
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
> include/gpu/GrInvariantOutput.h:30: fValidFlags &= ~kA_GrColorComponentFlag;
> fValidFlags = 0;
> fSingleComponent = true;
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
> include/gpu/GrInvariantOutput.h:34: fValidFlags = 0;
> fSingleComponent = false;
> 
> Should we make the name of this explicitly 4 channel? e.g.
setUnknownComponents
> ?
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
> include/gpu/GrInvariantOutput.h:39: fValidFlags |= kA_GrColorComponentFlag;
> fSingleComponent = false
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrInvariantOutpu...
> include/gpu/GrInvariantOutput.h:42: void setIsSingleComponent() {
> fIsSingleComponent = true; }
> Do we need this two helpers? seems always implied by the above helpers and the
> fields are all public.
> 
> https://codereview.chromium.org/791743003/diff/1/include/gpu/GrXferProcessor.h
> File include/gpu/GrXferProcessor.h (right):
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrXferProcessor....
> include/gpu/GrXferProcessor.h:98: void
computeInvariantOutput(GrInvariantOutput*
> inout) const;
> Do we need this at all?
> 
>
https://codereview.chromium.org/791743003/diff/1/include/gpu/GrXferProcessor....
> include/gpu/GrXferProcessor.h:111: * Subclass implements this to support
> getConstantColorComponents(...).
> comment is wrong?

feedback incorporated, and CL has been updated.  This cl is still rough around
optstate, but that will be the follow on CL

[bsalomon, 2014-12-10 15:27:00]

Should we make changes to the names of the helper functions on the inout thing
taken by FPs? Seems like the names should be pretty similar between GP and FP,
just "set" on GP versions and "mulBy", etc on FP.

https://codereview.chromium.org/791743003/diff/20001/gm/beziereffects.cpp
File gm/beziereffects.cpp (right):

https://codereview.chromium.org/791743003/diff/20001/gm/beziereffects.cpp#new...
gm/beziereffects.cpp:170: tt.target()->drawIndexed(&ds, gp.get(),
kTriangleFan_GrPrimitiveType, 0, 0,4,6);
Can't you just pass gp? I thought it would auto-cast to GrGP*.

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

https://codereview.chromium.org/791743003/diff/20001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:20: void setIsKnownFourComponents(GrColor color)
{
Maybe it's just me, but I find the setIs language kinda weird. setKnown,
setToKnown, ...

https://codereview.chromium.org/791743003/diff/20001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:31: void setIsUnknownAndOpaqueFourComponents() {
UnknownOpaqueFourComponents?

https://codereview.chromium.org/791743003/diff/20001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:38: fColor = alpha << GrColor_SHIFT_A;
I think this should have alpha replicated across all four channels and all four
flags set, but could be wrong. Or maybe either is right? Greg?

I want to make sure this works out to a known four channel:

GP: setIsKnownSingleComponent(a);
FP: mulByKnownFourComponent(color)

should get all flags set, a*color.

Maybe we need some unit tests around this procInfo stuff?

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrAARectRenderer...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrAARectRenderer...
src/gpu/GrAARectRenderer.cpp:28: static const GrGeometryProcessor*
set_rect_attribs(GrDrawState* drawState, GrColor color,
rename to create_rect_gp? drawState as const& now?

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrGeometryProces...
File src/gpu/GrGeometryProcessor.h (right):

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrGeometryProces...
src/gpu/GrGeometryProcessor.h:52: virtual void
computeOutputColor(GrInitInvariantOutput* out) const = 0;
I'm wondering if this should be "getInvariantColorOutput"? computeOutputeColor
sounds like it is related to emitting code for color computation. (also not
crazy about the name we're using for FP... "compute" doesn't seem like a useful
verb. updateInvariant()? updateInvariantOutput?)

Also, I wonder if we should go ahead with a non-virtual foo, virtual onFoo
pattern here. It wouldn't surprise if we want to do stuff in the base class at
some point (e.g. overrides). If the foo() call is simply an inline call to onFoo
then there is no extra expense. The number of GPs is smallish now so making the
switch is less painful.

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrGeometryProces...
src/gpu/GrGeometryProcessor.h:135: // TODO this is fragile, most gps set their
coverage to 0xff so this is okay
what's the plan here?

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrGeometryProces...
src/gpu/GrGeometryProcessor.h:203: * The path equivalent of the GP.  Can only
upload uniform color.
Maybe a comment that we plan to extend responsibility to specify coord
transforms to the prim proc subclasses?

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrGeometryProces...
src/gpu/GrGeometryProcessor.h:212: virtual uint8_t coverage() SK_OVERRIDE const
{ return 0xff; }
no need for virtual keyword on overrides.

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrInOrderDrawBuf...
File src/gpu/GrInOrderDrawBuffer.cpp (right):

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrInOrderDrawBuf...
src/gpu/GrInOrderDrawBuffer.cpp:65: static const GrGeometryProcessor*
set_vertex_attributes(GrDrawState* drawState,
create_rect_gp?

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrInOrderDrawBuf...
src/gpu/GrInOrderDrawBuffer.cpp:73:
drawState->setHint(GrDrawState::kVertexColorsAreOpaque_Hint, true);
just a note: replacing hint would be a good candidate for an initial use of the
bundle, tracker system.

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

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrProcessor.cpp#...
src/gpu/GrProcessor.cpp:197: //out->setIsSingleComponent();
why commented out? why calling helper and setting explicit fields? Does this
relate to my earlier comment on setIKSC() setting all the flags, comps?

[egdaniel, 2014-12-10 15:54:30]

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

https://codereview.chromium.org/791743003/diff/20001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:38: fColor = alpha << GrColor_SHIFT_A;
On 2014/12/10 15:26:59, bsalomon wrote:
> I think this should have alpha replicated across all four channels and all
four
> flags set, but could be wrong. Or maybe either is right? Greg?
> 
> I want to make sure this works out to a known four channel:
> 
> GP: setIsKnownSingleComponent(a);
> FP: mulByKnownFourComponent(color)
> 
> should get all flags set, a*color.
> 
> Maybe we need some unit tests around this procInfo stuff?

Yes we need the alpha replicated across all channels if it is single component.
However this is really only to easily handle the mulByKnown4Comp case, which we
actually do not have currently (but might be able to add with some work on color
filter stuff for example).

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrOptDrawState.h
File src/gpu/GrOptDrawState.h (right):

https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrOptDrawState.h...
src/gpu/GrOptDrawState.h:211: void setOutputStateInfo(const GrDrawState& ds,
GrXferProcessor::OptFlags,
this function is being removed in cl landing now

https://codereview.chromium.org/791743003/diff/20001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/791743003/diff/20001/tests/GLProgramsTest.cpp...
tests/GLProgramsTest.cpp:300: const GrPathProcessor* pp = NULL;
since below we have a primP as well, I feel like these should be named pathProc
and primProc so that there is no confusion as to what pp is.

[bsalomon, 2014-12-10 15:56:05]

https://codereview.chromium.org/791743003/diff/20001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/791743003/diff/20001/tests/GLProgramsTest.cpp...
tests/GLProgramsTest.cpp:300: const GrPathProcessor* pp = NULL;
On 2014/12/10 15:54:30, egdaniel wrote:
> since below we have a primP as well, I feel like these should be named
pathProc
> and primProc so that there is no confusion as to what pp is.

+1, we should avoid the acronym pp since it is ambiguous (and juvenile :-D).

[joshualitt, 2014-12-10 15:57:41]

I think I got most of the feedback with this updated cl

> Maybe we need some unit tests around this procInfo stuff?
I agree, I'd rather clean up everything first.  Probably I'm not the best guy to
write those tests.

> Also, I wonder if we should go ahead with a non-virtual foo, virtual onFoo
> pattern here. It wouldn't surprise if we want to do stuff in the base class at
> some point (e.g. overrides). If the foo() call is simply an inline call to
onFoo
> then there is no extra expense. The number of GPs is smallish now so making
the
> switch is less painful.
This is the pattern for GPs, it lives on the GP base class, and Path Processor
base class.


>
https://codereview.chromium.org/791743003/diff/20001/src/gpu/GrInOrderDrawBuf...
> src/gpu/GrInOrderDrawBuffer.cpp:73:
> drawState->setHint(GrDrawState::kVertexColorsAreOpaque_Hint, true);
> just a note: replacing hint would be a good candidate for an initial use of
the
> bundle, tracker system.
agreed

[joshualitt, 2014-12-10 16:12:02]

more feedback incorporated.  I think I got most of the 'PP' references, I'll
clean up more as I go.

[bsalomon, 2014-12-10 18:44:31]

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

https://codereview.chromium.org/791743003/diff/40001/include/gpu/GrInvariantO...
include/gpu/GrInvariantOutput.h:39: fValidFlags = kA_GrColorComponentFlag;
kRGBA?

https://codereview.chromium.org/791743003/diff/40001/src/gpu/GrAARectRenderer...
File src/gpu/GrAARectRenderer.cpp (right):

https://codereview.chromium.org/791743003/diff/40001/src/gpu/GrAARectRenderer...
src/gpu/GrAARectRenderer.cpp:28: static const GrGeometryProcessor*
create_rect_gp(const GrDrawState* drawState, GrColor color,
pass ds by ref?

https://codereview.chromium.org/791743003/diff/40001/src/gpu/GrGeometryProces...
File src/gpu/GrGeometryProcessor.h (right):

https://codereview.chromium.org/791743003/diff/40001/src/gpu/GrGeometryProces...
src/gpu/GrGeometryProcessor.h:52: virtual void
getOutputColor(GrInitInvariantOutput* out) const = 0;
still find these names confusing, I think it should have the word "invariant",
or something to that effect since the color/cov is variable and this is about
querying whether there are known aspects of it (always single-channel, the red
channel is known to be 0.5, etc).

[joshualitt, 2014-12-10 19:04:45]

Feedback incorporated.  I also updated GrOptProcInfo to fix a possible future
bug with readsfragposition and primitive processors.

[bsalomon, 2014-12-10 19:10:54]

lgtm

[joshualitt, 2014-12-10 21:39:06]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-12-10 21:39:49]

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

[commit-bot: I haz the power, 2014-12-10 21:49:02]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/c6bc58eded89b0c0a36b8e20e193c200f297a0da

[joshualitt, 2014-12-10 22:11:29]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/794843002/ by joshualitt@google.com.

The reason for reverting is: breaks mac.

[joshualitt, 2014-12-11 22:35:40]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-12-11 23:08:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-11 23:08:50]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[joshualitt, 2014-12-11 23:34:46]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-12-11 23:35:28]

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

[commit-bot: I haz the power, 2014-12-11 23:44:07]

Committed patchset #17 (id:300001) as
https://skia.googlesource.com/skia/+/56995b5cc00c9c83bd5fcf86bca9a67e939a96cb