[Mesa-stable] [Mesa-dev] [PATCH] Mesa: Fix error code for glTexImage3D in GLES

Wed Dec 21 00:42:57 UTC 2016

Thanks, Chad

I will update the patch per your suggestion. 

-----Original Message-----
From: Chad Versace [mailto:chadversary at chromium.org] 
Sent: Wednesday, December 21, 2016 5:05 AM
To: Xu, Randy <randy.xu at intel.com>
Cc: Ian Romanick <idr at freedesktop.org>; mesa-dev at lists.freedesktop.org; mesa-stable at lists.freedesktop.org
Subject: Re: [Mesa-stable] [Mesa-dev] [PATCH] Mesa: Fix error code for glTexImage3D in GLES

On Mon 19 Dec 2016, Xu, Randy wrote:
> Hi, Chad & Ian
> 
> Thanks for your suggestion, and I understand and agree your point, 
> while the texsubimage_error_check (in teximage.c) calls 
> _mesa_error_check_format_and_type first, and if error happens, it will 
> return immediately (in 2175) and not call 
> texture_format_error_check_gles (in 2184). So I did the patch this way.
> 
> Follow your suggestion, we'd better move 
> texture_format_error_check_gles ahead of 
> _mesa_error_check_format_and_type, i.e. handle the GLES API ahead. Do 
> you agree with that?

I'm afraid to move texture_format_error_check_gles() ahead of _mesa_error_check_format_and_type(). That may be the correct thing to do, but my understanding of this error-checking code is weak and I'm afraid of the unintended consequences of moving it. If someone more familiar with this code claims that it's safe to move the check, then moving it is ok with me.

So... please ignore my complaint. The check should remain in _mesa_error_check_format_and_type(), unless someone has a better suggestion. That functions already contains some gles-specific checks, so there is no harm in adding yet another.

I have more comments below.

> -----Original Message-----
> From: Ian Romanick [mailto:idr at freedesktop.org]
> Sent: Saturday, December 17, 2016 6:02 AM
> To: Chad Versace <chadversary at chromium.org>; Xu, Randy 
> <randy.xu at intel.com>; mesa-dev at lists.freedesktop.org; 
> mesa-stable at lists.freedesktop.org; Xu at freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH] Mesa: Fix error code for glTexImage3D 
> in GLES
> 
> On 12/16/2016 12:49 PM, Chad Versace wrote:
> > On Fri 16 Dec 2016, Chad Versace wrote:
> >> On Fri 16 Dec 2016, Randy Xu wrote:
> >>> From: "Xu,Randy" <randy.xu at intel.com>
> >>>
> >>> The ES specification says that TexImage3D should return 
> >>> GL_INVALID_OPERATION if the internal format is DEPTH_COMPONENT, DEPTH_-STENCIL or STENCIL_INDEX.

The above is true. The ES spec says that. However, the GL spec says the same thing. See page 158 in the GLES 3.2 spec and page 194 in the GL 4.5 spec. So, I believe referring to that text in the spec in the commit message is misleading because it is unrelated to the test failure.

It's also misleading misleading because the patch doesn't update any glTexImage3D code. The patch updates dimension-independent code that affects glTexImage1D, glTexImage2D, and glTexImage3D.

I inspected the test results more closely, with and without the patch.
The debug output differs on a single line, marked with '***'. From the debug info, it seems that one of the several causes for the test failure is that _mesa_error_check_format_and_type() gets called, and emits the wrong error, before Mesa rejects GL_DEPTH_STENCIL as an invalid target for glTexImage3D with GL_INVALID_OPERATION.

Without patch:

    Test case 'dEQP-GLES3.functional.negative_api.texture.teximage3d'..
    Mesa: User error: GL_INVALID_ENUM in glTexImage3D(target=GL_NO_ERROR)
    Mesa: User error: GL_INVALID_ENUM in glTexImage3D(target=GL_TEXTURE_2D)
    Mesa: User error: GL_INVALID_ENUM in glTexImage3D(incompatible format = GL_RGBA, type = GL_NO_ERROR)
    Mesa: User error: GL_INVALID_ENUM in glTexImage3D(incompatible format = GL_NO_ERROR, type = GL_UNSIGNED_BYTE)
    Mesa: User error: GL_INVALID_VALUE in glTexImage3D(internalFormat=GL_NO_ERROR)
 ***Mesa: User error: GL_INVALID_ENUM in glTexImage3D(incompatible format = GL_DEPTH_STENCIL, type = GL_UNSIGNED_BYTE)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage%dD(format = GL_DEPTH_COMPONENT, type = GL_UNSIGNED_BYTE, internalformat = GL_RGBA)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage%dD(format = GL_RGBA, type = GL_UNSIGNED_BYTE, internalformat = GL_RGB)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage3D(incompatible format = GL_RGB, type = GL_UNSIGNED_SHORT_4_4_4_4)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage3D(incompatible format = GL_RGB, type = GL_UNSIGNED_SHORT_5_5_5_1)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage%dD(format = GL_RGB, type = GL_UNSIGNED_INT_2_10_10_10_REV, internalformat = GL_RGB10_A2)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage%dD(format = GL_RGBA_INTEGER, type = GL_INT, internalformat = GL_RGBA32UI)
    Test case duration in microseconds = 691466 us
      Fail (Got invalid error)

    DONE!

    Test run totals:
      Passed:        0/1 (0.0%)
      Failed:        1/1 (100.0%)
      Not supported: 0/1 (0.0%)
      Warnings:      0/1 (0.0%)

With patch:

    Test case 'dEQP-GLES3.functional.negative_api.texture.teximage3d'..
    Mesa: User error: GL_INVALID_ENUM in glTexImage3D(target=GL_NO_ERROR)
    Mesa: User error: GL_INVALID_ENUM in glTexImage3D(target=GL_TEXTURE_2D)
    Mesa: User error: GL_INVALID_ENUM in glTexImage3D(incompatible format = GL_RGBA, type = GL_NO_ERROR)
    Mesa: User error: GL_INVALID_ENUM in glTexImage3D(incompatible format = GL_NO_ERROR, type = GL_UNSIGNED_BYTE)
    Mesa: User error: GL_INVALID_VALUE in glTexImage3D(internalFormat=GL_NO_ERROR)
 ***Mesa: User error: GL_INVALID_OPERATION in glTexImage3D(incompatible format = GL_DEPTH_STENCIL, type = GL_UNSIGNED_BYTE)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage%dD(format = GL_DEPTH_COMPONENT, type = GL_UNSIGNED_BYTE, internalformat = GL_RGBA)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage%dD(format = GL_RGBA, type = GL_UNSIGNED_BYTE, internalformat = GL_RGB)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage3D(incompatible format = GL_RGB, type = GL_UNSIGNED_SHORT_4_4_4_4)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage3D(incompatible format = GL_RGB, type = GL_UNSIGNED_SHORT_5_5_5_1)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage%dD(format = GL_RGB, type = GL_UNSIGNED_INT_2_10_10_10_REV, internalformat = GL_RGB10_A2)
    Mesa: User error: GL_INVALID_OPERATION in glTexImage%dD(format = GL_RGBA_INTEGER, type = GL_INT, internalformat = GL_RGBA32UI)
    Test case duration in microseconds = 690923 us
      Pass (Pass)

    DONE!

    Test run totals:
      Passed:        1/1 (100.0%)
      Failed:        0/1 (0.0%)
      Not supported: 0/1 (0.0%)
      Warnings:      0/1 (0.0%)

Based on the debug output, and on the code the patch actually touches, I believe the following spec quote is the correct quote for this patch:

    From the OpenGL ES 3.2 spec, Section 8.5 Texture Image
    Specification, page 158:

        An INVALID_OPERATION error is generated if a combination of
        values for format, type, and internalformat is specified that is
        not listed as a valid combination in tables 8.2 or 8.3.

So... if you update the commit message with that spec quote, and insert that quote into code, then the patch looks good to me.

Is there anything I overlooked?

[snip]

> > One more thing: Please insert any relevant quotes from the GLES spec 
> > into the code itself as comments. It's ok to put those quotes in the
> 
> I was going to suggest the same thing.
> 
> > commit message, but they should also go into the code. If it's in 
> > the code, developers will easily find the quote without needing to 
> > use git-blame.