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

[Francisco Jerez]

Alejandro Piñeiro <apinheiro at igalia.com> writes:

> 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.
>
In that case they must have changed their behavior.  I wrote the
arb_shader_image_load_store piglit tests against the nVidia driver
originally, and they would have failed almost every test if they had
implemented that rule to the letter, because I never bothered to set the
sampler filtering mode of image textures as you noticed here:

https://lists.freedesktop.org/archives/piglit/2016-July/020232.html

I guess the fact that nVidia is now enforcing this rule makes me
somewhat less nervous to make this change to make the CTS test happy,
since that means it's not very likely for applications to still be
relying on the less strict interpretation of the spec and break...

>>>> 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.
>

The spec seems rather sketchy for using the language of "texture
completeness" in the list of bullet points starting with "An access is
considered invalid if: [..]" in section 8.26 of the GL 4.4 spec, because
a texture being complete or not is dependent on sampler state that
doesn't have any significance for shader images.  If you bind a
base-complete but non-mipmap-complete texture to a texture unit along
with a sampler object specifying single-mipmap filtering, the texture is
considered complete by the GL spec.  Should it be considered incomplete
when the same base level is bound to an image unit instead of a texture
unit, even though no minification filtering will take effect and no
mipmapping is required?  (C.f. section 8.17 claiming that a texture
object should be considered incomplete if "[..] the minification filter
requires a mipmap [..] and the texture is not mipmap complete.").

Let's assume momentarily that it's only the minification filtering mode
baked into the texture object that matters for image units.  What should
happen in the following scenario?

 - The texture object has filtering modes set to NEAREST so no
   mipmapping is required and the above rule would consider the texture
   to be complete regardless of the non-base mipmap levels.

 - The texture object has inconsistent mipmap levels attached, e.g. some
   of the levels have incorrect size or inconsistent internal formats in
   a way that may make the miptree impossible to represent by the
   implementation.

 - Any of the non-base mipmap levels part of the texture object is bound
   to an image unit.

Should the image unit state be considered complete or not?  Section 8.26
implies that it should because the attached texture object is complete,
but that means that the implementation would have to support doing image
loads and stores on arbitrary levels of mipmap trees of inconsistent
layout (!).  OTOH Mesa would currently consider the case above to be
incomplete regardless of this patch due to the rather ad-hoc rule I
implemented in the same function.

I think my suggestion would be to change section 8.26 of the spec to
specify *explicitly* what kind of texture completeness is required, e.g.
the texture object should be mipmap-complete if any non-base level is
bound to the image unit, but only base-complete (by some definition of
base-completeness) if the level bound to the image unit is the base
level -- However that spec change would have the side effect of making
the CTS test you're trying to fix non-compliant...

>>
>>> 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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160722/dee91234/attachment.sig>