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

[Christoph Bumiller]

On 20.08.2011 22:07, Dan McCabe wrote:
> On 08/20/2011 12:16 AM, Kenneth Graunke 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.
>>
>> 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.
>>
The GL4.2 spec is quite clear on the issue:
from 2.11.7:
For all other [than boolean] uniform types, except for subroutine
uniforms and atomic counters, the Uniform* command used must match the
size and type of the uniform, as declared in the shader. No type
conversions are done.
An INVALID_OPERATION error will be generated if an attempt is made to
use a non-matching Uniform* command.

>> 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...)
>
> Ditto.
>
>> _______________________________________________
>> 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