[Piglit] [PATCH 1/4] Add test to verify glBlitFramebuffer with non matching parameters in multisample FBOs
Anuj Phogat
anuj.phogat at gmail.com
Mon Jun 18 06:54:52 PDT 2012
On Fri, Jun 15, 2012 at 2:26 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> 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.
>
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 <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).
>
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
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20120618/7ea084de/attachment-0001.html>
More information about the Piglit
mailing list