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

[Kenneth Graunke]

On Wednesday, June 8, 2016 4:57:14 PM PDT Patrick Baggett wrote:
> 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.

This seems like a reasonable plan.  If you'd like to send a patch to do
this, I'd be happy to review and commit it for you.

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

That probably is cheaper :)  Frankly, I'm not sure how useful it is to
specifically call out negative width vs. height vs. depth in an error
message.  In other words, we could probably just check this, and then
make the error message "width, height, or depth are negative" or
"subregion has negative dimensions" or such.

I'm happy to review a patch to do that as well.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160613/b721f98e/attachment.sig>