[Mesa-dev] [PATCH 2/2] mesa: Moves up error check for subtexture dimensions

[Eduardo Lima Mitev]

On 09/23/2015 11:59 PM, Jason Ekstrand wrote:
> On Tue, Sep 15, 2015 at 3:01 PM, Eduardo Lima Mitev <elima at 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 <elima at 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
>>>>
>>>
>>
>