On 23 July 2012 15:27, Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> These changes are required to enable no-draw-buffer-zero test case to use the shared code. Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>> --- .../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(); </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> @@ -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 </blockquote></div> My suggested changes are minor, so regardless of whether you decide to make them, this patch is: Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>>