On 01/14/2014 07:53 PM, Paul Berry wrote:
On 2 January 2014 03:58, Tapani Pälli <tapani.pa...@intel.com
<mailto:tapani.pa...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
