[Mesa-dev] [PATCH 2/2] meta: Write color values in to 'out' variables for all the draw buffers

[Matt Turner]

On Mon, May 19, 2014 at 1:20 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> _mesa_meta_setup_blit_shader() currently generates a fragment shader
> which, irrespective of the number of draw buffers, writes the color
> to only one output variable. Current shader rely on an undefined
> behavior and possibly works by chance.
>
> From OpenGL 4.0  spec, page 256:
>   "If a fragment shader writes to gl_FragColor, DrawBuffers specifies a
>    set of draw buffers into which the single fragment color defined by
>    gl_FragColor is written. If a fragment shader writes to gl_FragData,
>    or a user-defined varying out variable, DrawBuffers specifies a set
>    of draw buffers into which each of the multiple output colors defined
>    by these variables are separately written. If a fragment shader writes
>    to none of gl_FragColor, gl_FragData, nor any user defined varying out
>    variables, the values of the fragment colors following shader execution
>    are undefined, and may differ for each fragment color."
>
> OpenGL 4.4 spec, page 463, added an additional line in this section:
>   "If some, but not all user-defined output variables are written, the
>    values of fragment colors corresponding to unwritten variables are
>    similarly undefined."
>
> Cc: <mesa-stable at lists.freedesktop.org>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
>  src/mesa/drivers/common/meta.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
> index 87609b4..4897cd9 100644
> --- a/src/mesa/drivers/common/meta.c
> +++ b/src/mesa/drivers/common/meta.c
> @@ -247,7 +247,8 @@ _mesa_meta_setup_blit_shader(struct gl_context *ctx,
>     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 = "";
> +   const char *fs_output_decl = "", *for_loop = "";
> +   const int draw_buf_count = ctx->DrawBuffer->_NumColorDrawBuffers;
>
>     if (ctx->Const.GLSLVersion < 130) {
>        vs_input = "attribute";
> @@ -255,12 +256,23 @@ _mesa_meta_setup_blit_shader(struct gl_context *ctx,
>        fs_preprocess = "#extension GL_EXT_texture_array : enable";
>        fs_output = "gl_FragColor";
>     } else {
> -      vs_preprocess = fs_preprocess = "#version 130";
> +      vs_preprocess = "#version 130";
>        vs_input = fs_input = "in";
>        vs_output = "out";
> -      fs_output = "out_color";
> -      fs_output_decl = "out vec4 out_color;";
>        shader->func = "texture";
> +      if (draw_buf_count > 1) {
> +         fs_preprocess = ralloc_asprintf(mem_ctx,
> +                                         "#version 130\n"
> +                                         "#define NUM_DRAW_BUFS %d",
> +                                         draw_buf_count);
> +         fs_output = "out_color[i]";
> +         fs_output_decl = "out vec4 out_color[NUM_DRAW_BUFS];";
> +         for_loop = "   for (int i = 0; i < NUM_DRAW_BUFS; i++)\n   ";
> +      } else {
> +         fs_preprocess = "#version 130";
> +         fs_output = "out_color";
> +         fs_output_decl = "out vec4 out_color;";
> +      }

It's safe to emit a loop with only one iterations. The compiler will
happily optimize that (it's going to unroll all of these loops
anyway). Emitting GLSL code for the for loop unconditionally seems
like it would clean this up some.

With the comments for these two patches addressed, both of these are

Reviewed-by: Matt Turner <mattst88 at gmail.com>