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

[Tapani Pälli]

On 10/05/2015 10:21 AM, Ilia Mirkin wrote:
> On Mon, Oct 5, 2015 at 3:14 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.
>
> Don't you have this same problem with output attributes as well? Is
> the situation here that the packed varying pass adds its own varyings
> on top of the usual ones, and so things were getting (essentially)
> double-counted?

As far as I see the problem is only with active attribute variables, 
glGetActiveAttrib used to take in to account all resources with input 
type (as it was just calling _mesa_program_resource_find_index). Now it 
explicitly checks that varyings are not counted as active attribute 
variables.

For querying outputs via program resource queries I don't see any 
regressions on the current tests so I'm not convinced there is such problem.

For fragment shader outputs there is a problem but I consider it 
separate from this. Mesa lowers fragment output array as series of 
variables like 'gl_out_FragData0, gl_out_FragData1 ..' while user 
expects there to be 'array[0], array[1] ..' and query for array[0] does 
not match gl_out_FragData0. This one I was planning to tackle next.


>>
>> Patch fixes the issue by adding additional check to is_active_attrib
>> and iterating only active attribs (not any inputs) 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.
>>
>> v2: simplify and optimize with help of is_packed_varying bit
>>
>> 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 | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
>> index 7189676..6d54e61 100644
>> --- a/src/mesa/main/shader_query.cpp
>> +++ b/src/mesa/main/shader_query.cpp
>> @@ -108,7 +108,8 @@ is_active_attrib(const ir_variable *var)
>>
>>      switch (var->data.mode) {
>>      case ir_var_shader_in:
>> -      return var->data.location != -1;
>> +      return var->data.location != -1 &&
>> +             !var->data.is_packed_varying;
>>
>>      case ir_var_system_value:
>>         /* From GL 4.3 core spec, section 11.1.1 (Vertex Attributes):
>> @@ -153,12 +154,18 @@ _mesa_GetActiveAttrib(GLhandleARB program, GLuint desired_index,
>>         return;
>>      }
>>
>> -   struct gl_program_resource *res =
>> -      _mesa_program_resource_find_index(shProg, GL_PROGRAM_INPUT,
>> -                                        desired_index);
>> +   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(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;
>>      }
>> @@ -270,7 +277,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(RESOURCE_VAR(res))) {
>>
>>             const size_t length = strlen(RESOURCE_VAR(res)->name);
>>             if (length >= longest)
>> --
>> 2.4.3
>>