On 4 May 2012 15:02, 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"> This patch splits accuracy.cpp in to three files: common.h, common.cpp and accuracy.c. common.cpp defines the functions which can be utilized to develop new multisample test cases. Functions can be utilized to: - Draw a test image to default framebuffer. - Initialize a FBO with specified samples count. - Draw a test image to FBO. - Draw a reference image. - Verify the accuracy of multisample antialiasing in FBO. Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> </blockquote><div> It seems like you've gone to a bunch of effort to provide a C interface (rather than a C++ interface) between the accuracy test and common.cpp, so that the accuracy test can be written in straight C. Can you comment on the benefits of this? I see two downsides: a. it forces us to introduce extra wrapper functions that manipulate global state, and that doesn't seem desirable. b. if we later decide to add a test that re-uses part of the common structure but not all of it (e.g. by adding a new TestPattern-derived class), we won't be able to do so easily. Unless there's something I'm missing, I would prefer if we took an approach like this to sharing code: - Move the class declarations from accuracy.cpp into common.h. - Move the member function implementations from accuracy.cpp into common.cpp. - Leave the global variable Test *test = NULL; in accuracy.cpp, so that the global state is confined to the individual test file, and isn't part of the interface between accuracy and common. - Leave accuracy.cpp as a C++ program. If you're interested, I'd be willing to take a stab at this approach and send it to the list. Other comments follow. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> --- .../ext_framebuffer_multisample/CMakeLists.gl.txt | 2 +- tests/spec/ext_framebuffer_multisample/accuracy.c | 128 ++ .../spec/ext_framebuffer_multisample/accuracy.cpp | 1430 ------------------- tests/spec/ext_framebuffer_multisample/common.cpp | 1503 ++++++++++++++++++++ tests/spec/ext_framebuffer_multisample/common.h | 51 + 5 files changed, 1683 insertions(+), 1431 deletions(-) create mode 100644 tests/spec/ext_framebuffer_multisample/accuracy.c delete mode 100644 tests/spec/ext_framebuffer_multisample/accuracy.cpp create mode 100644 tests/spec/ext_framebuffer_multisample/common.cpp create mode 100644 tests/spec/ext_framebuffer_multisample/common.h diff --git a/tests/spec/ext_framebuffer_multisample/CMakeLists.gl.txt b/tests/spec/ext_framebuffer_multisample/CMakeLists.gl.txt index c451f9f..3548074 100644 --- a/tests/spec/ext_framebuffer_multisample/CMakeLists.gl.txt +++ b/tests/spec/ext_framebuffer_multisample/CMakeLists.gl.txt @@ -11,7 +11,7 @@ link_libraries ( ${GLUT_glut_LIBRARY} ) -piglit_add_executable (ext_framebuffer_multisample-accuracy accuracy.cpp) +piglit_add_executable (ext_framebuffer_multisample-accuracy common.cpp accuracy.c) piglit_add_executable (ext_framebuffer_multisample-dlist dlist.c) piglit_add_executable (ext_framebuffer_multisample-minmax minmax.c) piglit_add_executable (ext_framebuffer_multisample-negative-copypixels negative-copypixels.c) diff --git a/tests/spec/ext_framebuffer_multisample/accuracy.c b/tests/spec/ext_framebuffer_multisample/accuracy.c new file mode 100644 index 0000000..e660714 --- /dev/null +++ b/tests/spec/ext_framebuffer_multisample/accuracy.c @@ -0,0 +1,128 @@ +/* + * 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. + */ +#include "common.h" + +/** + * \file accuracy.c + * + * Verify the accuracy of multisample antialiasing. + * + * This test utilizes the functions defined in common.cpp to verfify the + * accuracy of MSAA. + * + * The test also accepts the following flags: + * + * - "small": Causes the MSAA image to be renedered in extremely tiny + * (16x16) tiles that are then stitched together. This verifies + * that MSAA works properly on very small buffers (a critical corner + * case on i965). + * + * - "depthstencil": Causes the framebuffers to use a combined + * depth/stencil buffer (as opposed to separate depth and stencil + * buffers). On some implementations (e.g. the nVidia proprietary + * driver for Linux) this is necessary for framebuffer completeness. + * On others (e.g. i965), this is an important corner case to test. + */ +int piglit_width = 512; int piglit_height = 256; +int piglit_window_mode = GLUT_DOUBLE | GLUT_RGBA | GLUT_ALPHA; + +/* Define extern variables declared in common.cpp */ +const int pattern_width = 256; const int pattern_height = 256; +const int supersample_factor = 16; + +void +print_usage_and_exit(char *prog_name) +{ + printf("Usage: %s <num_samples> <test_type> [options]\n" + " where <test_type> is one of:\n" + " color: test downsampling of color buffer\n" + " stencil_draw: test drawing using MSAA stencil buffer\n" + " stencil_resolve: test resolve of MSAA stencil buffer\n" + " depth_draw: test drawing using MSAA depth buffer\n" + " depth_resolve: test resolve of MSAA depth buffer\n" + "Available options:\n" + " small: use a very small (16x16) MSAA buffer\n" + " depthstencil: use a combined depth/stencil buffer\n", + prog_name); + piglit_report_result(PIGLIT_FAIL); +} + +void +piglit_init(int argc, char **argv) +{ + GLint max_samples; + int i, num_samples; + bool small = false; + bool combine_depth_stencil = false; + + 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]); + } + + for (i = 3; i < argc; ++i) { + if (strcmp(argv[i], "small") == 0) { + small = true; + } else if (strcmp(argv[i], "depthstencil") == 0) { + combine_depth_stencil = true; + } else { + print_usage_and_exit(argv[0]); + } + } + + piglit_require_gl_version(30); + piglit_require_GLSL_version(130); + + /* Skip the test if num_samples > GL_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_init("color", num_samples, small, combine_depth_stencil); + } else if (strcmp(argv[2], "stencil_draw") == 0) { + test_init("stencil_draw", num_samples, small, combine_depth_stencil); + } else if (strcmp(argv[2], "stencil_resolve") == 0) { + test_init("stencil_resolve", num_samples, small, combine_depth_stencil); + } else if (strcmp(argv[2], "depth_draw") == 0) { + test_init("depth_draw", num_samples, small, combine_depth_stencil); + } else if (strcmp(argv[2], "depth_resolve") == 0) { + test_init("depth_resolve", num_samples, small, combine_depth_stencil); + } else { + print_usage_and_exit(argv[0]); + } +} + +enum piglit_result +piglit_display() +{ + enum piglit_result result = test_run() ? PIGLIT_PASS : PIGLIT_FAIL; + + piglit_present_results(); + + return result; +} </blockquote><div> (snip) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> diff --git a/tests/spec/ext_framebuffer_multisample/common.cpp b/tests/spec/ext_framebuffer_multisample/common.cpp new file mode 100644 index 0000000..e8543b0 --- /dev/null +++ b/tests/spec/ext_framebuffer_multisample/common.cpp @@ -0,0 +1,1503 @@ +/* + * 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 common.cpp + * + * This file defines the functions which can be utilized to develop new + * multisample test cases. Functions can be utilized to: + * + * - Draw a test image to default framebuffer. + * - Initialize test_fbo with specified sample count. + * - Draw a test image to test_fbo. + * - Draw a reference image. + * - Verify the accuracy of multisample antialiasing in FBO. + * + * Accuracy verification is done by rendering a scene consisting of + * triangles that aren't perfectly aligned to pixel coordinates. Every + * triangle in the scene is rendered using a solid color whose color + * components are all 0.0 or 1.0. The scene is renederd in two ways: + * + * - At normal resoluation, using MSAA. + * + * - At very high resolution ("supersampled" by a factor of 16 in both + * X and Y dimensions), without MSAA. + * + * Then, the supersampled image is scaled down to match the resolution + * of the MSAA image, using a fragment shader to manually blend each + * block of 16x16 pixels down to 1 pixel. This produces a reference + * image, which is then compared to the MSAA image to measure the + * error introduced by MSAA. + * + * (Note: the supersampled image is actually larger than the maximum + * texture size that GL 3.0 requires all implementations to support + * (1024x1024), so it is actually done in 1024x1024 tiles that are + * then stitched together to form the reference image). + * + * In the piglit window, the MSAA image appears on the left; the + * reference image is on the right. + * + * For each color component of each pixel, if the reference image has + * a value of exactly 0.0 or 1.0, that pixel is presumed to be + * completely covered by a triangle, so the test verifies that the + * corresponding pixel in the MSAA image is exactly 0.0 or 1.0. Where + * the reference image has a value between 0.0 and 1.0, we know there + * is a triangle boundary that MSAA should smooth out, so the test + * estimates the accuracy of MSAA rendering by computing the RMS error + * between the reference image and the MSAA image for these pixels. + * + * In addition to the above test (the "color" test), there are functions + * which can also verify the proper behavior of the stencil MSAA buffer. + * This can be done in two ways: + * + * - "stencil_draw" test: after drawing the scene, we clear the MSAA + * color buffer and run a "manifest" pass which uses stencil + * operations to make a visual representation of the contents of the + * stencil buffer show up in the color buffer. The rest of the test + * operates as usual. This allows us to verify that drawing + * operations that use the stencil buffer operate correctly in MSAA + * mode. + * + * - "stencil_resolve" test: same as above, except that we blit the + * MSAA stencil buffer to a single-sampled FBO before running the + * "manifest" pass. This allows us to verify that the + * implementation properly downsamples the MSAA stencil buffer. + * + * There are similar variants "depth_draw" and "depth_resolve" for + * testing the MSAA depth buffer. + * + * Note that when downsampling the MSAA color buffer, implementations + * are expected to blend the values of each of the color samples; + * but when downsampling the stencil and depth buffers, they are + * expected to just choose one representative sample (this is because + * an intermediate stencil or depth value would not be meaningful). + * Therefore, the pass threshold is relaxed for the "stencil_resolve" + * and "depth_resolve" tests. + * + * Functions also accepts the following flags: + * + * - "small": Causes the MSAA image to be renedered in extremely tiny + * (16x16) tiles that are then stitched together. This verifies + * that MSAA works properly on very small buffers (a critical corner + * case on i965). + * + * - "depthstencil": Causes the framebuffers to use a combined + * depth/stencil buffer (as opposed to separate depth and stencil + * buffers). On some implementations (e.g. the nVidia proprietary + * driver for Linux) this is necessary for framebuffer completeness. + * On others (e.g. i965), this is an important corner case to test. + */ + +#include "common.h" + +/* Following extern variables must be defined in piglit test file */ +extern const int pattern_width; +extern const int pattern_height; +extern const int supersample_factor; + +/* Set this flag while calling draw_test_image() in piglit test code to render + * the test image in to default framebuffer. Flag automatically reset to + * false after draw_test_image(). + */ +bool render_to_default = false; </blockquote><div> Can you comment on why you chose to make this a global variable instead of a parameter to draw_test_image()? It seems like this sort of "action at a distance" is going to make the code unnecessarily difficult to follow. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +/* Set this flag while calling fbo_set_samples() in piglit test code to use + * a render buffer as color attachment in test_fbo in place of a texture + * for zero sample count. Flag automatically reset to false after + * fbo_set_samples(). This flag is insignificant for an FBO with sample + * count > 0 + */ +bool attach_render_buffer = false; </blockquote><div> Similar question about this global. (snip) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +/* + * Following functions provide C interface to utilize member functions of + * Test class. More such functions can be defined here based on requirement. + */ </blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +extern "C" void +test_init(const char *str, int n_samples, bool small, bool combine_depth_stencil) +{ + if (strcmp(str, "color") == 0) { + test = new Test(new Triangles(), NULL, false, 0); + } else if (strcmp(str, "stencil_draw") == 0) { + test = new Test(new StencilSunburst(), + new ManifestStencil(), + false, 0); + } else if (strcmp(str, "stencil_resolve") == 0) { + test = new Test(new StencilSunburst(), + new ManifestStencil(), + true, + GL_STENCIL_BUFFER_BIT); + } else if (strcmp(str, "depth_draw") == 0) { + test = new Test(new DepthSunburst(), + new ManifestDepth(), + false, 0); + } else if (strcmp(str, "depth_resolve") == 0) { + test = new Test(new DepthSunburst(), + new ManifestDepth(), + true, + GL_DEPTH_BUFFER_BIT); + } + + test->init(n_samples, small, combine_depth_stencil); +} </blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +extern "C" bool +test_run() +{ + return test->run(); +} </blockquote><div> This is an example of the kind of extra effort and unnecessary global state I am concerned about. It seems like the code would be easier to follow if we got rid of this function and had accuracy.cpp simply call test->run() directly. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +/* This function draws test image to default framebuffer or to + * test_fbo based on state of render_to_default flag + */ +extern "C" void +draw_test_image(bool default_fb) +{ + if (default_fb) { + render_to_default = true; + test->draw_test_image(NULL); + render_to_default = false; + glBindFramebufferEXT(GL_READ_FRAMEBUFFER_EXT, 0); + } + else { + test->draw_test_image(&(test->test_fbo)); + glBindFramebufferEXT(GL_READ_FRAMEBUFFER_EXT, + test->test_fbo.handle); + } +} </blockquote><div> This seems like it should be a member function of the Test class. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +/* This function draws a reference image blitted in to right half of + * default framebuffer*/ +extern "C" void +draw_reference_image(void) +{ + test->draw_reference_image(); +} + +/* This function verify the accuracy of MSAA by comparing the left half + * and right half of default framebuffer + */ +extern "C" bool +measure_accuracy(void) +{ + return test->measure_accuracy(); +} </blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + +/* This function allows to re initialize test_fbo with specified + * sample count + */ +extern "C" void +fbo_set_samples(int n_samples, bool use_render_buffer, + bool small, bool combine_depth_stencil) +{ + if (use_render_buffer) + attach_render_buffer = true; + glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER_EXT, + test->test_fbo.handle); + test->test_fbo.setup(n_samples, + small ? 16 : pattern_width, + small ? 16 : pattern_height, + combine_depth_stencil); + attach_render_buffer = false; +} </blockquote><div> I'm not sure why this refactor is helpful. The old Fbo::init() function did what Fbo::setup() does, except that it called glBindFramebuffer() first. If we restore Fbo::init(), and then make attach_render_buffer a parameter to it, it seems like we could eliminate this function entirely and just have the accuracy test call test->test_fbo.init(). </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> diff --git a/tests/spec/ext_framebuffer_multisample/common.h b/tests/spec/ext_framebuffer_multisample/common.h new file mode 100644 index 0000000..316e1d9 --- /dev/null +++ b/tests/spec/ext_framebuffer_multisample/common.h @@ -0,0 +1,51 @@ +/* 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 common.h + * This file declares functions which can be utilized to develop new multisample + * test cases. + */ + +#include "piglit-util.h" +#include "math.h" + +#ifdef __cplusplus +extern "C" +{ +#endif +void draw_test_image(bool default_fb); + +void draw_reference_image(void); + +void fbo_set_samples(int n_samples, bool use_render_buffer, + bool small, bool combine_depth_stencil); + +bool measure_accuracy(void); + +bool test_run(void); + +void test_init(const char *str, int n_samples, bool small, + bool combine_depth_stencil); +#ifdef __cplusplus +} +#endif -- 1.7.7.6 </blockquote></div>