[Piglit] [PATCH] glsl-1.50-geometry-end-primitive: ensure position data is reasonably aligned
Roland Scheidegger
sroland at vmware.com
Fri Jan 8 20:16:39 PST 2016
Am 09.01.2016 um 04:09 schrieb Ian Romanick:
> 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.
Oh indeed you're quite right, I didn't think of that (albeit the
compiler actually combined the first add into a mad together with the
preceding mul).
>
> 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?
Yes I was first thinking about mul + int conversion and back before I
thought a add is easier, but of course the actual conversion isn't
needed and floor should do. Something like 2^10 should probably do
(basically still have some minimal subpixel bits for this size of the vp).
I suppose maybe (tmp + hack1) * hack2 - hack1 would also do? (with hack2
being uniform 1.0 of course). Albeit technically the compiler could do
tmp*hack2 + hack1*hack2 - hack1 again, grmpf. Though I'm not sure why
the compiler would do this.
Roland
>
> Also, piglit uses hard tabs for indentation.
Right, I'll fix that.
>
>> "}\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