<div dir="ltr">On 16 June 2013 12:12, Fabian Bieler <span dir="ltr"><<a href="mailto:fabianbieler@fastmail.fm" target="_blank">fabianbieler@fastmail.fm</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">Test that glProgramParameter() triggers the required errors.<br>
<br>
The value of GEOMETRY_VERTICES_OUT is limited by the implementation<br>
dependend constants MAX_GEOMETRY_OUTPUT_VERTICES and<br>
MAX_VERTEX_VARYING_COMPONENTS.<br>
<br>
>From the ARB_geometry_shader4 spec (section Errors):<br>
"The error INVALID_VALUE is generated by ProgramParameteriARB if <pname> is<br>
GEOMETRY_VERTICES_OUT_ARB and <value> is negative.<br>
<br>
The error INVALID_VALUE is generated by ProgramParameteriARB if <pname> is<br>
GEOMETRY_VERTICES_OUT_ARB and <value> exceeds<br>
MAX_GEOMETRY_OUTPUT_VERTICES_ARB.<br>
<br>
The error INVALID_VALUE is generated by ProgramParameteriARB if <pname> is<br>
set to GEOMETRY_VERTICES_OUT_ARB and the product of <value> and the sum of<br>
all components of all active varying variables exceeds<br>
MAX_GEOMETRY_TOTAL_OUTPUT_COMPONENTS_ARB."<br></div></blockquote><div><br>There's an inconsistency in the spec here. Earlier in the spec, it says that MAX_GEOMETRY_TOTAL_OUTPUT_COMPONENTS_ARB is checked at link time, not at the time of the call to ProgramParameteriARB(). The OpenGL 3.2 and 4.3 specs also agree that this limit is checked at link time.<br>
<br>I think the spec writers just made a copy-paste error here--it's crazy to check MAX_GEOMETRY_TOTAL_OUTPUT_COMPONENTS_ARB at the time of the call to ProgramParameteriARB(), because at that time, it's possible that the geometry shader hasn't even been attached yet.<br>
<br>On an unrelated note, in general comments like the ones above should go at the top of the .c file rather than in the commit message. That way if someone gets a test failure later, and goes to the .c file to investigate why, they will see the relevant spec text rather than having to dig through git history.<br>
<br></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>v2: Don't count gl_Position against MAX_GEOMETRY_TOTAL_OUTPUT_COMPONENTS.<br>
<div class="im">---<br>
tests/all.tests | 2 +<br>
.../execution/program-parameter/CMakeLists.gl.txt | 1 +<br>
.../execution/program-parameter/vertices-out.c | 209 +++++++++++++++++++++<br>
3 files changed, 212 insertions(+)<br>
create mode 100644 tests/spec/arb_geometry_shader4/execution/program-parameter/vertices-out.c<br>
<br>
diff --git a/tests/all.tests b/tests/all.tests<br>
</div>index 0d47323..0bb7f24 100644<br>
--- a/tests/all.tests<br>
+++ b/tests/all.tests<br>
@@ -2316,6 +2316,8 @@ add_plain_test(arb_map_buffer_alignment, 'arb_map_buffer_alignment-sanity_test')<br>
<div class="im"> arb_geometry_shader4 = Group()<br>
add_concurrent_test(arb_geometry_shader4, 'program-parameter-input-type')<br>
add_concurrent_test(arb_geometry_shader4, 'program-parameter-output-type')<br>
</div>+for mode in ['1', 'tf 1', '4', 'tf 4', '32', 'tf 32']:<br>
<div class="im">+ add_concurrent_test(arb_geometry_shader4, 'program-parameter-vertices-out {0}'.format(mode))<br>
spec['ARB_geometry_shader4'] = arb_geometry_shader4<br>
add_shader_test_dir(spec['ARB_geometry_shader4'],<br>
os.path.join(testsDir, 'spec', 'arb_geometry_shader4'),<br>
diff --git a/tests/spec/arb_geometry_shader4/execution/program-parameter/CMakeLists.gl.txt b/tests/spec/arb_geometry_shader4/execution/program-parameter/CMakeLists.gl.txt<br>
index aee7a98..5768ead 100644<br>
--- a/tests/spec/arb_geometry_shader4/execution/program-parameter/CMakeLists.gl.txt<br>
+++ b/tests/spec/arb_geometry_shader4/execution/program-parameter/CMakeLists.gl.txt<br>
@@ -12,3 +12,4 @@ link_libraries (<br>
<br>
piglit_add_executable (program-parameter-input-type input-type.c)<br>
piglit_add_executable (program-parameter-output-type output-type.c)<br>
+piglit_add_executable (program-parameter-vertices-out vertices-out.c)<br>
diff --git a/tests/spec/arb_geometry_shader4/execution/program-parameter/vertices-out.c b/tests/spec/arb_geometry_shader4/execution/program-parameter/vertices-out.c<br>
new file mode 100644<br>
</div>index 0000000..3fdca1b<br>
<div><div>--- /dev/null<br>
+++ b/tests/spec/arb_geometry_shader4/execution/program-parameter/vertices-out.c<br>
@@ -0,0 +1,209 @@<br>
+/*<br>
+ * Copyright © 2013 The Piglit project<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<br>
+ * DEALINGS IN THE SOFTWARE.<br>
+ */<br>
+<br>
+/**<br>
+ * \file vertices-out.c<br>
+ *<br>
+ * Test required errors for wrong GL_GEOMETRY_VERTICES_OUT parameters with<br>
+ * variable number of output components. <br></div></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><div class="h5">
+ */<br>
+<br>
+#include "common.h"<br>
+<br>
+<br>
</div></div>+static const char gs_text_var[] =<br>
<div class="im">+ "uniform int vertex_count;\n"<br>
+ "varying out float var[n];\n"<br>
+ "void main()\n"<br>
+ "{\n"<br>
+ " for (int i = 0; i < vertex_count; i++) {\n"<br>
+ " gl_Position = vec4(0.0);\n"<br>
+ " for (int j = 0; j < n; j++)\n"<br>
+ " var[j] = 1.0 / exp2(float(j + 1));\n"<br>
+ " EmitVertex();\n"<br>
+ " }\n"<br>
+ "}\n";<br>
+<br>
</div>+static const char fs_text_var[] =<br>
<div><div class="h5">+ "varying float var[n];\n"<br>
+ "void main()\n"<br>
+ "{\n"<br>
+ " gl_FragColor = vec4(0.0);\n"<br>
+ " for (int j = 0; j < n; j++)\n"<br>
+ " gl_FragColor += vec4(var[j]);\n"<br>
+ "}\n";<br>
+<br>
+static bool transform_feedback = false;<br>
+static int components = -1;<br>
+<br>
+<br>
+PIGLIT_GL_TEST_CONFIG_BEGIN<br>
+ config.supports_gl_compat_version = 20;<br>
+ config.window_visual = PIGLIT_GL_VISUAL_DOUBLE | PIGLIT_GL_VISUAL_RGBA;<br>
+PIGLIT_GL_TEST_CONFIG_END<br>
+<br>
+<br>
+static bool<br>
+test_geometry_vertices_out(const GLuint prog)<br>
+{<br>
+ bool pass = true;<br>
+ int max_geometry_output_vertices;<br>
+ int max_geometry_total_output_components;<br>
+ int max_vertices_out;<br>
+<br>
+ glGetIntegerv(GL_MAX_GEOMETRY_OUTPUT_VERTICES,<br>
+ &max_geometry_output_vertices);<br>
+ glGetIntegerv(GL_MAX_GEOMETRY_TOTAL_OUTPUT_COMPONENTS,<br>
+ &max_geometry_total_output_components);<br>
+<br>
+ printf("Testing negative (-1) vertices out.\n");<br>
+ glProgramParameteri(prog, GL_GEOMETRY_VERTICES_OUT_ARB, -1);<br>
+ pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;<br>
+<br>
+ /* Setting GEOMETRY_VERTICES_OUT to zero should generate no error but<br>
+ * linking with a geometry shader should fail.<br>
+ */<br>
+ printf("Testing zero (0) vertices out.\n");<br>
+ glProgramParameteri(prog, GL_GEOMETRY_VERTICES_OUT_ARB, 0);<br>
+ glLinkProgram(prog);<br>
</div></div>+ pass = piglit_check_gl_error(GL_NO_ERROR) && !piglit_link_check_status_quiet(prog) && pass;<br>
<div class="im">+<br>
+ printf("Testing too many (%d) vertices out.\n", max_geometry_output_vertices + 1);<br>
+ glProgramParameteri(prog, GL_GEOMETRY_VERTICES_OUT_ARB,<br>
+ max_geometry_output_vertices + 1);<br>
+ pass = piglit_check_gl_error(GL_INVALID_VALUE) && pass;<br>
+<br>
+ /* If MAX_GEOMETRY_TOTAL_OUTPUT_COMPONENTS results in an even smaller limit for<br>
+ * GEOMETRY_VERTICES_OUT than GL_MAX_GEOMETRY_OUTPUT_VERTICES test that, too.<br>
+ */<br>
</div>+ if ((components != 0) &&<br>
+ (max_geometry_total_output_components / components + 1 < max_geometry_output_vertices + 1)) {<br></blockquote><div><br>The "+ 1"s seem unnecessary. How about just "if (max_geometry_total_output_components / components < max_geometry_output_vertices)"?<br>
<br>Also, the piglit coding conventions are to keep lines less than 80 columns, except in all.tests, and in rare situations where it would make the code difficult to read.<br> </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">+ /* The spec requires ProgramParameter to throw an INVALID_VALUE if<br>
+ * (GEOMETRY_VERTICES_OUT * (sum of components of all out varyings)) ><br>
+ * MAX_GEOMETRY_TOTAL_OUTPUT_COMPONENTS. But the number of output<br>
+ * components can only be infered from the geometry shader source<br>
+ * (and a geometry shader might not even be attached to the program<br>
</div>+ * object when ProgramParameter is called). Also accept failed linking.<br>
+ */<br>
+ printf("Testing too many total output components (%d vertices => %d total components).\n",<br>
+ max_geometry_total_output_components / components + 1,<br>
+ (max_geometry_total_output_components / components + 1) * components);<br>
<div class="im">+ glProgramParameteri(prog, GL_GEOMETRY_VERTICES_OUT_ARB,<br>
+ max_geometry_total_output_components / components + 1);<br>
+ if (!piglit_check_gl_error(GL_INVALID_VALUE) &&<br>
</div>+ (glLinkProgram(prog), piglit_link_check_status_quiet(prog))) {<br></blockquote><div><br>Considering the discussion above, I think this code should just try to link and check the link status. IMHO no implementation is going to generate this error at the time of the call to glProgramParameteri().<br>
</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">+ pass = false;<br>
+ }<br>
+ }<br>
+<br>
</div>+ max_vertices_out = (components != 0) ?<br>
+ MIN2(max_geometry_output_vertices, max_geometry_total_output_components / components) :<br>
+ max_geometry_output_vertices;<br>
<div class="im">+ printf("Testing maximal (%d) vertices out.\n", max_vertices_out);<br>
+ glProgramParameteri(prog, GL_GEOMETRY_VERTICES_OUT_ARB,<br>
+ max_vertices_out);<br>
+ glLinkProgram(prog);<br>
</div>+ pass = piglit_check_gl_error(GL_NO_ERROR) && piglit_link_check_status(prog) && pass;<br>
<div class="im">+<br>
+ return pass;<br>
+}<br>
+<br>
+<br>
</div>+/* Parse command line arguments.<br>
+ *<br>
+ * Recognized command line arguments are:<br>
+ * * The optional argument "tf": Use transform feedback.<br>
+ * * An integer indicating the number of per vertex varying components<br>
+ * written by the geometry shader (including gl_Position if transform<br>
+ * feedback is not used).<br>
+ */<br>
<div class="im">+static void<br>
+parse_cmd_line(int argc, char **argv)<br>
+{<br>
+ int i;<br>
+<br>
+ for (i = 1; i < argc; i++) {<br>
+ if (strcmp(argv[i], "tf") == 0) {<br>
+ transform_feedback = true;<br>
+ } else if(argv[i][0] != '-') {<br>
+ char *e;<br>
+ long int l = strtol(argv[i], &e, 10);<br>
+ if (*e == '\0')<br>
+ components = l;<br>
+ }<br>
+ }<br>
+<br>
</div><div class="im">+ if ((components < 0) || components > 32) {<br>
+ fprintf(stderr, "Please specify number of components "<br>
</div><div class="im">+ "from 0 to 32 (inclusive) on the command line\n");<br>
+ piglit_report_result(PIGLIT_FAIL);<br>
+ }<br></div></blockquote><div><br>It seems weird to place a maximum of 32 on the number of components. Why not just have the test run once with 1 component and once with MAX_GEOMETRY_VARYING_COMPONENTS_ARB?<br> </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>
+<br>
</div><div class="im">+void<br>
+piglit_init(int argc, char **argv)<br>
+{<br>
+ bool pass = true;<br>
+ GLuint prog;<br>
</div><div class="im">+ const char *gs_string, *fs_string;<br>
+<br>
+ parse_cmd_line(argc, argv);<br>
+<br>
</div>+ piglit_require_extension("GL_ARB_geometry_shader4");<br>
<div class="im">+ /* NV_geometry_shader4 relaxes some restrictions on valid program<br>
+ * parameters.<br>
+ */<br>
+ piglit_require_not_extension("GL_NV_geometry_shader4");<br>
+ if (transform_feedback)<br>
+ piglit_require_extension("GL_EXT_transform_feedback");<br>
+<br>
</div>+ /* Prepare shader source strings. */<br>
+ if (components == 0) {<br>
<div class="im">+ gs_string = gs_text4;<br>
+ fs_string = fs_text4;<br>
+ } else {<br>
+ asprintf((char **)&gs_string, "#extension GL_ARB_geometry_shader4: enable\n"<br>
</div>+ "const int n = %d;\n%s", components, gs_text_var);<br>
<div class="im">+ if (transform_feedback)<br>
+ fs_string = NULL;<br>
+ else<br>
</div>+ asprintf((char **)&fs_string, "const int n = %d;\n%s", components, fs_text_var);<br>
+ }<br>
+<br>
+ printf("Running Test with %d component(s) per output vertex and transform feedback %s.\n",<br>
+ components, transform_feedback ? "enabled" : "disabled");<br>
<div class="im">+<br>
+ /* Create shader and run test. */<br>
+ prog = create_shader(vs_text, gs_string, fs_string);<br>
+ pass = test_geometry_vertices_out(prog) && pass;<br>
+ glDeleteProgram(prog);<br>
+<br>
+ piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);<br>
+}<br>
+<br>
+enum piglit_result<br>
+piglit_display(void)<br>
+{<br>
+ /* Should never be reached */<br>
+ return PIGLIT_FAIL;<br>
+}<br>
--<br>
1.8.1.2<br>
<br>
<br>
</div><div class=""><div class="h5">_______________________________________________<br>
Piglit mailing list<br>
<a href="mailto:Piglit@lists.freedesktop.org">Piglit@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a><br>
</div></div></blockquote></div><br></div></div>