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-sta...@lists.freedesktop.org>
>>> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
>>> Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
>>> Signed-off-by: Tapani Pälli <tapani.pa...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
