Support for serializing a linked program to binary.

Support for serializing a linked program to binary.

The format has a text section followed by a binary section. The binary section contains an image of the device caps and the two shader executables. The text section has everything else newline deliminated.

Ran WebGL conformance tests with temporary change to glLinkProgram that round trips all linked programs through glGetProgramBinary and glProgramBinary. No regressions.
Committed: https://code.google.com/p/angleproject/source/detail?r=1199

[apatrick1, 2012-06-15 22:35:57]

Daniel,

Please review. Thanks.

Al

[dgkoch, 2012-06-19 21:10:15]

Do you really want to have text-based stuff at the beginning? It seems like it
might be more compact if it was all written out in binary.

I noticed the ProgramBinary class still has an issueSerial() method defined, but
not implemented anywhere. Presumably this is left over from the refactoring and
should be removed.

What are Uniform values set to when a binary is loaded?
OES_get_program_binary doesn't seem to state this, but ARB_get_program_binary
says:  
"A successful call to ProgramBinary will reset all uniform variables to their
initial values. The initial value is either the value of the variable's
initializer as specified in the original shader source, or 0 if no initializer
was present."

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary.cpp
File src/libGLESv2/ProgramBinary.cpp (right):

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary....
src/libGLESv2/ProgramBinary.cpp:1596: bool ProgramBinary::load(const void
*binary, GLsizei length)
For any case that this fails we need to
 - add a reason to the info log.
We also need to update the Program object's link status to TRUE (if loaded
successfully) or FALSE (if loading fails).  Perhaps we should call unlink(false)
if the programbinary->load() fails?
These may mean a bit of refactoring between the way programs and program
binaries work (currently the info log is in the program binary, so if we delete
the program binary, the info log is gone…).

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary....
src/libGLESv2/ProgramBinary.cpp:1815: D3DCAPS9 caps =
getDisplay()->getDeviceCaps();
I was actually thinking of the D3DADAPTER_IDENTIFIER9 structure since this
includes the driver version and description.  Although given that we use both
the adapter ID and the caps in various spots, I suppose it would be safest to
just include both (that's a lot of space though!).  Thoughts?  Perhaps we could
just reduce it to *only* the driver description and version?

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary....
src/libGLESv2/ProgramBinary.cpp:1820: return false;
I believe 0 needs to be written to *length in this case.

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary....
src/libGLESv2/ProgramBinary.cpp:1839: ASSERT(ptr - totalLength == binary);
Perhaps we should also fail saving in this case?

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/libGLESv2.cpp
File src/libGLESv2/libGLESv2.cpp (right):

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/libGLESv2.cpp#...
src/libGLESv2/libGLESv2.cpp:4625: // FIXME: This is a hack for testing. Will not
check in.
Don't forget to remove this!

[apatrick1, 2012-06-19 21:27:24]

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary.cpp
File src/libGLESv2/ProgramBinary.cpp (right):

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary....
src/libGLESv2/ProgramBinary.cpp:1596: bool ProgramBinary::load(const void
*binary, GLsizei length)
On 2012/06/19 21:10:15, dgkoch wrote:
> For any case that this fails we need to
>  - add a reason to the info log.

Will do.

> We also need to update the Program object's link status to TRUE (if loaded
> successfully) or FALSE (if loading fails).  Perhaps we should call
unlink(false)
> if the programbinary->load() fails?

I changed link status a bit. The program is linked if it has a program binary.
If the program binary is null then it is unlinked. I thought glProgramBinary set
it to null on failure but apparently not. I'll fix that.

> These may mean a bit of refactoring between the way programs and program
> binaries work (currently the info log is in the program binary, so if we
delete
> the program binary, the info log is gone…).

Right.

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary....
src/libGLESv2/ProgramBinary.cpp:1815: D3DCAPS9 caps =
getDisplay()->getDeviceCaps();
On 2012/06/19 21:10:15, dgkoch wrote:
> I was actually thinking of the D3DADAPTER_IDENTIFIER9 structure since this
> includes the driver version and description.  Although given that we use both
> the adapter ID and the caps in various spots, I suppose it would be safest to
> just include both (that's a lot of space though!).  Thoughts?  Perhaps we
could
> just reduce it to *only* the driver description and version?

Or how about just the DeviceIdentifier guid?

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary....
src/libGLESv2/ProgramBinary.cpp:1820: return false;
On 2012/06/19 21:10:15, dgkoch wrote:
> I believe 0 needs to be written to *length in this case.
Yep.

https://codereview.appspot.com/6295092/diff/6001/src/libGLESv2/ProgramBinary....
src/libGLESv2/ProgramBinary.cpp:1839: ASSERT(ptr - totalLength == binary);
On 2012/06/19 21:10:15, dgkoch wrote:
> Perhaps we should also fail saving in this case?

I think this asserting means a logic error rather than a runtime error.

[apatrick1, 2012-06-19 21:34:25]

On 2012/06/19 21:10:15, dgkoch wrote:
> Do you really want to have text-based stuff at the beginning? It seems like it
> might be more compact if it was all written out in binary.

It made debugging easier. My though was that I could (mostly) just replace all
the << and >> operators with calls to some binary read or write function if that
seemed important. I don't mind either way, although binary would probably make
GL_PROGRAM_BINARY_LENGTH_OES less awkward.

> 
> I noticed the ProgramBinary class still has an issueSerial() method defined,
but
> not implemented anywhere. Presumably this is left over from the refactoring
and
> should be removed.

Yeah that's left over. Will remove.

> 
> What are Uniform values set to when a binary is loaded?
> OES_get_program_binary doesn't seem to state this, but ARB_get_program_binary
> says:  
> "A successful call to ProgramBinary will reset all uniform variables to their
> initial values. The initial value is either the value of the variable's
> initializer as specified in the original shader source, or 0 if no initializer
> was present."

If it's undefined, I can safely copy ARB_get_program_binary I think.

[apatrick1, 2012-06-19 22:02:06]

> > What are Uniform values set to when a binary is loaded?
> > OES_get_program_binary doesn't seem to state this, but
ARB_get_program_binary
> > says:  
> > "A successful call to ProgramBinary will reset all uniform variables to
their
> > initial values. The initial value is either the value of the variable's
> > initializer as specified in the original shader source, or 0 if no
initializer
> > was present."
> 
> If it's undefined, I can safely copy ARB_get_program_binary I think.

Actually, while GLSL supports default values for uniforms, I think GLSL ES does
not. It's error S0052. Is that maybe why ARB_get_program_binary addresses the
issue while OES_get_program_binary does not?

http://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf

So the initial value should be zero I think. Uniform's constructor already does
that. The only other thing that makes sense to put in them is whatever state was
set with glUniform, but the spec doesn't say to do that as far as I can see.

[dgkoch, 2012-06-20 14:30:26]

On 2012/06/19 21:34:25, apatrick1 wrote:
> On 2012/06/19 21:10:15, dgkoch wrote:
> > Do you really want to have text-based stuff at the beginning? It seems like
it
> > might be more compact if it was all written out in binary.
> 
> It made debugging easier. My though was that I could (mostly) just replace all
> the << and >> operators with calls to some binary read or write function if
that
> seemed important. I don't mind either way, although binary would probably make
> GL_PROGRAM_BINARY_LENGTH_OES less awkward.

I agree it makes for easier debugging.  It doesn't necessarily have to be
changed for the first version, but it might be a good thing to do eventually.

[dgkoch, 2012-06-20 14:31:19]

On 2012/06/19 22:02:06, apatrick1 wrote:
> > > What are Uniform values set to when a binary is loaded?
> > > OES_get_program_binary doesn't seem to state this, but
> ARB_get_program_binary
> > > says:  
> > > "A successful call to ProgramBinary will reset all uniform variables to
> their
> > > initial values. The initial value is either the value of the variable's
> > > initializer as specified in the original shader source, or 0 if no
> initializer
> > > was present."
> > 
> > If it's undefined, I can safely copy ARB_get_program_binary I think.
> 
> Actually, while GLSL supports default values for uniforms, I think GLSL ES
does
> not. It's error S0052. Is that maybe why ARB_get_program_binary addresses the
> issue while OES_get_program_binary does not?
> 
>
http://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf
> 
> So the initial value should be zero I think. Uniform's constructor already
does
> that. The only other thing that makes sense to put in them is whatever state
was
> set with glUniform, but the spec doesn't say to do that as far as I can see.

Right, ES doesn't have initializers.  That'll make it simpler :-) 
Agree that we should initialize them to zero.

[apatrick1, 2012-06-20 17:59:02]

How about the issue of identifying the device and driver and using
D3DADAPTER_IDENTIFIER9::DeviceIdentifier for that purpose as opposed to other
methods?

Docs say:

"DeviceIdentifier
Type: GUID

Can be queried to check changes in the driver and chip set. This GUID is a
unique identifier for the driver and chip set pair. Query this member to track
changes to the driver and chip set in order to generate a new profile for the
graphics subsystem. DeviceIdentifier can also be used to identify particular
problematic drivers."


I think the text formatted section should go to binary at some point. If we ever
want to put floats in there for some reason, there will be binary -> decimal ->
binary precision issues. Also strings can't contain whitespace at the moment.
The purpose of quoting them is only so that empty strings are not enveloped by
the newline delimiters surrounding them. That's not obvious IMHO.

[dgkoch, 2012-06-20 18:23:00]

On 2012/06/20 17:59:02, apatrick1 wrote:
> How about the issue of identifying the device and driver and using
> D3DADAPTER_IDENTIFIER9::DeviceIdentifier for that purpose as opposed to other
> methods?
> 
> Docs say:
> 
> "DeviceIdentifier
> Type: GUID
> 
> Can be queried to check changes in the driver and chip set. This GUID is a
> unique identifier for the driver and chip set pair. Query this member to track
> changes to the driver and chip set in order to generate a new profile for the
> graphics subsystem. DeviceIdentifier can also be used to identify particular
> problematic drivers."

Yes that sounds perfect for what we want.

> I think the text formatted section should go to binary at some point. 
Agreed.
> If we ever
> want to put floats in there for some reason, there will be binary -> decimal
->
> binary precision issues. 
Definitely wouldn't want floats written out in ascii :-)
> Also strings can't contain whitespace at the moment.
> The purpose of quoting them is only so that empty strings are not enveloped by
> the newline delimiters surrounding them. That's not obvious IMHO.
Also agree it's non-obvious.  Took me a bit to figure out the quoting, and why
you were sub stringing :-)
Perhaps we should make it binary now, or at least comment what is going on.

[apatrick1, 2012-06-28 21:42:55]

Support for serializing a linked program to binary.

The format has a text section followed by a binary section. The binary section
contains an image of the device caps and the two shader executables. The text
section has everything else newline deliminated.

Ran WebGL conformance tests with temporary change to glLinkProgram that round
trips all linked programs through glGetProgramBinary and glProgramBinary. No
regressions.

[apatrick1, 2012-06-28 21:44:52]

I addressed the issues and also added getProgramBinaryLength to Program. WebGL
conformance tests did not regress. It's still text. I can switch it to binary in
a future patch.

[dgkoch, 2012-07-05 03:26:37]

LGTM

https://codereview.appspot.com/6295092/diff/15001/src/libGLESv2/libGLESv2.cpp
File src/libGLESv2/libGLESv2.cpp (right):

https://codereview.appspot.com/6295092/diff/15001/src/libGLESv2/libGLESv2.cpp...
src/libGLESv2/libGLESv2.cpp:4623: // FIXME: This is a hack for testing. Will not
check in.
Don't forget to delete this :-)

https://codereview.appspot.com/6295092/diff/15001/src/libGLESv2/libGLESv2.cpp...
src/libGLESv2/libGLESv2.cpp:6947: {"glProgramBinaryOES",
(__eglMustCastToProperFunctionPointerType)glProgramBinaryOES},    };
I think we forgot to add these to the .def file as well.

[apatrick1, 2012-07-09 22:14:31]

Support for serializing a linked program to binary.

The format has a text section followed by a binary section. The binary section
contains an image of the device caps and the two shader executables. The text
section has everything else newline deliminated.

Ran WebGL conformance tests with temporary change to glLinkProgram that round
trips all linked programs through glGetProgramBinary and glProgramBinary. No
regressions.