[Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level

[Alejandro Piñeiro]

Hi,

On 15/07/16 22:46, Francisco Jerez wrote:
> Alejandro Piñeiro <apinheiro at igalia.com> writes:
>
>> On 14/07/16 21:24, Francisco Jerez wrote:
>>> Alejandro Piñeiro <apinheiro at igalia.com> writes:
>>>
>>>> Without this commit, a image is considered valid if the level of the
>>>> texture bound to the image is complete, something we can check as mesa
>>>> save independently if it is "base incomplete" of "mipmap incomplete".
>>>>
>>>> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
>>>> Image Loads and Stores"):
>>>>
>>>>   "An access is considered invalid if:
>>>>     the texture bound to the selected image unit is incomplete;"
>>>>
>>>> This implies that the access to the image unit is invalid if the
>>>> texture is incomplete, no mattering details about the specific texture
>>>> level bound to the image.
>>>>
>>>> This fixes:
>>>> GL44-CTS.shader_image_load_store.incomplete_textures
>>>> ---
>>>>
>>>> Current piglit test is not testing what this commit tries to fix. I
>>>> will send a patch to piglit in short.
>>>>
>>>>  src/mesa/main/shaderimage.c | 14 +++++++++++---
>>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
>>>> index 90643c4..d20cd90 100644
>>>> --- a/src/mesa/main/shaderimage.c
>>>> +++ b/src/mesa/main/shaderimage.c
>>>> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx, struct gl_image_unit *u)
>>>>     if (!t->_BaseComplete && !t->_MipmapComplete)
>>>>         _mesa_test_texobj_completeness(ctx, t);
>>>>  
>>>> +   /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture Image
>>>> +    * Loads and Stores:
>>>> +    *
>>>> +    *  "An access is considered invalid if:
>>>> +    *    the texture bound to the selected image unit is incomplete;"
>>>> +    */
>>>> +   if (!t->_BaseComplete ||
>>>> +       !t->_MipmapComplete)
>>>> +      return GL_FALSE;
>>> I don't think this is correct, AFAIUI a texture having _MipmapComplete
>>> equal to false doesn't imply that the texture as a whole would be
>>> considered incomplete according to the GL's definition of completeness.
>>> Whether or not it's considered complete usually depends on the sampler
>>> state while you're doing regular texture sampling: If the sampler a
>>> texture object is used with has any of the mipmap filtering modes
>>> enabled you need to check _MipmapComplete, otherwise you need to check
>>> _BaseComplete.  The problem when you attempt to carry over this
>>> definition to shader images (as the spec implies) is that image units
>>> have no sampler state as such, and that they can only ever access one
>>> specified level of the texture at a time (potentially a texture level
>>> other than the base).  This patch makes image units behave like a
>>> sampler unit with mipmap filtering enabled for the purpose of texture
>>> completeness validation, which is almost definitely too strong.
>> Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
>> checking the state at all. Thanks for pointing it.
>>
>>> An alternative would be to do something along the lines of:
>>>
>>> | if (!_mesa_is_texture_complete(t, &t->Sampler))
>>> |    return GL_FALSE;
>> Yes, that is what I wanted, to return false if the texture is incomplete.
>>
>>> The problem is that you would then run into problems when some of the
>>> non-base mipmap levels are missing but the sampler state baked into the
>>> gl_texture_object says that you aren't mipmapping, so the GL spec would
>>> normally consider the texture to be complete and
>>> _mesa_is_texture_complete would return true accordingly, but still you
>>> wouldn't be able to use any of the missing texture levels as shader
>>> image if the application tried to bind them to an image unit (that's the
>>> reason for the u->Level vs t->BaseLevel checks below you're removing).
>> Ok, then if I understand correctly, the solution is not about replacing
>> the level checks for _mesa_is_texture_complete, but keeping current
>> checks, and add a _mesa_is_texture_complete check. Just checked and
>> everything seems to work fine (except that now the behaviour is more
>> strict, see below). I will send a patch in short.
>>
> Yeah, that would likely work and get the CTS test to pass, but it would
> still be more strict than the spec says and consider cases that are OK
> according to the spec to be incomplete, so I was reluctant to call it a
> solution.
>
> I think the ideal solution would be for the state of an image unit to be
> independent from the filtering and sampling state, and depend on the
> completeness of the bound level *only*.  Any idea if this CTS (or your
> equivalent piglit test) passes on other GL implementations that support
> image load/store (e.g. nVidia's -- I would be surprised if it does).

Just checked today with NVIDIA 352.30 and 352.30. I was not able to
directly test this issue with the CTS test, as it fails before with a
different error (the shader doesn't compile, and the shader info log is
garbage). But I tested it with the equivalent piglit subtest that I sent
to the piglit list. It passes on both cases.

>>> Honestly I think the current definition of image unit completeness is
>>> broken, because it considers the image unit state to be complete in
>>> cases where the image unit state is unusable (because the target image
>>> level may be missing even though the texture object itself is considered
>>> complete), and because it considers it to be incomplete in cases where
>>> you really have no reason to care (e.g. why should you care what the
>>> filtering mode from the sampler state says if you aren't doing any
>>> filtering at all, as long as the one level you've bound to the image
>>> unit is present and complete).
>> Well, yes in general the spec is being perhaps too strict. But I assume
>> that if we want to follow the spec, we don't have any other option.
>>
> I think the rule is not only unnecessarily strict...  If my reading of
> the spec is correct it's also impossible to implement to the letter,
> because the spec expects you to consider the state of an image unit to
> be image-complete just because the bound texture object is
> sampler-complete even though the bound texture level may be missing.  

Sorry but I was not able to find any reference to that expectation on
the spec. Although it is true that Im using as reference only this spec:
https://www.opengl.org/registry/specs/ARB/shader_image_load_store.txt

Did I skip that expectation on the spec, or it is included on other spec ?

BTW, when re-reading the spec, I found an issue related to texture
completeness:

    "(7) How do shader image loads and stores interact with texture
        completeness?  What happens if you bind a texture with inconsistent
        mipmaps?

      RESOLVED:  The image unit is treated as if nothing were bound, where 
      all accesses are treated as invalid."


So I guess that it was discussed.

> It
> might be worth filing a GL bug at Khronos.

In order to do that, I would need to clearly point the contradiction.
Sorry if I was not able to find it.

>
>> FWIW, when testing the patch I mentioned before, some of the piglit
>> tests start to fail. The reason is that with that patch everything is
>> more strict, so the piglit tests need to set the filtering to NEAREST in
>> order to not fail. I will send a piglit patch soon too.
>>>> +
>>>>     if (u->Level < t->BaseLevel ||
>>>> -       u->Level > t->_MaxLevel ||
>>>> -       (u->Level == t->BaseLevel && !t->_BaseComplete) ||
>>>> -       (u->Level != t->BaseLevel && !t->_MipmapComplete))
>>>> +       u->Level > t->_MaxLevel)
>>>>        return GL_FALSE;
>>>>  
>>>>     if (_mesa_tex_target_is_layered(t->Target) &&
>>>> -- 
>>>> 2.7.4


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160718/05392837/attachment-0001.sig>