[Mesa-dev] [PATCH] Fix glGetInteger*(GL_SAMPLER_BINDING).

[Alan Hourihane]

On 03/07/13 00:12, Brian Paul wrote:
> On 03/06/2013 11:46 AM, Alan Hourihane wrote:
>> On 03/06/13 18:36, Brian Paul wrote:
>>> On 03/06/2013 11:23 AM, Alan Hourihane wrote:
>>>> If the sampler object has been deleted on another context, an
>>>> alternative context may reference the old sampler. So ensure the
>>>> sampler
>>>> object still exists.
>>>
>>> Alan, is this specified somewhere in a spec? I can't find a
>>> description of this behaviour and we don't do this for texture
>>> objects or buffer objects, etc.
>>
>> I can't see it specifically mentioned, apart from the note that when
>> deleting the sampler object it should be unbound from the texture
>> unit, and I did consider the case of buffer & texture objects whether
>> to do this there too.
>>
>> But getting the GL_SAMPLER_BINDING id when switching contexts and
>> attempting to re-bind with glBindSampler() gives a GL error, which
>> seems wrong to me.
>>
>> I checked with the NVIDIA driver and no GL error is generated.
>
> OK, sounds good.
>
> BTW, just a few minor comments on your patch:
>
>
> The subject of the patch should start with "mesa: fix glGetInteger..."
>

O.k.

> And, candidate for the stable branches?
>

Yes.

>
>
> > If the sampler object has been deleted on another context, an
> > alternative context may reference the old sampler. So ensure the 
> sampler
> > object still exists.
> >
> > Signed-off-by: Alan Hourihane <alanh at vmware.com>
> > ---
> >  src/mesa/main/get.c        |   12 +++++++++++-
> >  src/mesa/main/samplerobj.c |    2 +-
> >  src/mesa/main/samplerobj.h |    2 ++
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> > index 2399f9c..c627dd6 100644
> > --- a/src/mesa/main/get.c
> > +++ b/src/mesa/main/get.c
> > @@ -34,6 +34,7 @@
> >  #include "state.h"
> >  #include "texcompress.h"
> >  #include "framebuffer.h"
> > +#include "samplerobj.h"
> >
> >  /* This is a table driven implemetation of the glGet*v() functions.
> >   * The basic idea is that most getters just look up an int somewhere
> > @@ -827,7 +828,16 @@ find_custom_value(struct gl_context *ctx, const 
> struct valu
> > e_desc *d, union valu
> >        {
> >           struct gl_sampler_object *samp =
> > ctx->Texture.Unit[ctx->Texture.CurrentUnit].Sampler;
> > -         v->value_int = samp ? samp->Name : 0;
> > +
> > +         /*
> > +      * The sampler object may have been deleted on another context,
> > +      * so we try to lookup the sampler object before returning 
> it's Name.
>
> "its"
>

Done.

>
> > +      */
> > +     if (samp && _mesa_lookup_samplerobj(ctx, samp->Name)) {
> > +        v->value_int = samp->Name;
> > +     } else {
> > +        v->value_int = 0;
> > +     }
>
> Looks like a mix of spaces and tabs for indentation.  Can you just use 
> spaces?
>

Sure, done.

>
> >        }
> >        break;
> >     /* GL_ARB_uniform_buffer_object */
> > diff --git a/src/mesa/main/samplerobj.c b/src/mesa/main/samplerobj.c
> > index 4664cc3..5cff329 100644
> > --- a/src/mesa/main/samplerobj.c
> > +++ b/src/mesa/main/samplerobj.c
> > @@ -40,7 +40,7 @@
> >  #include "main/samplerobj.h"
> >
> >
> > -static struct gl_sampler_object *
> > +struct gl_sampler_object *
> >  _mesa_lookup_samplerobj(struct gl_context *ctx, GLuint name)
> >  {
> >     if (name == 0)
> > diff --git a/src/mesa/main/samplerobj.h b/src/mesa/main/samplerobj.h
> > index 3114257..69e3899 100644
> > --- a/src/mesa/main/samplerobj.h
> > +++ b/src/mesa/main/samplerobj.h
> > @@ -62,6 +62,8 @@ _mesa_reference_sampler_object(struct gl_context 
> *ctx,
> >        _mesa_reference_sampler_object_(ctx, ptr, samp);
> >  }
> >
> > +extern struct gl_sampler_object *
> > +_mesa_lookup_samplerobj(struct gl_context *ctx, GLuint name);
> >
> >  extern struct gl_sampler_object *
> >  _mesa_new_sampler_object(struct gl_context *ctx, GLuint name);
>
>
> Reviewed-by: Brian Paul <brianp at vmware.com>

Thanks Brian,

Alan.