[Piglit] [PATCH] Split accuracy test to allow new multisample tests utilize this code
Anuj Phogat
anuj.phogat at gmail.com
Mon May 7 13:28:02 PDT 2012
On Mon, May 7, 2012 at 11:53 AM, Paul Berry <stereotype441 at gmail.com> wrote:
>
> 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 provided the C interface in sync with my plans to develop new
multisample test cases using C. There are no specific benefits of this
choice.
> 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.
>
I agree. That would require some extra efforts.
> 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.
>
I initially re factored using a similar approach. But later thoughts
of providing a "C" interface overwhelmed me.
I am fine with your suggestion and comfortable developing new tests in C++.
> If you're interested, I'd be willing to take a stab at this approach and send it to the list.
>
Can you modify few member functions as per testing requirements of
turn-on-off.c ([PATCH] Add test to turn on/off MSAA in a FBO) ?
e.g define test_fbo, provide an option of rendering test image to
default fb, using render buffer as color attachment for 0 sample
count, re initializing test_fbo with specified sample_count.
I am fine with making above listed changes to re factored
accuracy.cpp, if you face any issues understanding exact requirements.
> 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.
>
Yes, I could have passed them as parameters.
> (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.
I agree.
>
>>
>> +
>> +/* 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.
>
Yes, This can be added as member function with a different name.
>>
>> +
>> +/* 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().
>
We need to call Fbo::setup() separately to reinitialize an existing
Fbo (test_fbo) with a different sample count. Old Fbo::init() function
generates a new fbo handle every time it is called. I am utilizing
fbo_set_samples() function in turn-on-off.c ([PATCH] Add test to turn
on/off MSAA in a FBO).
Thanks
Anuj
More information about the Piglit
mailing list