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

Tue Dec 20 21:04:43 UTC 2016

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.