Create an optimized draw state but not hooked in yet to gpu pipeline

Create an optimized draw state but not hooked in yet to gpu pipeline

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/3658f382cc129e463d7f40a7e68214d04d50fe14

[egdaniel, 2014-08-26 18:01:34]

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

[bsalomon, 2014-08-26 18:41:44]

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

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrDrawState.h#ne...
src/gpu/GrDrawState.h:31: GrDrawState(const SkMatrix& initialViewMatrix)
:fCachedOptState(NULL) {
space after :

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

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:112: int firstCoverageStage = 0;
why don't we check getConstantColorComponents here?

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:133: return fVertexSize == that.fVertexSize;
Comment about why this is required here and not in the base?

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

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrOptDrawState.h...
src/gpu/GrOptDrawState.h:16: * Sub class that is derived from GrRODrawState.
This class holds an optimized version of a
Subclass of GrRODrawState that holds an... ?

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrOptDrawState.h...
src/gpu/GrOptDrawState.h:39: /*
\n

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrOptDrawState.h...
src/gpu/GrOptDrawState.h:49: * The flags must be in the same order as
GrVertexAttribBinding array. In otherwords the lowest
I think removeVAFlag should be plural since it can have multiple bits set.

"The flags mus be in" -> "The flags are in"?

Or maybe:

"If bit i of removeVAFlag is set then vertex attributes with binding
(GrVertexAttribBinding)i will be removed."?

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrOptDrawState.h...
src/gpu/GrOptDrawState.h:56: bool            fInputColorIsUsed;
What's the use case for these?

[egdaniel, 2014-08-26 18:50:52]

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

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:112: int firstCoverageStage = 0;
On 2014/08/26 18:41:43, bsalomon wrote:
> why don't we check getConstantColorComponents here?

I guess there is no reason we can't. I guess at the time I thought there weren't
many cases where we would have a coverage shader that outputed constant coverage

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

https://codereview.chromium.org/508663002/diff/20001/src/gpu/GrOptDrawState.h...
src/gpu/GrOptDrawState.h:56: bool            fInputColorIsUsed;
On 2014/08/26 18:41:43, bsalomon wrote:
> What's the use case for these?

Ahh comment only made into last cl for these. Basically we must keep track of if
we using the input color so that we don't make a uniform color that is not used.
We will get a shader compile warning/error for unused uniforms

[bsalomon, 2014-08-27 13:30:38]

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

https://codereview.chromium.org/508663002/diff/40001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:113: /**
Will doxygen see this comment? Maybe just use // style

[egdaniel, 2014-08-28 13:33:12]

ptal

[bsalomon, 2014-08-28 13:49:27]

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

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:155: fViewMatrix.reset();
Does this not invalidate the opt state? This is a case where I think we *could*
just copy the new matrix into the opt state (if it exists) since we know the
matrix isn't used in any optimization.

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:342: fDrawState->invalidateOptState();
Again, could be more optimal here... There are probably lots of places like
this.

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

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:142: return fVertexSize == that.fVertexSize;
Isn't this checked by the base class now?

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrRODrawState.h
File src/gpu/GrRODrawState.h (right):

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrRODrawState.h#...
src/gpu/GrRODrawState.h:45: size_t getVertexSize() const { return fVertexSize; }
Have you rebased? I thought this was now stride...

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrRODrawState.h#...
src/gpu/GrRODrawState.h:76: const int* getFixedFunctionVertexAttribIndices()
const {
Does this need to be public? Comment about what it returns (length of the array,
how it's indexed).

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrRODrawState.h#...
src/gpu/GrRODrawState.h:224: BlendOptFlags getBlendOpts(bool forceCoverage =
false,
What was it that is keeping this public now? I know you told me, just can't
remember.

[egdaniel, 2014-08-28 14:39:38]

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

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:155: fViewMatrix.reset();
On 2014/08/28 13:49:27, bsalomon wrote:
> Does this not invalidate the opt state? This is a case where I think we
*could*
> just copy the new matrix into the opt state (if it exists) since we know the
> matrix isn't used in any optimization.

Yeah it will invalidate the opt. In regards to copying in, how does the
localcoorcchange effect the stages? Even if the localCoordChange doesn't
directly effect optimizations, we still need to copy in the new stages. However,
at this point the optimized state may have already removed stages, moved
coverage to color, etc., that it could be not much more expensive to just
recompute the whole optState.

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:342: fDrawState->invalidateOptState();
On 2014/08/28 13:49:27, bsalomon wrote:
> Again, could be more optimal here... There are probably lots of places like
> this.

Again feel like this one doesn't gain much from simply copying. But there are
some other places where we may want to. I think for this cl I'd rather be over
zealous in invalidating, then go back over in a different cl and "optimize" away
these invalidations where possible

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

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrOptDrawState.c...
src/gpu/GrOptDrawState.cpp:142: return fVertexSize == that.fVertexSize;
On 2014/08/28 13:49:27, bsalomon wrote:
> Isn't this checked by the base class now?

yes

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrRODrawState.h
File src/gpu/GrRODrawState.h (right):

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrRODrawState.h#...
src/gpu/GrRODrawState.h:45: size_t getVertexSize() const { return fVertexSize; }
On 2014/08/28 13:49:27, bsalomon wrote:
> Have you rebased? I thought this was now stride...

Woops thought I had rebased

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrRODrawState.h#...
src/gpu/GrRODrawState.h:76: const int* getFixedFunctionVertexAttribIndices()
const {
On 2014/08/28 13:49:27, bsalomon wrote:
> Does this need to be public? Comment about what it returns (length of the
array,
> how it's indexed).

moved to protected

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrRODrawState.h#...
src/gpu/GrRODrawState.h:224: BlendOptFlags getBlendOpts(bool forceCoverage =
false,
On 2014/08/28 13:49:27, bsalomon wrote:
> What was it that is keeping this public now? I know you told me, just can't
> remember.

Needed to stay public in this CL since the opt state stuff still isn't connected
in. Goes protected in follow cl once we are using the opt state

[bsalomon, 2014-08-28 15:05:02]

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

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:155: fViewMatrix.reset();
On 2014/08/28 14:39:38, egdaniel wrote:
> On 2014/08/28 13:49:27, bsalomon wrote:
> > Does this not invalidate the opt state? This is a case where I think we
> *could*
> > just copy the new matrix into the opt state (if it exists) since we know the
> > matrix isn't used in any optimization.
> 
> Yeah it will invalidate the opt. In regards to copying in, how does the
> localcoorcchange effect the stages? Even if the localCoordChange doesn't
> directly effect optimizations, we still need to copy in the new stages.
However,
> at this point the optimized state may have already removed stages, moved
> coverage to color, etc., that it could be not much more expensive to just
> recompute the whole optState.  

I think a future evolution could remove GrEffectStage from GrOptDrawState and
instead have arrays of coord change matrices for color/coverage effects. Then if
the GrDS is the sole owner of the GrODS it can set a flag that GrODS's matrices
are dirty and copy the vm plus coord change matrices to GrODS next time the
createOptDrawState function is called.

https://codereview.chromium.org/508663002/diff/80001/src/gpu/GrDrawState.cpp#...
src/gpu/GrDrawState.cpp:342: fDrawState->invalidateOptState();
On 2014/08/28 14:39:38, egdaniel wrote:
> On 2014/08/28 13:49:27, bsalomon wrote:
> > Again, could be more optimal here... There are probably lots of places like
> > this.
> 
> Again feel like this one doesn't gain much from simply copying. But there are
> some other places where we may want to. I think for this cl I'd rather be over
> zealous in invalidating, then go back over in a different cl and "optimize"
away
> these invalidations where possible

See comment above, but definitely don't want to do this in this CL. Just
thinking about future iterations of GrODS. It is starting to feel like the
common base class will become a burden over time, but should keep for now.

https://codereview.chromium.org/508663002/diff/100001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/508663002/diff/100001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:106: ~AutoVertexAttribRestore() {
fDrawState->internalSetVertexAttribs(fVAPtr, fVACount, fVAStride); }
This guy no longer fits on one line.

https://codereview.chromium.org/508663002/diff/100001/src/gpu/GrRODrawState.h
File src/gpu/GrRODrawState.h (right):

https://codereview.chromium.org/508663002/diff/100001/src/gpu/GrRODrawState.h...
src/gpu/GrRODrawState.h:390: const int* getFixedFunctionVertexAttribIndices()
const {
If this is protected and the field it returns is protected, do we need it?

[egdaniel, 2014-08-28 15:24:24]

https://codereview.chromium.org/508663002/diff/100001/src/gpu/GrRODrawState.h
File src/gpu/GrRODrawState.h (right):

https://codereview.chromium.org/508663002/diff/100001/src/gpu/GrRODrawState.h...
src/gpu/GrRODrawState.h:390: const int* getFixedFunctionVertexAttribIndices()
const {
On 2014/08/28 15:05:02, bsalomon wrote:
> If this is protected and the field it returns is protected, do we need it?

Ugh forgot to hit save when test compiling last time. The function has to stay
public so that GrOptDrawState::GrOptDrawState(const GrDrawState& drawState) can
make a copy of it. Even passing in a GrRO wouldn't help since you can only
access protect members of a parent from an object of same type.

Alternatively we could remove this function completely and just call each
[pos/color/coverage/localCord]AttributeIndex function to set the values but this
seems like a waste.

[bsalomon, 2014-08-28 15:47:01]

https://codereview.chromium.org/508663002/diff/100001/src/gpu/GrRODrawState.h
File src/gpu/GrRODrawState.h (right):

https://codereview.chromium.org/508663002/diff/100001/src/gpu/GrRODrawState.h...
src/gpu/GrRODrawState.h:390: const int* getFixedFunctionVertexAttribIndices()
const {
On 2014/08/28 15:24:24, egdaniel wrote:
> On 2014/08/28 15:05:02, bsalomon wrote:
> > If this is protected and the field it returns is protected, do we need it?
> 
> Ugh forgot to hit save when test compiling last time. The function has to stay
> public so that GrOptDrawState::GrOptDrawState(const GrDrawState& drawState)
can
> make a copy of it. Even passing in a GrRO wouldn't help since you can only
> access protect members of a parent from an object of same type.
> 
> Alternatively we could remove this function completely and just call each
> [pos/color/coverage/localCord]AttributeIndex function to set the values but
this
> seems like a waste.

Let's just keep it public for now then.

[egdaniel, 2014-09-08 16:05:35]

Probably should land the change to EffectStage copy ctor before this, but this
is first step in putting opt state in

[bsalomon, 2014-09-08 20:59:03]

https://codereview.chromium.org/508663002/diff/160001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/508663002/diff/160001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:183: const GrEffect* setGeometryProcessor(const GrEffect*
effect, int attr0 = -1, int attr1 = -1) {
Don't need to inval here?

https://codereview.chromium.org/508663002/diff/160001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:341: // TODO: we should copy new blend
incomplete comment?

https://codereview.chromium.org/508663002/diff/160001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:553: void invalidateBlendOptFlags() const {
if invalidateOptState() is the only caller of this, might as well fold it into
that function.

[egdaniel, 2014-09-11 15:35:42]

https://codereview.chromium.org/508663002/diff/160001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.chromium.org/508663002/diff/160001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:183: const GrEffect* setGeometryProcessor(const GrEffect*
effect, int attr0 = -1, int attr1 = -1) {
Added inval (missed function from rebase)
On 2014/09/08 20:59:03, bsalomon wrote:
> Don't need to inval here?

https://codereview.chromium.org/508663002/diff/160001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:341: // TODO: we should copy new blend
removed
On 2014/09/08 20:59:03, bsalomon wrote:
> incomplete comment?

https://codereview.chromium.org/508663002/diff/160001/src/gpu/GrDrawState.h#n...
src/gpu/GrDrawState.h:553: void invalidateBlendOptFlags() const {
Merged together
On 2014/09/08 20:59:03, bsalomon wrote:
> if invalidateOptState() is the only caller of this, might as well fold it into
> that function.

[bsalomon, 2014-09-11 17:00:22]

lgtm

[egdaniel, 2014-09-15 13:54:20]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2014-09-15 13:55:16]

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

[commit-bot: I haz the power, 2014-09-15 14:02:03]

Committed patchset #11 (id:200001) as 3658f382cc129e463d7f40a7e68214d04d50fe14