OptState owns program descriptor

OptState owns program descriptor

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/79f8faeea2692d2948c0f634e956d1e7fc8333e0

[joshualitt, 2014-10-22 21:03:17]

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

[joshualitt, 2014-10-23 15:30:38]

feedback inc

[joshualitt, 2014-10-23 16:45:24]

feedback inc

[bsalomon, 2014-10-23 17:08:52]

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

https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.h:444: GrProgramDesc::PrimaryOutputType 
fPrimaryOutputType : 8;
Is this stuff part of the key already? Wondering if we need to store it here
anymore.

[egdaniel, 2014-10-23 17:15:07]

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

https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.h:444: GrProgramDesc::PrimaryOutputType 
fPrimaryOutputType : 8;
On 2014/10/23 17:08:52, bsalomon wrote:
> Is this stuff part of the key already? Wondering if we need to store it here
> anymore.

An option would be to store these values locally in OptState's ctor, and then
pass them into the buildKey function.

[joshualitt, 2014-10-23 17:45:07]

feedback inc

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

https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.h:444: GrProgramDesc::PrimaryOutputType 
fPrimaryOutputType : 8;
On 2014/10/23 17:15:07, egdaniel wrote:
> On 2014/10/23 17:08:52, bsalomon wrote:
> > Is this stuff part of the key already? Wondering if we need to store it here
> > anymore.
> 
> An option would be to store these values locally in OptState's ctor, and then
> pass them into the buildKey function.

Seems a bit icky.  We calculate these types while building the opt state so
might be best to just leave them on the opt state.

[egdaniel, 2014-10-23 17:53:02]

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

https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.h:444: GrProgramDesc::PrimaryOutputType 
fPrimaryOutputType : 8;
On 2014/10/23 17:45:06, joshualitt wrote:
> On 2014/10/23 17:15:07, egdaniel wrote:
> > On 2014/10/23 17:08:52, bsalomon wrote:
> > > Is this stuff part of the key already? Wondering if we need to store it
here
> > > anymore.
> > 
> > An option would be to store these values locally in OptState's ctor, and
then
> > pass them into the buildKey function.
> 
> Seems a bit icky.  We calculate these types while building the opt state so
> might be best to just leave them on the opt state.

I don't think it is that wacky. The only reason we held this values on the ODS
was for building the header. I actually propose extending the idea and do this
same thing for fInputColorIsUsed, fInputCoverageIsUsed, fReadsDst,
fReadsFragPosition, and fRequiresLocalCoordAttrib. These values are meaningless
once we have built the header since all the info is stored there. Maybe make a
descriptor struct for all these values that we pass into the buildKey/Build
functions. No reason for them to take up space on the ODS.

[joshualitt, 2014-10-23 18:00:28]

On 2014/10/23 17:53:02, egdaniel wrote:
> https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState.h
> File src/gpu/GrOptDrawState.h (right):
> 
>
https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState....
> src/gpu/GrOptDrawState.h:444: GrProgramDesc::PrimaryOutputType 
> fPrimaryOutputType : 8;
> On 2014/10/23 17:45:06, joshualitt wrote:
> > On 2014/10/23 17:15:07, egdaniel wrote:
> > > On 2014/10/23 17:08:52, bsalomon wrote:
> > > > Is this stuff part of the key already? Wondering if we need to store it
> here
> > > > anymore.
> > > 
> > > An option would be to store these values locally in OptState's ctor, and
> then
> > > pass them into the buildKey function.
> > 
> > Seems a bit icky.  We calculate these types while building the opt state so
> > might be best to just leave them on the opt state.
> 
> I don't think it is that wacky. The only reason we held this values on the ODS
> was for building the header. I actually propose extending the idea and do this
> same thing for fInputColorIsUsed, fInputCoverageIsUsed, fReadsDst,
> fReadsFragPosition, and fRequiresLocalCoordAttrib. These values are
meaningless
> once we have built the header since all the info is stored there. Maybe make a
> descriptor struct for all these values that we pass into the buildKey/Build
> functions. No reason for them to take up space on the ODS.

Hmm, I guess I'm confused, I thought one of the main purposes of the optstate
was to hold on to what we need to build the descriptor.  Is that not the case?

[egdaniel, 2014-10-23 18:06:07]

On 2014/10/23 18:00:28, joshualitt wrote:
> On 2014/10/23 17:53:02, egdaniel wrote:
> >
https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState.h
> > File src/gpu/GrOptDrawState.h (right):
> > 
> >
>
https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState....
> > src/gpu/GrOptDrawState.h:444: GrProgramDesc::PrimaryOutputType 
> > fPrimaryOutputType : 8;
> > On 2014/10/23 17:45:06, joshualitt wrote:
> > > On 2014/10/23 17:15:07, egdaniel wrote:
> > > > On 2014/10/23 17:08:52, bsalomon wrote:
> > > > > Is this stuff part of the key already? Wondering if we need to store
it
> > here
> > > > > anymore.
> > > > 
> > > > An option would be to store these values locally in OptState's ctor, and
> > then
> > > > pass them into the buildKey function.
> > > 
> > > Seems a bit icky.  We calculate these types while building the opt state
so
> > > might be best to just leave them on the opt state.
> > 
> > I don't think it is that wacky. The only reason we held this values on the
ODS
> > was for building the header. I actually propose extending the idea and do
this
> > same thing for fInputColorIsUsed, fInputCoverageIsUsed, fReadsDst,
> > fReadsFragPosition, and fRequiresLocalCoordAttrib. These values are
> meaningless
> > once we have built the header since all the info is stored there. Maybe make
a
> > descriptor struct for all these values that we pass into the buildKey/Build
> > functions. No reason for them to take up space on the ODS.
> 
> Hmm, I guess I'm confused, I thought one of the main purposes of the optstate
> was to hold on to what we need to build the descriptor.  Is that not the case?

The goal of the OptState was to calculate what we need to build the descriptor.
Currently the ODS calculated these values then had to store them since we didn't
build the descriptor at the same time. However, now that we are building the
descriptor in the ODS with this change, we can just pass these calculated values
straight to it. We still want the ODS to calculate them, but there is no need to
hang onto them.

[bsalomon, 2014-10-23 18:10:30]

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

https://codereview.chromium.org/674543004/diff/120001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.h:444: GrProgramDesc::PrimaryOutputType 
fPrimaryOutputType : 8;
On 2014/10/23 17:53:02, egdaniel wrote:
> On 2014/10/23 17:45:06, joshualitt wrote:
> > On 2014/10/23 17:15:07, egdaniel wrote:
> > > On 2014/10/23 17:08:52, bsalomon wrote:
> > > > Is this stuff part of the key already? Wondering if we need to store it
> here
> > > > anymore.
> > > 
> > > An option would be to store these values locally in OptState's ctor, and
> then
> > > pass them into the buildKey function.
> > 
> > Seems a bit icky.  We calculate these types while building the opt state so
> > might be best to just leave them on the opt state.
> 
> I don't think it is that wacky. The only reason we held this values on the ODS
> was for building the header. I actually propose extending the idea and do this
> same thing for fInputColorIsUsed, fInputCoverageIsUsed, fReadsDst,
> fReadsFragPosition, and fRequiresLocalCoordAttrib. These values are
meaningless
> once we have built the header since all the info is stored there. Maybe make a
> descriptor struct for all these values that we pass into the buildKey/Build
> functions. No reason for them to take up space on the ODS.

I'm not sure about the actual mechanics but I agree that storing them in both
places is a bit wasteful and provides an avenue for inconsistency. If desc is a
member of ODS then it seems like ODS can rely on desc to hold on to this data.

https://codereview.chromium.org/674543004/diff/120001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.chromium.org/674543004/diff/120001/src/gpu/gl/GrGpuGL.h#ne...
src/gpu/gl/GrGpuGL.h:109: virtual void buildKey(const GrOptDrawState* optState,
This name seems vague. buildProgramDesc?

https://codereview.chromium.org/674543004/diff/140001/src/gpu/GrProgramDesc.h
File src/gpu/GrProgramDesc.h (right):

https://codereview.chromium.org/674543004/diff/140001/src/gpu/GrProgramDesc.h...
src/gpu/GrProgramDesc.h:21: // Dummy constructor creates a zombie descriptor
This seems like an unhelpful comment. Can you either leave uncommented or
clarify it, e.g. "Creates an uninitialized key that must be populated populated
by GrGpu::buildKey()"

https://codereview.chromium.org/674543004/diff/140001/src/gpu/GrProgramDesc.h...
src/gpu/GrProgramDesc.h:98: uint8_t                          fDstReadKey;   //
set by GrGLShaderBuilder if there
Not broken by your change, but the alignment in here is all screwy.

https://codereview.chromium.org/674543004/diff/140001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.cpp (right):

https://codereview.chromium.org/674543004/diff/140001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.cpp:348: void
GrGLProgramDesc::Finalize(GrProgramDesc* desc) {
Should this be moved to GrProgramDesc (even if it is just a helper called by
this file)? Seems like it is backend api-neutral

https://codereview.chromium.org/674543004/diff/140001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/674543004/diff/140001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:18: /** This class describes a program to generate.
It also serves as a program cache key. Very little
Update comment?

https://codereview.chromium.org/674543004/diff/140001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:20: class GrGLProgramDesc {
Maybe this needs a new name since it isn't a Desc subclass.

https://codereview.chromium.org/674543004/diff/140001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:26: // The key, stored in fKey, is composed of five
parts(first 2 are defined in the base class):
update comment?

[joshualitt, 2014-10-23 19:41:10]

feedback inc

[bsalomon, 2014-10-23 19:47:20]

https://codereview.chromium.org/674543004/diff/190001/src/gpu/GrProgramDesc.h
File src/gpu/GrProgramDesc.h (right):

https://codereview.chromium.org/674543004/diff/190001/src/gpu/GrProgramDesc.h...
src/gpu/GrProgramDesc.h:22: // Creates an uninitialized key that must be
populated populated by GrGpu::buildKey()
::buildProgramDesc now, right?

https://codereview.chromium.org/674543004/diff/190001/src/gpu/GrProgramDesc.h...
src/gpu/GrProgramDesc.h:205: // This is simply a different representation of
info in fVertexAttribs and thus does
comment seems out of place now.

https://codereview.chromium.org/674543004/diff/190001/src/gpu/GrProgramDesc.h...
src/gpu/GrProgramDesc.h:214: // These flags give aggregated info on the effect
stages that are used when building programs.
wrap

https://codereview.chromium.org/674543004/diff/190001/src/gpu/GrProgramDesc.h...
src/gpu/GrProgramDesc.h:243: // The key, stored in fKey, is composed of five
parts:
Maybe change 4 and 5 to just be "backend-specific data and backend-processor key
data"

https://codereview.chromium.org/674543004/diff/190001/src/gpu/GrProgramDesc.h...
src/gpu/GrProgramDesc.h:260: kEffectKeyOffsetsAndLengthOffset = kHeaderOffset +
kHeaderSize,
This seems wrong now to have here now, right?

[joshualitt, 2014-10-23 20:03:33]

feedback inc

[egdaniel, 2014-10-24 14:59:31]

https://codereview.chromium.org/674543004/diff/210001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/674543004/diff/210001/src/gpu/GrGpu.h#newcode12
src/gpu/GrGpu.h:12: #include "GrProgramDesc.h"
alphabetize

https://codereview.chromium.org/674543004/diff/210001/src/gpu/GrGpu.h#newcode352
src/gpu/GrGpu.h:352: virtual void buildProgramDesc(const GrOptDrawState*,
make this const GrOptDrawState&? ODS will never be NULL (func is only called
within ODS), and the function doesn't need to hold on to ODS as far as I can
tell.

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

https://codereview.chromium.org/674543004/diff/210001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:382: bool explicitLocalCoords =
this->fDesc.header().fLocalCoordAttributeIndex;
should this be != -1?

https://codereview.chromium.org/674543004/diff/210001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/674543004/diff/210001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:45: * the caps of the GrGpuGL are also inputs. It
also outputs the color and coverage stages
This comment is out dated. Specifically that parts referring to stages.

[joshualitt, 2014-10-24 15:15:38]

https://codereview.chromium.org/674543004/diff/210001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/674543004/diff/210001/src/gpu/GrGpu.h#newcode12
src/gpu/GrGpu.h:12: #include "GrProgramDesc.h"
On 2014/10/24 14:59:30, egdaniel wrote:
> alphabetize 

Acknowledged.

https://codereview.chromium.org/674543004/diff/210001/src/gpu/GrGpu.h#newcode352
src/gpu/GrGpu.h:352: virtual void buildProgramDesc(const GrOptDrawState*,
On 2014/10/24 14:59:30, egdaniel wrote:
> make this const GrOptDrawState&? ODS will never be NULL (func is only called
> within ODS), and the function doesn't need to hold on to ODS as far as I can
> tell.

GrOptDrawState.h includes GrGpu.h

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

https://codereview.chromium.org/674543004/diff/210001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.cpp:382: bool explicitLocalCoords =
this->fDesc.header().fLocalCoordAttributeIndex;
On 2014/10/24 14:59:30, egdaniel wrote:
> should this be != -1?

Acknowledged.

https://codereview.chromium.org/674543004/diff/210001/src/gpu/gl/GrGLProgramD...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/674543004/diff/210001/src/gpu/gl/GrGLProgramD...
src/gpu/gl/GrGLProgramDesc.h:45: * the caps of the GrGpuGL are also inputs. It
also outputs the color and coverage stages
On 2014/10/24 14:59:31, egdaniel wrote:
> This comment is out dated. Specifically that parts referring to stages.

Acknowledged.

[joshualitt, 2014-10-24 15:29:55]

feedback incorporated

[joshualitt, 2014-10-27 14:09:46]

On 2014/10/24 15:29:55, joshualitt wrote:
> feedback incorporated

Brian, did you have anymore feedback about this patch?

[bsalomon, 2014-10-27 14:59:43]

one minor nit, lgtm otherwise

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

https://codereview.chromium.org/674543004/diff/240001/src/gpu/GrOptDrawState....
src/gpu/GrOptDrawState.h:277: const GrProgramDesc& desc() const { return fDesc;
}
Desc seems too vague. programDesc?

[joshualitt, 2014-10-29 00:49:39]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-10-29 00:50:21]

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

[commit-bot: I haz the power, 2014-10-29 00:59:34]

Committed patchset #18 (id:320001) as 79f8faeea2692d2948c0f634e956d1e7fc8333e0