Re: [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, srol...@vmware.com wrote:
 From: Roland Scheidegger 

 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 

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

[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, srol...@vmware.com wrote:
>>> From: Roland Scheidegger 
>>>
>>> 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 
>>
>> (Although I would appreciate some additional info on the commit message)
> Ok, thanks for reviewing, I added 

Re: [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, srol...@vmware.com wrote:
>> From: Roland Scheidegger 
>>
>> 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 
> 
> (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.
> 

Re: [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, srol...@vmware.com wrote:
> From: Roland Scheidegger 
>
> 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 

(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 

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

[Roland Scheidegger]

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, srol...@vmware.com wrote:
 From: Roland Scheidegger 

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

Roland

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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

Re: [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, srol...@vmware.com wrote:
>>> From: Roland Scheidegger 
>>>
>>> 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);
>>
>


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Roland Scheidegger]

Am 27.01.2018 um 09:52 schrieb Alejandro Piñeiro:
> On 27/01/18 01:59, srol...@vmware.com wrote:
>> From: Roland Scheidegger 
>>
>> 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.
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);
> 
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Alejandro Piñeiro]

On 27/01/18 01:59, srol...@vmware.com wrote:
> From: Roland Scheidegger 
>
> 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 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);


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev