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
