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

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

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

As you mentioned, both interpretations have a little of inconsistency.
If khronos, or someone chiming in, clarifies in the opposite direction
we can just go back.

>
> Roland
>
>>> Nevertheless, the properties exist.
>>>
>>> hRoland
>>>
>>>
>>>>> This breaks some piglit arb_internalformat_query2 tests (which I belive to
>>>>> be wrong).
>>>>> ---
>>>>>  src/mesa/main/formatquery.c | 3 ---
>>>>>  1 file changed, 3 deletions(-)
>>>>>
>>>>> diff --git a/src/mesa/main/formatquery.c b/src/mesa/main/formatquery.c
>>>>> index 2214f97e67..f345140518 100644
>>>>> --- a/src/mesa/main/formatquery.c
>>>>> +++ b/src/mesa/main/formatquery.c
>>>>> @@ -960,9 +960,6 @@ _mesa_GetInternalformativ(GLenum target, GLenum internalformat, GLenum pname,
>>>>>        mesa_format texformat;
>>>>>  
>>>>>        if (target != GL_RENDERBUFFER) {
>>>>> -         if (!_mesa_legal_get_tex_level_parameter_target(ctx, target, true))
>>>>> -            goto end;
>>>>> -
>>>>>           baseformat = _mesa_base_tex_format(ctx, internalformat);
>>>>>        } else {
>>>>>           baseformat = _mesa_base_fbo_format(ctx, internalformat);
>>
>