Refactor Program into Program and ProgramBinary.

Refactor Program into Program and ProgramBinary.

Program manages the state and lifetime of the program object.

ProgramBinary holds the linked program and the code to do the linking.

There should be no functional change. WebGL conformance tests did not regress.
Committed: https://code.google.com/p/angleproject/source/detail?r=1143

[apatrick1, 2012-06-04 22:22:15]

Hi Daniel,

I started looking into implementing OES_get_program_binary. As part of that, I
split Program into two classes, mostly to get an understanding of what is an
input to the linking process, rather than an output.

This doesn't have to go in. Having figured out how it all works, I can just as
easily implement the extension on Program as is. One advantage of checking this
in is it will prevent glProgramBinary from accessing any information that it
shouldn't from the attached shaders, which the spec says should be ignored.

Up to you :)

The plan is for Chrome to cache the binaries and automatically attempt to use
them when possible. OES_get_program_binary won't be exposed client side. We'll
just intercept and replace glLinkProgram calls where possible.

Al

[dgkoch, 2012-06-06 16:16:27]

Hi Al,

This does seem like a reasonable change.  However I would like to see it done a
bit more incrementally.  With the current patch set it is a bit hard to
distinguish between something that moved and something that changed (especially
since the ProgramBinary files seems to have started off as a clone of the
Program files somehow).  I'd recommend the following approach:

1) Make the functional changes first (i.e. adding AttributeBindings class,
changing link(), etc).
2) Factor into 2 classes in-place (so that you'll have Program and ProgramBinary
classes in the same file initially.)
3) Move the ProgramBinary classes to separate files.

Also note the we just made some fixes to Program.cpp yesterday, so this will
need to be regenerated either way.

https://codereview.appspot.com/6267047/diff/1/src/libGLESv2/libGLESv2.cpp
File src/libGLESv2/libGLESv2.cpp (right):

https://codereview.appspot.com/6267047/diff/1/src/libGLESv2/libGLESv2.cpp#new...
src/libGLESv2/libGLESv2.cpp:2853: return
programObject->getProgramBinary()->getAttributeLocation(name);
I'd prefer if these could be rewritten so that we didn't need to call
getProgramBinary() twice.  (Same for many other locations in this file)

[apatrick1, 2012-06-06 23:08:27]

Daniel,

Step 2 of 3. Either this patch or the OES_get_program_binary patch will need to
be updated, depending on which one lands first.

[apatrick1, 2012-06-06 23:13:45]

Oh and I ran the WebGL conformance tests again with this in Chrome and there was
no regression still.

[dgkoch, 2012-06-07 02:00:37]

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:4228: element->UsageIndex =
program->getProgramBinary()->getSemanticIndex(i);
Maybe we could retrieve program->GetProgramBinary() once at the start of the
function, instead of looking it up repeatedly here?

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp
File src/libGLESv2/Program.cpp (right):

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2517: *length = 0;
length == NULL not handled.

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2518: infoLog[0] = '\0';
bufsize == 0 not handled.

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2598: name[0] = '\0';
bufsize == 0 isn't handled here

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2599: *length = 0;
length == NULL case is not handled here.

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2719: name[0] = '\0';
bufsize == 0 case isn't handled here.

[apatrick1, 2012-06-07 19:04:51]

Fixed. Thanks.

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:4228: element->UsageIndex =
program->getProgramBinary()->getSemanticIndex(i);
On 2012/06/07 02:00:37, dgkoch wrote:
> Maybe we could retrieve program->GetProgramBinary() once at the start of the
> function, instead of looking it up repeatedly here?

This is the only invocation of getProgramBinary in
VertexDeclarationCache::applyDeclaration. The one above is over a thousand lines
away :)

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp
File src/libGLESv2/Program.cpp (right):

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2517: *length = 0;
On 2012/06/07 02:00:37, dgkoch wrote:
> length == NULL not handled.

Done.

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2518: infoLog[0] = '\0';
On 2012/06/07 02:00:37, dgkoch wrote:
> bufsize == 0 not handled.

Done.

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2598: name[0] = '\0';
On 2012/06/07 02:00:37, dgkoch wrote:
> bufsize == 0 isn't handled here

Done.

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2599: *length = 0;
On 2012/06/07 02:00:37, dgkoch wrote:
> length == NULL case is not handled here.

Done.

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Program.cpp#ne...
src/libGLESv2/Program.cpp:2719: name[0] = '\0';
On 2012/06/07 02:00:37, dgkoch wrote:
> bufsize == 0 case isn't handled here.

Done.

[dgkoch, 2012-06-07 20:52:12]

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

https://codereview.appspot.com/6267047/diff/3002/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:4228: element->UsageIndex =
program->getProgramBinary()->getSemanticIndex(i);
On 2012/06/07 19:04:51, apatrick1 wrote:
> On 2012/06/07 02:00:37, dgkoch wrote:
> > Maybe we could retrieve program->GetProgramBinary() once at the start of the
> > function, instead of looking it up repeatedly here?
> 
> This is the only invocation of getProgramBinary in
> VertexDeclarationCache::applyDeclaration. The one above is over a thousand
lines
> away :)

Yeah, but it's in the middle of a loop which could be executed 16 times.

[dgkoch, 2012-06-07 21:03:23]

LGTM if you fix the one item mentioned

https://codereview.appspot.com/6267047/diff/7005/src/libGLESv2/Context.cpp
File src/libGLESv2/Context.cpp (right):

https://codereview.appspot.com/6267047/diff/7005/src/libGLESv2/Context.cpp#ne...
src/libGLESv2/Context.cpp:4228: element->UsageIndex =
program->getProgramBinary()->getSemanticIndex(i);
This getProgramBinary call could stand to be factored out of the loop.