[Piglit] [PATCH] Split accuracy test to allow new multisample tests utilize this code
Paul Berry
stereotype441 at gmail.com
Mon May 7 11:53:12 PDT 2012
On 4 May 2012 15:02, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> 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 <anuj.phogat at gmail.com>
>
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.
> ---
> .../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;
> +}
>
(snip)
> 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;
>
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.
> +
> +/* 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;
>
Similar question about this global.
(snip)
+
> +/*
> + * Following functions provide C interface to utilize member functions of
> + * Test class. More such functions can be defined here based on
> requirement.
> + */
>
+
> +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);
> +}
>
+
> +extern "C" bool
> +test_run()
> +{
> + return test->run();
> +}
>
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.
> +
> +/* 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);
> + }
> +}
>
This seems like it should be a member function of the Test class.
> +
> +/* 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();
> +}
>
+
> +/* 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;
> +}
>
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().
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20120507/b85c1bc0/attachment-0001.htm>
More information about the Piglit
mailing list