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-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*
>
> 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
