[Mesa-dev] [PATCH] mesa: handle errors in _mesa_unpack_image instead in unpack_image

[Liu Aleaxander]

On Thu, Sep 15, 2011 at 11:04 PM, Brian Paul <brianp at vmware.com> wrote:
> On 09/15/2011 08:44 AM, Liu Aleaxander wrote:
>>
>> On Thu, Sep 15, 2011 at 10:17 PM, Brian Paul<brianp at vmware.com>  wrote:
>>>
>>> On 09/14/2011 11:34 PM, Yuanhan Liu wrote:
>>>>
>>>> Handle errors in _mesa_unpack_image instead in unpack_image. This
>>>> would make the error message more detailed and specified.
>>>>
>>>> This patch does:
>>>>  1. trigger a GL_INVALID_VALUE if (width<    0 || height<    0 || depth<
>>>>    0)
>>>
>>> It's incorrect to generate an error like that during display list
>>> construction.  The error should be raised when the command/list is
>>> actually
>>> executed.
>>
>> Got it.  Thanks.
>>
>>>
>>>>
>>>>  2. do not trigger an error if (width == 0 || height == 0 || depth == 0)
>>>>
>>>>     the old code would trigger a GL_OUT_OF_MEMORY error if user called
>>>>     glDrawPixels function with width == 0 and height == 0. This is wrong
>>>>     and will misguide user.
>>>
>>> Yes, that needs to be fixed.
>>>
>>>>
>>>>  3. trigger a GL_INVALID_ENUM error if bad format or type is met.
>>>
>>> But not at list compilation time.
>>>
>>>
>>>>  4. do trigger a GL_OUT_OF_MEMORY error just when malloc failed.
>>>>
>>>> Signed-off-by: Yuanhan Liu<yuanhan.liu at linux.intel.com>
>>>> ---
>>>>  src/mesa/main/dlist.c |    9 +--------
>>>>  src/mesa/main/pack.c  |   27 ++++++++++++++++++---------
>>>>  2 files changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/dlist.c b/src/mesa/main/dlist.c
>>>> index 6e075b4..21840e6 100644
>>>> --- a/src/mesa/main/dlist.c
>>>> +++ b/src/mesa/main/dlist.c
>>>> @@ -881,12 +881,8 @@ unpack_image(struct gl_context *ctx, GLuint
>>>> dimensions,
>>>>  {
>>>>     if (!_mesa_is_bufferobj(unpack->BufferObj)) {
>>>>        /* no PBO */
>>>> -      GLvoid *image = _mesa_unpack_image(dimensions, width, height,
>>>> depth,
>>>> +      return _mesa_unpack_image(dimensions, width, height, depth,
>>>>                                           format, type, pixels, unpack);
>>>> -      if (pixels&&    !image) {
>>>> -         _mesa_error(ctx, GL_OUT_OF_MEMORY, "display list
>>>> construction");
>>>> -      }
>>>> -      return image;
>>>>     }
>>>>     else if (_mesa_validate_pbo_access(dimensions, unpack, width,
>>>> height,
>>>>                                        depth, format, type, INT_MAX,
>>>> pixels)) {
>>>> @@ -908,9 +904,6 @@ unpack_image(struct gl_context *ctx, GLuint
>>>> dimensions,
>>>>
>>>>        ctx->Driver.UnmapBuffer(ctx, unpack->BufferObj);
>>>>
>>>> -      if (!image) {
>>>> -         _mesa_error(ctx, GL_OUT_OF_MEMORY, "display list
>>>> construction");
>>>> -      }
>>>>        return image;
>>>>     }
>>>>     /* bad access! */
>>>> diff --git a/src/mesa/main/pack.c b/src/mesa/main/pack.c
>>>> index fd3f89d..ba5917d 100644
>>>> --- a/src/mesa/main/pack.c
>>>> +++ b/src/mesa/main/pack.c
>>>> @@ -5102,12 +5102,15 @@ _mesa_unpack_image( GLuint dimensions,
>>>>  {
>>>>     GLint bytesPerRow, compsPerRow;
>>>>     GLboolean flipBytes, swap2, swap4;
>>>> +   GET_CURRENT_CONTEXT(ctx);
>>>>
>>>> -   if (!pixels)
>>>> -      return NULL;  /* not necessarily an error */
>>>> -
>>>> -   if (width<= 0 || height<= 0 || depth<= 0)
>>>> -      return NULL;  /* generate error later */
>>>> +   if (width<    0 || height<    0 || depth<    0) {
>>>> +      _mesa_error(ctx, GL_INVALID_VALUE, __func__);
>>>> +      return NULL;
>>>> +   } else if (!pixels || width == 0 || height == 0 || depth == 0) {
>>>> +      /* not necessarily an error */
>>>> +      return NULL;
>>>> +   }
>>>>
>>>>     if (type == GL_BITMAP) {
>>>>        bytesPerRow = (width + 7)>>    3;
>>>> @@ -5123,8 +5126,12 @@ _mesa_unpack_image( GLuint dimensions,
>>>>        if (_mesa_type_is_packed(type))
>>>>            components = 1;
>>>>
>>>> -      if (bytesPerPixel<= 0 || components<= 0)
>>>> -         return NULL;   /* bad format or type.  generate error later */
>>>> +      if (bytesPerPixel<= 0 || components<= 0) {
>>>> +         /* bad format or type.  generate error later */
>>>> +         _mesa_error(ctx, GL_INVALID_ENUM, __func__);
>>>
>>> This call and the one below are incorrect.  __func__ will evaluate to
>>> "_mesa_unpack_image" but that's not what we want to report to the user.
>>
>> Yeah, I was trying to figure out a good error message then, but didn't
>> find one.
>>
>>>
>>>
>>>> +         return NULL;
>>>> +      }
>>>> +
>>>>        bytesPerRow = bytesPerPixel * width;
>>>>        bytesPerComp = bytesPerPixel / components;
>>>>        flipBytes = GL_FALSE;
>>>> @@ -5139,8 +5146,10 @@ _mesa_unpack_image( GLuint dimensions,
>>>>           = (GLubyte *) malloc(bytesPerRow * height * depth);
>>>>        GLubyte *dst;
>>>>        GLint img, row;
>>>> -      if (!destBuffer)
>>>> -         return NULL;   /* generate GL_OUT_OF_MEMORY later */
>>>> +      if (!destBuffer) {
>>>> +         _mesa_error(ctx, GL_OUT_OF_MEMORY, __func__);
>>>> +         return NULL;
>>>> +      }
>>>>
>>>>        dst = destBuffer;
>>>>        for (img = 0; img<    depth; img++) {
>>>
>>>
>>> The attached patch fixes the issues you've raised but defers error
>>> generation from compile-time to execute-time.  OK?
>>
>> It's OK to me. But it seems that you are not just defer the error, but
>> dismiss it, right?
>
> No.  The user's original width, height, format, type, etc. still get
> compiled into the display list as-is.  Later, when the list is executed and
> glDrawPixels() is called (for example), the parameters will be checked and
> errors generated if needed.

Got it. Thanks.

>
>
>> BTW, what should I do with the issue. Resend the patch you attached,
>> or you just push it to git repo?
>
> I'll post the patch for review.

Ok.
>
> -Brian
>