[Piglit] [PATCH] glsl-1.50-geometry-end-primitive: ensure position data is reasonably aligned

Ian Romanick idr at freedesktop.org
Fri Jan 8 19:09:25 PST 2016


On 01/08/2016 05:12 PM, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
> 
> The rect halves comparison is in fact not valid in general. The reason is that
> the same geometry does not have the same precision even if it it just shifted
> by some fixed amount in one direction.
> As an example, some calculated x value near 7 will be near 263 if drawn with
> a viewport x value of 256. The value near 7 has 5 bits more precision -
> when calculating the fixed-point values for rasterization (with subpixel
> accuracy), the lower value thus may be rounded in a different direction
> than the higher one, even with correct rounding (not even entirely sure what
> "correct" rounding here means, nearest-floor or nearest-even or whatever, the
> problem is independent from that). This can then cause different pixels to be
> covered by the primitive.
> This causes a failure with this test in llvmpipe when the rounding mode is
> changed slightly (from "mostly" nearest-away-from-zero to nearest-even). I was
> not able to reproduce this with this test on nvidia or amd hardware, but they
> are definitely affected in theory by the same issue (proven by some quick and
> dirty test).
> So, simply add then subtract some fixed amount to the position values, which
> ensures there should be no differences in the position later even after
> viewport translate (we use window size + 1 here, so the values in range
> -1.0 - +1.0 will all have the same exponent (just above the window size)).
> (We must do that with uniforms otherwise mesa will optimize the add/sub away -
> this is definitely unsafe math optimization but may well be allowed.)
> 
> This may be a problem with more tests, albeit most tend to use vertex data
> which is already sufficiently aligned (e.g. drawing simple aligned rects).
> ---
>  tests/general/quad-invariance.c                    | 55 +++++++++-------------
>  .../glsl-1.50/execution/geometry/end-primitive.c   | 15 +++++-
>  2 files changed, 36 insertions(+), 34 deletions(-)
> 
> diff --git a/tests/general/quad-invariance.c b/tests/general/quad-invariance.c
> index b5741d0..fd51dec 100644
> --- a/tests/general/quad-invariance.c
> +++ b/tests/general/quad-invariance.c
> @@ -38,9 +38,13 @@
>  
>  #include "piglit-util-gl.h"
>  
> +#define PATTERN_SIZE 256
> +
>  PIGLIT_GL_TEST_CONFIG_BEGIN
>  
>  	config.supports_gl_compat_version = 10;
> +	config.window_width = 2*PATTERN_SIZE;
> +	config.window_height = PATTERN_SIZE;
>  
>  	config.window_visual = PIGLIT_GL_VISUAL_RGB | PIGLIT_GL_VISUAL_DOUBLE;
>  
> @@ -49,41 +53,24 @@ PIGLIT_GL_TEST_CONFIG_END
>  enum piglit_result
>  piglit_display(void)
>  {
> +	int i;
>  	GLboolean pass = GL_TRUE;
> -	float verts[12][2] = {
> +	float verts[3][2] = {
>  		/* prim 1: left half of screen. */
> -		{-1.0, -1.0},
> -		{ 0.0, -1.0},
> -		{ 0.0,  1.0},
> -		{-1.0,  1.0},
> -		/* prim 2: right half of screen. */
> -		{ 0.0, -1.0},
> -		{ 1.0, -1.0},
> -		{ 1.0,  1.0},
> +		{-1.0 + 2.0f / PATTERN_SIZE, -1.0},
> +		{-1.0 + 2.0f / PATTERN_SIZE,  1.0},
>  		{ 0.0,  1.0},
> -		/* prim 3: somewhere off the screen. */
> -		{ 2.0, -1.0},
> -		{ 3.0, -1.0},
> -		{ 3.0,  1.0},
> -		{ 2.0,  1.0},
>  	};
> -	float colors[12][4] = {
> -		{1.0, 0.0, 0.0, 0.0},
> -		{0.0, 1.0, 0.0, 0.0},
> -		{0.0, 0.0, 1.0, 0.0},
> -		{1.0, 1.0, 1.0, 0.0},
> -
> -		{1.0, 0.0, 0.0, 0.0},
> -		{0.0, 1.0, 0.0, 0.0},
> -		{0.0, 0.0, 1.0, 0.0},
> -		{1.0, 1.0, 1.0, 0.0},
> -
> -		{1.0, 0.0, 0.0, 0.0},
> -		{0.0, 1.0, 0.0, 0.0},
> -		{0.0, 0.0, 1.0, 0.0},
> -		{1.0, 1.0, 1.0, 0.0},
> +	float colors[3][4] = {
> +		{1.0, 0.0, 0.0, 1.0},
> +		{1.0, 0.0, 0.0, 1.0},
> +		{1.0, 0.0, 0.0, 1.0},
> +
>  	};
> +
>  	static GLboolean once = GL_TRUE;
> +	for (i = 0; i < (1 << 24) && pass; i++) {
> +		verts[1][0] += 5.96046447754E-08f * 2.0f;
>  
>  	glClearColor(0.0, 0.0, 0.0, 0.0);
>  	glClear(GL_COLOR_BUFFER_BIT);
> @@ -94,19 +81,21 @@ piglit_display(void)
>  	glEnableClientState(GL_VERTEX_ARRAY);
>  
>  	/* left: 1 prim */
> -	glDrawArrays(GL_QUADS, 0, 4);
> +	glViewport(0, 0, piglit_width/2, piglit_height);
> +	glDrawArrays(GL_TRIANGLES, 0, 3);
>  
> +	glViewport(piglit_width/2, 0, piglit_width/2, piglit_height);
>  	/* right: 1 prim */
> -	glDrawArrays(GL_QUADS, 4, 8);
> +	glDrawArrays(GL_TRIANGLES, 0, 3);
>  
>  	if (once) {
>  		printf("Left and right half should match.\n");
>  		once = GL_FALSE;
>  	}
>  
> -	pass = piglit_probe_rect_halves_equal_rgba(0, 0, piglit_width,
> +	pass &= piglit_probe_rect_halves_equal_rgba(0, 0, piglit_width,
>  	                                           piglit_height);
> -
> +	}
>  	piglit_present_results();
>  
>  	return pass ? PIGLIT_PASS : PIGLIT_WARN;
> diff --git a/tests/spec/glsl-1.50/execution/geometry/end-primitive.c b/tests/spec/glsl-1.50/execution/geometry/end-primitive.c
> index 6df3a89..e251ad0 100644
> --- a/tests/spec/glsl-1.50/execution/geometry/end-primitive.c
> +++ b/tests/spec/glsl-1.50/execution/geometry/end-primitive.c
> @@ -92,6 +92,8 @@ static const char *spiral_text =
>  	"#version 150\n"
>  	"\n"
>  	"uniform int num_vertices;\n"
> +	"uniform float hack1;\n"
> +	"uniform float hack2;\n"
>  	"\n"
>  	"vec2 spiral(int vertex_id)\n"
>  	"{\n"
> @@ -105,7 +107,10 @@ static const char *spiral_text =
>  	"  if (vertex_id % 2 == 1) r += 1.0;\n"
>  	"  float max_r = b*sqrt(a*float(num_vertices)) + 1.0;\n"
>  	"  r /= max_r;\n"
> -	"  return r*vec2(cos(theta), sin(theta));\n"
> +	"  vec2 tmp = r*vec2(cos(theta), sin(theta));\n"
> +        "  // ensure reasonably aligned vertices \n"
> +        "  // note cannot use literals as they'd get optimized away... \n"
> +        "  return tmp + hack1 - hack2;\n"

This works only by luck, and it's probably the only use-case I've seen
for the precise keyword. :)  If the compiler stack decided to rearrange
this expression as tmp + (hack1 - hack2), you'd be right back where you
started.  Without the precise keyword, the GLSL compiler is allowed to
do that.

I haven't looked at the rest of the shader, so I don't have an alternate
suggestion.  Maybe do something like floor(tmp * large_constant) /
large_constant?

Also, piglit uses hard tabs for indentation.

>  	"}\n";
>  
>  /**
> @@ -303,6 +308,10 @@ draw_ref_pattern()
>  	glUseProgram(prog_ref);
>  	glUniform1i(glGetUniformLocation(prog_ref, "num_vertices"),
>  		    num_vertices);
> +	glUniform1f(glGetUniformLocation(prog_ref, "hack1"),
> +		    (float)PATTERN_SIZE * 2.0f + 1.0f);
> +	glUniform1f(glGetUniformLocation(prog_ref, "hack2"),
> +		    (float)PATTERN_SIZE * 2.0f + 1.0f);
>  	glEnable(GL_PRIMITIVE_RESTART);
>  	glPrimitiveRestartIndex(0xffffffff);
>  
> @@ -342,6 +351,10 @@ draw_test_pattern()
>  	glUseProgram(prog_test);
>  	glUniform1i(glGetUniformLocation(prog_test, "num_vertices"),
>  		    num_vertices);
> +	glUniform1f(glGetUniformLocation(prog_ref, "hack1"),
> +		    (float)PATTERN_SIZE * 2.0f + 1.0f);
> +	glUniform1f(glGetUniformLocation(prog_ref, "hack2"),
> +		    (float)PATTERN_SIZE * 2.0f + 1.0f);
>  	glDrawArrays(GL_POINTS, 0, 3);
>  }
>  
> 



More information about the Piglit mailing list