[Mesa-dev] [PATCH] glsl: initialize samplers mapping with 0

[Vadim Girlin]

On Sat, 2012-04-28 at 11:42 -0700, Kenneth Graunke wrote:
> On 04/28/2012 11:20 AM, Vadim Girlin wrote:
> > According to GLSL 1.30 specification, initial value for all uniforms
> > without initializer is 0.  Some applications rely on this behaviour,
> > e.g. google's MapGL doesn't set all sampler uniforms.
> >
> > (see https://bugs.freedesktop.org/show_bug.cgi?id=49088 )
> >
> > Signed-off-by: Vadim Girlin<vadimgirlin at gmail.com>
> > ---
> >
> > Tested with r600g only - no regressions.
> 
> Awesome find!  I was at a complete loss here. :)
> 
> It turns out this is in the 1.20 spec too; it looks like 1.10 doesn't 
> say (but that isn't too surprising).  I might add a comment:
> 
> /* From the GLSL 1.20 specification, page 24, section 4.3.5 "Uniform":
>   * "The link time initial value is either the value of the variable's
>   *  initializer, or 0 if no initializer present.  Sampler types cannot
>   *  have initializers."
>   */
> 
> Also, do you really need the memsets in ir_to_mesa and st_glsl_to_tgsi? 
>   Everything should go through the linker, so that seems somewhat 
> redundant.  I tested with just the link_uniforms change and that was 
> enough to fix MapsGL on i965/Sandybridge.

Without the memset in the st_glsl_to_tgsi firefox crashed with MapGL.
Probably some code in the state tracker relies on the synchronized
values of the SamplerUnits arrays in the gl_program and
gl_shader_program. Also, _mesa_uniform function compares these arrays to
check for update, and some piglit test failed due to the following
problem:

First array is initialized: 0, 0, 0, ...
Second (without memset)	  : 0, 1, 2, ...

The app calls Uniform1i to set sampler[1] to 1, so we do first[1]=1, but
then it's equal with second[1] and update is not detected.

Vadim

> 
> Assuming you drop the other hunks and add a comment, you can add:
> Reviewed-and-tested-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> Ian, does this look right to you?
> 
> >   src/glsl/link_uniforms.cpp                 |    4 +---
> >   src/mesa/program/ir_to_mesa.cpp            |    1 +
> >   src/mesa/state_tracker/st_glsl_to_tgsi.cpp |    1 +
> >   3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> > index 2f1c9f5..cb80caf 100644
> > --- a/src/glsl/link_uniforms.cpp
> > +++ b/src/glsl/link_uniforms.cpp
> > @@ -329,9 +329,7 @@ link_assign_uniform_locations(struct gl_shader_program *prog)
> >         prog->UniformHash = new string_to_uint_map;
> >      }
> >
> > -   for (unsigned i = 0; i<  Elements(prog->SamplerUnits); i++) {
> > -      prog->SamplerUnits[i] = i;
> > -   }
> > +   memset(prog->SamplerUnits, 0, sizeof(prog->SamplerUnits));
> >
> >      /* First pass: Count the uniform resources used by the user-defined
> >       * uniforms.  While this happens, each active uniform will have an index
> > diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
> > index 840648e..685c1b9 100644
> > --- a/src/mesa/program/ir_to_mesa.cpp
> > +++ b/src/mesa/program/ir_to_mesa.cpp
> > @@ -2865,6 +2865,7 @@ get_mesa_program(struct gl_context *ctx,
> >      prog = ctx->Driver.NewProgram(ctx, target, shader_program->Name);
> >      if (!prog)
> >         return NULL;
> > +   memset(prog->SamplerUnits, 0, sizeof(prog->SamplerUnits));
> >      prog->Parameters = _mesa_new_parameter_list();
> >      v.ctx = ctx;
> >      v.prog = prog;
> > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > index 9e68deb..7c4275a 100644
> > --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> > @@ -4779,6 +4779,7 @@ get_mesa_program(struct gl_context *ctx,
> >      prog = ctx->Driver.NewProgram(ctx, target, shader_program->Name);
> >      if (!prog)
> >         return NULL;
> > +   memset(prog->SamplerUnits, 0, sizeof(prog->SamplerUnits));
> >      prog->Parameters = _mesa_new_parameter_list();
> >      v->ctx = ctx;
> >      v->prog = prog;
>