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

[Kenneth Graunke]

On 11/20/2013 10:36 PM, Tapani Pälli wrote:
> On 11/21/2013 12:04 AM, 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*
> 
> ok, I'll add this to the patch.

It's really unrelated, and as Roland pointed out, the existing
implementation is probably fine.

I would just push v2 of your patch.  You have push access.

Sorry for the trouble.
--Ken