On 09/15/2011 08:44 AM, Liu Aleaxander wrote:
On Thu, Sep 15, 2011 at 10:17 PM, Brian Paul<bri...@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....@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
