<div dir="ltr">On 6 November 2013 17:20, Anuj Phogat <span dir="ltr"><<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">V2: Get rid of redundant projection matrix.<br>
</div>V3: Draw to a multisample texture and use it later to verify<br>
<div class="im"> the expected color of each sample.<br>
</div> Use piglit_draw_rect() and get rid of redundant code.<br>
<div class="im"><br>
Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>><br>
---<br>
tests/all.tests | 5 +<br>
.../arb_sample_shading/execution/CMakeLists.gl.txt | 1 +<br>
</div> .../execution/builtin-gl-sample-id.cpp | 216 +++++++++++++++++++++<br>
3 files changed, 222 insertions(+)<br>
<div class="im"> create mode 100644 tests/spec/arb_sample_shading/execution/builtin-gl-sample-id.cpp<br>
<br>
diff --git a/tests/all.tests b/tests/all.tests<br>
</div>index 883fbec..5f66e43 100644<br>
<div class="im">--- a/tests/all.tests<br>
+++ b/tests/all.tests<br>
@@ -1342,6 +1342,11 @@ for num_samples in TEST_SAMPLE_COUNTS:<br>
executable = 'arb_sample_shading-{0} -auto'.format(test_name)<br>
arb_sample_shading[test_name] = PlainExecTest(executable)<br>
<br>
+for num_samples in TEST_SAMPLE_COUNTS:<br>
+ test_name = 'builtin-gl-sample-id {0}'.format(num_samples)<br>
+ executable = 'arb_sample_shading-{0} -auto'.format(test_name)<br>
+ arb_sample_shading[test_name] = PlainExecTest(executable)<br>
+<br>
# Group ARB_debug_output<br>
arb_debug_output = Group()<br>
spec['ARB_debug_output'] = arb_debug_output<br>
diff --git a/tests/spec/arb_sample_shading/execution/CMakeLists.gl.txt b/tests/spec/arb_sample_shading/execution/CMakeLists.gl.txt<br>
index 56fa0da..35f2905 100644<br>
--- a/tests/spec/arb_sample_shading/execution/CMakeLists.gl.txt<br>
+++ b/tests/spec/arb_sample_shading/execution/CMakeLists.gl.txt<br>
@@ -12,4 +12,5 @@ link_libraries (<br>
<br>
piglit_add_executable (arb_sample_shading-api api.c)<br>
piglit_add_executable (arb_sample_shading-builtin-gl-num-samples builtin-gl-num-samples.cpp)<br>
+piglit_add_executable (arb_sample_shading-builtin-gl-sample-id builtin-gl-sample-id.cpp)<br>
# vim: ft=cmake:<br>
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<br>
new file mode 100644<br>
</div>index 0000000..ebbb6a0<br>
--- /dev/null<br>
+++ b/tests/spec/arb_sample_shading/execution/builtin-gl-sample-id.cpp<br>
@@ -0,0 +1,216 @@<br>
<div><div class="h5">+/*<br>
+ * Copyright © 2013 Intel Corporation<br>
+ *<br>
+ * Permission is hereby granted, free of charge, to any person obtaining a<br>
+ * copy of this software and associated documentation files (the "Software"),<br>
+ * to deal in the Software without restriction, including without limitation<br>
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
+ * and/or sell copies of the Software, and to permit persons to whom the<br>
+ * Software is furnished to do so, subject to the following conditions:<br>
+ *<br>
+ * The above copyright notice and this permission notice (including the next<br>
+ * paragraph) shall be included in all copies or substantial portions of the<br>
+ * Software.<br>
+ *<br>
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL<br>
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS<br>
+ * IN THE SOFTWARE.<br>
+ */<br>
+<br>
+/** \file builtin-gl-sample-id.cpp<br>
+ * This test verifies that using gl_SampleID in fragment shader program<br>
+ * works as per ARB_sample_shading specification.<br>
+ *<br>
+ **/<br>
+<br>
+#include "piglit-fbo.h"<br>
+using namespace piglit_util_fbo;<br>
+<br>
+const int pattern_width = 128; const int pattern_height = 128;<br>
+<br>
+PIGLIT_GL_TEST_CONFIG_BEGIN<br>
+<br>
</div></div>+ config.supports_gl_compat_version = 21;<br>
<div class="im">+<br>
+ config.window_width = pattern_width;<br>
+ config.window_height = pattern_height;<br>
+ config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;<br>
+<br>
+PIGLIT_GL_TEST_CONFIG_END<br>
+<br>
+static int num_samples;<br>
</div>+static unsigned prog_0, prog_1;<br>
+static Fbo multisampled_fbo, multisampled_tex;<br>
<div class="im">+<br>
+static void<br>
+print_usage_and_exit(char *prog_name)<br>
+{<br>
+ printf("Usage: %s <num_samples>\n", prog_name);<br>
+ piglit_report_result(PIGLIT_FAIL);<br>
+}<br>
+<br>
+void<br>
+compile_shader(void)<br>
+{<br>
+ static const char *vert =<br>
+ "#version 130\n"<br>
+ "in vec2 pos;\n"<br>
+ "void main()\n"<br>
+ "{\n"<br>
</div>+ " gl_Position = gl_ModelViewProjectionMatrix *\n"<br>
+ " vec4(pos, 0.0, 1.0);\n"<br>
+ "}\n";<br></blockquote><div><br></div><div>To interface properly with piglit_draw_rect, you really want your vertex shader to look like this:<br><br></div><div>#version 130<br></div><div>in vec4 piglit_vertex;<br>
</div><div>void main()<br>{<br></div><div> gl_Position = piglit_vertex;<br>}<br><br></div><div>(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.<br>
<br></div><div>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.<br>
</div><div><br></div><div>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:<br><br></div><div>config.supports_gl_core_version = 31;<br>
<br>to the test config.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ static const char *frag_0 =<br>
<div>+ "#version 130\n"<br>
+ "#extension GL_ARB_sample_shading : enable\n"<br>
+ "uniform int samples;\n"<br>
+ "out vec4 out_color;\n"<br>
+ "void main()\n"<br>
+ "{\n"<br>
+ " if(samples == 0)\n"<br>
+ " out_color = vec4(0.0, 1.0, 0.0, 1.0);\n"<br>
+ " else\n"<br>
+ " out_color = vec4(0.0, float(gl_SampleID) / samples, 0.0, 1.0);\n" <br></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">
+ "}\n";<br>
+<br>
</div>+ static const char *frag_1 =<br>
+ "#version 130\n"<br>
+ "#extension GL_ARB_texture_multisample : require\n"<br>
+ "uniform sampler2DMS ms_tex;\n"<br>
<div class="im">+ "uniform int samples;\n"<br>
+ "out vec4 out_color;\n"<br>
+ "void main()\n"<br>
+ "{\n"<br>
</div>+ " int i;\n"<br>
+ " bool pass = true;\n"<br>
+ /* Use a small tolerance value to account for the precision<br>
+ * loss in floating point color values.<br>
+ */<br>
+ " float color_tolerance = 0.02;\n"<br>
+ " for (i = 0; i < samples; i++) {\n"<br>
+ " vec4 sample_color =\n"<br>
+ " texelFetch(ms_tex, ivec2(gl_FragCoord.xy), i);\n"<br>
+ " float sample_id_float = sample_color.g * samples;\n"<br>
+ " int sample_id_int = int(round(sample_id_float));\n"<br>
+ " if (sample_id_int != i ||\n"<br>
+ " abs(sample_id_int - sample_id_float) > color_tolerance)\n"<br></blockquote><div><br></div>The second condition (abs(sample_id_int - sample_id_float) > color_tolerance) seems wrong for two reasons:<br>
<br></div><div class="gmail_quote">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.<br>
<br></div><div class="gmail_quote">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.<br>
<br></div><div class="gmail_quote">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.<br>
<br><br></div><div class="gmail_quote">Unrelated: the contents of the "for" loop aren't uniformly indented, making it hard to read.<br></div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ " pass = false;\n"<br>
+ " }\n"<br>
+ "\n"<br>
+ " if(pass)\n"<br>
<div class="im">+ " out_color = vec4(0.0, 1.0, 0.0, 1.0);\n"<br>
+ " else\n"<br>
</div>+ " out_color = vec4(1.0, 0.0, 0.0, 1.0);\n"<br>
+ "}\n";<br>
+<br>
<div class="im">+ /* Compile program */<br>
</div>+ prog_0 = glCreateProgram();<br>
<div class="im">+ GLint vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vert);<br>
</div>+ glAttachShader(prog_0, vs);<br>
+ piglit_check_gl_error(GL_NO_ERROR);<br>
+ GLint fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, frag_0);<br>
+ glAttachShader(prog_0, fs);<br>
+ glBindAttribLocation(prog_0, 0, "pos");<br></blockquote><div><br></div><div>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 <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ glLinkProgram(prog_0);<br>
+ if (!piglit_link_check_status(prog_0)) {<br>
+ piglit_report_result(PIGLIT_FAIL);<br>
+ }<br>
+<br>
+ prog_1 = glCreateProgram();<br>
+ vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vert);<br>
+ glAttachShader(prog_1, vs);<br>
+ piglit_check_gl_error(GL_NO_ERROR);<br>
+ fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, frag_1);<br>
+ glAttachShader(prog_1, fs);<br>
+ glBindAttribLocation(prog_1, 0, "pos");<br>
+ glLinkProgram(prog_1);<br>
+ if (!piglit_link_check_status(prog_1)) {<br>
<div class="im">+ piglit_report_result(PIGLIT_FAIL);<br>
+ }<br>
+}<br>
+<br>
</div><div class="im">+void<br>
+piglit_init(int argc, char **argv)<br>
+{<br>
+ if (argc != 2)<br>
+ print_usage_and_exit(argv[0]);<br>
+<br>
+ /* 1st arg: num_samples */<br>
+ char *endptr = NULL;<br>
+ num_samples = strtol(argv[1], &endptr, 0);<br>
+ if (endptr != argv[1] + strlen(argv[1]))<br>
+ print_usage_and_exit(argv[0]);<br>
+<br>
</div>+ piglit_require_extension("GL_ARB_texture_multisample");<br>
<div class="im">+ piglit_require_extension("GL_ARB_sample_shading");<br>
+<br>
+ /* Skip the test if num_samples > GL_MAX_SAMPLES */<br>
+ GLint max_samples;<br>
+ glGetIntegerv(GL_MAX_SAMPLES, &max_samples);<br>
+ if (num_samples > max_samples)<br>
+ piglit_report_result(PIGLIT_SKIP);<br>
+<br>
</div>+ piglit_ortho_projection(pattern_width, pattern_height, false);<br>
<div class="im">+ FboConfig msConfig(num_samples, pattern_width, pattern_height);<br>
+ multisampled_fbo.setup(msConfig);<br>
</div>+ msConfig.attach_texture = true;<br>
+ multisampled_tex.setup(msConfig);<br>
<div class="im">+<br>
+ compile_shader();<br>
+ if (!piglit_check_gl_error(GL_NO_ERROR)) {<br>
+ piglit_report_result(PIGLIT_FAIL);<br>
+ }<br>
+}<br>
+<br>
</div><div class="im">+enum piglit_result<br>
+piglit_display()<br>
+{<br>
+ bool pass = true;<br>
</div>+ int samples;<br>
+ float expected[4] = {0.0, 1.0, 0.0, 1.0};<br>
+<br>
+ glUseProgram(prog_0);<br>
+ glBindFramebuffer(GL_DRAW_FRAMEBUFFER, multisampled_fbo.handle);<br>
+ glGetIntegerv(GL_SAMPLES, &samples);<br>
+ glClear(GL_COLOR_BUFFER_BIT);<br>
+ glUniform1i(glGetUniformLocation(prog_0, "samples"), samples);<br>
+ piglit_draw_rect(0, 0, pattern_width, pattern_height);<br>
+<br>
+ glBindFramebuffer(GL_READ_FRAMEBUFFER, multisampled_fbo.handle);<br>
+ glBindFramebuffer(GL_DRAW_FRAMEBUFFER, multisampled_tex.handle);<br>
<div class="im">+ glClear(GL_COLOR_BUFFER_BIT);<br>
+ glBlitFramebuffer(0, 0,<br>
+ pattern_width, pattern_height,<br>
+ 0, 0,<br>
+ pattern_width, pattern_height,<br>
+ GL_COLOR_BUFFER_BIT,<br>
+ GL_NEAREST);<br></div></blockquote><div><br></div><div>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?<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
+<br>
</div>+ glBindFramebuffer(GL_READ_FRAMEBUFFER, multisampled_tex.handle);<br>
+ glBindFramebuffer(GL_DRAW_FRAMEBUFFER, piglit_winsys_fbo);<br>
+ glClear(GL_COLOR_BUFFER_BIT);<br>
<div class="im">+ if (samples == 0) {<br>
</div><div class="im">+ glBlitFramebuffer(0, 0,<br>
+ pattern_width, pattern_height,<br>
+ 0, 0,<br>
+ pattern_width, pattern_height,<br>
+ GL_COLOR_BUFFER_BIT,<br>
+ GL_NEAREST);<br>
</div>+ } else {<br>
+ glUseProgram(prog_1);<br>
+ glUniform1i(glGetUniformLocation(prog_1, "ms_tex"), 0);<br>
+ glUniform1i(glGetUniformLocation(prog_1, "samples"), samples);<br>
+ piglit_draw_rect_tex(0, 0, pattern_width, pattern_height,<br>
+ 0, 0, pattern_width, pattern_height);<br></blockquote><div><br></div><div>Since your vertex shader doesn't use piglit_texcoord, you should just call piglit_draw_rect() here.<br></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+ glBindFramebuffer(GL_READ_FRAMEBUFFER, piglit_winsys_fbo);<br>
<div class="im">+ pass = piglit_probe_rect_rgba(0, 0, pattern_width,<br>
+ pattern_width, expected)<br>
+ && pass;<br>
</div>+ piglit_present_results();<br>
<div class=""><div class="h5">+ return pass ? PIGLIT_PASS : PIGLIT_FAIL;<br>
+}<br>
--<br>
1.8.1.4<br>
<br>
</div></div></blockquote></div><br></div><div class="gmail_extra">With those changes, this patch is:<br><br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br></div></div>