On Fri, Jun 15, 2012 at 2:26 PM, Paul Berry <[email protected]> wrote:
> On 8 June 2012 14:41, <[email protected]> wrote: > >> From: Anuj Phogat <[email protected]> >> >> 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 <[email protected]> >> --- >> 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. > I think we can skip verifying that no blitting occurs after generating the error. > > >> + * >> + * 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. > As suggested, I will split this test in to three individual test cases. > > >> + * >> + * 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 <[email protected]> >> + */ >> + >> +#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). > Thanks for noticing this. > > 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. > I will make the relevant changes to fix this problem in the test case. > + >> +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 >> >> >
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
