[Mesa-dev] [PATCH 2/6] meta: Rearrange shader creation in setup_glsl_generate_mipmap

[Ian Romanick]

On 10/05/2012 05:36 PM, Kenneth Graunke wrote:
> On 10/05/2012 03:56 PM, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> The diff looks weird, but this moves the code from the first 'if
>> (ctx->Const.GLSLVersion < 130)' block down into the second block.  It
>> also moves some variable decalarations closer to their use.
>>
>> NOTE: This is a candidate for the 9.0 branch.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>   src/mesa/drivers/common/meta.c | 85
>> +++++++++++++++++++++---------------------
>>   1 file changed, 43 insertions(+), 42 deletions(-)
>>
>> diff --git a/src/mesa/drivers/common/meta.c
>> b/src/mesa/drivers/common/meta.c
>> index 0c9ee59..10dc495 100644
>> --- a/src/mesa/drivers/common/meta.c
>> +++ b/src/mesa/drivers/common/meta.c
>> @@ -3047,7 +3047,6 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>>      };
>>      struct glsl_sampler *sampler;
>>      const char *vs_source;
>> -   const char *fs_template;
>>
>>      static const char *vs_int_source =
>>         "#version 130\n"
>> @@ -3070,11 +3069,41 @@ setup_glsl_generate_mipmap(struct gl_context
>> *ctx,
>>         "   out_color = texture(tex2d, texCoords.xy);\n"
>>         "}\n";
>>      char *fs_source;
>> -   const char *extension_mode;
>>      GLuint vs, fs;
>>      void *mem_ctx;
>>
>> +   /* Check if already initialized */
>> +   if (mipmap->ArrayObj == 0) {
>> +
>> +      /* create vertex array object */
>> +      _mesa_GenVertexArrays(1, &mipmap->ArrayObj);
>> +      _mesa_BindVertexArray(mipmap->ArrayObj);
>> +
>> +      /* create vertex array buffer */
>> +      _mesa_GenBuffersARB(1, &mipmap->VBO);
>> +      _mesa_BindBufferARB(GL_ARRAY_BUFFER_ARB, mipmap->VBO);
>> +
>> +      /* setup vertex arrays */
>> +      _mesa_VertexAttribPointerARB(0, 2, GL_FLOAT, GL_FALSE,
>> +                                   sizeof(struct vertex), OFFSET(x));
>> +      _mesa_VertexAttribPointerARB(1, 3, GL_FLOAT, GL_FALSE,
>> +                                   sizeof(struct vertex), OFFSET(tex));
>> +   }
>> +
>> +   /* Generate a fragment shader program appropriate for the texture
>> target */
>> +   sampler = setup_texture_sampler(target, mipmap);
>> +   assert(sampler != NULL);
>> +   if (sampler->shader_prog != 0) {
>> +      mipmap->ShaderProg = sampler->shader_prog;
>> +      return;
>> +   }
>> +
>> +   mem_ctx = ralloc_context(NULL);
>> +
>
> I would combine this with the declaration:
>
>     void *mem_ctx = ralloc_context(NULL);

There's an early return that would have to change of the ralloc_context 
was moved to the top, and the declaration can't be moved down because 
this is C.  Having them split, while annoying, seemed the safest route. 
  Right?

> Otherwise, this looks okay.  Coalescing the two if-statements seems like
> a good idea.  It does also let you kill both mistakes with a single line
> in the next patch.
>
> For the series:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> This also replaces Oliver's series.
>
>>      if (ctx->Const.GLSLVersion < 130) {
>> +      const char *fs_template;
>> +      const char *extension_mode;
>> +
>>         vs_source =
>>            "attribute vec2 position;\n"
>>            "attribute vec3 textureCoords;\n"
>> @@ -3092,7 +3121,18 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>>            "{\n"
>>            "   gl_FragColor = %s(texSampler, %s);\n"
>>            "}\n";
>> -   } else {
>> +
>> +      extension_mode = ((target == GL_TEXTURE_1D_ARRAY) ||
>> +                        (target == GL_TEXTURE_2D_ARRAY)) ?
>> +                       "require" : "disable";
>> +
>> +      fs_source = ralloc_asprintf(mem_ctx, fs_template,
>> +                                  extension_mode, sampler->type,
>> +                                  sampler->func, sampler->texcoords);
>> +   }
>> +   else {
>> +      const char *fs_template;
>> +
>>         vs_source =
>>            "#version 130\n"
>>            "in vec2 position;\n"
>> @@ -3113,46 +3153,7 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>>            "{\n"
>>            "   out_color = texture(texSampler, %s);\n"
>>            "}\n";
>> -    }
>> -
>> -   /* Check if already initialized */
>> -   if (mipmap->ArrayObj == 0) {
>> -
>> -      /* create vertex array object */
>> -      _mesa_GenVertexArrays(1, &mipmap->ArrayObj);
>> -      _mesa_BindVertexArray(mipmap->ArrayObj);
>> -
>> -      /* create vertex array buffer */
>> -      _mesa_GenBuffersARB(1, &mipmap->VBO);
>> -      _mesa_BindBufferARB(GL_ARRAY_BUFFER_ARB, mipmap->VBO);
>> -
>> -      /* setup vertex arrays */
>> -      _mesa_VertexAttribPointerARB(0, 2, GL_FLOAT, GL_FALSE,
>> -                                   sizeof(struct vertex), OFFSET(x));
>> -      _mesa_VertexAttribPointerARB(1, 3, GL_FLOAT, GL_FALSE,
>> -                                   sizeof(struct vertex), OFFSET(tex));
>> -   }
>>
>> -   /* Generate a fragment shader program appropriate for the texture
>> target */
>> -   sampler = setup_texture_sampler(target, mipmap);
>> -   assert(sampler != NULL);
>> -   if (sampler->shader_prog != 0) {
>> -      mipmap->ShaderProg = sampler->shader_prog;
>> -      return;
>> -   }
>> -
>> -   mem_ctx = ralloc_context(NULL);
>> -
>> -   if (ctx->Const.GLSLVersion < 130) {
>> -      extension_mode = ((target == GL_TEXTURE_1D_ARRAY) ||
>> -                        (target == GL_TEXTURE_2D_ARRAY)) ?
>> -                       "require" : "disable";
>> -
>> -      fs_source = ralloc_asprintf(mem_ctx, fs_template,
>> -                                  extension_mode, sampler->type,
>> -                                  sampler->func, sampler->texcoords);
>> -   }
>> -   else {
>>         fs_source = ralloc_asprintf(mem_ctx, fs_template,
>>                                     sampler->type, "vec4",
>>                                     sampler->texcoords);
>>
>