[Mesa-dev] [PATCH] mesa: enable GL_TEXTURE_LOD_BIAS set/get v2

[Roland Scheidegger]

On 11/20/2013 10:04 PM, Kenneth Graunke wrote:
> On 11/20/2013 09:45 AM, Ian Romanick wrote:
>> On 11/20/2013 09:08 AM, Kenneth Graunke wrote:
>>> On 11/20/2013 03:27 AM, Tapani Pälli wrote:
>>>> Earlier comments suggest this was removed from GL core spec but it is
>>>> still there. Enabling makes 'texture_lod_bias_getter' Khronos
>>>> conformance tests pass, also removes some errors from Metro Last Light
>>>> game which is using this API.
>>>>
>>>> v2: leave NOTE comment (Ian)
>>>>
>>>> Cc: "9.0 9.1 9.2 10.0" <mesa-stable at lists.freedesktop.org>
>>>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>>>> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>> ---
>>>>   src/mesa/main/texparam.c | 16 ++++++++--------
>>>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c
>>>> index d56b7d9..f77e7f6 100644
>>>> --- a/src/mesa/main/texparam.c
>>>> +++ b/src/mesa/main/texparam.c
>>>> @@ -684,11 +684,8 @@ set_tex_parameterf(struct gl_context *ctx,
>>>>         return GL_FALSE;
>>>>
>>>>      case GL_TEXTURE_LOD_BIAS:
>>>> -      /* NOTE: this is really part of OpenGL 1.4, not EXT_texture_lod_bias.
>>>> -       * It was removed in core-profile, and it has never existed in OpenGL
>>>> -       * ES.
>>>> -       */
>>>> -      if (ctx->API != API_OPENGL_COMPAT)
>>>> +      /* NOTE: this is really part of OpenGL 1.4, not EXT_texture_lod_bias. */
>>>> +      if (_mesa_is_gles(ctx))
>>>>            goto invalid_pname;
>>>>
>>>>         if (!target_allows_setting_sampler_parameters(texObj->Target))
>>>> @@ -1513,7 +1510,7 @@ _mesa_GetTexParameterfv( GLenum target, GLenum pname, GLfloat *params )
>>>>            *params = (GLfloat) obj->DepthMode;
>>>>            break;
>>>>         case GL_TEXTURE_LOD_BIAS:
>>>> -         if (ctx->API != API_OPENGL_COMPAT)
>>>> +         if (_mesa_is_gles(ctx))
>>>>               goto invalid_pname;
>>>>
>>>>            *params = obj->Sampler.LodBias;
>>>> @@ -1701,10 +1698,13 @@ _mesa_GetTexParameteriv( GLenum target, GLenum pname, GLint *params )
>>>>            *params = (GLint) obj->DepthMode;
>>>>            break;
>>>>         case GL_TEXTURE_LOD_BIAS:
>>>> -         if (ctx->API != API_OPENGL_COMPAT)
>>>> +         if (_mesa_is_gles(ctx))
>>>>               goto invalid_pname;
>>>>
>>>> -         *params = (GLint) obj->Sampler.LodBias;
>>>> +         /* GL spec 'Data Conversions' section specifies that floating-point
>>>> +          * value in integer Get function is rounded to nearest integer
>>>> +          */
>>>> +         *params = (GLint) roundf(obj->Sampler.LodBias);
>>>
>>> Yikes!  I missed this change in the original patch.
>>>
>>> The call to roundf here is definitely wrong.  If you look above, you'll
>>> find...
>>>
>>> void GLAPIENTRY
>>> _mesa_GetTexLevelParameterfv( GLenum target, GLint level,
>>>                                GLenum pname, GLfloat *params )
>>> {
>>>     GLint iparam;
>>>     _mesa_GetTexLevelParameteriv( target, level, pname, &iparam );
>>>     *params = (GLfloat) iparam;
>>> }
>>>
>>> ...so, your change means that we'll always round to the nearest integer,
>>> even for GetTexLevelParameterfv.
>>
>> But his patch modifies GetTexParameter.  There are already separate
>> paths for GetTexParameterfv and GetTexParameteriv.  Note the last two
>> hunks of the patch.
>
> You're right; I totally read the wrong thing...so my concern is invalid,
> and this patch is probably fine.
>
>>> I think you should drop the roundf change from this patch, making it
>>> entirely about exposing the values.  Later patches can fix the values.
>>>
>>> The other thing that concerns me is that there are other floating point
>>> values being returned, and none of them have this roundf treatment.
>>> Assuming the justification in your comment is correct (I haven't
>>> checked), then they'd all be wrong, and all need to be fixed.
>>
>> I don't see anything in _mesa_GetTexParameteriv that's incorrectly
>> handling floats.  The only other floats that I noticed were colors or
>> other values that get returned as fixed-point.  Did you notice anything
>> specific?  Or were you looking at GetTexLevelParameteriv? :)
>
> GL_TEXTURE_MAX_ANISOTROPY is also a float.  I think that's the only one,
> though, so maybe that can just get fixed separately.  *shrug*
>
Not that the float makes a whole lot of sense and anyone would actually 
try to use non-integer values there :-).

Roland