[Mesa-dev] [PATCH 1/2] mesa: fix uniforms calculation in glGetProgramiv

[Timothy Arceri]

On Sun, 2015-11-01 at 08:03 +0200, Tapani Pälli wrote:
> On 11/01/2015 01:20 AM, Timothy Arceri wrote:
> > On Sat, 2015-10-31 at 06:52 +0200, Tapani Pälli wrote:
> > > On 10/30/2015 05:57 PM, Ilia Mirkin wrote:
> > > > On Fri, Oct 30, 2015 at 8:30 AM, Tapani Pälli <tapani.palli at intel.com>
> > > > wrote:
> > > > > Since introduction of SSBO, UniformStorage contains not just
> > > > > uniforms
> > > > > but also buffer variables, this needs to be taken in to account when
> > > > > calculating active uniforms with GL_ACTIVE_UNIFORMS and
> > > > > GL_ACTIVE_UNIFORM_MAX_LENGTH.
> > > > > 
> > > > > No Piglit regressions.
> > > > > 
> > > > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > > > ---
> > > > >    src/mesa/main/shaderapi.c | 14 ++++++++++++--
> > > > >    1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> > > > > index 765602e..ac40891 100644
> > > > > --- a/src/mesa/main/shaderapi.c
> > > > > +++ b/src/mesa/main/shaderapi.c
> > > > > @@ -630,9 +630,16 @@ get_programiv(struct gl_context *ctx, GLuint
> > > > > program, GLenum pname,
> > > > >       case GL_ACTIVE_ATTRIBUTE_MAX_LENGTH:
> > > > >          *params = _mesa_longest_attribute_name_length(shProg);
> > > > >          return;
> > > > > -   case GL_ACTIVE_UNIFORMS:
> > > > > -      *params = shProg->NumUniformStorage - shProg
> > > > > ->NumHiddenUniforms;
> > > > > +   case GL_ACTIVE_UNIFORMS: {
> > > > > +      unsigned i;
> > > > > +      const unsigned num_uniforms =
> > > > > +         shProg->NumUniformStorage - shProg->NumHiddenUniforms;
> > > > Are the hidden uniforms guaranteed to be at the end of that list? If
> > > > not, the subtraction needs to happen at the end. Also didn't I see
> > > Good point, I don't think that's guaranteed.
> > It should be guaranteed see: assign_hidden_uniform_slot_id() in
> > link_uniforms.cpp
> 
> OK, I did not remember how it goes I thought to check this only later :) 
> Then this patch seems correct after all.

Yeah it looks correct to me:
Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

It does look like one day we should just split up gl_uniform_storage but
that's work for another day.
> 
> > > I'll need to to move the
> > > subtraction part or perhaps easier just skip hidden ones in the loop.
> > > 
> > > > some patches making dedicated lists for UBOs and SSBOs?
> > > UBO and SSBO index spaces were separated but they both still utilize
> > > gl_uniform_storage for their contents, uniforms and buffer variables.
> > > 
> > Right good point.
> > 
> > > > > +      for (*params = 0, i = 0; i < num_uniforms; i++) {
> > > > > +         if (!shProg->UniformStorage[i].is_shader_storage)
> > > > > +            (*params)++;
> > > > > +      }
> > > > >          return;
> > > > > +   }
> > > > >       case GL_ACTIVE_UNIFORM_MAX_LENGTH: {
> > > > >          unsigned i;
> > > > >          GLint max_len = 0;
> > > > > @@ -640,6 +647,9 @@ get_programiv(struct gl_context *ctx, GLuint
> > > > > program, GLenum pname,
> > > > >             shProg->NumUniformStorage - shProg->NumHiddenUniforms;
> > > > > 
> > > > >          for (i = 0; i < num_uniforms; i++) {
> > > > > +         if (shProg->UniformStorage[i].is_shader_storage)
> > > > > +            continue;
> > > > > +
> > > > >            /* Add one for the terminating NUL character for a non
> > > > > -array,
> > > > > and
> > > > >             * 4 for the "[0]" and the NUL for an array.
> > > > >             */
> > > > > --
> > > > > 2.4.3
> > > > > 
> > > > > _______________________________________________
> > > > > mesa-dev mailing list
> > > > > mesa-dev at lists.freedesktop.org
> > > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>