Step towards variable length effect keys.

This moves us towards variable length effect keys. The overall program key now allows for it. After the header it stores an array of offsets to effect keys. This allows us to grab the effect keys to pass to effects when they generate code. It also ensures that we can't get a collision by sets of keys that are different lengths but are the same when appended  together.

Committed: https://skia.googlesource.com/skia/+/848faf00ec33d39ab3e31e9a11d805cae6ac6562

[robertphillips, 2014-07-10 12:15:50]

lgtm + requests for more comments
Also do you think this is unit testable somehow? Or would that even be
worthwhile?

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
File include/gpu/GrBackendEffectFactory.h (right):

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:30: 
// The EffectKeyBuilder class helps assemble a key. It takes a pointer to the
desired destination but doesn't assume ownership of it ?

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:44: SkTArray<uint8_t, true>* fData;
Couldn't we replace fCount with (fData->count()/4) ?

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:51: 
// Return true ... ?

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:62: virtual const char* name() const = 0;
Remove this '\' ?

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrTBackendEf...
File include/gpu/GrTBackendEffectFactory.h (right):

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrTBackendEf...
include/gpu/GrTBackendEffectFactory.h:44: // effectKey must be first because it
is what will be returned by
pass -> passed ?

https://codereview.chromium.org/356513003/diff/40001/src/gpu/gl/GrGLProgramDe...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/356513003/diff/40001/src/gpu/gl/GrGLProgramDe...
src/gpu/gl/GrGLProgramDesc.h:197: /** Used to provide effects' keys to their
emitCode() function. */
// add comment here r.e. the structure of GrGLProgramDesc->fKey ?

https://codereview.chromium.org/356513003/diff/40001/src/gpu/gl/GrGLProgramDe...
src/gpu/gl/GrGLProgramDesc.h:205: EffectKeyProvider(const GrGLProgramDesc* desc,
EffectType type) : fDesc(desc) {
// The coverage effects come after the color effects ?

https://codereview.chromium.org/356513003/diff/40001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/356513003/diff/40001/tests/GLProgramsTest.cpp...
tests/GLProgramsTest.cpp:39: 
I think that somewhere we just need some ASCII art like:

| ??  | 'numStage' offsets to effect keys | color effect key 0 .. | coverage
effect key 0
      ^
      |
    kEffectKeyLengthsOffset

or maybe just a big comment

[bsalomon, 2014-07-10 17:24:35]

As for testing: We should probably be checking that different programs never
cause a key collision and that the same shader text never produces two different
keys. Maybe that's some variation of the GLPrograms test?  I don't know that our
need to test this is any different before or after this CL (i.e. I don't think
this CL makes the testing problem better or worse).

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
File include/gpu/GrBackendEffectFactory.h (right):

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:30: 
On 2014/07/10 12:15:50, robertphillips wrote:
> // The EffectKeyBuilder class helps assemble a key. It takes a pointer to the
> desired destination but doesn't assume ownership of it ?

I didn't add the bit about ownership to the top doc since it seems more targeted
at effect writers. But I did add a comment on the member var about ownership.

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:45: int fCount;
No, it is the count of uint32_ts added since this was created. fData's count
doesn't start at 0 when this object is created.

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:51: 
On 2014/07/10 12:15:50, robertphillips wrote:
> // Return true ... ?

The subclass GrTBackendEffectFactory has a comment. Added some language about
failure.

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:62: virtual const char* name() const = 0;
On 2014/07/10 12:15:50, robertphillips wrote:
> Remove this '\' ?

Done.

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrTBackendEf...
File include/gpu/GrTBackendEffectFactory.h (right):

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrTBackendEf...
include/gpu/GrTBackendEffectFactory.h:44: // effectKey must be first because it
is what will be returned by
On 2014/07/10 12:15:50, robertphillips wrote:
> pass -> passed ?

Done.

https://codereview.chromium.org/356513003/diff/40001/src/gpu/gl/GrGLProgramDe...
File src/gpu/gl/GrGLProgramDesc.h (right):

https://codereview.chromium.org/356513003/diff/40001/src/gpu/gl/GrGLProgramDe...
src/gpu/gl/GrGLProgramDesc.h:197: /** Used to provide effects' keys to their
emitCode() function. */
On 2014/07/10 12:15:50, robertphillips wrote:
> // add comment here r.e. the structure of GrGLProgramDesc->fKey ?

Updated the existing comment about the key structure.

https://codereview.chromium.org/356513003/diff/40001/src/gpu/gl/GrGLProgramDe...
src/gpu/gl/GrGLProgramDesc.h:205: EffectKeyProvider(const GrGLProgramDesc* desc,
EffectType type) : fDesc(desc) {
On 2014/07/10 12:15:50, robertphillips wrote:
> // The coverage effects come after the color effects ?

Done.

https://codereview.chromium.org/356513003/diff/40001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/356513003/diff/40001/tests/GLProgramsTest.cpp...
tests/GLProgramsTest.cpp:39: 
On 2014/07/10 12:15:50, robertphillips wrote:
> I think that somewhere we just need some ASCII art like:
> 
> | ??  | 'numStage' offsets to effect keys | color effect key 0 .. | coverage
> effect key 0
>       ^
>       |
>     kEffectKeyLengthsOffset
> 
> or maybe just a big comment

Maybe the comment in GrGLProgramDesc suffices?

[robertphillips, 2014-07-10 18:13:17]

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
File include/gpu/GrBackendEffectFactory.h (right):

https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
include/gpu/GrBackendEffectFactory.h:45: int fCount;
On 2014/07/10 17:24:34, bsalomon wrote:
> No, it is the count of uint32_ts added since this was created. fData's count
> doesn't start at 0 when this object is created.

That probably needs a comment

[bsalomon, 2014-07-11 13:44:19]

On 2014/07/10 18:13:17, robertphillips wrote:
>
https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
> File include/gpu/GrBackendEffectFactory.h (right):
> 
>
https://codereview.chromium.org/356513003/diff/40001/include/gpu/GrBackendEff...
> include/gpu/GrBackendEffectFactory.h:45: int fCount;
> On 2014/07/10 17:24:34, bsalomon wrote:
> > No, it is the count of uint32_ts added since this was created. fData's count
> > doesn't start at 0 when this object is created.
> 
> That probably needs a comment

There is a comment now

[jvanverth1, 2014-07-11 15:33:12]

LGTM + some comments.

https://codereview.chromium.org/356513003/diff/140001/include/gpu/GrBackendEf...
File include/gpu/GrBackendEffectFactory.h (right):

https://codereview.chromium.org/356513003/diff/140001/include/gpu/GrBackendEf...
include/gpu/GrBackendEffectFactory.h:32: * Used by effects to build their keys.
It incorpates each per-effect key into a larger shader key.
Typo: incorporates

https://codereview.chromium.org/356513003/diff/140001/include/gpu/GrTBackendE...
File include/gpu/GrTBackendEffectFactory.h (right):

https://codereview.chromium.org/356513003/diff/140001/include/gpu/GrTBackendE...
include/gpu/GrTBackendEffectFactory.h:41: EffectKey attribKey =
GrGLProgramEffects::GenAttribKey(drawEffect);
It took me a while to figure out what this is doing (the comment "Failure
occurs..." above helped, but only as a guide). Maybe an additional comment?

[bsalomon, 2014-07-11 16:49:18]

On 2014/07/11 15:33:12, jvanverth1 wrote:
> LGTM + some comments.
> 
>
https://codereview.chromium.org/356513003/diff/140001/include/gpu/GrBackendEf...
> File include/gpu/GrBackendEffectFactory.h (right):
> 
>
https://codereview.chromium.org/356513003/diff/140001/include/gpu/GrBackendEf...
> include/gpu/GrBackendEffectFactory.h:32: * Used by effects to build their
keys.
> It incorpates each per-effect key into a larger shader key.
> Typo: incorporates
> 
>
https://codereview.chromium.org/356513003/diff/140001/include/gpu/GrTBackendE...
> File include/gpu/GrTBackendEffectFactory.h (right):
> 
>
https://codereview.chromium.org/356513003/diff/140001/include/gpu/GrTBackendE...
> include/gpu/GrTBackendEffectFactory.h:41: EffectKey attribKey =
> GrGLProgramEffects::GenAttribKey(drawEffect);
> It took me a while to figure out what this is doing (the comment "Failure
> occurs..." above helped, but only as a guide). Maybe an additional comment?

Jim, both of your comments are incorporated into the next change in this series,
up for review now!

https://codereview.chromium.org/379113004/


Jim, I think your

[bsalomon, 2014-07-11 16:49:25]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-07-11 16:49:53]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/bsalomon@google.com/356513003/140001

[commit-bot: I haz the power, 2014-07-11 17:01:09]

Change committed as 848faf00ec33d39ab3e31e9a11d805cae6ac6562