[Mesa-dev] [PATCH 3/3] mesa: skip validation of legality of size/type queries for format queries

[Roland Scheidegger]

Am 22.02.2018 um 08:58 schrieb Alejandro Piñeiro:
> On 30/01/18 01:24, Roland Scheidegger wrote:
>> Am 29.01.2018 um 17:03 schrieb Alejandro Piñeiro:
>>> On 29/01/18 16:38, Roland Scheidegger wrote:
>>>> Am 29.01.2018 um 09:09 schrieb Alejandro Piñeiro:
>>>>> On 27/01/18 12:09, Roland Scheidegger wrote:
>>>>>> Am 27.01.2018 um 09:52 schrieb Alejandro Piñeiro:
>>>>>>> On 27/01/18 01:59, sroland at vmware.com wrote:
>>>>>>>> From: Roland Scheidegger <sroland at vmware.com>
>>>>>>>>
>>>>>>>> The size/type query is always legal (if we made it that far).
>>>>>>>> This causes a difference for GL_TEXTURE_BUFFER - the reason is that these
>>>>>>>> parameters are valid only with GetTexLevelParameter() if gl 3.1 is supported,
>>>>>>>> but not if only ARB_texture_buffer_object is supported.
>>>>>>>> However, while the spec says that these queries return "the same information
>>>>>>>> as querying GetTexLevelParameter" I believe we're not expected to return just
>>>>>>>> zeros here. By definition, these pnames are always valid (unlike for the
>>>>>>>> GetTexLevelParameter() function which would return an error without GL 3.1),
>>>>>>>> so returning 0 but no error makes no sense to me.
>>>>>>> But in general, AFAIU, this is how GetInternalFormat works. The
>>>>>>> extension only raises an error if their API is not used properly.  But
>>>>>>> not if the combination being requested doesn't make sense.
>>>>>>>
>>>>>>> For example, the pname GET_TEXTURE_IMAGE_FORMAT, would return the same
>>>>>>> value that using GetTexImage. But if the resource is not supported by
>>>>>>> GetTextImage, it should return NONE, but not raising an error. So for
>>>>>>> example, you could use as target GL_RENDERBUFFER for
>>>>>>> GET_TEXTURE_IMAGE_FORMAT query, and  just get a GL_NONE, but no an
>>>>>>> error. While that one would raise an error on GetTexImage.
>>>>>> This is correct, but I don't think that's really comparable. The case
>>>>>> you cited is if something isn't supported - this is not the case here at
>>>>>> all, if some format with TEXTURE_BUFFER is supported, we're just lying
>>>>>> about the size/type because ARB_tbo made these properties non-queryable.
>>>>> Are not queryable for specific GL versions, as you said. For me it would
>>>>> be inconsistent if you are able to query the tbo sizes with
>>>>> GetInternalformat, but not with gettexlevelparameter on a given opengl
>>>>> version. Raising or not an error should not be a reason to discard it,
>>>>> because as I said, ARB_internalformat_query2 in general avoid raising GL
>>>>> error for unsupported cases.
>>>>>
>>>>> What I'm trying to say is that the current implementation is not wrong,
>>>>> just more literal to the current spec wording. Yes, perhaps too literal
>>>>> in some cases. But I also have the feeling that your patch pushes too
>>>>> much on the supposed original intention of this query.
>>>>>
>>>>> Having said so, this is just my opinion. So if anyone else agrees with
>>>>> your interpretation of the desired behaviour on this query, I don't
>>>>> think that it is a big deal to include this patch.
>>>> There's actually a specific bit in the internalformat_query2 which makes
>>>> me think is a pretty good hint that "return the same information as
>>>> querying GetTeXLevelParameter" should not be taken literally and is just
>>>> informational: The part about GL_INTERNALFORMAT_STENCIL_TYPE - since the
>>>> language applies to that as well, however an equivalent pname for
>>>> GetTeXLevelParameter does not even exist.
>>> Hmm, good point. And as the code is right now, it filters the target
>>> based on being supported or not by GetTexLevelParameter, but then (if
>>> the target is not filtered), returns a value for STENCIL_TYPE, even
>>> although GetTexLevelParameter doesn't have a equivalent pname/query.
>>>
>>>> And imho even if you take it literally, it's ambiguos at best, since the
>>>> same paragraph doesn't just mention the "return the same as..." part,
>>>> but also explicitly says that 0/none is returned if the format is
>>>> unsupported, which contradicts this part (well you can of course
>>>> interpret it that this is not a sufficient condition to return 0, but I
>>>> don't think that was the intention).
>>> In general, this spec has a lot (too much?) of room for interpretation.
>> Yes, I agree with that. (Of course it doesn't help that the dependency
>> section is absolutely massive.)
>>
>>>> I agree though my interpretation is more in line what I think was
>>>> probably the intention and can't directly be derived from the wording -
>>>> in general however you can always use all pname/target bits and get
>>>> valid answers iff the format/pname combination is supported, so this not
>>>> working texture_buffer would be very awkward imho.
>>>>
>>>> But maybe someone more familiar with the spec could chime in...
>>> I will open a spec issue for this. Meanwhile they reply:
>>>
>>> Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>
>>>
>>> (Although I would appreciate some additional info on the commit message)
>> Ok, thanks for reviewing, I added some more info on the commit message.
> 
> FWIW, the spec issue was discussed by Khronos, and agreed on your
> interpretation. The spec is already updated:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.khronos.org_registry_OpenGL_extensions_ARB_ARB-5Finternalformat-5Fquery2.txt&d=DwIDaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=jP5-fSz6QDyI5teEuAJevbcJ2ZZ5iMYggoX6FOHPpIk&s=JiNA8MbLB_qzBm95JMBI7I9-rVoI51IEmkkOehIG1eU&e=
> 
> There are some tweaks on some paragraphs, but it is explicitly explained
> on the issue 16 added.
> 
> So now we are sure that this part is spec compliant. Thanks for pushing
> for this, in spite my initial stubbornness.
> 
Thanks for getting this clarified. It's always nice to have such unclear
specification cleaned up to be more concise.

Roland