[Mesa-dev] [PATCH] mesa: fix GetProgramiv/GetActiveAttrib regression

[Ilia Mirkin]

On Mon, Sep 28, 2015 at 5:12 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
> Commit 4639cea2921669527eb43dcb49724c05afb27e8e added packed varyings
> as part of program resource list. We need to take this to account with
> all functions dealing with active input attributes.
>
> Patch fixes the issue by adding additional check to is_active_attrib
> and iterating active attribs explicitly in GetActiveAttrib function.
>
> I've tested that fix works for Assault Android Cactus (demo version)
> and does not cause Piglit or CTS regressions in glGetProgramiv tests.
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92122
> ---
>  src/mesa/main/shader_query.cpp | 44 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index 99d9e10..d023504 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -101,15 +101,34 @@ _mesa_BindAttribLocation(GLhandleARB program, GLuint index,
>      */
>  }
>
> +/* Function checks if given input variable is packed varying by checking
> + * if there also exists output variable with the same name.
> + */
> +static bool
> +is_packed_varying(struct gl_shader_program *shProg, const ir_variable *input)
> +{
> +   assert(input->data.mode == ir_var_shader_in);

No statements before variable decls in core code.

> +   unsigned array_index = 0;
> +   struct gl_program_resource *out =
> +      _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT, input->name,
> +                                       &array_index);

This is slow, right? I.e. it's a loop?

> +   if (out)
> +      return true;
> +   return false;
> +}
> +
>  static bool
> -is_active_attrib(const ir_variable *var)
> +is_active_attrib(struct gl_shader_program *shProg,
> +                 const ir_variable *var)
>  {
>     if (!var)
>        return false;
>
>     switch (var->data.mode) {
>     case ir_var_shader_in:
> -      return var->data.location != -1;
> +      return (var->data.location != -1 &&
> +              var->data.location_frac == 0 &&
> +              !is_packed_varying(shProg, var));
>
>     case ir_var_system_value:
>        /* From GL 4.3 core spec, section 11.1.1 (Vertex Attributes):
> @@ -154,19 +173,25 @@ _mesa_GetActiveAttrib(GLhandleARB program, GLuint desired_index,
>        return;
>     }
>
> -   struct gl_program_resource *res =
> -      _mesa_program_resource_find_index(shProg, GL_PROGRAM_INPUT,
> -                                        desired_index);

You appear to replace this function, which is O(n), with a loop around
what is essentially this same function, which is O(n^2). Could you
write a more targeted helper which achieves what you want with a
single scan of the program resource list?

> +   struct gl_program_resource *res = shProg->ProgramResourceList;
> +   unsigned index = -1;
> +   for (unsigned i = 0; i < shProg->NumProgramResourceList; i++, res++) {
> +      if (res->Type != GL_PROGRAM_INPUT ||
> +          !is_active_attrib(shProg, RESOURCE_VAR(res)))
> +         continue;
> +      if (++index == desired_index)
> +         break;
> +   }
>
>     /* User asked for index that does not exist. */
> -   if (!res) {
> +   if (!res || index != desired_index) {
>        _mesa_error(ctx, GL_INVALID_VALUE, "glGetActiveAttrib(index)");
>        return;
>     }
>
>     const ir_variable *const var = RESOURCE_VAR(res);
>
> -   if (!is_active_attrib(var))
> +   if (!is_active_attrib(shProg, var))
>        return;
>
>     const char *var_name = var->name;
> @@ -252,7 +277,7 @@ _mesa_count_active_attribs(struct gl_shader_program *shProg)
>     for (unsigned j = 0; j < shProg->NumProgramResourceList; j++, res++) {
>        if (res->Type == GL_PROGRAM_INPUT &&
>            res->StageReferences & (1 << MESA_SHADER_VERTEX) &&
> -          is_active_attrib(RESOURCE_VAR(res)))
> +          is_active_attrib(shProg, RESOURCE_VAR(res)))
>           count++;
>     }
>     return count;
> @@ -271,7 +296,8 @@ _mesa_longest_attribute_name_length(struct gl_shader_program *shProg)
>     size_t longest = 0;
>     for (unsigned j = 0; j < shProg->NumProgramResourceList; j++, res++) {
>        if (res->Type == GL_PROGRAM_INPUT &&
> -          res->StageReferences & (1 << MESA_SHADER_VERTEX)) {
> +          res->StageReferences & (1 << MESA_SHADER_VERTEX) &&
> +          is_active_attrib(shProg, RESOURCE_VAR(res))) {
>
>            const size_t length = strlen(RESOURCE_VAR(res)->name);
>            if (length >= longest)
> --
> 2.4.3
>