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

[Brian Paul]

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.


> +
>   #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.


> +   };
> +   const char *api_name;
>
> +   api_name = api<  Elements(api_names) ? api_names[api] : "N/A";
>      count = 0;
> -   mask = Elements(table) - 1;
> +   mask = Elements(table(api)) - 1;
>      memset(collisions, 0, sizeof collisions);
>
> -   for (i = 0; i<  Elements(table); i++) {
> -      if (!table[i])
> +   for (i = 0; i<  Elements(table(api)); i++) {
> +      if (!table(api)[i])
>            continue;
>         count++;
> -      d =&values[table[i]];
> +      d =&values[table(api)[i]];
>         hash = (d->pname * prime_factor);
>         j = 0;
>         while (1) {
> -         if (values[table[hash&  mask]].pname == d->pname)
> +         if (values[table(api)[hash&  mask]].pname == d->pname)
>               break;
>            hash += prime_step;
>            j++;
> @@ -1419,7 +1434,8 @@ print_table_stats(void)
>            collisions[10]++;
>      }
>
> -   printf("number of enums: %d (total %d)\n", count, Elements(values));
> +   printf("number of enums for %s: %d (total %ld)\n",
> +         api_name, count, Elements(values));
>      for (i = 0; i<  Elements(collisions) - 1; i++)
>         if (collisions[i]>  0)
>            printf("  %d enums with %d %scollisions\n",
> @@ -1438,10 +1454,14 @@ print_table_stats(void)
>   void _mesa_init_get_hash(struct gl_context *ctx)
>   {
>      int i, hash, index, mask;
> +   int api;
>      int api_mask = 0, api_bit;
>
> -   mask = Elements(table) - 1;
> -   api_bit = 1<<  ctx->API;
> +   api = ctx->API;
> +   init_table(api);
> +
> +   mask = Elements(table(api)) - 1;
> +   api_bit = 1<<  api;
>
>      for (i = 0; i<  Elements(values); i++) {
>         if (values[i].type == TYPE_API_MASK) {
> @@ -1454,8 +1474,8 @@ void _mesa_init_get_hash(struct gl_context *ctx)
>         hash = (values[i].pname * prime_factor)&  mask;
>         while (1) {
>            index = hash&  mask;
> -         if (!table[index]) {
> -            table[index] = i;
> +         if (!table(api)[index]) {
> +            table(api)[index] = i;
>               break;
>            }
>            hash += prime_step;
> @@ -1986,11 +2006,13 @@ find_value(const char *func, GLenum pname, void **p, union value *v)
>      struct gl_texture_unit *unit;
>      int mask, hash;
>      const struct value_desc *d;
> +   int api;
>
> -   mask = Elements(table) - 1;
> +   api = ctx->API;
> +   mask = Elements(table(api)) - 1;
>      hash = (pname * prime_factor);
>      while (1) {
> -      d =&values[table[hash&  mask]];
> +      d =&values[table(api)[hash&  mask]];
>
>         /* If the enum isn't valid, the hash walk ends with index 0,
>          * which is the API mask entry at the beginning of values[]. */