On 6 November 2013 17:20, Anuj Phogat <anuj.pho...@gmail.com> wrote: > V2: Get rid of redundant projection matrix. > V3: Draw to a multisample texture and use it later to verify > the expected color of each sample. > Use piglit_draw_rect() and get rid of redundant code. > > Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com> > --- > tests/all.tests | 5 + > .../arb_sample_shading/execution/CMakeLists.gl.txt | 1 + > .../execution/builtin-gl-sample-id.cpp | 216 > +++++++++++++++++++++ > 3 files changed, 222 insertions(+) > create mode 100644 > tests/spec/arb_sample_shading/execution/builtin-gl-sample-id.cpp > > diff --git a/tests/all.tests b/tests/all.tests > index 883fbec..5f66e43 100644 > --- a/tests/all.tests > +++ b/tests/all.tests > @@ -1342,6 +1342,11 @@ for num_samples in TEST_SAMPLE_COUNTS: > executable = 'arb_sample_shading-{0} -auto'.format(test_name) > arb_sample_shading[test_name] = PlainExecTest(executable) > > +for num_samples in TEST_SAMPLE_COUNTS: > + test_name = 'builtin-gl-sample-id {0}'.format(num_samples) > + executable = 'arb_sample_shading-{0} -auto'.format(test_name) > + arb_sample_shading[test_name] = PlainExecTest(executable) > + > # Group ARB_debug_output > arb_debug_output = Group() > spec['ARB_debug_output'] = arb_debug_output > diff --git a/tests/spec/arb_sample_shading/execution/CMakeLists.gl.txt > b/tests/spec/arb_sample_shading/execution/CMakeLists.gl.txt > index 56fa0da..35f2905 100644 > --- a/tests/spec/arb_sample_shading/execution/CMakeLists.gl.txt > +++ b/tests/spec/arb_sample_shading/execution/CMakeLists.gl.txt > @@ -12,4 +12,5 @@ link_libraries ( > > piglit_add_executable (arb_sample_shading-api api.c) > piglit_add_executable (arb_sample_shading-builtin-gl-num-samples > builtin-gl-num-samples.cpp) > +piglit_add_executable (arb_sample_shading-builtin-gl-sample-id > builtin-gl-sample-id.cpp) > # vim: ft=cmake: > diff --git > a/tests/spec/arb_sample_shading/execution/builtin-gl-sample-id.cpp > b/tests/spec/arb_sample_shading/execution/builtin-gl-sample-id.cpp > new file mode 100644 > index 0000000..ebbb6a0 > --- /dev/null > +++ b/tests/spec/arb_sample_shading/execution/builtin-gl-sample-id.cpp > @@ -0,0 +1,216 @@ > +/* > + * Copyright © 2013 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 builtin-gl-sample-id.cpp > + * This test verifies that using gl_SampleID in fragment shader program > + * works as per ARB_sample_shading specification. > + * > + **/ > + > +#include "piglit-fbo.h" > +using namespace piglit_util_fbo; > + > +const int pattern_width = 128; const int pattern_height = 128; > + > +PIGLIT_GL_TEST_CONFIG_BEGIN > + > + config.supports_gl_compat_version = 21; > + > + config.window_width = pattern_width; > + config.window_height = pattern_height; > + config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | > PIGLIT_GL_VISUAL_RGBA; > + > +PIGLIT_GL_TEST_CONFIG_END > + > +static int num_samples; > +static unsigned prog_0, prog_1; > +static Fbo multisampled_fbo, multisampled_tex; > + > +static void > +print_usage_and_exit(char *prog_name) > +{ > + printf("Usage: %s <num_samples>\n", prog_name); > + piglit_report_result(PIGLIT_FAIL); > +} > + > +void > +compile_shader(void) > +{ > + static const char *vert = > + "#version 130\n" > + "in vec2 pos;\n" > + "void main()\n" > + "{\n" > + " gl_Position = gl_ModelViewProjectionMatrix *\n" > + " vec4(pos, 0.0, 1.0);\n" > + "}\n"; >
To interface properly with piglit_draw_rect, you really want your vertex shader to look like this: #version 130 in vec4 piglit_vertex; void main() { gl_Position = piglit_vertex; } (the reason is that piglit_draw_rect specifically looks for a vertex shader input called "piglit_vertex", and uses it if it finds it). I suspect that the code you've written has undefined behaviour, and is working by luck. You also need to link the program using piglit_link_simple_program(), piglit_build_simple_program(), piglit_link_simple_program_multiple_shaders(), or piglit_build_simple_program_multiple_shaders(). Those functions ensure that piglit_vertex gets assigned to the proper attribute slot. An additional advantage of writing the vertex shader like this is that you won't have to call piglit_ortho_projection() anymore, which means you can add: config.supports_gl_core_version = 31; to the test config. > + static const char *frag_0 = > + "#version 130\n" > + "#extension GL_ARB_sample_shading : enable\n" > + "uniform int samples;\n" > + "out vec4 out_color;\n" > + "void main()\n" > + "{\n" > + " if(samples == 0)\n" > + " out_color = vec4(0.0, 1.0, 0.0, 1.0);\n" > + " else\n" > + " out_color = vec4(0.0, float(gl_SampleID) / samples, > 0.0, 1.0);\n" > + "}\n"; > + > + static const char *frag_1 = > + "#version 130\n" > + "#extension GL_ARB_texture_multisample : require\n" > + "uniform sampler2DMS ms_tex;\n" > + "uniform int samples;\n" > + "out vec4 out_color;\n" > + "void main()\n" > + "{\n" > + " int i;\n" > + " bool pass = true;\n" > + /* Use a small tolerance value to account for the > precision > + * loss in floating point color values. > + */ > + " float color_tolerance = 0.02;\n" > + " for (i = 0; i < samples; i++) {\n" > + " vec4 sample_color =\n" > + " texelFetch(ms_tex, ivec2(gl_FragCoord.xy), i);\n" > + " float sample_id_float = sample_color.g * samples;\n" > + " int sample_id_int = int(round(sample_id_float));\n" > + " if (sample_id_int != i ||\n" > + " abs(sample_id_int - sample_id_float) > > color_tolerance)\n" > The second condition (abs(sample_id_int - sample_id_float) > color_tolerance) seems wrong for two reasons: 1. It shouldn't be necessary. Any rounding errors that happen should be far smaller than 1/samples, so when you compute int(round(sample_id_float)) it shoud always be exactly correct. 2. The expression abs(sample_id_int - sample_id_float) doesn't do anything with the expected value (i). This means that if the distance between sample_id_int and sample_id_float is ever larger than 0.02, the test will pass, even if sample_id_float is so wrong that it can't be explained by rounding error. I'd recommend simplifying the if test to just "if (sample_id_int != i)". If that causes the test to fail, then I think we should suspect a genuine bug rather than a rounding error. Unrelated: the contents of the "for" loop aren't uniformly indented, making it hard to read. > + " pass = false;\n" > + " }\n" > + "\n" > + " if(pass)\n" > + " out_color = vec4(0.0, 1.0, 0.0, 1.0);\n" > + " else\n" > + " out_color = vec4(1.0, 0.0, 0.0, 1.0);\n" > + "}\n"; > + > + /* Compile program */ > + prog_0 = glCreateProgram(); > + GLint vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vert); > + glAttachShader(prog_0, vs); > + piglit_check_gl_error(GL_NO_ERROR); > + GLint fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, frag_0); > + glAttachShader(prog_0, fs); > + glBindAttribLocation(prog_0, 0, "pos"); > Once you're using piglit_vertex in your vertex shader, it won't be necessary to make this call to glBindAttribLocation() anymore. piglit_draw_rect() will > + glLinkProgram(prog_0); > + if (!piglit_link_check_status(prog_0)) { > + piglit_report_result(PIGLIT_FAIL); > + } > + > + prog_1 = glCreateProgram(); > + vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vert); > + glAttachShader(prog_1, vs); > + piglit_check_gl_error(GL_NO_ERROR); > + fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, frag_1); > + glAttachShader(prog_1, fs); > + glBindAttribLocation(prog_1, 0, "pos"); > + glLinkProgram(prog_1); > + if (!piglit_link_check_status(prog_1)) { > + piglit_report_result(PIGLIT_FAIL); > + } > +} > + > +void > +piglit_init(int argc, char **argv) > +{ > + if (argc != 2) > + print_usage_and_exit(argv[0]); > + > + /* 1st arg: num_samples */ > + 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_extension("GL_ARB_texture_multisample"); > + piglit_require_extension("GL_ARB_sample_shading"); > + > + /* 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); > + > + piglit_ortho_projection(pattern_width, pattern_height, false); > + FboConfig msConfig(num_samples, pattern_width, pattern_height); > + multisampled_fbo.setup(msConfig); > + msConfig.attach_texture = true; > + multisampled_tex.setup(msConfig); > + > + compile_shader(); > + if (!piglit_check_gl_error(GL_NO_ERROR)) { > + piglit_report_result(PIGLIT_FAIL); > + } > +} > + > +enum piglit_result > +piglit_display() > +{ > + bool pass = true; > + int samples; > + float expected[4] = {0.0, 1.0, 0.0, 1.0}; > + > + glUseProgram(prog_0); > + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, multisampled_fbo.handle); > + glGetIntegerv(GL_SAMPLES, &samples); > + glClear(GL_COLOR_BUFFER_BIT); > + glUniform1i(glGetUniformLocation(prog_0, "samples"), samples); > + piglit_draw_rect(0, 0, pattern_width, pattern_height); > + > + glBindFramebuffer(GL_READ_FRAMEBUFFER, multisampled_fbo.handle); > + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, multisampled_tex.handle); > + glClear(GL_COLOR_BUFFER_BIT); > + glBlitFramebuffer(0, 0, > + pattern_width, pattern_height, > + 0, 0, > + pattern_width, pattern_height, > + GL_COLOR_BUFFER_BIT, > + GL_NEAREST); > It looks like you're drawing into an FBO that's backed by a multisampled renderbuffer, then blitting that to an FBO that's backed by a multisampled texture, then checking the result. Why not just drop the first FBO and draw directly into the second? > + > + glBindFramebuffer(GL_READ_FRAMEBUFFER, multisampled_tex.handle); > + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, piglit_winsys_fbo); > + glClear(GL_COLOR_BUFFER_BIT); > + if (samples == 0) { > + glBlitFramebuffer(0, 0, > + pattern_width, pattern_height, > + 0, 0, > + pattern_width, pattern_height, > + GL_COLOR_BUFFER_BIT, > + GL_NEAREST); > + } else { > + glUseProgram(prog_1); > + glUniform1i(glGetUniformLocation(prog_1, "ms_tex"), 0); > + glUniform1i(glGetUniformLocation(prog_1, "samples"), > samples); > + piglit_draw_rect_tex(0, 0, pattern_width, pattern_height, > + 0, 0, pattern_width, pattern_height); > Since your vertex shader doesn't use piglit_texcoord, you should just call piglit_draw_rect() here. > + } > + glBindFramebuffer(GL_READ_FRAMEBUFFER, piglit_winsys_fbo); > + pass = piglit_probe_rect_rgba(0, 0, pattern_width, > + pattern_width, expected) > + && pass; > + piglit_present_results(); > + return pass ? PIGLIT_PASS : PIGLIT_FAIL; > +} > -- > 1.8.1.4 > > With those changes, this patch is: Reviewed-by: Paul Berry <stereotype...@gmail.com>
_______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit