[Piglit] [PATCH] gs: Test interaction between gl_PrimitiveIDIn and primitive restart.

[Chad Versace]

On 08/26/2013 11:38 PM, Paul Berry wrote:
> Verified using the NVIDIA proprietary driver for linux.
> ---
>   tests/all.tests                                    |   9 +
>   .../glsl-1.50/execution/geometry/CMakeLists.gl.txt |   1 +
>   .../execution/geometry/primitive-id-restart.c      | 265 +++++++++++++++++++++
>   3 files changed, 275 insertions(+)
>   create mode 100644 tests/spec/glsl-1.50/execution/geometry/primitive-id-restart.c



> +#define LONGEST_INPUT_SEQUENCE 12

Did you choose 12 to give gl_PrimitiveIDIn an opportunity to increment
for GL_TRIANGLES_ADJANCENCY? If so, comment please. If not, then comment
double-please, because I don't see the reason.

> +#define NUM_INPUT_ELEMENTS \
> +	(LONGEST_INPUT_SEQUENCE * (LONGEST_INPUT_SEQUENCE + 3) / 2)
> +#define MAX_TOTAL_PRIMS \
> +	(LONGEST_INPUT_SEQUENCE * (LONGEST_INPUT_SEQUENCE + 1) / 2)

The term 'INPUT' in the var name NUM_INPUT_ELEMENTS confused me. So went my
mind... "The ELEMENTS
are qualified as INPUT ELEMENTS, which distinguishes them from... OUTPUT ELEMENTS?
SOMETHING_ELSE ELEMENTS? What type of elements are these, if there be INPUT and
OTHER_TYPES of elements? I do not know these ELEMENTS." Eventually, I saw that they
were just vanilla ELEMENTS. Oh well.

The expression for MAX_TOTAL_PRIMS may look mysterious to the uninitiated. Please
comment with
/* Sum of 1, 2, ..., LONGEST_INPUT_SEQUENCE. */

I found the expression for NUM_INPUT_ELEMENTS mysterious, and eventually discovered
its round-about derivation. The following comment, or something similar, is strongly needed:
/* Sum of 2, 3, ..., LONGEST_INPUT_SEQUENCE + 1. */



> +static const char *vs_text =
> +	"#version 150\n"
> +	"\n"
> +	"void main()\n"
> +	"{\n"
> +	"}\n";
> +
> +static const char *gs_template =
> +	"#version 150\n"
> +	"#define INPUT_LAYOUT %s\n"
> +	"layout(INPUT_LAYOUT) in;\n"

This shader preamble feels weird. I expected the macro INPUT_LAYOUT to
be used elsewhere, but it isn't. I think
   #version 150
   layout(%s) in;
is easier to read. Anyways... just ignore me here if you want to call bikeshed.

> +	"layout(points, max_vertices = 1) out;\n"
> +	"\n"
> +	"out int primitive_id;\n"
> +	"\n"
> +	"void main()\n"
> +	"{\n"
> +	"  primitive_id = gl_PrimitiveIDIn;\n"
> +	"  EmitVertex();\n"
> +	"}\n";




> +	/* Find out how many times the GS got invoked so we'll know
> +	 * how many transform feedback outputs to examine.
> +	 */
> +	glGetQueryObjectuiv(generated_query, GL_QUERY_RESULT,
> +			    &primitives_generated);
> +	if (primitives_generated > MAX_TOTAL_PRIMS) {
> +		printf("Expected no more than %d primitives, got %d\n",
> +		       MAX_TOTAL_PRIMS, primitives_generated);
> +		pass = false;
> +
> +		/* Set primitives_generated to MAX_TOTAL_PRIMS so that
> +		 * we don't overflow the buffer in the loop below.
> +		 */

The test neglects to clamp primitives_generated like the comment says.

> +	}
> +
> +	/* Check transform feedback outputs. */
> +	readback = glMapBuffer(GL_TRANSFORM_FEEDBACK_BUFFER, GL_READ_ONLY);
> +	for (i = 0; i < primitives_generated; i++) {
> +		if (readback[i] != i) {
> +			printf("Expected primitive %d to have gl_PrimitiveIDIn"
> +			       " = %d, got %d instead\n", i, i, readback[i]);
> +			pass = false;

If gl_PrimitiveIDIn fails to increment at the first occurrence of the primitive
restart, then this errors gets emitted up to sum(i, {i, 2, 12}) = 77 times.
I'd prefer that this error message not spam the console and instead be emitted
only once. But, if you disagree, then I defer.

> +		}
> +	}
> +	glUnmapBuffer(GL_TRANSFORM_FEEDBACK_BUFFER);
> +
> +	piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
> +}


Other than the requests for clarification, and the clamping issue,
the test's behavior looks correct to me.