On 09/23/2015 11:59 PM, Jason Ekstrand wrote: > On Tue, Sep 15, 2015 at 3:01 PM, Eduardo Lima Mitev <el...@igalia.com> wrote: >> On 09/15/2015 09:23 PM, Jason Ekstrand wrote: >>> On Tue, Sep 15, 2015 at 4:47 AM, Eduardo Lima Mitev <el...@igalia.com> >>> wrote: >>>> For consistency and efficiency, the (sub)texture dimension error check >>>> should go before the validation of format, type and internal format. >>> >>> You mentioned in another patch that this fixes a bug or, at the very >>> least, prevents one. What bug is that? I ask because my personal >>> inclination would be to keep them in the order they were before. >>> --Jason >>> >> >> These tests fail without this patch, because they fail the validation of >> format+type+internalFormat (INVALID_OPERATION) which happens before the >> check for texture dimensions (INVALID_VALUE, the expected error). >> >> dEQP-GLES2.functional.negative_api.texture.texsubimage2d_neg_offset >> dEQP-GLES2.functional.negative_api.texture.texsubimage2d_offset_allowed >> dEQP-GLES2.functional.negative_api.texture.texsubimage2d_neg_wdt_hgt >> >> In all 3, the failing combination is: >> >> format = GL_RGB, type = GL_UNSIGNED_BYTE, internalFormat = GL_RGBA >> >> Following the spec, the effective internal format should be GL_RGB8 (per >> table 3.12, page 128 of OpenGL-ES 3.0.4). So, it is actually failing the >> double-check I added in the previous patch, in which I obtain again the >> base format from the effective internal format to compare it with the >> original internal format in base form passed to the function. So GL_RGB8 >> -> GL_RGB which != GL_RGBA. >> >> Now I'm puzzled. Is the combination above valid? As I interpret the >> spec, it isn't. Which means the tests would be wrong, but that's strange >> too. > > I could easily see the test being wrong here. As per table 3.2 in the > ES 3.0 spec, you can't use GL_RGB with GL_UNSIGNED_BYTE and GL_RGBA. > Just switching the format to GL_RGBA or the internalFormat to GL_RGB8 > in the test would probably fix it. I think the best path forward > there would be to make a patch to the test and try to get that > upstream to dEQP. I'm not sure what the procedure is there. >
Just sent a new version of the series dropping the last patch: http://lists.freedesktop.org/archives/mesa-dev/2015-September/095191.html And failed a bug against dEQP (oddly, to AOSP actually) for fixing the buggy tests, including a reference patch: https://code.google.com/p/android/issues/detail?id=187348&thanks=187348&ts=1443083425 Thanks a lot for the feedback! Eduardo > --Jason > >>>> --- >>>> src/mesa/main/teximage.c | 12 ++++++------ >>>> 1 file changed, 6 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c >>>> index d9453e3..09040d5 100644 >>>> --- a/src/mesa/main/teximage.c >>>> +++ b/src/mesa/main/teximage.c >>>> @@ -2117,6 +2117,12 @@ texsubimage_error_check(struct gl_context *ctx, >>>> GLuint dimensions, >>>> return GL_TRUE; >>>> } >>>> >>>> + if (error_check_subtexture_dimensions(ctx, dimensions, >>>> + texImage, xoffset, yoffset, >>>> zoffset, >>>> + width, height, depth, >>>> callerName)) { >>>> + return GL_TRUE; >>>> + } >>>> + >>>> err = _mesa_error_check_format_and_type(ctx, format, type); >>>> if (err != GL_NO_ERROR) { >>>> _mesa_error(ctx, err, >>>> @@ -2145,12 +2151,6 @@ texsubimage_error_check(struct gl_context *ctx, >>>> GLuint dimensions, >>>> return GL_TRUE; >>>> } >>>> >>>> - if (error_check_subtexture_dimensions(ctx, dimensions, >>>> - texImage, xoffset, yoffset, >>>> zoffset, >>>> - width, height, depth, >>>> callerName)) { >>>> - return GL_TRUE; >>>> - } >>>> - >>>> if (_mesa_is_format_compressed(texImage->TexFormat)) { >>>> if (_mesa_format_no_online_compression(ctx, >>>> texImage->InternalFormat)) { >>>> _mesa_error(ctx, GL_INVALID_OPERATION, >>>> -- >>>> 2.4.6 >>>> >>> >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
