<div dir="ltr">On 2 January 2014 03:58, Tapani Pälli <span dir="ltr"><<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Signed-off-by: Tapani Pälli <<a href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>><br>
---<br>
src/mesa/main/shaderapi.c | 44 ++++++++++++++++++++++++++++++++++++++------<br>
1 file changed, 38 insertions(+), 6 deletions(-)<br>
<br>
diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c<br>
index 57511e8..c07b226 100644<br>
--- a/src/mesa/main/shaderapi.c<br>
+++ b/src/mesa/main/shaderapi.c<br>
@@ -57,6 +57,7 @@<br>
#include "../glsl/ir.h"<br>
#include "../glsl/ir_uniform.h"<br>
#include "../glsl/program.h"<br>
+#include "../glsl/shader_cache.h"<br>
<br>
/** Define this to enable shader substitution (see below) */<br>
#define SHADER_SUBST 0<br>
@@ -1632,8 +1633,26 @@ _mesa_GetProgramBinary(GLuint program, GLsizei bufSize, GLsizei *length,<br>
if (length != NULL)<br>
*length = 0;<br>
<br>
- (void) binaryFormat;<br>
- (void) binary;<br>
+ size_t size = 0;<br>
+ char *data = mesa_program_serialize(shProg, &size);<br>
+<br>
+ /* we have more data that can fit to user given buffer */<br>
+ if (size > bufSize) {<br>
+ _mesa_error(ctx, GL_INVALID_OPERATION, __FUNCTION__);<br>
+ if (data)<br>
+ free(data);<br></blockquote><div><br></div><div>Why would we ever expect mesa_program_serialize to set size to a nonzero value but return NULL? It seems like this could only happen if there's a bug, in which case this really ought to be<br>
<br></div><div>assert(data !=NULL);<br><br></div><div>Also, it's safe to call free() on a NULL pointer. According to the C standard, freeing a NULL pointer does nothing.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ return;<br>
+ }<br>
+<br>
+ if (data) {<br></blockquote><div><br></div><div>Similarly, this if-statement is unnecessary.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ memcpy(binary, data, size);<br>
+ free(data);<br>
+ }<br>
+<br>
+ if (length != NULL)<br>
+ *length = size;<br>
+<br>
+ *binaryFormat = 0;<br>
}<br>
<br>
void GLAPIENTRY<br>
@@ -1647,10 +1666,23 @@ _mesa_ProgramBinary(GLuint program, GLenum binaryFormat,<br>
if (!shProg)<br>
return;<br>
<br>
- (void) binaryFormat;<br>
- (void) binary;<br>
- (void) length;<br>
- _mesa_error(ctx, GL_INVALID_OPERATION, __FUNCTION__);<br>
+ if (length <= 0)<br>
+ return;<br></blockquote><div><br></div><div>In the case of an invalid length, we need to make sure to set the program's LinkStatus to false. Also, the information log needs to be cleared, in accordance with this text from OES_get_program_binary:<br>
<br> If ProgramBinaryOES failed, any information about a previous link or load of<br> that program object is lost. Thus, a failed load does not restore the old<br> state of <program>.<br><br></div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ /* free possible existing data and initialize structure */<br>
+ _mesa_free_shader_program_data(ctx, shProg);<br>
+ _mesa_init_shader_program(ctx, shProg);<br>
+<br>
+ /* fill structure from a binary blob */<br>
+ if (mesa_program_deserialize(shProg, binary, length)) {<br>
+ _mesa_error(ctx, GL_INVALID_VALUE, "glProgramBinary(binary incompatible)");<br></blockquote><div><br></div><div>This seems wrong to me. From the OES_get_program_binary spec:<br><br> ... <binaryFormat> and <binary> must be<br>
those returned by a previous call to GetProgramBinaryOES, and <length> must<br> be the length of the program binary as returned by GetProgramBinaryOES or<br> GetProgramiv with <pname> PROGRAM_BINARY_LENGTH_OES. The program binary<br>
will fail to load if these conditions are not met.<br><br> ...<br><br> A program object's program binary is replaced by calls to LinkProgram or<br> ProgramBinaryOES. Either command sets the program object's LINK_STATUS to<br>
TRUE or FALSE, as queried with GetProgramiv, to reflect success or failure.<br> Either command also updates its information log, queried with<br> GetProgramInfoLog, to provide details about warnings or errors.<br>
<br></div><div>I believe this means that if deserialization fails, it should not be a GL error. It should simply be treated as a link failure.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ return;<br>
+ }<br>
+<br>
+ /* driver specific link, optimizations and what not */<br>
+ ctx->Driver.LinkShader(ctx, shProg);<br></blockquote><div><br></div><div>Now I'm confused. I thought a major part of the purpose of this extension was that it would store the post-link program, so that not only does glProgramBinary() avoid the runtime penalty of parsing and compiling, it also avoids the runtime penalty of link-time optimizations. Calling ctx->Driver.LinkShader seems like it defeats that purpose. It seems like what we ought to be doing is store the data that ctx->Driver.LinkShader *produces* in the binary blob, so that once it's loaded there's no further linking necessary. If there is a small amount of driver-specific hook necessary, that should be placed in a new ctx->Driver function rather than incurring the overhead of another link.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ _mesa_ValidateProgram(program);<br></blockquote><div><br></div><div>I don't think this is correct. ValidateProgram() doesn't mean "check that the program is well-formed". It means "check whether the program can execute given the current GL state". A lot of GL apps compile all their shaders during startup, long before they've established the correct GL state for running those programs. It's reasonable to assume that apps using this extension will make their calls to glProgramBinary() during startup as well. So calling ValidateProgram() from glProgramBinary() will just put unexpected bogus warnings into the info log. Also, I don't see anything in either the OES_get_program_binary or ARB_get_program_binary spec saying we should do this.<br>
</div></div></div></div>