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 <srol...@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 <apinhe...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
