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?
BTW, what should I do with the issue. Resend the patch you attached,
or you just push it to git repo?
Thanks,
Yuanhan Liu
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
