[Piglit] [PATCH 1/4] Add test to verify glBlitFramebuffer with non matching parameters in multisample FBOs
Paul Berry
stereotype441 at gmail.com
Fri Jun 15 14:26:59 PDT 2012
On 8 June 2012 14:41, <anuj.phogat at gmail.com> wrote:
> From: Anuj Phogat <anuj.phogat at gmail.com>
>
> This test verifies if glBlitFramebuffer() throws expected GL errors with
> multisample framebuffers and no blitting occurs in case of error.
>
> V2: Test is rewritten to utilize the functionality defined in common.cpp
> and to match the testing pattern of other blitting tests written by
> Paul Berry. Command line options are provided to choose sample count
> and color/depth/stencil buffers for testing.
>
> V3: Modified the code inline with recent changes in common.cpp to setup the
> FBO. Also made changes to avoid sRGB related failures on NVIDIA
>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
> tests/all.tests | 7 +
> .../ext_framebuffer_multisample/CMakeLists.gl.txt | 1 +
> .../non-matching-blit.cpp | 288
> ++++++++++++++++++++
> 3 files changed, 296 insertions(+), 0 deletions(-)
> create mode 100644
> tests/spec/ext_framebuffer_multisample/non-matching-blit.cpp
>
> diff --git a/tests/all.tests b/tests/all.tests
> index 129a923..463e927 100644
> --- a/tests/all.tests
> +++ b/tests/all.tests
> @@ -1356,6 +1356,13 @@ for num_samples in (2, 4, 8, 16, 32):
> ext_framebuffer_multisample[test_name] =
> PlainExecTest(executable)
>
> for num_samples in (2, 4, 8, 16, 32):
> + for buffer_type in ('color', 'depth', 'stencil'):
> + test_name = ' ' .join(['non-matching-blit',
> str(num_samples), buffer_type])
> + executable = 'ext_framebuffer_multisample-{0}
> -auto'.format(
> + test_name)
> + ext_framebuffer_multisample[test_name] =
> PlainExecTest(executable)
> +
> +for num_samples in (2, 4, 8, 16, 32):
> test_name = ' ' .join(['line-smooth', str(num_samples)])
> executable = 'ext_framebuffer_multisample-{0} -auto'.format(
> test_name)
> diff --git a/tests/spec/ext_framebuffer_multisample/CMakeLists.gl.txt
> b/tests/spec/ext_framebuffer_multisample/CMakeLists.gl.txt
> index 1660067..e18f410 100644
> --- a/tests/spec/ext_framebuffer_multisample/CMakeLists.gl.txt
> +++ b/tests/spec/ext_framebuffer_multisample/CMakeLists.gl.txt
> @@ -20,6 +20,7 @@ piglit_add_executable
> (ext_framebuffer_multisample-negative-copyteximage negativ
> piglit_add_executable (ext_framebuffer_multisample-negative-max-samples
> negative-max-samples.c)
> piglit_add_executable
> (ext_framebuffer_multisample-negative-mismatched-samples
> negative-mismatched-samples.c)
> piglit_add_executable (ext_framebuffer_multisample-negative-readpixels
> negative-readpixels.c)
> +piglit_add_executable (ext_framebuffer_multisample-non-matching-blit
> common.cpp non-matching-blit.cpp)
> piglit_add_executable (ext_framebuffer_multisample-point-smooth
> common.cpp point-smooth.cpp)
> piglit_add_executable (ext_framebuffer_multisample-polygon-smooth
> common.cpp polygon-smooth.cpp)
> piglit_add_executable (ext_framebuffer_multisample-renderbuffer-samples
> renderbuffer-samples.c)
> diff --git a/tests/spec/ext_framebuffer_multisample/non-matching-blit.cpp
> b/tests/spec/ext_framebuffer_multisample/non-matching-blit.cpp
> new file mode 100644
> index 0000000..432c26f
> --- /dev/null
> +++ b/tests/spec/ext_framebuffer_multisample/non-matching-blit.cpp
> @@ -0,0 +1,288 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +/**
> + * @file non-matching-blit.cpp
> + *
> + * This test verifies if glBlitFramebuffer() throws expected GL errors
> with
> + * multisample framebuffers.
> + *
> + * We generate FBOs with specified sample count, draw a test pattern in to
> + * them, do blitting operation and then compare test image with the
> reference
> + * image.
>
Wow, it looks like you put a lot of effort into this test, and I appreciate
your thoroughness in wanting to verify that no blitting has occurred, so
I'm sorry to say that I think we could achieve an equally effective test
with a lot less work. Here's what I have in mind:
First of all, I'm not convinced that it's necessary to check that no
blitting occurred. For one thing, we can tell by code inspection that Mesa
does all of its error checking before attempting the blit. So as long as
we verify that Mesa reported the proper error (GL_INVALID_OPERATION)
there's little danger that it went ahead and did the blit anyway.
Secondly, a properly written client app is not going to exercise the error
checking paths very often (hopefully, almost never), so making sure that
the implementation *doesn't* blit when there's an error is a lot less
critical than making sure that it blits properly when there's no error.
I realize that may not convince you; after all, if we removed the code that
verifies that no blitting has occurred, the test would be strictly weaker.
But on the other hand, it would be a lot shorter (and therefore easier to
understand, so if it ever fails, it will be easier to find and fix the Mesa
bug).
If it's really important to you to verify that no blit occurred, I'd
recommend using solid colors as your test patterns, rather than the complex
test patterns from common.cpp, so that the test does less work. In
addition to making the test simpler, this will reduce the risk of an
unrelated bug elsewhere in the GL implementation causing the test to
produce confusing results. For example, one possibility would be to do the
following:
1. Clear src_fbo to red
2. Clear dst_fbo to green
3. Try to blit src_fbo to dst_fbo and verify that the appropriate error was
generated.
4. Blit dst_fbo to resolve_fbo.
5. Verify that resolve_fbo is all green (and therefore, src_fbo wasn't
copied to dst_fbo).
Also, I don't think it's necessary to blit the final comparison images to
the screen; that's only useful when checking that MSAA rendering is correct
(so that we can diagnose bugs if it isn't). Since this test is trying to
verify that *no* blitting occurs, being able to see an image if it does
occur won't be a useful debugging aid.
Finally, I don't think it's necessary to run this test for every possible
sample count and buffer type. Eliminating these variations would let you
remove command-line parsing from the test, as well as the complications of
using a ManifestProgram.
> + *
> + * Tests following cases:
>
I would recommend splitting these into three separate tests, each testing a
single case. That way if one of the cases fails we'll be able to tell
directly from the Piglit output what went wrong. Also, the "non-matching
sample count" test needs to skip sometimes (see my comments on
"test_blit_ms_ms_non_matching_samples" below), and we still want the other
tests to run if it skips.
> + * - Blit multisample-to-multisample with non-matching sample count
> + * - Blit multisample-to-multisample with non-matching format
> + * - Blit multisample-to-multisample non-matching buffer size
>
Actually, the third item, blitting between multisample framebuffers of
non-matching buffer sizes, is ok, provided that the source and destination
rectangles of the blit are the same. I would recommend splitting this
third "non-matching buffer size" case out to its own test, and changing it
accordingly.
> + *
> + * No blitting should happen in all the above cases and the driver should
> + * report GL_INVALID_OPERATION.
> + *
> + * Left half of default framebuffer draws test image.
> + * Right half of default framebuffer draws reference image.
> + *
> + * Author: Anuj Phogat <anuj.phogat at gmail.com>
> + */
> +
> +#include "common.h"
> +
> +int piglit_width = 512; int piglit_height = 256;
> +int piglit_window_mode =
> + GLUT_DOUBLE | GLUT_RGBA | GLUT_ALPHA | GLUT_DEPTH | GLUT_STENCIL;
> +
> +const int pattern_width = 256; const int pattern_height = 256;
> +int num_samples;
> +
> +Fbo src_fbo, dst_fbo, resolve_fbo;
> +TestPattern *test_pattern = NULL;
> +ManifestProgram *manifest_program = NULL;
> +GLbitfield buffer_to_test;
> +
> +static GLenum color_formats[] = {
> + GL_RED,
> + GL_RG,
> + GL_RGB,
> + GL_ALPHA};
> +
> +void blit_fbo_to_fbo(Fbo *src, struct Fbo *dst, float scale)
> +{
> + glBindFramebufferEXT(GL_READ_FRAMEBUFFER_EXT, src->handle);
> + glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER_EXT, dst->handle);
> +
> + glBlitFramebufferEXT(0, 0,
> + src->config.width * scale,
> + src->config.height * scale,
> + 0, 0,
> + dst->config.width,
> + dst->config.height,
> + buffer_to_test, GL_NEAREST);
> +}
> +
> +void blit_fbo_to_default(Fbo *src, int x_offset, int y_offset)
> +{
> + glBindFramebufferEXT(GL_READ_FRAMEBUFFER_EXT, src->handle);
> + glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER_EXT, 0);
> +
> + /* In case of multisample FBO, glBlitFramebufferEXT invokes the
> + * multisample to single sample resolution for each pixel */
> + glBlitFramebufferEXT(0, 0,
> + src->config.width,
> + src->config.height,
> + x_offset, y_offset,
> + pattern_width + x_offset,
> + pattern_height + y_offset,
> + GL_COLOR_BUFFER_BIT, GL_NEAREST);
> +}
> +
> +static bool
> +test_blit_ms_ms(GLfloat scale)
> +{
> + bool pass = true;
> +
> + /* Clear default framebuffer */
> + glClear(GL_COLOR_BUFFER_BIT);
> +
> + float proj[4][4] = {
> + { 1, 0, 0, 0 },
> + { 0, 1, 0, 0 },
> + { 0, 0, 1, 0 },
> + { 0, 0, 0, 1 }
> + };
>
FYI, I recently did a refactor so that we don't need to duplicate these
identity matrices all over the place. You can now just pass
TestPattern::no_projection to the draw() call. Of course, if we replace
the complex test patterns with solid colors this won't be relevant.
> +
> + /* Draw the test pattern in dst_fbo. */
> + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, dst_fbo.handle);
> + dst_fbo.set_viewport();
> + glClear(buffer_to_test);
>
This call to glClear isn't technically necessary, since all of the draw()
functions begin with a call to glClear().
> + test_pattern->draw(proj);
> +
> + /* Blit dst_fbo first in to resolve_fbo and manifest in to color
> + * image if required, followed by blitting to right half of default
> + * framebuffer. This is used as a reference image.
> + */
> + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, resolve_fbo.handle);
> + resolve_fbo.set_viewport();
> + glClear(buffer_to_test);
>
+
> + blit_fbo_to_fbo(&dst_fbo, &resolve_fbo, 1.0 /* scale */);
> +
> + if (manifest_program)
> + manifest_program->run();
> +
> + blit_fbo_to_default(&resolve_fbo, pattern_width, 0);
> +
> + /* Clear the src_fbo to a default color/depth/stencil value */
> + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, src_fbo.handle);
> + src_fbo.set_viewport();
> + glClear(buffer_to_test);
> +
> + /* Blit from src_fbo to dst_fbo. scale variable is used to change
> + * the buffer size used for blitting.
> + */
> + blit_fbo_to_fbo(&src_fbo, &dst_fbo, scale);
> +
> + pass = piglit_check_gl_error(GL_INVALID_OPERATION) && pass;
> +
> + /* Resolve dst_fbo into resolve_fbo and manifest in to color image
> if
> + * required. Then blit resolve_fbo to the left half of the window
> + * system framebuffer. This is the test image.
> + */
> + blit_fbo_to_fbo(&dst_fbo, &resolve_fbo, 1.0 /* scale */);
> +
> + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, resolve_fbo.handle);
> + resolve_fbo.set_viewport();
> + if (manifest_program)
> + manifest_program->run();
> +
> + blit_fbo_to_default(&resolve_fbo, 0, 0);
> +
> + /* Check that the left and right halves of the screen match.
> + * If they don't, that means blitting operation modified
> + * dst_fbo.
> + */
> + glBindFramebuffer(GL_READ_FRAMEBUFFER, 0);
> + pass = piglit_probe_rect_halves_equal_rgba(0, 0, piglit_width,
> + piglit_height) && pass;
> +
> + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0);
> + return pass;
> +}
> +
> +
> +static bool
> +test_blit_ms_ms_non_matching_samples(void)
> +{
> + src_fbo.setup(FboConfig(num_samples / 2,
> + pattern_width,
> + pattern_height));
> + return test_blit_ms_ms(1.0 /* scale */);
> +}
>
This isn't going to work for i965/Gen6, because it only supports 4x
oversampling, so every multisampled framebuffer always has a matching
sample count, no matter what sample count you requested. A similar
situation exists today for i965/Gen7 (although soon I hope to implement 8x
oversampling for Gen7).
Here's what I would recommend doing instead for the non-matching samples
test:
1. Create a framebuffer requesting num_samples=1. The implementation is
supposed to round this up to the next available sample count, so this will
give you the minimum supported sample count.
2. Query the sample count of the framebuffer and call it N. If N ==
GL_MAX_SAMPLES, then skip the test, because this means that the
implementation only supports one possible sample count.
3. Create a second framebuffer requesting num_samples=N+1. This guarantees
that the second framebuffer will have a different sample count than the
first framebuffer.
4. Try to blit between the two framebuffers and verify that the proper
error is generated.
> +
> +static bool
> +test_blit_ms_ms_non_matching_buffer_size(void)
> +{
> + return test_blit_ms_ms(0.5 /* scale */ );
> +}
> +
> +static bool
> +test_blit_ms_ms_non_matching_formats(void)
> +{
> + GLuint i, array_size;
> + bool result = true;
> + FboConfig config_ms(num_samples, pattern_width, pattern_height);
> +
> + array_size = ARRAY_SIZE(color_formats);
> +
> + for(i = 0; i < array_size; i++) {
> + config_ms.color_internalformat = color_formats[i];
> + src_fbo.setup(config_ms);
> + result = test_blit_ms_ms(1.0 /* scale */ ) && result;
> + }
> + return result;
> +}
> +
> +enum piglit_result
> +piglit_display()
> +{
> + bool pass = true, nm_samples = true, nm_size = true, nm_formats =
> true;
> +
> + /* Blit multisample-to-multisample with non-matching sample count
> */
> + nm_samples = test_blit_ms_ms_non_matching_samples();
> + if(!nm_samples && !piglit_automatic)
> + printf("test_blit_ms_ms_non_matching_samples failed\n");
> +
> + /* Blit multisample-to-multisample with non-matching buffer size */
> + nm_size = test_blit_ms_ms_non_matching_buffer_size();
> + if(!nm_size && !piglit_automatic)
> + printf("test_blit_ms_ms_non_matching_buffer_size
> failed\n");
> +
> + if (buffer_to_test == GL_COLOR_BUFFER_BIT) {
> + /* Blit multisample-to-multisample with non-matching
> format */
> + nm_formats = test_blit_ms_ms_non_matching_formats();
> + if(!nm_formats && !piglit_automatic)
> + printf("test_blit_ms_ms_non_matching_formats
> failed\n");
> + }
> +
> + pass = piglit_check_gl_error(GL_NO_ERROR) && pass;
> +
> + if (!piglit_automatic)
> + piglit_present_results();
> +
> + pass = pass && nm_samples && nm_size && nm_formats;
> + return (pass ? PIGLIT_PASS : PIGLIT_FAIL);
> +}
> +
> +void
> +print_usage_and_exit(char *prog_name)
> +{
> + printf("Usage: %s <num_samples> <buffer_type>\n"
> + " where <buffer_type> is one of:\n"
> + " color\n"
> + " stencil\n"
> + " depth\n",
> + prog_name);
> + piglit_report_result(PIGLIT_FAIL);
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> + if (argc < 3)
> + print_usage_and_exit(argv[0]);
> + {
> + char *endptr = NULL;
> + num_samples = strtol(argv[1], &endptr, 0);
> + if (endptr != argv[1] + strlen(argv[1]))
> + print_usage_and_exit(argv[0]);
> + }
> +
> + piglit_require_gl_version(30);
> +
> + /* Skip the test if num_samples > GL_MAX_SAMPLES */
> + GLint max_samples;
> + glGetIntegerv(GL_MAX_SAMPLES, &max_samples);
> + if (num_samples > max_samples)
> + piglit_report_result(PIGLIT_SKIP);
> +
> + if (strcmp(argv[2], "color") == 0) {
> + test_pattern = new Triangles();
> + buffer_to_test = GL_COLOR_BUFFER_BIT;
> + } else if (strcmp(argv[2], "depth") == 0) {
> + test_pattern = new DepthSunburst();
> + manifest_program = new ManifestDepth();
> + buffer_to_test = GL_DEPTH_BUFFER_BIT;
> + } else if (strcmp(argv[2], "stencil") == 0) {
> + test_pattern = new StencilSunburst();
> + manifest_program = new ManifestStencil();
> + buffer_to_test = GL_STENCIL_BUFFER_BIT;
> + } else {
> + print_usage_and_exit(argv[0]);
> + }
> +
> + test_pattern->compile();
> + if (manifest_program)
> + manifest_program->compile();
> +
> + dst_fbo.setup(FboConfig(num_samples, pattern_width,
> pattern_height));
> + resolve_fbo.setup(FboConfig(0, pattern_width, pattern_height));
> +}
> --
> 1.7.7.6
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20120615/07149d7d/attachment-0001.html>
More information about the Piglit
mailing list