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

[Kenneth Graunke]

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);

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);
>