[Mesa-dev] [PATCH] mesa: Clamp negative values in glGetUniformuiv

[Nicolai Hähnle]

On 10.12.2016 18:19, Kai Wasserbäch wrote:
> Nicolai Hähnle wrote on 10.12.2016 14:54:
>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> Section 2.2.2 (Data Conversions For State Query Commands) of the OpenGL 4.5
>> (Compatibility Profile) spec says:
>>
>>     "If a value is so large in magnitude that it cannot be represented by the
>>      returned data type, then the nearest value representable using the
>>      requested type is returned."
>>
>> This is reasonably interpreted as saying that e.g. float uniforms with a
>> negative value should be clamped to 0 when queried as an unsigned integer.
>>
>> Fixes GL45-CTS.gpu_shader_fp64.state_query.
>> ---
>>  src/mesa/main/imports.h         | 17 +++++++++++++++++
>>  src/mesa/main/uniform_query.cpp | 21 ++++++++++++++++++++-
>>  2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/imports.h b/src/mesa/main/imports.h
>> index ef7c378..2af0add 100644
>> --- a/src/mesa/main/imports.h
>> +++ b/src/mesa/main/imports.h
>> @@ -158,20 +158,37 @@ static inline int IROUNDD(double d)
>>  }
>>
>>  /**
>>   * Convert float to int64 by rounding to nearest integer.
>>   */
>>  static inline GLint64 IROUND64(float f)
>>  {
>>     return (GLint64) ((f >= 0.0F) ? (f + 0.5F) : (f - 0.5F));
>>  }
>>
>> +/**
>> + * Convert float to unsigned int by rounding to nearest integer, away from zero.
>> + */
>> +static inline unsigned int UROUND(float f)
>> +{
>> +   return (unsigned int) ((f >= 0.0F) ? (f + 0.5F) : 0.0F);
>> +}
>> +
>> +/**
>> + * Convert double to unsigned int by rounding to nearest integer, away from
>> + * zero.
>> + */
>> +static inline unsigned int UROUNDD(double d)
>> +{
>> +   return (unsigned int) ((d >= 0.0) ? (d + 0.5) : 0.0);
>> +}
>> +
>>
>>  /**
>>   * Convert positive float to int by rounding to nearest integer.
>>   */
>>  static inline int IROUND_POS(float f)
>>  {
>>     assert(f >= 0.0F);
>>     return (int) (f + 0.5F);
>>  }
>>
>> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
>> index 3108d34..8e390cc 100644
>> --- a/src/mesa/main/uniform_query.cpp
>> +++ b/src/mesa/main/uniform_query.cpp
>> @@ -415,21 +415,20 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>>                    double tmp = src[sidx].f;
>>                    memcpy(&dst[didx].f, &tmp, sizeof(tmp));
>>  		  break;
>>                 }
>>  	       default:
>>  		  assert(!"Should not get here.");
>>  		  break;
>>  	       }
>>  	       break;
>>  	    case GLSL_TYPE_INT:
>> -	    case GLSL_TYPE_UINT:
>>  	       switch (uni->type->base_type) {
>>  	       case GLSL_TYPE_FLOAT:
>>  		  /* While the GL 3.2 core spec doesn't explicitly
>>  		   * state how conversion of float uniforms to integer
>>  		   * values works, in section 6.2 "State Tables" on
>>  		   * page 267 it says:
>>  		   *
>>  		   *     "Unless otherwise specified, when floating
>>  		   *      point state is returned as integer values or
>>  		   *      integer state is returned as floating-point
>> @@ -452,20 +451,40 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>>                    memcpy(&tmp, &src[sidx].f, sizeof(tmp));
>>                    dst[didx].i = IROUNDD(tmp);
>>  		  break;
>>                 }
>>  	       default:
>>  		  assert(!"Should not get here.");
>>  		  break;
>>  	       }
>>  	       break;
>>
>> +	    case GLSL_TYPE_UINT:
>> +	       switch (uni->type->base_type) {
>> +	       case GLSL_TYPE_FLOAT:
>> +		  dst[didx].u = UROUND(src[sidx].f);
>> +		  break;
>> +	       case GLSL_TYPE_BOOL:
>> +		  dst[didx].u = src[sidx].i ? 1 : 0;
>> +		  break;
>> +               case GLSL_TYPE_DOUBLE: {
>> +                  double tmp;
>> +                  memcpy(&tmp, &src[sidx].f, sizeof(tmp));
>> +                  dst[didx].u = UROUNDD(tmp);
>> +		  break;
>> +               }
>> +	       default:
>> +		  assert(!"Should not get here.");
>
> Shouldn't this be a call to unreachable()? (And if so, then probably change the
> default a few lines below as well?)

Probably yes, but given that this matches the surrounding code, it 
should be a separate change.

Cheers,
Nicolai

>> +		  break;
>> +	       }
>> +	       break;
>> +
>>  	    default:
>>  	       assert(!"Should not get here.");
>>  	       break;
>>  	    }
>>  	 }
>>        }
>>     }
>>  }
>>
>>  static void
>
> The rest of the change looks reasonable to me.
>
> Cheers,
> Kai
>