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

[Antía Puentes]

On lun, 2016-12-12 at 10:43 +0100, Nicolai Hähnle wrote:
> 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.

This patch was never pushed, was it? and GL45-CTS.gpu_shader_fp64.state_query
fails in the new vk-gl-cts repository because it expects these negative
values to be clamped.

> 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.");
> > 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev