[Mesa-dev] [PATCH 4/5] mesa: Fix glGetUniformfv of native integer uniforms.

[Eric Anholt]

On Sat, 20 Aug 2011 00:16:05 -0700, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 08/19/2011 05:56 PM, Eric Anholt wrote:
> > We have to actually convert the values on the way out.  Fixes piglit
> > ARB_shader_objects/getuniform.
> > ---
> >  src/mesa/main/uniforms.c |   32 ++++++++++++++++++++++++++++----
> >  1 files changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
> > index cda840f..fbaa810 100644
> > --- a/src/mesa/main/uniforms.c
> > +++ b/src/mesa/main/uniforms.c
> > @@ -55,13 +55,11 @@ static GLenum
> >  base_uniform_type(GLenum type)
> >  {
> >     switch (type) {
> > -#if 0 /* not needed, for now */
> >     case GL_BOOL:
> >     case GL_BOOL_VEC2:
> >     case GL_BOOL_VEC3:
> >     case GL_BOOL_VEC4:
> >        return GL_BOOL;
> > -#endif
> >     case GL_FLOAT:
> >     case GL_FLOAT_VEC2:
> >     case GL_FLOAT_VEC3:
> > @@ -408,8 +406,12 @@ get_uniform(struct gl_context *ctx, GLuint program, GLint location,
> >     else {
> >        const struct gl_program_parameter *p =
> >           &prog->Parameters->Parameters[paramPos];
> > +      gl_constant_value (*values)[4];
> >        GLint rows, cols, i, j, k;
> >        GLsizei numBytes;
> > +      GLenum storage_type;
> > +
> > +      values = prog->Parameters->ParameterValues + paramPos + offset;
> >  
> >        get_uniform_rows_cols(p, &rows, &cols);
> >  
> > @@ -421,15 +423,37 @@ get_uniform(struct gl_context *ctx, GLuint program, GLint location,
> >           return;
> >        }
> >  
> > +      if (ctx->Const.NativeIntegers) {
> > +	 storage_type = base_uniform_type(p->DataType);
> > +      } else {
> > +	 storage_type = GL_FLOAT;
> > +      }
> > +
> >        switch (returnType) {
> >        case GL_FLOAT:
> >           {
> >              GLfloat *params = (GLfloat *) paramsOut;
> >              k = 0;
> >              for (i = 0; i < rows; i++) {
> > -               const int base = paramPos + offset + i;
> >                 for (j = 0; j < cols; j++ ) {
> > -                  params[k++] = prog->Parameters->ParameterValues[base][j].f;
> > +		  switch (storage_type) {
> > +		  case GL_FLOAT:
> > +		     params[k++] = values[i][j].f;
> > +		     break;
> > +		  case GL_INT:
> > +		     params[k++] = values[i][j].i;
> > +		     break;
> > +		  case GL_UNSIGNED_INT:
> > +		     params[k++] = values[i][j].u;
> > +		     break;
> > +		  case GL_BOOL:
> > +		     params[k++] = values[i][j].b;
> > +		     break;
> > +		  default:
> > +		     _mesa_problem(ctx, "Invalid type in glGetUniform()");
> > +		     params[k++] = 0.0;
> > +		     break;
> > +		  }
> >                 }
> >              }
> >           }
> 
> Argh.  The OpenGL specification is so unclear on what happens when you
> call GetUniform{f, i, ui}v on a value of a different type.  Your code
> here assumes that it's legal to call GetUniformfv on an int/bool/uint
> uniform, and that the uniform's value should be implicitly converted to
> floating point.  That seems plausible.

Note that I came up with the testcase to improve our coverage after the
bug was caught by a ogles2conform case.  I'm confident in this behavior
of glGetUniform being the correct one.

> But what about calling GetUniformiv or GetUniformuiv on a float uniform?
>  Is it supposed to convert the float value to an int/uint?  By what
> rounding rules...truncate?  We're certainly not doing that now.  If
> that's the proper behavior, we need your switch statement in the GL_INT
> and GL_UNSIGNED_INT returnType cases as well.

Ooh, good point.  We were just casting to int, while it looks like for
glGetIntegerv, we do IROUND.  I suspect the correct answer is IROUND in
that case?

> We also do need it for the GL_DOUBLE returnType case.  (Admittedly
> that's a GL 4 function we don't implement, but we may as well get it
> right now...)

The GL_DOUBLE case was already completely broken unrelated to native
integers, nor is it testable in Mesa currently, so I didn't touch it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110822/0d7fb458/attachment.pgp>