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

[Brian Paul]

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.

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


> +         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?

-Brian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dlist.c.patch
Type: application/pgp-keys
Size: 1289 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110915/c1056c6f/attachment.key>