[Mesa-dev] [Mesa-stable] [PATCH] mesa: Fix querying location of nth element of an array variable

Anuj Phogat anuj.phogat at gmail.com
Tue Apr 22 16:20:20 PDT 2014


On Fri, Apr 18, 2014 at 3:41 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 04/03/2014 04:13 PM, Anuj Phogat wrote:
>> This patch changes the behavior of glGetAttribLocation(),
>> glGetFragDataLocation() and glGetFragDataIndex() functions.
>>
>> Code changes handle the cases described in following example:
>> vertex shader:
>> layout(location = 1)in vec4[4] a;
>> void main()
>> {
>> }
>>
>> fragment shader:
>> layout (location = 1) out vec4[4] c;
>> void main()
>> {
>> }
>>
>> Currently glGetAttribLocation("a") and glGetFragDataLocation("c") returns
>> 1. glGetAttribLocation("a[i]") and glGetFragDataLocation("c[i]"), where
>> i = {0, 1, 2, 3}, returns -1. But expected locations for array indices
>> are: 1, 2, 3 and 4 respectively.
>>
>> OpenGL spec doesn't say anything specific about getting the location
>> of an array element. But it looks sensible to allow such query.
>>
>> Fixes failures in Khronos OpenGL CTS tests:
>> explicit_attrib_location_room
>> draw_instanced_max_vertex_attribs
>>
>> Proprietary linux drivers of AMD and NVIDIA matches current output on
>> Mesa (i.e. returns -1).
>
> I think this is a change (clarification?) that occured when
> ARB_program_interface_query was added in OpenGL 4.3.  (Did the drivers
> you tested support OpenGL 4.3?)  Specifically, the page 345 (page 367 of
> the PDF) of the OpenGL 4.4 spec says:
Yes, OpenGL 4.3 was supported on both the drivers. Updating NVIDIA drivers
to OpenGL 4.4 matched the new behavior expected by spec.
>
>     "Otherwise, [GetAttribLocation] is equivalent to
>
>         GetProgramResourceLocation(program, PROGRAM_INPUT, name);"
>
> And page 107 (page 192 of the PDF) says:
>
>     "A string provided to GetProgramResourceLocation or
>     GetProgramResourceLocationIndex is considered to match an active
>     variable if
>
>     • the string exactly matches the name of the active variable;
>     • if the string identifies the base name of an active array, where
>       the string would exactly match the name of the variable if the
>       suffix "[0]" were appended to the string; or
>     • if the string identifies an active element of the array, where
>       the string ends with the concatenation of the "[" character, an
>       integer (with no "+" sign, extra leading zeroes, or whitespace)
>       identifying an array element, and the "]" character, the integer
>       is less than the number of active elements of the array variable,
>       and where the string would exactly match the enumerated name of
>       the array if the decimal integer were replaced with zero."
>
> We should capture this information in the commit message.
>
I think this information can be considered as a clarification for
GetAttribLocation(). I'll add this to commit message. Thanks for looking
it up Ian.

> I think this code is much more complex than necessary.  It seems like
> the inner search looks should be something like:
>
I'll work on simplifying this code. Thanks for the suggestion.
>    const char *const paren = strchr(name, '[');
>    const unsigned len = (paren != NULL) ? paren - name : strlen(name);
>    unsigned idx = 0;
>
>    ...
>
>    if (paren != NULL) {
>       /* Validate format of index. */
>       ...
>
>       idx = (unsigned) strtol(paren + 1, 10, NULL);
>    }
>
>    for (...) {
>       if (strncmp(var->name, name, len) && var->name[len] == '\0') {
>          /* This part should be refactored. */
>          if (var->type->is_array()) {
>             if (idx >= var->type->length)
>                 /* error */
>
>             /* return something sensible. */
>          } else {
>             /* Can't have an array index if variable isn't an array.
>              */
>             if (paren != NULL)
>                 /* error */
>
>             /* return something sensible. */
>          }
>       }
>    }
>
>> Cc: <mesa-stable at lists.freedesktop.org>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> ---
>>  src/mesa/main/shader_query.cpp | 76 +++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 75 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
>> index e1afe53..2256482 100644
>> --- a/src/mesa/main/shader_query.cpp
>> +++ b/src/mesa/main/shader_query.cpp
>> @@ -131,6 +131,59 @@ _mesa_GetActiveAttrib(GLhandleARB program, GLuint desired_index,
>>     _mesa_error(ctx, GL_INVALID_VALUE, "glGetActiveAttrib(index)");
>>  }
>>
>> +/* This function checks if the 'name' matches with var->name appended with
>> + * valid array index for the variable. For example the var->name = "color"
>> + * is declared as an array in shader program:
>> + * in vec4[4] color;
>> + *
>> + * Match the queried 'name' with "color[0]", "color[1]", "color[2]" and
>> + * "color[3]" to return the matched index of array.
>> + */
>> +int static inline
>> +get_matching_array_index(const ir_variable *const var,
>> +                         const char *name,
>> +                         const char *gl_func_name) {
>> +   /* Allowed array length for an input attribute or fragment shader output
>> +    * is not exceeding 100 anytime soon.
>> +    */
>> +   assert(var->type->length >= 0 && var->type->length <= 100);
>> +   GET_CURRENT_CONTEXT(ctx);
>> +
>> +   const char *str1 = var->name;
>> +   const char *str2 = "[0]";
>> +   int len1 = strlen(str1);
>> +   int len2 = strlen(str2);
>> +   char *result = (char*) malloc(len1 + len2
>> +                                 + 1 /* space for extra digit */
>> +                                 + 1 /* space for null char */);
>> +   if (result == NULL) {
>> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, gl_func_name);
>> +      return -1;
>> +   }
>> +   memcpy(result, str1, len1);
>> +   memcpy(result + len1, str2, len2 + 1);
>> +   int len = strlen(result);
>> +
>> +   for (unsigned i = 0; i < var->type->length; i++) {
>> +      /* Increment the array index */
>> +      if (i < 10)
>> +         result[len - 2] = (char)(((int)'0') + i);
>> +      else {
>> +         /* We allocated extra spaces to handle double digit indices */
>> +         result[len - 2] = (char)(((int)'0') + i / 10);
>> +         result[len - 1] = (char)(((int)'0') + i % 10);
>> +         result[len] = ']';
>> +         result[len + 1] = '\0';
>> +      }
>> +      if (strcmp(result, name) == 0) {
>> +         free(result);
>> +         return i;
>> +      }
>> +   }
>> +   free(result);
>> +   return -1;
>> +}
>> +
>>  GLint GLAPIENTRY
>>  _mesa_GetAttribLocation(GLhandleARB program, const GLcharARB * name)
>>  {
>> @@ -176,6 +229,14 @@ _mesa_GetAttribLocation(GLhandleARB program, const GLcharARB * name)
>>
>>        if (strcmp(var->name, name) == 0)
>>        return var->data.location - VERT_ATTRIB_GENERIC0;
>> +
>> +      if (var->type->length > 0) {
>> +         int index = get_matching_array_index(var,
>> +                                              (const char *) name,
>> +                                              "glGetAttribLocation()");
>> +         if (index >= 0)
>> +            return var->data.location + index - VERT_ATTRIB_GENERIC0;
>> +      }
>>     }
>>
>>     return -1;
>> @@ -338,8 +399,13 @@ _mesa_GetFragDataIndex(GLuint program, const GLchar *name)
>>            || var->data.location < FRAG_RESULT_DATA0)
>>           continue;
>>
>> -      if (strcmp(var->name, name) == 0)
>> +      if (strcmp(var->name, name) == 0
>> +          || (var->type->length > 0
>> +              && get_matching_array_index(var,
>> +                                          (const char *) name,
>> +                                          "glGetFragDataIndex()") >= 0)) {
>>           return var->data.index;
>> +      }
>>     }
>>
>>     return -1;
>> @@ -396,6 +462,14 @@ _mesa_GetFragDataLocation(GLuint program, const GLchar *name)
>>
>>        if (strcmp(var->name, name) == 0)
>>        return var->data.location - FRAG_RESULT_DATA0;
>> +
>> +      if (var->type->length > 0) {
>> +         int index = get_matching_array_index(var,
>> +                                              (const char *) name,
>> +                                              "glGetFragDataLocation()");
>> +         if (index >= 0)
>> +            return var->data.location + index - FRAG_RESULT_DATA0;
>> +      }
>>     }
>>
>>     return -1;
>>
>


More information about the mesa-dev mailing list