[Mesa-dev] [wip 8/9] glsl: functions to serialize gl_shader and gl_shader_program
Paul Berry
stereotype441 at gmail.com
Tue Jan 14 09:53:40 PST 2014
On 2 January 2014 03:58, Tapani Pälli <tapani.palli at intel.com> wrote:
> diff --git a/src/glsl/shader_cache.cpp b/src/glsl/shader_cache.cpp
> new file mode 100644
> index 0000000..4b5de45
> --- /dev/null
> +++ b/src/glsl/shader_cache.cpp
> @@ -0,0 +1,489 @@
> +/* -*- c++ -*- */
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include "main/shaderobj.h"
> +#include "main/uniforms.h"
> +#include "main/macros.h"
> +#include "program/hash_table.h"
> +#include "ir_serialize.h"
> +#include "ir_deserializer.h"
> +#include "shader_cache_magic.h"
> +
> +/**
> + * 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,
OES_get_program_binary was purposefully architected so taht we don't need
to store the unlinked shaders. GL has always maintained two independent
collections of state about any given program: pre-link state and post-link
state. There's a one-way flow of information from the pre-link state to
the post-link state--once you've linked a program using glLinkShader(),
there's no way to get the unlinked information back. This isn't a problem
for most users of GL because most people don't try to modify the pre-link
state after linking the program. However it's perfectly permissible for
someone to create a program, attach shaders, link, and then detatch all the
shaders. Since detatching the shaders affects pre-link state, the program
would still work after this point (since the post-link information would
not have changed since the link) but it would be impossible to query
information about the individual shaders anymore.
A similar situation exists when the client uses OES_get_attached_shaders.
glProgramBinaryOES() bypasses the pre-link state completely, and drops the
compiled binary directly into the post-link state. From the
OES_get_program_binary spec:
Note that ProgramBinaryOES disregards any shader objects attached to the
program object, as these shader objects are used only by LinkProgram.
And later:
7. Can BindAttribLocation be called after ProgramBinaryOES to remap an
attribute location used by the program binary?
RESOLVED: No. BindAttribLocation only affects the result of a
subsequent
call to LinkProgram. LinkProgram operates on the attached shader
objects
and replaces any program binary loaded prior to LinkProgram. So there
is no
mechanism to remap an attribute location after loading a program binary.
However, an application is free to remap an attribute location prior to
retrieving the program binary. By calling BindAttribLocation followed
by
LinkProgram, an application can remap the attribute location. If this
is
followed by a call to GetProgramBinaryOES, the retrieved program binary
will
include the desired attribute location assignment.
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 makes a change to the
spec that makes it necessary to store unlinked shaders) we can always
change the code in a future patch.
Also, incidentally, the way you've used STORE_UNLINKED_SHADERS wouldn't
have worked anyway. You've defined it as a const bool (which is evaluated
by the compiler) but later when you try to conditionally compile based on
it, you use the preprocessor. Since the preprocessor operates on the code
before the rest of the compiler, it doesn't know about the const
declaration above--even if you changed it to true, #ifdef
STORE_UNLINKED_SHADERS would still evaluate to false.
> +
> +
> +static void
> +write_header(gl_shader *shader, memory_writer &blob)
> +{
> + GET_CURRENT_CONTEXT(ctx);
> +
> + blob.write_string(mesa_get_shader_cache_magic());
> + blob.write_string((const char *)ctx->Driver.GetString(ctx, GL_VENDOR));
> + blob.write_string((const char *)ctx->Driver.GetString(ctx,
> GL_RENDERER));
> + blob.write_uint32_t(shader->Version);
> + blob.write_uint32_t(shader->Type);
> + blob.write_uint8_t(shader->IsES);
> +
> + /* post-link data */
> + blob.write_uint32_t(shader->num_samplers);
> + blob.write_uint32_t(shader->active_samplers);
> + blob.write_uint32_t(shader->shadow_samplers);
> + blob.write_uint32_t(shader->num_uniform_components);
> + blob.write_uint32_t(shader->num_combined_uniform_components);
> +
> + uint8_t uses_builtins = shader->uses_builtin_functions;
> + blob.write_uint8_t(uses_builtins);
> +
> + 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.
> +
> + for (unsigned i = 0; i < MAX_SAMPLERS; i++)
> + blob.write_uint8_t(shader->SamplerUnits[i]);
> +
> + for (unsigned i = 0; i < MAX_SAMPLERS; i++) {
> + int32_t target = shader->SamplerTargets[i];
> + blob.write_int32_t(target);
> + }
> +}
> +
> +
> +
> +
> +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.
> +
> +
> +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.
> +
> + char *key = map->read_string();
> + uint32_t value = map->read_uint32_t();
> +
> + /* put() adds +1 bias on the value (see hash_table.h), this
> + * is taken care here when reading
> + */
> + hash->put(value-1, key);
> + }
> +}
> +
> +
> +static void
> +read_uniform_storage(void *mem_ctx, gl_uniform_storage *uni, memory_map
> &map)
>
As with serialize_uniform_storage(), I believe this is not necessary.
> +
> +
> +/**
> + * Deserialize gl_shader_program structure
> + */
> +extern "C" int
> +mesa_program_deserialize(struct gl_shader_program *prog,
> + const GLvoid *blob, size_t size)
> +{
> + memory_map map;
> + map.map((const void*)blob, size);
> +
> + prog->Type = map.read_uint32_t();
> + prog->NumShaders = map.read_uint32_t();
> + prog->LinkStatus = map.read_uint8_t();
> + prog->Version = map.read_uint32_t();
> + prog->IsES = map.read_uint8_t();
> + prog->NumUserUniformStorage = map.read_uint32_t();
> + prog->UniformLocationBaseScale = map.read_uint32_t();
> + prog->LastClipDistanceArraySize = map.read_uint32_t();
> + prog->FragDepthLayout = (gl_frag_depth_layout) map.read_uint8_t();
> +
> + prog->UniformStorage = NULL;
> + prog->Label = NULL;
> +
> + /* these already allocated by _mesa_init_shader_program */
> + read_hash_table(prog->AttributeBindings, &map);
> + read_hash_table(prog->FragDataBindings, &map);
> + read_hash_table(prog->FragDataIndexBindings, &map);
> +
> + prog->UniformHash = new string_to_uint_map;
> + 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.
> +
> + /* uniform storage */
> + prog->UniformStorage = rzalloc_array(prog, struct gl_uniform_storage,
> + prog->NumUserUniformStorage);
> +
> + for (unsigned i = 0; i < prog->NumUserUniformStorage; i++)
> + read_uniform_storage(prog, &prog->UniformStorage[i], map);
> +
> + GET_CURRENT_CONTEXT(ctx);
> +
> +#if (STORE_UNLINKED_SHADERS)
> + /* Shaders array (unlinked */
> + prog->Shaders = (struct gl_shader **)
> + _mesa_realloc(prog->Shaders, 0,
> + (prog->NumShaders) * sizeof(struct gl_shader *));
> +
> + for (unsigned i = 0; i < prog->NumShaders; i++) {
> +
> + struct gl_shader *sha = read_shader(prog, map);
> +
> + if (sha) {
> + prog->Shaders[i] = NULL; /* alloc did not initialize */
> + _mesa_reference_shader(ctx, &prog->Shaders[i], sha);
> + CACHE_DEBUG("%s: read unlinked shader, index %d (%p) size %d\n",
> + __func__, i, sha, shader_size);
> + }
> + }
> +#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.
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.
> +#endif
> +
> + /* init list, cache can contain only some shader types */
> + for (unsigned i = 0; i < MESA_SHADER_TYPES; i++)
> + prog->_LinkedShaders[i] = NULL;
> +
> + /* read _LinkedShaders */
> + while(map.position() < size) {
> + uint32_t index = map.read_uint32_t();
> +
> + struct gl_shader *sha = read_shader(prog, map);
> +
> + /* note, for 'automatic cache' in case of failure we would
> + * need to fallback to compiling/linking the shaders in the
> + * prog->Shaders list
> + */
> + if (!sha) {
> + CACHE_DEBUG("failed to read shader (index %d)\n", index);
> + return -1;
> + }
> +
> + resolve_uniform_types(prog, sha);
> +
> + _mesa_reference_shader(ctx, &prog->_LinkedShaders[index], sha);
> + CACHE_DEBUG("%s: read a linked shader, index %d (%p)\n",
> + __func__, index, sha);
> + }
> +
> + return 0;
> +}
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140114/ee087e52/attachment-0001.html>
More information about the mesa-dev
mailing list