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

[Ian Romanick]

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.

> 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? :)

>>           break;
>>        case GL_TEXTURE_CROP_RECT_OES:
>>           if (ctx->API != API_OPENGLES || !ctx->Extensions.OES_draw_texture)
>>