[Piglit] [PATCH 1/2] msaa: Make few changes to shared code to accomodate no-draw-buffer-zero test

[Paul Berry]

On 23 July 2012 15:27, Anuj Phogat <anuj.phogat at gmail.com> wrote:

> These changes are required to enable no-draw-buffer-zero test case to use
> the
> shared code.
>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
>  .../draw-buffers-common.cpp                        |   24
> +++++++++++++++----
>  1 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git
> a/tests/spec/ext_framebuffer_multisample/draw-buffers-common.cpp
> b/tests/spec/ext_framebuffer_multisample/draw-buffers-common.cpp
> index 2829405..9158a0b 100644
> --- a/tests/spec/ext_framebuffer_multisample/draw-buffers-common.cpp
> +++ b/tests/spec/ext_framebuffer_multisample/draw-buffers-common.cpp
> @@ -88,6 +88,7 @@ static int pattern_width;
>  static int pattern_height;
>
>  static bool is_buffer_zero_integer_format = false;
> +static GLenum draw_buffer_zero_format;
>
>  static const int num_components = 4; /* for RGBA formats */
>  static const int num_color_bits = 8; /* for GL_RGBA & GL_RGBA8I formats */
> @@ -440,10 +441,16 @@ compute_expected(bool sample_alpha_to_coverage,
>                         coverage[i] = 1.0;
>         }
>
> -       if (buffer_to_test == GL_COLOR_BUFFER_BIT)
> -               compute_expected_color(sample_alpha_to_coverage,
> -                                      sample_alpha_to_one,
> -                                      draw_buffer_count);
> +       if (buffer_to_test == GL_COLOR_BUFFER_BIT) {
> +               /* Don't compute expected color for color buffer zero
> +                * if no renderbuffer is attached to it.
> +                */
> +               if(draw_buffer_count ||
> +                  draw_buffer_zero_format != GL_NONE)
> +                       compute_expected_color(sample_alpha_to_coverage,
> +                                              sample_alpha_to_one,
> +                                              draw_buffer_count);
> +       }
>         else if (buffer_to_test == GL_DEPTH_BUFFER_BIT)
>                 compute_expected_depth();
>

I realize this isn't part of your patch, but it took me a while to
understand that "draw_buffer_count" means "index of the draw buffer we are
computing expected values for" and not "count of how many draw buffers
there are".  You might want to consider a follow-up patch that changes the
name of the variable to "draw_buffer_index".

Two other things that made it difficult for me to follow this change:
- Treating draw_buffer_count as a boolean in order to implicitly compare it
with zero.
- The if statement computes when the expected color *should* be computed,
and the comment says when the expected color *should not* be computed.

You might want to consider an alternative like this:

/* Don't compute expected color for color buffer zero
 * if no renderbuffer is attached to it.
 */
if (draw_buffer_count == 0 && draw_buffer_zero_format == GL_NONE)
    return;
compute_expected_color(...);

As an added benefit, this would make the similarity more evident between
this if test and the one below.


>
> @@ -461,6 +468,11 @@ probe_framebuffer_color(void)
>         int rect_height = pattern_height / num_rects;
>
>         for (int i = 0; i < num_draw_buffers; i++) {
> +               /* Don't probe color buffer zero if no renderbuffer is
> +                * attached to it.
> +                */
> +               if( i == 0 && draw_buffer_zero_format == GL_NONE)
> +                       continue;
>                 bool is_integer_operation = is_buffer_zero_integer_format
> && !i;
>
>                 if(is_integer_operation) {
> @@ -730,6 +742,7 @@ ms_fbo_and_draw_buffers_setup(int samples,
>
>         pattern_width = width;
>         pattern_height = height;
> +       draw_buffer_zero_format = color_buffer_zero_format;
>
>         /* Setup frame buffer objects with required configuration */
>         FboConfig ms_config(samples, pattern_width, pattern_height);
> @@ -752,7 +765,8 @@ ms_fbo_and_draw_buffers_setup(int samples,
>                 resolve_int_fbo.setup(resolve_config);
>                 is_buffer_zero_integer_format = true;
>         }
> -       else if (color_buffer_zero_format != GL_RGBA){
> +       else if (color_buffer_zero_format != GL_RGBA &&
> +                color_buffer_zero_format != GL_NONE) {
>                 printf("Draw buffer zero format is not"
>                        " supported by test functions.\n");
>                 piglit_report_result(PIGLIT_FAIL);
> --
> 1.7.7.6
>
>
My suggested changes are minor, so regardless of whether you decide to make
them, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20120815/c2f66c05/attachment-0001.html>