[Mesa-dev] [PATCH 2/2] mesa: rearrange texture error checking order

[Ilia Mirkin]

On Thu, Jul 23, 2015 at 6:49 PM, Brian Paul <brianp at vmware.com> wrote:
> On 07/23/2015 04:37 PM, Ilia Mirkin wrote:
>>
>> On Thu, Jul 23, 2015 at 6:34 PM, Brian Paul <brianp at vmware.com> wrote:
>>>
>>> On 07/23/2015 04:24 PM, Ilia Mirkin wrote:
>>>>
>>>>
>>>> On Wed, Jul 22, 2015 at 1:44 PM, Brian Paul <brianp at vmware.com> wrote:
>>>>>
>>>>>
>>>>> On 07/22/2015 11:29 AM, Ilia Mirkin wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Jul 22, 2015 at 1:20 PM, Brian Paul <brianp at vmware.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 07/22/2015 11:02 AM, Ilia Mirkin wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> This moves the width/height/depth == 0 check to the front and avoids
>>>>>>>> doing any other checking when that is the case.
>>>>>>>>
>>>>>>>> Also moves the dimensions check after the format/type checks so that
>>>>>>>> we
>>>>>>>> don't bail out with success on a width/height/depth == 0 request
>>>>>>>> when
>>>>>>>> the format/type don't match.
>>>>>>>>
>>>>>>>> Bugzilla:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D91425&d=BQIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=Y1TotaJmX1E7sCopvmMJc8PQKFXlHu6QxfcYOydKuII&s=L0lmmuk31G6M4hZEiwnX-IC4CPhW7rsrSCgEYtDgXjw&e=
>>>>>>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I suspect this isn't really needed in light of my patch to
>>>>>>> getteximage-invalid-format-for-packed-type.c
>>>>>>>
>>>>>>> I believe the test was in error, or at least sloppy, in that it was
>>>>>>> calling
>>>>>>> glGetTexImage to test format/type validation when there wasn't even a
>>>>>>> texture image to return.  The OpenGL spec doesn't specify an order
>>>>>>> for
>>>>>>> error
>>>>>>> checking multiple things, so if there's multiple errors, you can't be
>>>>>>> sure
>>>>>>> which one will be reported first.  My patch to the test removes that
>>>>>>> ambiguity.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Well, irrespective of the piglit test, the current code is a little
>>>>>> suspect -- it does the width/height/depth == 0 checks after it ensures
>>>>>> that height == 1 for 1d and depth == 1 for 2d. Any thoughts on that?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Yeah, that makes sense.  I guess we don't have any tests that exercise
>>>>> getting a 0 x 0 texture image.
>>>>>
>>>>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>>>
>>>>
>>>>
>>>> Ugh, so Dylan points out that this breaks
>>>>
>>>> bin/arb_direct_state_access-get-textures
>>>>
>>>> (which I didn't notice, because I guess it got recently enabled on mesa)
>>>>
>>>> It has this check:
>>>>
>>>>           /* No Storage
>>>>            *
>>>>            * The spec doesn't say what should happen in this case.  This
>>>> is
>>>>            * addressed by Khronos Bug 13223.
>>>>            */
>>>>           glCreateTextures(GL_TEXTURE_CUBE_MAP, 1, &name);
>>>>           glGetTextureImage(name, 0, GL_RGBA, GL_UNSIGNED_BYTE, 0,
>>>> data);
>>>>           pass &= piglit_check_gl_error(GL_INVALID_OPERATION);
>>>>           glDeleteTextures(1, &name);
>>>>
>>>> But my width == 0 || ... check at the top of dimensions_error_check
>>>> short-circuits all that.
>>>>
>>>> Brian, what situation is the "width == 0 || height == 0 || depth == 0"
>>>> supposed to account for?
>>>
>>>
>>>
>>> After error checking, it just lets up skip all the
>>> ctx->Driver.GetTexSubImage() calls and associated stuff (mapping PBOs,
>>> etc).
>>
>>
>> But some of the error checking makes sure that width/height/depth
>> aren't 0. And in case of a missing image, width/height/depth are all
>> set to 0. That was my confusion.
>
>
> OK, it looks like we can remove the height==1 and depth==1 checks. NVIDIA's
> driver does not generate an error if height=0 or depth=0 in the 1D/2D cases.
>
> Can you take care of that?

Yeah, I broke it, so I get to fix it :) Should have something out in a
few hours.