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

[Anuj Phogat]

On Mon, May 19, 2014 at 2:45 PM, Matt Turner <mattst88 at gmail.com> wrote:
> 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.
>
I wasn't sure if that'll generate extra instructions for one
iteration. I'll clean it
up before pushing.

> With the comments for these two patches addressed, both of these are
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>