[Mesa-dev] Fwd: [PATCH] mesa: Correctly set base and max texture levels in glTexparameterf

[Anuj Phogat]

On Wed, Jan 2, 2013 at 11:50 PM, Brian Paul <brianp at vmware.com> wrote:
>
> On 01/02/2013 10:39 AM, Paul Berry wrote:
>>
>> On 2 January 2013 07:38, Anuj Phogat <anuj.phogat at gmail.com
>> <mailto:anuj.phogat at 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.phogat at gmail.com
>>     <mailto:anuj.phogat at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev