[Mesa-dev] [PATCH v2 06/12] mesa: glGet: fix parameter lookup for apps using multiple APIs

[Imre Deak]

On Tue, 2012-09-11 at 09:40 -0600, Brian Paul wrote:
> On 09/10/2012 12:41 AM, Imre Deak wrote:
> > The glGet hash was initialized only once for a single GL API, even if
> > the application later created a context for a different API. This
> > resulted in glGet failing for otherwise valid parameters in a context
> > if that parameter was invalid in another context created earlier.
> >
> > Fix this by using a separate hash table for each API.
> >
> > Signed-off-by: Imre Deak<imre.deak at intel.com>
> > ---
> >   src/mesa/main/context.c |    5 ++---
> >   src/mesa/main/get.c     |   50 ++++++++++++++++++++++++++++++++++-------------
> >   2 files changed, 38 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> > index 1167ea5..77c7ef0 100644
> > --- a/src/mesa/main/context.c
> > +++ b/src/mesa/main/context.c
> > @@ -404,9 +404,6 @@ one_time_init( struct gl_context *ctx )
> >
> >         _mesa_get_cpu_features();
> >
> > -      /* context dependence is never a one-time thing... */
> > -      _mesa_init_get_hash(ctx);
> > -
> >         for (i = 0; i<  256; i++) {
> >            _mesa_ubyte_to_float_color_tab[i] = (float) i / 255.0F;
> >         }
> > @@ -425,6 +422,8 @@ one_time_init( struct gl_context *ctx )
> >
> >      /* per-API one-time init */
> >      if (!(api_init_mask&  (1<<  ctx->API))) {
> > +      _mesa_init_get_hash(ctx);
> > +
> >         /*
> >          * This is fine as ES does not use the remap table, but it may not be
> >          * future-proof.  We cannot always initialize the remap table because
> > diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> > index 6800cc3..63fe296 100644
> > --- a/src/mesa/main/get.c
> > +++ b/src/mesa/main/get.c
> > @@ -1385,29 +1385,44 @@ static const struct value_desc values[] = {
> >    * collisions for any enum (typical numbers).  And the code is very
> >    * simple, even though it feels a little magic. */
> >
> > -static unsigned short table[1024];
> > +typedef unsigned short table_t[1024];
> > +static table_t *table_set[API_NUM];
> > +#define table(api) (*table_set[api])
> >   static const int prime_factor = 89, prime_step = 281;
> >
> > +static void init_table(int api)
> > +{
> > +   /* The following will be freed only implicitly at process exit. */
> > +   table_set[api] = calloc(1, sizeof(*table_set[api]));
> > +   assert(table_set[api]);
> > +}
> 
> I'm not sure this has to be separate function since it's only called 
> from one place and is only two lines of code.
> 
> And actually, we could probably just declare the table(s) as:
> 
> static unsigned short table[API_NUM][1024];
> 
> It's only 8KB and would avoid the calloc-but-no-free issue.

Agreed, it makes things also simpler and faster.

Note that in the build time generated version later in the patchset I'm
still using the above array of pointers, so that APIs with identical
enum lists (atm. GL and GL_CORE) can share a single hash table.

> > +
> >   #ifdef GET_DEBUG
> >   static void
> > -print_table_stats(void)
> > +print_table_stats(int api)
> >   {
> >      int i, j, collisions[11], count, hash, mask;
> >      const struct value_desc *d;
> > +   const char *api_names[] = {
> > +      [API_OPENGL] = "GL", [API_OPENGL_CORE] = "GL_CORE",
> > +      [API_OPENGLES] = "GLES", [API_OPENGLES2] = "GLES2",
> 
> Let's put each of these four entries on one line for better readability.

Ok.

--Imre