On Thu, Sep 15, 2011 at 11:04 PM, Brian Paul <bri...@vmware.com> wrote:
> 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.
Got it. Thanks.
>
>
>> 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.
Ok.
>
> -Brian
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
