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

[Brian Paul]

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

And, candidate for the stable branches?



 > 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"


 > +      */
 > +     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?


 >        }
 >        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>