[Mesa-dev] [wip 9/9] mesa: OES_get_program_binary extension functionality

[Paul Berry]

On 2 January 2014 03:58, Tapani Pälli <tapani.palli at intel.com> wrote:

> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>  src/mesa/main/shaderapi.c | 44
> ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 57511e8..c07b226 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -57,6 +57,7 @@
>  #include "../glsl/ir.h"
>  #include "../glsl/ir_uniform.h"
>  #include "../glsl/program.h"
> +#include "../glsl/shader_cache.h"
>
>  /** Define this to enable shader substitution (see below) */
>  #define SHADER_SUBST 0
> @@ -1632,8 +1633,26 @@ _mesa_GetProgramBinary(GLuint program, GLsizei
> bufSize, GLsizei *length,
>     if (length != NULL)
>        *length = 0;
>
> -   (void) binaryFormat;
> -   (void) binary;
> +   size_t size = 0;
> +   char *data = mesa_program_serialize(shProg, &size);
> +
> +   /* we have more data that can fit to user given buffer */
> +   if (size > bufSize) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION, __FUNCTION__);
> +      if (data)
> +         free(data);
>

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

assert(data !=NULL);

Also, it's safe to call free() on a NULL pointer.  According to the C
standard, freeing a NULL pointer does nothing.


> +      return;
> +   }
> +
> +   if (data) {
>

Similarly, this if-statement is unnecessary.


> +      memcpy(binary, data, size);
> +      free(data);
> +   }
> +
> +   if (length != NULL)
> +      *length = size;
> +
> +   *binaryFormat = 0;
>  }
>
>  void GLAPIENTRY
> @@ -1647,10 +1666,23 @@ _mesa_ProgramBinary(GLuint program, GLenum
> binaryFormat,
>     if (!shProg)
>        return;
>
> -   (void) binaryFormat;
> -   (void) binary;
> -   (void) length;
> -   _mesa_error(ctx, GL_INVALID_OPERATION, __FUNCTION__);
> +   if (length <= 0)
> +      return;
>

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:

    If ProgramBinaryOES failed, any information about a previous link or
load of
    that program object is lost.  Thus, a failed load does not restore the
old
    state of <program>.



> +
> +   /* free possible existing data and initialize structure */
> +   _mesa_free_shader_program_data(ctx, shProg);
> +   _mesa_init_shader_program(ctx, shProg);
> +
> +   /* fill structure from a binary blob */
> +   if (mesa_program_deserialize(shProg, binary, length)) {
> +      _mesa_error(ctx, GL_INVALID_VALUE, "glProgramBinary(binary
> incompatible)");
>

This seems wrong to me.  From the OES_get_program_binary spec:

    ... <binaryFormat> and <binary> must be
    those returned by a previous call to GetProgramBinaryOES, and <length>
must
    be the length of the program binary as returned by GetProgramBinaryOES
or
    GetProgramiv with <pname> PROGRAM_BINARY_LENGTH_OES.  The program binary
    will fail to load if these conditions are not met.

    ...

    A program object's program binary is replaced by calls to LinkProgram or
    ProgramBinaryOES.  Either command sets the program object's LINK_STATUS
to
    TRUE or FALSE, as queried with GetProgramiv, to reflect success or
failure.
    Either command also updates its information log, queried with
    GetProgramInfoLog, to provide details about warnings or errors.

I believe this means that if deserialization fails, it should not be a GL
error.  It should simply be treated as a link failure.


> +      return;
> +   }
> +
> +   /* driver specific link, optimizations and what not */
> +   ctx->Driver.LinkShader(ctx, shProg);
>

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.


> +
> +   _mesa_ValidateProgram(program);
>

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140115/f52a13a0/attachment-0001.html>