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

[Liu Aleaxander]

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?

BTW, what should I do with the issue. Resend the patch you attached,
or you just push it to git repo?

Thanks,
Yuanhan Liu