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

[Ian Romanick]

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 current code returns INVALID_ENUM as _mesa_error_check_format_and_type is
>>> used by glReadPixels also and the GL specification defines "INVALID_ENUM is
>>> generated if format is DEPTH_STENCIL and type is not UNSIGNED_INT_24_8 or
>>> FLOAT_32_UNSIGNED_INT_24_8_- REV".
>>>
>>> This patch only impacts GLES, which can generate GL_INVALID_OPERATION because
>>> glReadPixels cannot be used to read depth or stencil buffer.
>>> Fixes dEQP-GLES3.functional.negative_api.texture.teximage3d.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99076
>>>
>>> Signed-off-by: Xu,Randy <randy.xu at intel.com>
>>> ---
>>>  src/mesa/main/glformats.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>
>> Thanks for fixing the dEQP failure. But I think your patch applies the
>> fix to wrong portion of code.
>>
>> The commit message mentions the internalFormat, but the patch updates
>> a function to which validates the *format* (not internalFormat).
>> I believe the change should instead be placed in
>> teximage.c:texture_format_error_check_gles(), which is better for
>> 2 reasons:
>>     - That function specifically checks GLES-specific requirements like
>>       this.
>>     - It checks the *internalFormat* in addition to the *format*.
>>
>> Also, in the future, please remove the empty lines between the tags
>> (Bugzilla:, Signed-of-by:) in the commit message. The empty lines can
>> confuse scripts that parse those tags.
> 
> 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.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev