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

[Brian Paul]

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.
>
> 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.
>
> 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.
>
>     +         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