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

[Xu, Randy]

Hi, Chad & Ian

I check the code again, we cannot easily move texture_format_error_check_gles ahead of _mesa_error_check_format_and_type,  as the texture_format_error_check_gles is based on _mesa_error_check_format_and_type to handles some additional restrictions from GLES API.  

I am afraid it's not a minor effort and we should be very careful. Please let me know your idea or suggestion. 

Thanks,
Randy

-----Original Message-----
From: Xu, Randy 
Sent: Monday, December 19, 2016 10:47 AM
To: 'Ian Romanick' <idr at freedesktop.org>; Chad Versace <chadversary at chromium.org>; mesa-dev at lists.freedesktop.org; mesa-stable at lists.freedesktop.org
Subject: RE: [Mesa-dev] [PATCH] Mesa: Fix error code for glTexImage3D in GLES

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?

Thanks,
Randy 

2131 static GLboolean
2132 texsubimage_error_check(struct gl_context *ctx, GLuint dimensions,
2133                         struct gl_texture_object *texObj,
2134                         GLenum target, GLint level,
2135                         GLint xoffset, GLint yoffset, GLint zoffset,
2136                         GLint width, GLint height, GLint depth,
2137                         GLenum format, GLenum type, const GLvoid *pixels,
2138                         bool dsa, const char *callerName)
2139 {

2169    err = _mesa_error_check_format_and_type(ctx, format, type);
2170    if (err != GL_NO_ERROR) {
2171       _mesa_error(ctx, err,
2172                   "%s(incompatible format = %s, type = %s)",
2173                   callerName, _mesa_enum_to_string(format),
2174                   _mesa_enum_to_string(type));
2175       return GL_TRUE;
2176    }

2183    if (_mesa_is_gles(ctx) &&
2184        texture_format_error_check_gles(ctx, format, type,
2185                                        texImage->InternalFormat,
2186                                        dimensions, callerName)) {
2187       return GL_TRUE;
2188    }



-----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 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