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:i...@freedesktop.org]
Sent: Saturday, December 17, 2016 6:02 AM
To: Chad Versace <chadvers...@chromium.org>; Xu, Randy <randy...@intel.com>;
mesa-dev@lists.freedesktop.org; mesa-sta...@lists.freedesktop.org;
x...@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...@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...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
