[Mesa-dev] [PATCH] mesa: return INVALID_OPERATION if there's no image in GetTex*Image

[Brian Paul]

On 07/24/2015 10:05 AM, Ilia Mirkin wrote:
> On Fri, Jul 24, 2015 at 9:55 AM, Brian Paul <brianp at vmware.com> wrote:
>> The commit subject line doesn't seem to match the code.
>
> It matches the code if you read the whole function... I think. Right
> now if there's no image, it'll succeed, whereas after this change,
> it'll return INVALID_OPERATION.

Hmm, both with and without your patch the piglit 
arb_get_texture_sub_image-errors test passes.  It tests getting of a 
non-existent image.

Note that "no image" can be interpreted in two ways:
1. the gl_texture_object::Image[face][level] pointer is NULL
2. the gl_texture_object::Image[face][level] image exists but 
width,height,depth are zero.

In my piglit test, the former case generates INVALID_OPERATION.

I'm adding some additional piglit tests to check the latter case.  Your 
patch does catch the case of width = 0 and xoffset+width>texwidth which 
we didn't catch before.

I guess I'd simply name your patch "mesa: fix error checking for getting 
zero-sized texture images".

-Brian

>
> Happy to use a different subject line, let me know what you had in mind.
>
>>
>> The code looks good though.
>>
>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>
>>
>> On 07/23/2015 06:40 PM, Ilia Mirkin wrote:
>>>
>>> Commit 17f714836 (mesa: rearrange texture error checking order) moved
>>> the width/height/depth == 0 allowance before checking if the image was
>>> there. This was in part due to depth having to be == 1 for 2D images and
>>> width having to be == 1 for 1D images. Instead relax the height/depth
>>> checks to also accept 0 as valid.
>>>
>>> With this change,
>>>
>>>     bin/arb_direct_state_access-get-textures
>>>
>>> starts passing again.
>>>
>>> Fixes: 17f714836 (mesa: rearrange texture error checking order)
>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>
>>> ---
>>>    src/mesa/main/texgetimage.c | 18 +++++++++---------
>>>    1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/mesa/main/texgetimage.c b/src/mesa/main/texgetimage.c
>>> index cdbd618..c0ccce3 100644
>>> --- a/src/mesa/main/texgetimage.c
>>> +++ b/src/mesa/main/texgetimage.c
>>> @@ -928,13 +928,6 @@ dimensions_error_check(struct gl_context *ctx,
>>>       const struct gl_texture_image *texImage;
>>>       int i;
>>>
>>> -   if (width == 0 || height == 0 || depth == 0) {
>>> -      /* Not an error, but nothing to do.  Return 'true' so that the
>>> -       * caller simply returns.
>>> -       */
>>> -      return true;
>>> -   }
>>> -
>>>       if (xoffset < 0) {
>>>          _mesa_error(ctx, GL_INVALID_VALUE, "%s(xoffset = %d)", caller,
>>> xoffset);
>>>          return true;
>>> @@ -973,7 +966,7 @@ dimensions_error_check(struct gl_context *ctx,
>>>                         "%s(1D, yoffset = %d)", caller, yoffset);
>>>             return true;
>>>          }
>>> -      if (height != 1) {
>>> +      if (height > 1) {
>>>             _mesa_error(ctx, GL_INVALID_VALUE,
>>>                         "%s(1D, height = %d)", caller, height);
>>>             return true;
>>> @@ -987,7 +980,7 @@ dimensions_error_check(struct gl_context *ctx,
>>>                         "%s(zoffset = %d)", caller, zoffset);
>>>             return true;
>>>          }
>>> -      if (depth != 1) {
>>> +      if (depth > 1) {
>>>             _mesa_error(ctx, GL_INVALID_VALUE,
>>>                         "%s(depth = %d)", caller, depth);
>>>             return true;
>>> @@ -1086,6 +1079,13 @@ dimensions_error_check(struct gl_context *ctx,
>>>          }
>>>       }
>>
>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>
>>
>>>
>>> +   if (width == 0 || height == 0 || depth == 0) {
>>> +      /* Not an error, but nothing to do.  Return 'true' so that the
>>> +       * caller simply returns.
>>> +       */
>>> +      return true;
>>> +   }
>>> +
>>>       return false;
>>>    }
>>>
>>>
>>