On 23/07/16 00:31, Francisco Jerez wrote:
> Alejandro Piñeiro <apinhe...@igalia.com> writes:
>
>> Hi,
>>
>> On 15/07/16 22:46, Francisco Jerez wrote:
>>> Alejandro Piñeiro <apinhe...@igalia.com> writes:
>>>
>>>> On 14/07/16 21:24, Francisco Jerez wrote:
>>>>> Alejandro Piñeiro <apinhe...@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...Ok. >>>>> 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. Ok, thanks for the long explanation. > 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 And thanks for the suggestion. > -- However that spec change would have the side effect of making > the CTS test you're trying to fix non-compliant... This would not be the first case of a CTS test that is not correct due the complexities/ambiguities of the OpenGL spec. Or worse, CTS tests that are just So what about this: I open a Khronos bug (public unless someone thinks that it would be better private), using your explanation as a base to the description (although tempted to just point this email), and we put the patch/bugfix on hold until we get an answer? BR > >>>> 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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
