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

[Roland Scheidegger]

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.

Roland

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