<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 01/14/2014 07:53 PM, Paul Berry
wrote:<br>
</div>
<blockquote
cite="mid:CA+yLL66AosweoQWHL_rwoibHfWbGuD6Un5q9Gb4x4UNj88gCpQ@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div dir="ltr">On 2 January 2014 03:58, Tapani Pälli <span
dir="ltr"><<a moz-do-not-send="true"
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">diff --git
a/src/glsl/shader_cache.cpp b/src/glsl/shader_cache.cpp<br>
new file mode 100644<br>
+/**<br>
+ * It is currently unknown if we need these to be
available.<br>
+ * There can be calls in mesa/main (like
glGetAttachedShaders) that use<br>
+ * the Shaders list.<br>
+ */<br>
+const bool STORE_UNLINKED_SHADERS = false;<br>
</blockquote>
<div><br>
</div>
<div>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, </div>
</div>
</div>
</div>
</blockquote>
<br>
8<<br>
<br>
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.<br>
<br>
<blockquote
cite="mid:CA+yLL66AosweoQWHL_rwoibHfWbGuD6Un5q9Gb4x4UNj88gCpQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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.<br>
<br>
</div>
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 </div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL66AosweoQWHL_rwoibHfWbGuD6Un5q9Gb4x4UNj88gCpQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<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">
+ blob.write(&shader->Geom,
sizeof(shader->Geom));<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I will add it, the plan is to get both desktop GL and the OES
extension supported.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL66AosweoQWHL_rwoibHfWbGuD6Un5q9Gb4x4UNj88gCpQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<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">
+static void<br>
+serialize_uniform_storage(gl_uniform_storage *uni,
memory_writer &blob)<br>
</blockquote>
<div><br>
</div>
<div>I don't think this is right. The
ARB_get_program_binary spec says:<br>
<br>
8. How does ProgramBinary interact with uniform
values, including<br>
shader-specified defaults?<br>
<br>
RESOLVED: All uniforms specified in the binary are
reset to their shader-<br>
specified defaults, or to zero if they aren't
specified, when the program<br>
binary is loaded. The spec language has been updated
to specify this<br>
behavior.<br>
<br>
</div>
<div>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:<br>
<br>
7. How does this spec differ from the
OES_get_program_binary and why?<br>
<br>
...<br>
<br>
There are other areas where language was clarified,
but this is meant to<br>
be consistent with the intent of the original
specification and not to<br>
alter previously established behavior.<br>
<br>
</div>
<div>So I believe we shouldn't be serializing uniform
values.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
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?<br>
<br>
<blockquote
cite="mid:CA+yLL66AosweoQWHL_rwoibHfWbGuD6Un5q9Gb4x4UNj88gCpQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<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">
+<br>
+<br>
+static void<br>
+read_hash_table(struct string_to_uint_map *hash,
memory_map *map)<br>
+{<br>
+ uint32_t size = map->read_uint32_t();<br>
+<br>
+ for (unsigned i = 0; i < size; i++) {<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
ok, will fix<br>
<br>
<blockquote
cite="mid:CA+yLL66AosweoQWHL_rwoibHfWbGuD6Un5q9Gb4x4UNj88gCpQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<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">
+ read_hash_table(prog->UniformHash, &map);<br>
+<br>
+ map.read(&prog->Geom, sizeof(prog->Geom));<br>
+ map.read(&prog->Vert, sizeof(prog->Vert));<br>
+<br>
+ /* just zero for now */<br>
+ prog->LinkedTransformFeedback.Outputs = NULL;<br>
+ prog->LinkedTransformFeedback.Varyings = NULL;<br>
+ prog->LinkedTransformFeedback.NumVarying = 0;<br>
+ prog->LinkedTransformFeedback.NumOutputs = 0;<br>
</blockquote>
<div><br>
</div>
<div>Did you forget to finish writing this code? We need to
serialize this stuff, since it's part of post-link state.<br>
</div>
<div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This part is work in progress, I need to get myself more familiar
how transform feedback API works and what is required.<br>
<br>
<blockquote
cite="mid:CA+yLL66AosweoQWHL_rwoibHfWbGuD6Un5q9Gb4x4UNj88gCpQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<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">
+#else<br>
+ prog->Shaders = NULL;<br>
+ prog->NumShaders = 0;<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:CA+yLL66AosweoQWHL_rwoibHfWbGuD6Un5q9Gb4x4UNj88gCpQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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:<br>
<br>
</div>
<div>struct gl_shader_program {<br>
</div>
<div> struct {<br>
...<br>
</div>
<div> } PreLink;<br>
</div>
<div> struct {<br>
...<br>
</div>
<div> } PostLink;<br>
};<br>
<br>
</div>
<div>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. <br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This is fine for me. I will make patches for it, I guess this could
be done separately before rest of the set.<br>
<br>
// Tapani<br>
<br>
</body>
</html>