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

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

> Nevertheless, the properties exist.
>
> Roland
>
>
>>> 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);
>>
>