[Mesa-dev] [PATCH] mesa: Make TexSubImage check negative dimensions sooner.

[Patrick Baggett]

Sorry, didn't CC mesa-dev, trying again...

On Wed, Jun 8, 2016 at 4:11 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Two dEQP tests expect INVALID_VALUE errors for negative width/height
> parameters, but get INVALID_OPERATION because they haven't actually
> created a destination image.  This is arguably not a bug in Mesa, as
> there's no specified ordering of error conditions.
>
> However, it's also really easy to make the tests pass, and there's
> no real harm in doing these checks earlier.
>
> Fixes:
> dEQP-GLES3.functional.negative_api.texture.texsubimage3d_neg_width_height
> dEQP-GLES31.functional.debug.negative_coverage.get_error.texture.texsubimage3d_neg_width_height
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/main/teximage.c | 68 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 49 insertions(+), 19 deletions(-)
>
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index 58b7f27..d4f8278 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -1102,6 +1102,32 @@ _mesa_legal_texture_dimensions(struct gl_context *ctx, GLenum target,
>     }
>  }
>
> +static bool
> +error_check_subtexture_negative_dimensions(struct gl_context *ctx,
> +                                           GLuint dims,
> +                                           GLsizei subWidth,
> +                                           GLsizei subHeight,
> +                                           GLsizei subDepth,
> +                                           const char *func)
> +{
> +   /* Check size */
> +   if (subWidth < 0) {
> +      _mesa_error(ctx, GL_INVALID_VALUE, "%s(width=%d)", func, subWidth);
> +      return true;
> +   }
> +
> +   if (dims > 1 && subHeight < 0) {
> +      _mesa_error(ctx, GL_INVALID_VALUE, "%s(height=%d)", func, subHeight);
> +      return true;
> +   }
> +
> +   if (dims > 2 && subDepth < 0) {
> +      _mesa_error(ctx, GL_INVALID_VALUE, "%s(depth=%d)", func, subDepth);
> +      return true;
> +   }
> +

What do you think of a structure like:

switch(dims) {
    case 3:
        if(subDepth < 0) {
            ...
        }
        /* fall through */
    case 2:
        if(subHeight < 0) {
            ...
        }
       /* fall through *
    default:
        if(subWidth < 0) {
            ...
        }
}
return true;

I think this would reduce the overall number of expressions to check.
If you just want to check whether any are < 0, you can OR the sign
bits:


int result = 0;
switch(dims) {
    case 3: result |= subDepth & (1 << 31);
    case 2: result |= subHeight & (1 << 31);
    default: result |= subWidth & (1 << 31);
}
return (bool)(result>>31);

...then later call that function to generate a more detailed error
message about specifically which dimension was negative.

> +   return false;
> +}
>
>  /**
>   * Do error checking of xoffset, yoffset, zoffset, width, height and depth
> @@ -1119,25 +1145,6 @@ error_check_subtexture_dimensions(struct gl_context *ctx, GLuint dims,
>     const GLenum target = destImage->TexObject->Target;
>     GLuint bw, bh, bd;
>
> -   /* Check size */
> -   if (subWidth < 0) {
> -      _mesa_error(ctx, GL_INVALID_VALUE,
> -                  "%s(width=%d)", func, subWidth);
> -      return GL_TRUE;
> -   }
> -
> -   if (dims > 1 && subHeight < 0) {
> -      _mesa_error(ctx, GL_INVALID_VALUE,
> -                  "%s(height=%d)", func, subHeight);
> -      return GL_TRUE;
> -   }
> -
> -   if (dims > 2 && subDepth < 0) {
> -      _mesa_error(ctx, GL_INVALID_VALUE,
> -                  "%s(depth=%d)", func, subDepth);
> -      return GL_TRUE;
> -   }
> -
>     /* check xoffset and width */
>     if (xoffset < - (GLint) destImage->Border) {
>        _mesa_error(ctx, GL_INVALID_VALUE, "%s(xoffset)", func);
> @@ -2104,6 +2111,12 @@ texsubimage_error_check(struct gl_context *ctx, GLuint dimensions,
>        return GL_TRUE;
>     }
>
> +   if (error_check_subtexture_negative_dimensions(ctx, dimensions,
> +                                                  width, height, depth,
> +                                                  callerName)) {
> +      return GL_TRUE;
> +   }
> +
>     texImage = _mesa_select_tex_image(texObj, target, level);
>     if (!texImage) {
>        /* non-existant texture level */
> @@ -2140,6 +2153,12 @@ texsubimage_error_check(struct gl_context *ctx, GLuint dimensions,
>        return GL_TRUE;
>     }
>
> +   if (error_check_subtexture_negative_dimensions(ctx, dimensions,
> +                                                  width, height, depth,
> +                                                  callerName)) {
> +      return GL_TRUE;
> +   }
> +
>     if (error_check_subtexture_dimensions(ctx, dimensions,
>                                           texImage, xoffset, yoffset, zoffset,
>                                           width, height, depth, callerName)) {
> @@ -2497,6 +2516,11 @@ copytexsubimage_error_check(struct gl_context *ctx, GLuint dimensions,
>        return GL_TRUE;
>     }
>
> +   if (error_check_subtexture_negative_dimensions(ctx, dimensions,
> +                                                  width, height, 1, caller)) {
> +      return GL_TRUE;
> +   }
> +
>     if (error_check_subtexture_dimensions(ctx, dimensions, texImage,
>                                           xoffset, yoffset, zoffset,
>                                           width, height, 1, caller)) {
> @@ -4387,6 +4411,12 @@ compressed_subtexture_error_check(struct gl_context *ctx, GLint dims,
>        return GL_TRUE;
>     }
>
> +   if (error_check_subtexture_negative_dimensions(ctx, dims,
> +                                                  width, height, depth,
> +                                                  callerName)) {
> +      return GL_TRUE;
> +   }
> +
>     if (error_check_subtexture_dimensions(ctx, dims,
>                                           texImage, xoffset, yoffset, zoffset,
>                                           width, height, depth,
> --
> 2.8.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev