[Mesa-dev] [PATCH] mesa: implement GL_ARB_texture_buffer_range v5

[Eric Anholt]

Christoph Bumiller <e0425955 at student.tuwien.ac.at> writes:

> On 28.01.2013 19:54, Ian Romanick wrote:
>> On 01/27/2013 12:19 PM, Eric Anholt wrote:
>>> Christoph Bumiller <e0425955 at student.tuwien.ac.at> writes:
>>>
>>>> On 27.01.2013 00:58, Eric Anholt wrote:
>>>>> Christoph Bumiller <e0425955 at student.tuwien.ac.at> writes:
>>>>>> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
>>>>>> index 31a559e..e71f6e1 100644
>>>>>> --- a/src/mesa/main/teximage.c
>>>>>> +++ b/src/mesa/main/teximage.c
>>>>>> +/** GL_ARB_texture_buffer_object */
>>>>>> +void GLAPIENTRY
>>>>>> +_mesa_TexBuffer(GLenum target, GLenum internalFormat, GLuint buffer)
>>>>>> +{
>>>>>> +   struct gl_buffer_object *bufObj;
>>>>>> +
>>>>>> +   GET_CURRENT_CONTEXT(ctx);
>>>>>> +
>>>>>> +   if (!ctx->Extensions.ARB_texture_buffer_object) {
>>>>>> +      _mesa_error(ctx, GL_INVALID_OPERATION, "glTexBuffer");
>>>>>> +      return;
>>>>>> +   }
>>>>> The check for also ctx->API == API_OPENGL_CORE here has been
>>>>> dropped, so
>>>>> I think the i965 driver would start accepting this function in compat
>>>>> contexts when it shouldn't.
>>>>>
>>>>> If that check gets re-added, this is
>>>>> Reviewed-by: Eric Anholt <eric at anholt.net>
>>>>
>>>> To quote Ian on a reply to an earlier version of this, where I had the
>>>> check in the _mesa_TexBufferRange function also:
>>>>
>>>> "
>>>> if (!(ctx->API == API_OPENGL_CORE &&
>>>>
>>>> This check /shouldn't/ be necessary.  Since Paul's rework of the
>>>> dispatch table code, the TexBufferRange function should only be put in
>>>> the dispatch table if the context is the correct profile.  A trivial
>>>> piglit test will verify that: check that GL_INVALID_OPERATION is
>>>> generated if GL_ARB_texture_buffer_range is not supported and
>>>>
>>>>      glTexBufferRange(GL_TEXTURE_BUFFER, GL_RGBA, 0, 0, 0);
>>>>
>>>> is called.
>>>> "
>>>>
>>>> Why would this work for TexBufferRange and not TexBuffer non-Range ?
>>>
>>> Oh, I forgot Ian ruled that Intel can expose it on non-core as well, so
>>> the code before was just broken (we never noticed, since the test
>>> requires 1.40, which we don't have for compat).
>>>
>>> (the thing Ian was probably thinking of, though, was the "deprecated"
>>> flag in the XML for leaving things on in compat and removing them from
>>> core, not the other way around)
>>
>> I just talked to Paul, and it turns out I was wrong.  We only leave
>> out functions that are marked as deprecated.  Sorry about that. :(
>>
>
> So do I have to re-add the check now, in both TexBuffer and TexBufferRange ?
> "that Intel can expose it on non-core as well, so the code before was
> just broken" sounds like the check should be gone anyway ...

Agreed, it was broken.  This is a behavior change slipped into another
patch, but it happens to be a good one.  I'd love to see it done as a
patch before the refactor, since we've had a bunch of discussion on it.
Either way, it's got my r-b.

> (Also, it looks hackish; using the dispatch table like you thought was
> the case seems much nicer.
> And why does calling unsupported API entry points have to have defined
> behaviour ... a crash would be much for verbose ;)

What's worse would be some undefined behavior where the call runs to
completion and doesn't throw an error.  Then they never figure it out
until we make a change and apps start to crash.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130129/98064541/attachment.pgp>