[Mesa-dev] [PATCH 1/2] meta: Refactor _mesa_meta_setup_blit_shader() to avoid duplicate shader code

[Matt Turner]

On Mon, May 19, 2014 at 1:20 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> Cc: <mesa-stable at lists.freedesktop.org>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
>  src/mesa/drivers/common/meta.c | 97 +++++++++++++++++++-----------------------
>  1 file changed, 44 insertions(+), 53 deletions(-)
>
> diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
> index 3ef3f79..87609b4 100644
> --- a/src/mesa/drivers/common/meta.c
> +++ b/src/mesa/drivers/common/meta.c
> @@ -242,10 +242,26 @@ _mesa_meta_setup_blit_shader(struct gl_context *ctx,
>                               GLenum target,
>                               struct blit_shader_table *table)
>  {
> -   const char *vs_source;
> -   char *fs_source;
> +   char *vs_source, *fs_source;
>     void *const mem_ctx = ralloc_context(NULL);
>     struct blit_shader *shader = choose_blit_shader(target, table);
> +   const char *vs_input, *vs_output, *fs_input, *fs_output;
> +   const char *vs_preprocess = "", *fs_preprocess = "";
> +   const char *fs_output_decl = "";
> +
> +   if (ctx->Const.GLSLVersion < 130) {
> +      vs_input = "attribute";
> +      vs_output = fs_input = "varying";
> +      fs_preprocess = "#extension GL_EXT_texture_array : enable";
> +      fs_output = "gl_FragColor";
> +   } else {
> +      vs_preprocess = fs_preprocess = "#version 130";
> +      vs_input = fs_input = "in";
> +      vs_output = "out";
> +      fs_output = "out_color";

vs_output means "vertex shader output keyword" but fs_output means
"fragment shader output variable". Maybe change
s/fs_output/fs_output_var/?

> +      fs_output_decl = "out vec4 out_color;";
> +      shader->func = "texture";
> +   }

This block would be clearer if we assigned the same variables in the
same order. Instead of initializing variables, I'd set them in both
blocks. Multiple assignments on the same line also make it less
obviously correct.

>
>     assert(shader != NULL);
>
> @@ -254,57 +270,32 @@ _mesa_meta_setup_blit_shader(struct gl_context *ctx,
>        return;
>     }
>
> -   if (ctx->Const.GLSLVersion < 130) {
> -      vs_source =
> -         "attribute vec2 position;\n"
> -         "attribute vec4 textureCoords;\n"
> -         "varying vec4 texCoords;\n"
> -         "void main()\n"
> -         "{\n"
> -         "   texCoords = textureCoords;\n"
> -         "   gl_Position = vec4(position, 0.0, 1.0);\n"
> -         "}\n";
> -
> -      fs_source = ralloc_asprintf(mem_ctx,
> -                                  "#extension GL_EXT_texture_array : enable\n"
> -                                  "#extension GL_ARB_texture_cube_map_array: enable\n"
> -                                  "uniform %s texSampler;\n"
> -                                  "varying vec4 texCoords;\n"
> -                                  "void main()\n"
> -                                  "{\n"
> -                                  "   gl_FragColor = %s(texSampler, %s);\n"
> -                                  "   gl_FragDepth = gl_FragColor.x;\n"
> -                                  "}\n",
> -                                  shader->type,
> -                                  shader->func, shader->texcoords);
> -   }
> -   else {
> -      vs_source = ralloc_asprintf(mem_ctx,
> -                                  "#version 130\n"
> -                                  "in vec2 position;\n"
> -                                  "in vec4 textureCoords;\n"
> -                                  "out vec4 texCoords;\n"
> -                                  "void main()\n"
> -                                  "{\n"
> -                                  "   texCoords = textureCoords;\n"
> -                                  "   gl_Position = vec4(position, 0.0, 1.0);\n"
> -                                  "}\n");
> -      fs_source = ralloc_asprintf(mem_ctx,
> -                                  "#version 130\n"
> -                                  "#extension GL_ARB_texture_cube_map_array: enable\n"
> -                                  "uniform %s texSampler;\n"
> -                                  "in vec4 texCoords;\n"
> -                                  "out vec4 out_color;\n"
> -                                  "\n"
> -                                  "void main()\n"
> -                                  "{\n"
> -                                  "   out_color = texture(texSampler, %s);\n"
> -                                  "   gl_FragDepth = out_color.x;\n"
> -                                  "}\n",
> -                                  shader->type,
> -                                  shader->texcoords);
> -   }
> -
> +   vs_source = ralloc_asprintf(mem_ctx,
> +                "%s\n"
> +                "%s vec2 position;\n"
> +                "%s vec4 textureCoords;\n"
> +                "%s vec4 texCoords;\n"
> +                "void main()\n"
> +                "{\n"
> +                "   texCoords = textureCoords;\n"
> +                "   gl_Position = vec4(position, 0.0, 1.0);\n"
> +                "}\n",
> +                vs_preprocess, vs_input, vs_input, vs_output);
> +
> +   fs_source = ralloc_asprintf(mem_ctx,
> +                "%s\n"
> +                "#extension GL_ARB_texture_cube_map_array: enable\n"
> +                "uniform %s texSampler;\n"
> +                "%s vec4 texCoords;\n"
> +                "%s\n"
> +                "void main()\n"
> +                "{\n"
> +                "   vec4 color = %s(texSampler, %s);\n"
> +                "   %s = color;\n"
> +                "   gl_FragDepth = color.x;\n"
> +                "}\n",
> +                fs_preprocess, shader->type, fs_input, fs_output_decl,
> +                shader->func, shader->texcoords, fs_output);
>
>     _mesa_meta_compile_and_link_program(ctx, vs_source, fs_source,
>                                         ralloc_asprintf(mem_ctx, "%s blit",
> --
> 1.8.3.1