Refactor of GrGpuGLShaders

Refactoring the GrGpuGLShaders2 into 2 classes: GrGpuGLShaders
and GrGLProgram.  The change also contains stubs and placeholders for GrEffect (work in progress), which will extend shader and rendering capabilities in Ganesh.  The hash keys for the program cache table have been modified to be able to accomodate variable-length keys, which will be required for GrEffect support.

[junov, 2011-03-24 19:40:17]

PTAL

[reed1, 2011-03-24 20:16:32]

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuFactory.cpp
File gpu/src/GrGpuFactory.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuFactory.cpp#newcode45
gpu/src/GrGpuFactory.cpp:45: static const char* useEffectStr =
getenv("GR_USE_EFFECT");
Hmmm, not sure we need to introduce getenv into our cannon yet.

[twiz1, 2011-03-24 20:21:46]

A quick first pass.  Mostly style comments, as I wasn't extremely intimate with
this code in it's previous incarnation.

I see a lot of C-style casts.  Should we change them to C++ style ones?

WRT style, there's several instances of vertical whitespace after an opening
brace.  Do we want to standardize (prohibit) on this?

Jeff

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h
File gpu/src/GrBinHashKey.h (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode21
gpu/src/GrBinHashKey.h:21: #include "GrConfig.h"
GrConfig.h must be included before stdint.h.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode45
gpu/src/GrBinHashKey.h:45: *  so that the entire key can be stored in the second
pass.
80 column comment issues.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode89
gpu/src/GrBinHashKey.h:89: GrBinHashKey<Entry, StackSize>& operator = (const
GrBinHashKey<Entry, StackSize>& key) {
80 cols.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode90
gpu/src/GrBinHashKey.h:90: memcpy(this, &key, sizeof(GrBinHashKey<Entry,
StackSize>));
Use sizeof(*this), instead of the type.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode131
gpu/src/GrBinHashKey.h:131: delete []fHeapData;
I think this should be a GrFree, and not a delete[] call.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode215
gpu/src/GrBinHashKey.h:215: int                     fPass;                 
//Key generation pass counter
80 col issues.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLEffect.h
File gpu/src/GrGLEffect.h (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLEffect.h#newcode34
gpu/src/GrGLEffect.h:34: * This class is currently a stub.
A stub for what?  - Expand the comment slightly.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLEffect.h#newcode43
gpu/src/GrGLEffect.h:43: static GrGLEffect* Create(GrEffect* effect) {return
NULL;}
Spacing nits.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp#newcode25
gpu/src/GrGLProgram.cpp:25: #if GR_SUPPORT_GLES2
These are going to have to be replaced with something more dynamic.  I'm
surprised that this is compiling - have you synced recently?

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp#newcode104
gpu/src/GrGLProgram.cpp:104: for(int stage = 0; stage <
GrDrawTarget::kNumStages; stage++) {
Nit: I think pre-increment is preferred.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp#newcode115
gpu/src/GrGLProgram.cpp:115: key.keyData((uint8_t*)&fProgramDesc,
sizeof(ProgramDesc));
Change to C++ style casts.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp#newcode117
gpu/src/GrGLProgram.cpp:117: for(int stage = 0; stage <
GrDrawTarget::kNumStages; stage++) {
Pre-inc.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp#newcode139
gpu/src/GrGLProgram.cpp:139: bool GrGLProgram::doGLSetup(GrPrimitiveType type,
GrGLProgram::CachedData* programData) const {
80 column issues.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.h
File gpu/src/GrGLProgram.h (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.h#newcode23
gpu/src/GrGLProgram.h:23: #define TEX_ATTR_LOCATION(X) (1 + X)
Needs the (X).

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.h#newcode29
gpu/src/GrGLProgram.h:29: class GrBinHashKeyBuilder;
Ordering of forward declares should be alphabetical.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.h#newcode34
gpu/src/GrGLProgram.h:34: * we can specify the attribute locations so that they
are constant
information <period>  We can specify

Is that the sentence flow that you intended?

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.h#newcode48
gpu/src/GrGLProgram.h:48: void buildKey(GrBinHashKeyBuilder& key) const;
Were these routines documented previously?  Some brief docs would be helpful.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.h#newcode94
gpu/src/GrGLProgram.h:94: 
Unnecessary vertical white space.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newcode2
gpu/src/GrGpuGLShaders.cpp:2: Copyright 2010 Google Inc.
Use 2011 in new files.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:24: #include "GrBinHashKey.h"
Include ordering.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:69: int compare(const ProgramHashKey& key) const
{return fKey.compare(key);}
Spaces around '{' are required.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:72: GrGLProgram::CachedData  fProgramData;
Consistent tabs required here.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:77: // if hash bits is changed, need to change hash
function
I'm not sure I follow this comment, at least as a reader of the code.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:80: static const int MAX_ENTRIES = 32;
Style-guide recommends using anonymous enums to declare constant int values.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:88: fCurrLRUStamp = 0;
Use constructor list here.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:108: GrGLProgram::CachedData* getProgramData(const
GrGLProgram& desc, const GrDrawTarget* target) {
80 cols.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:150: GR_DEBUGCODE(memset(programData, 0,
sizeof(GrGLProgram::CachedData));)
Use the variable name for the sizeof arg, not the type.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:191: fHWGeometryState.fVertexOffset  = ~0;
Double-space here.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.h
File gpu/src/GrGpuGLShaders.h (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.h#newcode2
gpu/src/GrGpuGLShaders.h:2: Copyright 2010 Google Inc.
Since this is a new file, you should use 2011.

[bsalomon, 2011-03-24 20:24:41]

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h
File gpu/src/GrBinHashKey.h (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode101
gpu/src/GrBinHashKey.h:101: GR_DEBUGCODE(key.fIsValid = false;)
Maybe we should make this an uncopyable and have an explicit takeOwnership() or
transfer() function?

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode212
gpu/src/GrBinHashKey.h:212: mutable uint8_t*        fHeapData;             
//Dynamically allocated extended key storage
Would GrTArray work here (or made to work easily)?

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp#newcode44
gpu/src/GrGLProgram.cpp:44: 
It's my fault, not yours, but we *so* need to expand the interface of
GrStringBuilder to have a sprintf-like appender. (Not necessary for this change,
just a general comment).

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuFactory.cpp
File gpu/src/GrGpuFactory.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuFactory.cpp#newcode44
gpu/src/GrGpuFactory.cpp:44: {
The idea of reading a env var to configure Gr-behavior is handy. This might be
especially useful in Chrome to avoid the long link-time when testing.

Is there a way we could make this more generally useful for other configurable
aspects of Gr? For example, we have two options to attempt to improve rect
rendering perf. One is to use a [0,1]x[0,1] square VB with matrices, and the
other is to cpu-transform and batch them into one big drawElements.

I'd like us to formalize how we do this before we start adding yet more ways to
configure Gr. Can we live with a #if GR_USE_EFFECT for now? Actually, we should
probably call this GR_USE_NEW_GLSHADERS or something since Effect isn't enabled
but this does use your replacement for GrGpuGLShaders2.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:58: , fKey(entry.fKey) // ownership transfer
We're really inconsistent in our initializer syntax. I think this is the best
way to do it. Can you add it to the style guide on the codesite?

[reed1, 2011-03-24 20:58:30]

Hurray! Another Cache/Key set of utilities :)

Can we write a unittest in tests/ for this? Partly I ask to ensure that (a) we
can't combine this with the other cache/key utilities we already have, and (b)
to help document how to use this.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h
File gpu/src/GrBinHashKey.h (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode20
gpu/src/GrBinHashKey.h:20: #include <stdint.h>
Why GrConfig.h instead of GrTypes.h? The std pattern should be that we always
directly or indirectly include GrTypes.h (and explicitly not mention the
config.h unless we really have to, since GrTypes may want to play pre/post
#define games around it).

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode21
gpu/src/GrBinHashKey.h:21: #include "GrConfig.h"
On 2011/03/24 20:21:46, twiz1 wrote:
> GrConfig.h must be included before stdint.h.

<stdint.h> itself may be questionable on all platforms. Is that line necessary?
If so, we should fix it in GrTypes.h (as we did for skia I think).

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode209
gpu/src/GrBinHashKey.h:209: // For accumulating the variable length binary key
General alignment questions. Can we round up StackSize to a multiple of 4 or 8,
and/or declar fStackData to be intptr_t or something to keep its address nicely
aligned (and no invisible padding afterwards)? Related nit, can we move fUseHeap
to the end (next to the other bool) in case its only 1 byte.

[junov, 2011-03-25 18:39:48]

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h
File gpu/src/GrBinHashKey.h (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode101
gpu/src/GrBinHashKey.h:101: GR_DEBUGCODE(key.fIsValid = false;)
On 2011/03/24 20:24:41, bsalomon wrote:
> Maybe we should make this an uncopyable and have an explicit takeOwnership()
or
> transfer() function?
I chose implicit ownership transfer here because it is transparent to the hash
table class, which uses the assignment operator on keys.  Right now, this
approach is proving to be safe. The debug assertions on fIsValid that I peppered
all arround the place should catch any regressions related to ownership
transfer.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode212
gpu/src/GrBinHashKey.h:212: mutable uint8_t*        fHeapData;             
//Dynamically allocated extended key storage
On 2011/03/24 20:24:41, bsalomon wrote:
> Would GrTArray work here (or made to work easily)?
We could, but there is not much benefit since the array size management and
re-allocation logic in GrTArray is not useful since GrBinHashKey uses it's own
approach to optimize re-allocation (two-pass algorithm).

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp#newcode44
gpu/src/GrGLProgram.cpp:44: 
On 2011/03/24 20:24:41, bsalomon wrote:
> It's my fault, not yours, but we *so* need to expand the interface of
> GrStringBuilder to have a sprintf-like appender. (Not necessary for this
change,
> just a general comment).
Yeah, I agree, and it needs to handle floats without getting screwed by the
locale settings.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuFactory.cpp
File gpu/src/GrGpuFactory.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuFactory.cpp#newcode44
gpu/src/GrGpuFactory.cpp:44: {
Just logged it:
http://code.google.com/p/skia/issues/detail?id=192&start=100
On 2011/03/24 20:24:41, bsalomon wrote:
> The idea of reading a env var to configure Gr-behavior is handy. This might be
> especially useful in Chrome to avoid the long link-time when testing.
> 
> Is there a way we could make this more generally useful for other configurable
> aspects of Gr? For example, we have two options to attempt to improve rect
> rendering perf. One is to use a [0,1]x[0,1] square VB with matrices, and the
> other is to cpu-transform and batch them into one big drawElements.
> 
> I'd like us to formalize how we do this before we start adding yet more ways
to
> configure Gr. Can we live with a #if GR_USE_EFFECT for now? Actually, we
should
> probably call this GR_USE_NEW_GLSHADERS or something since Effect isn't
enabled
> but this does use your replacement for GrGpuGLShaders2.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:58: , fKey(entry.fKey) // ownership transfer
On 2011/03/24 20:24:41, bsalomon wrote:
> We're really inconsistent in our initializer syntax. I think this is the best
> way to do it. Can you add it to the style guide on the codesite?

Done.

http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:77: // if hash bits is changed, need to change hash
function
It is no longer relevant.  It got copied/pasted from GrGpuGLShaders2.  I am
removing it.
On 2011/03/24 20:21:46, twiz1 wrote:
> I'm not sure I follow this comment, at least as a reader of the code.

[bsalomon, 2011-03-28 20:48:54]

On 2011/03/25 18:39:48, junov wrote:
> http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode101
> gpu/src/GrBinHashKey.h:101: GR_DEBUGCODE(key.fIsValid = false;)
> On 2011/03/24 20:24:41, bsalomon wrote:
> > Maybe we should make this an uncopyable and have an explicit takeOwnership()
> or
> > transfer() function?
> I chose implicit ownership transfer here because it is transparent to the hash
> table class, which uses the assignment operator on keys.  Right now, this
> approach is proving to be safe. The debug assertions on fIsValid that I
peppered
> all arround the place should catch any regressions related to ownership
> transfer.

Hmm, it seems like having to note/remember everywhere there is an assignment
that its really a transfer outweighs the benefit of familiar = syntax. In
general I feel that overloaded operators should do the intuitive thing or they
shouldn't really overload.

> 
> http://codereview.appspot.com/4309045/diff/1/gpu/src/GrBinHashKey.h#newcode212
> gpu/src/GrBinHashKey.h:212: mutable uint8_t*        fHeapData;             
> //Dynamically allocated extended key storage
> On 2011/03/24 20:24:41, bsalomon wrote:
> > Would GrTArray work here (or made to work easily)?
> We could, but there is not much benefit since the array size management and
> re-allocation logic in GrTArray is not useful since GrBinHashKey uses it's own
> approach to optimize re-allocation (two-pass algorithm).

Make sense, no reason to use it then.

> 
> http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp
> File gpu/src/GrGLProgram.cpp (right):
> 
> http://codereview.appspot.com/4309045/diff/1/gpu/src/GrGLProgram.cpp#newcode44
> gpu/src/GrGLProgram.cpp:44: 
> On 2011/03/24 20:24:41, bsalomon wrote:
> > It's my fault, not yours, but we *so* need to expand the interface of
> > GrStringBuilder to have a sprintf-like appender. (Not necessary for this
> change,
> > just a general comment).
> Yeah, I agree, and it needs to handle floats without getting screwed by the
> locale settings.

Ouch, hadn't thought of that one. Good thought.

Re. twiz's comment about casts. Skia/Gr hasn't been using c++-style casts,
though it probably should. I don't think its necessary for Justin to fix the
copied code from Shaders2, but moving forward it's a good idea for us to use
static_cast, et al.

[junov, 2011-03-30 18:58:21]

http://codereview.appspot.com/4309045/diff/12001/gpu/src/GrGLProgram.h
File gpu/src/GrGLProgram.h (right):

http://codereview.appspot.com/4309045/diff/12001/gpu/src/GrGLProgram.h#newcod...
gpu/src/GrGLProgram.h:141: delete []fEffectUniLocationsExtended;
Oops, should be using GrFree here, will be in my next upload

http://codereview.appspot.com/4309045/diff/12001/gpu/src/GrGLProgram.h#newcod...
gpu/src/GrGLProgram.h:152: delete []fEffectUniLocationsExtended;
GrFree here

[bsalomon, 2011-03-30 20:22:08]

LGTM other than the GR_ARRAY_COUNT question.

http://codereview.appspot.com/4309045/diff/16002/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4309045/diff/16002/gpu/src/GrGLProgram.cpp#newc...
gpu/src/GrGLProgram.cpp:326: if (GR_ARRAY_COUNT(GrShaderPrecision()) > 1) {
won't this use the size of the ptr rather than the array?

[junov, 2011-03-30 20:49:43]

Is everyone else OK with this now?

http://codereview.appspot.com/4309045/diff/16002/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4309045/diff/16002/gpu/src/GrGLProgram.cpp#newc...
gpu/src/GrGLProgram.cpp:326: if (GR_ARRAY_COUNT(GrShaderPrecision()) > 1) {
Looks like this code evolved in GrGpuGLShader2 since the time I copy/pasted it. 
I will replace this with strlen. Before submitting.
On 2011/03/30 20:22:08, bsalomon wrote:
> won't this use the size of the ptr rather than the array?

[reed1, 2011-03-31 19:42:39]

LGTM