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

[Chad Versace]

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