[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-stable
mailing list