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

[Kenneth Graunke]

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*

--Ken