On Wed, Jan 2, 2013 at 11:50 PM, Brian Paul <bri...@vmware.com> wrote:
>
> On 01/02/2013 10:39 AM, Paul Berry wrote:
>>
>> On 2 January 2013 07:38, Anuj Phogat <anuj.pho...@gmail.com
>> <mailto:anuj.pho...@gmail.com>> wrote:
>>
>> gles3 conformance sgis_texture_lod_basic_getter.test expects base
>> and max texture levels to be rounded to nearest integer. e.g. round
>> 2.3 to 2 and 2.6 to 3. OpenGL 3.0 and OpenGL ES 3.0 specifications
>> suggest similar rounding to select level when filtering mode is
>> GL_NEAREST.
>>
>> This patch makes sgis_texture_lod_basic_getter.test pass.
>>
>>
>> It looks to me from reading the spec like this logic should be applied
>> in all cases where _mesa_TexParameterf converts floats to ints, not
>> just base and max texture level. GL 4.3, section 8.10 ("Texture
>> Parameters") simply says "Data conversions are performed as specified
>> in section 2.2.1.", and section 2.2.1 ("Data Conversion For
>> State-Setting Commands") says that "Floating-point values are rounded
>> to the nearest integer." There's no mention that this rule should
>> apply only to base and max texture level.
>>
Yeah. GL 4.3 spec is more clear on data conversions. I'll include other cases
as well.
>> So rather than create a separate case in the switch statement to
>> handle GL_TEXTURE_BASE_LEVEL and GL_TEXTURE_MAX_LEVEL, I'd recommend
>> just fixing the existing case to do the rounding properly.
>>
>> Also, the switch statement as it exists today contains two separate
>> blocks with identical code (one to handle
>> GL_TEXTURE_SWIZZLE_{R,G,B,A}_EXT and one to handle all other
>> float-to-int conversions). That's silly--we should have just one block.
>>
I agree. I'll fix it.
>> One other comment below:
>>
>>
>> Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com
>> <mailto:anuj.pho...@gmail.com>>
>>
>> ---
>> src/mesa/main/texparam.c | 24 ++++++++++++++++++++++--
>> 1 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c
>> index ca5a21f..f3f97ca 100644
>> --- a/src/mesa/main/texparam.c
>> +++ b/src/mesa/main/texparam.c
>> @@ -654,8 +654,6 @@ _mesa_TexParameterf(GLenum target, GLenum
>> pname, GLfloat param)
>> case GL_TEXTURE_WRAP_S:
>> case GL_TEXTURE_WRAP_T:
>> case GL_TEXTURE_WRAP_R:
>> - case GL_TEXTURE_BASE_LEVEL:
>> - case GL_TEXTURE_MAX_LEVEL:
>> case GL_GENERATE_MIPMAP_SGIS:
>> case GL_TEXTURE_COMPARE_MODE_ARB:
>> case GL_TEXTURE_COMPARE_FUNC_ARB:
>> @@ -670,6 +668,28 @@ _mesa_TexParameterf(GLenum target, GLenum
>> pname, GLfloat param)
>> need_update = set_tex_parameteri(ctx, texObj, pname, p);
>> }
>> break;
>> + case GL_TEXTURE_BASE_LEVEL:
>> + case GL_TEXTURE_MAX_LEVEL:
>> + {
>> + GLint p[4];
>> + /* Check if param exceeds maximum value an integer can
>> hold */
>> + if (param > 2147483647.0f) {
>> + p[0] = (GLint) 2147483647.0f;
>> + }
>> + else if (param < 0.0f) {
>> + p[0] = 0;
>> + }
>>
>>
>> I'm not convinced that this amount of thoroughness is necessary.
>> There's no language in the spec (or test cases that I'm aware of) to
>> indicate that out-of-range floats need to be translated to INT_MAX or
>> 0. I think it would be adequate to just do "p[0] = (int)
>> round(param);" and just let the behaviour be undefined for
>> out-of-range values.
Yes, there is no such text in the spec. But gles3 conformance is testing
this case. GTF/Source/GL3Tests/GTFTestSGISTextureLod.c in gles3
conformance sets param=8589934592.0f using glTexParameterf() and
expect 2147483647 when queried using glGetTexParameteriv(). if not
handled correctly we get param=-2147483648.
What conformance test expects is logical but not exactly follow
the text in spec. I'm not sure if we should file a bug in conformance
test suite.
>> + else {
>> + /* Round float param to nearest integer. e.g round
>> 2.3 to 2
>> + * and 2.8 to 3.
>> + */
>> + p[0] = (GLint) (param + 0.5);
>> + }
>> +
>> + p[1] = p[2] = p[3] = 0;
>> + need_update = set_tex_parameteri(ctx, texObj, pname, p);
>> + }
>> + break;
>> case GL_TEXTURE_SWIZZLE_R_EXT:
>> case GL_TEXTURE_SWIZZLE_G_EXT:
>> case GL_TEXTURE_SWIZZLE_B_EXT:
>> --
>
>
> I second Paul's comments.
>
> -Brian
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
