[Mesa-dev] [wip 8/9] glsl: functions to serialize gl_shader and gl_shader_program

[Tapani]

On 01/14/2014 07:53 PM, Paul Berry wrote:
> On 2 January 2014 03:58, Tapani Pälli <tapani.palli at intel.com 
> <mailto:tapani.palli at intel.com>> wrote:
>
>     diff --git a/src/glsl/shader_cache.cpp b/src/glsl/shader_cache.cpp
>     new file mode 100644
>     +/**
>     + * It is currently unknown if we need these to be available.
>     + * There can be calls in mesa/main (like glGetAttachedShaders)
>     that use
>     + * the Shaders list.
>     + */
>     +const bool STORE_UNLINKED_SHADERS = false;
>
>
> I think it really exaggerates our level of uncertainty to say that it 
> is "currently unknown" whether we need unlinked shaders to be 
> available.  Speaking for myself at least, I'm quite convinced that we 
> don't.  AFAICT,

8<

Thanks for the explanation. I've been holding this until now as I was 
not sure if these can be left out but now I cam convinced. I will cut 
them out.

> So if the user creates a program and calls glProgramBinaryOES() 
> followed by glGetAttachedShaders(), they will see no shaders, since 
> the pre-link state is still in its initial configuration of having no 
> shaders attached.
>
> IMHO, trying to plan for the contingency case where we're wrong about 
> this is just going to lead to confusion and make the code hard to 
> maintain.  I think we should drop this const, and remove the code that 
> would have been executed if STORE_UNLINKED_SHADERS were true.  In the 
> unlikely event that it turns out we were wrong about this (or Khronos

I'll do this, I was basically worried if all the errors cases will be 
handled properly as there will be a lot of uninitialized pointers and 
such out there. But these can be found out and the correct thing to do 
is to make sure everything's checked in the shaderapi correctly.


>     +   blob.write(&shader->Geom, sizeof(shader->Geom));
>
>
> It seems a little strange to see geometry-shader-specific information 
> being dumped into the binary, since OES_get_program_binary is a GLES 
> extension, and GLES doesn't support geometry shaders.  I assume you 
> are doing this because you also want this code to work for 
> ARB_get_program_binary?  If so it would be nice to have a comment 
> explaining that.

I will add it, the plan is to get both desktop GL and the OES extension 
supported.


>     +static void
>     +serialize_uniform_storage(gl_uniform_storage *uni, memory_writer
>     &blob)
>
>
> I don't think this is right.  The ARB_get_program_binary spec says:
>
>     8. How does ProgramBinary interact with uniform values, including
>        shader-specified defaults?
>
>     RESOLVED: All uniforms specified in the binary are reset to their 
> shader-
>     specified defaults, or to zero if they aren't specified, when the 
> program
>     binary is loaded. The spec language has been updated to specify this
>     behavior.
>
> The OES_get_program_binary spec doesn't mention uniforms at all, but I 
> believe this is not intended to indicate a behavioural 
> difference--it's just a consequence of the fact that 
> ARB_get_program_binary was written later, so they had a chance to 
> clarify things.  In fact, issue #7 in ARB_get_program_binary 
> specifically says:
>
>     7. How does this spec differ from the OES_get_program_binary and why?
>
>     ...
>
>     There are other areas where language was clarified, but this is 
> meant to
>     be consistent with the intent of the original specification and not to
>     alter previously established behavior.
>
> So I believe we shouldn't be serializing uniform values.

For me this seemed much easier way to serialize than recreate it though. 
Would it be enough if I reset the default values in place?

>     +
>     +
>     +static void
>     +read_hash_table(struct string_to_uint_map *hash, memory_map *map)
>     +{
>     +   uint32_t size = map->read_uint32_t();
>     +
>     +   for (unsigned i = 0; i < size; i++) {
>
>
> Security issue: we need to bail out of this loop if we try to read 
> beyond the end of the binary.  Otherwise a bogus size could cause us 
> to read from garbage memory for a very long time.

ok, will fix

>     +   read_hash_table(prog->UniformHash, &map);
>     +
>     +   map.read(&prog->Geom, sizeof(prog->Geom));
>     +   map.read(&prog->Vert, sizeof(prog->Vert));
>     +
>     +   /* just zero for now */
>     +   prog->LinkedTransformFeedback.Outputs = NULL;
>     +   prog->LinkedTransformFeedback.Varyings = NULL;
>     +   prog->LinkedTransformFeedback.NumVarying = 0;
>     +   prog->LinkedTransformFeedback.NumOutputs = 0;
>
>
> Did you forget to finish writing this code?  We need to serialize this 
> stuff, since it's part of post-link state.

This part is work in progress, I need to get myself more familiar how 
transform feedback API works and what is required.

>     +#else
>     +   prog->Shaders = NULL;
>     +   prog->NumShaders = 0;
>
>
> This is not correct.  Shader objects are part of pre-link state.  That 
> means that glProgramBinaryOES() should ignore them and leave them 
> unchanged.  It shouldn't throw out any attached shaders.
>

You are right, this is done here only because these are not initialized 
properly and when deleting resources we would fail. Correct way is to 
intialize these when constructing the program structure.

> Incidentally, it occurs to me that maybe we should do a refactor at 
> the start of this series that splits gl_shader_program into two parts:
>
> struct gl_shader_program {
>   struct {
>     ...
>   } PreLink;
>   struct {
>     ...
>   } PostLink;
> };
>
> That way future maintainers won't have to rely on deep understanding 
> of the GL spec to keep track of which program state is pre-link and 
> which is post-link.  They can just look in mtypes.h.

This is fine for me. I will make patches for it, I guess this could be 
done separately before rest of the set.

// Tapani

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140115/662230e2/attachment-0001.html>