On Fri, Dec 6, 2013 at 3:04 PM, Paul Berry <stereotype...@gmail.com> wrote: > > 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>
Test works fine with the suggested changes, Thanks for the useful comments Paul. _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit