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

[Kai Wasserbäch]

Nicolai Hähnle wrote on 10.12.2016 19:12:
> 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.

Sure, will you do it or should I prepare a patch?

Cheers,
Kai


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161210/27b1b4c5/attachment-0001.sig>