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

[Brian Paul]

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.


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

-Brian