[Mesa-dev] [PATCH] mesa: Clamp GetUniformuiv values to be >= 0.

[Nicolai Hähnle]

On 12.12.2016 00:25, Kenneth Graunke wrote:
> Section 2.2.2 (Data Conversions For State Query Commands) of the
> OpenGL 4.5 October 24th 2016 specification says:
>
> "If a command returning unsigned integer data is called, such as
>  GetSamplerParameterIuiv, negative values are clamped to zero."
>
> Fixes GL44-CTS.gpu_shader_fp64.state_query.
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/main/uniform_query.cpp | 48 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 9 deletions(-)
>
> Hey Nicolai,
>
> I wrote a similar patch a while back, but never got around to sending it,
> since I realized that the gl45release branch expects our current behavior,
> and the change to make the CTS expect clamping is only on the master branch.
>
> Apparently I made some additional changes, compared to yours.  I figured
> I'd send this along and let you see if you think any of my extra changes
> are still necessary.  If so, feel free to fold them into your patch.
>
> I also think we need to fix several other glGet* commands...it's just that
> this is the only one currently tested.  A bunch work because the values
> returned can't be negative.

I think your patch is a strict superset of what mine does and should be 
used instead. I do have one comment below, with that fixed it has my R-b.

There is the more general question of how to cope with those cases where 
the CTS requires non-standard behavior. I think we should insist on 
doing the right thing in Mesa, and push for changes to the CTS.

Until quite recently, I've been occupied by radeonsi- and 
Gallium-specific bugs, but that's changing and I'm looking into using 
CTS master rather than back-porting fixes to the dead gl45release branch 
(hence this patch).

>
>  --Ken
>
> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
> index db700df..f05a29f 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
> @@ -347,14 +347,10 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>         * just memcpy the data.  If the types are not compatible, perform a
>         * slower convert-and-copy process.
>         */
> -      if (returnType == uni->type->base_type
> -	  || ((returnType == GLSL_TYPE_INT
> -	       || returnType == GLSL_TYPE_UINT)
> -	      &&
> -	      (uni->type->base_type == GLSL_TYPE_INT
> -	       || uni->type->base_type == GLSL_TYPE_UINT
> -               || uni->type->base_type == GLSL_TYPE_SAMPLER
> -               || uni->type->base_type == GLSL_TYPE_IMAGE))) {
> +      if (returnType == uni->type->base_type ||
> +          ((returnType == GLSL_TYPE_INT || returnType == GLSL_TYPE_UINT) &&
> +           (uni->type->base_type == GLSL_TYPE_SAMPLER ||
> +            uni->type->base_type == GLSL_TYPE_IMAGE))) {
>  	 memcpy(paramsOut, src, bytes);
>        } else {
>  	 union gl_constant_value *const dst =
> @@ -422,7 +418,6 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>  	       }
>  	       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
> @@ -447,6 +442,9 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>  	       case GLSL_TYPE_BOOL:
>  		  dst[didx].i = src[sidx].i ? 1 : 0;
>  		  break;
> +               case GLSL_TYPE_UINT:
> +                  dst[didx].i = src[sidx].i;

I think this should be

dst[didx].i = MIN2(src[sidx].u, INT_MAX);

Cheers,
Nicolai

> +                  break;
>                 case GLSL_TYPE_DOUBLE: {
>                    double tmp;
>                    memcpy(&tmp, &src[sidx].f, sizeof(tmp));
> @@ -458,6 +456,38 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>  		  break;
>  	       }
>  	       break;
> +            case GLSL_TYPE_UINT:
> +               switch (uni->type->base_type) {
> +               case GLSL_TYPE_FLOAT:
> +                  /* The spec isn't terribly clear how to handle negative
> +                   * values with an unsigned return type.
> +                   *
> +                   * GL 4.5 section 2.2.2 ("Data Conversions for State
> +                   * Query Commands") 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."
> +                   */
> +                  dst[didx].i = src[sidx].f < 0.0f ? 0 : IROUND(src[sidx].f);
> +                  break;
> +               case GLSL_TYPE_BOOL:
> +                  dst[didx].i = src[sidx].i ? 1 : 0;
> +                  break;
> +               case GLSL_TYPE_INT:
> +                  dst[didx].i = MAX2(src[sidx].i, 0);
> +                  break;
> +               case GLSL_TYPE_DOUBLE: {
> +                  double tmp;
> +                  memcpy(&tmp, &src[sidx].f, sizeof(tmp));
> +                  dst[didx].i = tmp < 0.0 ? 0 : IROUNDD(tmp);
> +                  break;
> +               }
> +               default:
> +                  unreachable("invalid uniform type");
> +               }
> +               break;
>
>  	    default:
>  	       assert(!"Should not get here.");
>