[Mesa-dev] [PATCH] draw: fix clip test with NaNs
Roland Scheidegger
sroland at vmware.com
Thu Dec 17 11:19:09 PST 2015
Am 17.12.2015 um 19:13 schrieb Brian Paul:
> On 12/17/2015 09:58 AM, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> NaNs mean it should be clipped, otherwise the NaNs might get passed to
>> the
>> next stages (if clipping didn't happen for another reason already), which
>> might cause all kind of problems.
>> The llvm path got this right already (possibly by luck), but this
>> isn't used
>> when there's a gs active.
>> Found by code inspection, verified with some hacked piglit test and
>> some more
>> hacked debug output.
>> (Note the clipper can still itself incorrectly generate NaN and INF
>> position
>> values in its output prims (at least after w divide / viewport
>> transform) even
>> if the inputs weren't NaNs, if the position data of the vertices is
>> "sufficiently bad".)
>> ---
>> src/gallium/auxiliary/draw/draw_cliptest_tmp.h | 28
>> +++++++++++++-------------
>> src/gallium/auxiliary/draw/draw_llvm.c | 4 ++++
>> 2 files changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>> b/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>> index 779b237..d8866cd 100644
>> --- a/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>> +++ b/src/gallium/auxiliary/draw/draw_cliptest_tmp.h
>> @@ -95,30 +95,31 @@ static boolean TAG(do_cliptest)( struct pt_post_vs
>> *pvs,
>> out->pre_clip_pos[i] = position[i];
>> }
>>
>> + /* Be careful with NaNs. Comparisons must be true for them. */
>> /* Do the hardwired planes first:
>> */
>> if (flags & DO_CLIP_XY_GUARD_BAND) {
>> - if (-0.50 * position[0] + position[3] < 0) mask |= (1<<0);
>> - if ( 0.50 * position[0] + position[3] < 0) mask |= (1<<1);
>> - if (-0.50 * position[1] + position[3] < 0) mask |= (1<<2);
>> - if ( 0.50 * position[1] + position[3] < 0) mask |= (1<<3);
>> + if (!(-0.50 * position[0] + position[3] >= 0)) mask |=
>> (1<<0);
>> + if (!( 0.50 * position[0] + position[3] >= 0)) mask |=
>> (1<<1);
>> + if (!(-0.50 * position[1] + position[3] >= 0)) mask |=
>> (1<<2);
>> + if (!( 0.50 * position[1] + position[3] >= 0)) mask |=
>> (1<<3);
>> }
>> else if (flags & DO_CLIP_XY) {
>> - if (-position[0] + position[3] < 0) mask |= (1<<0);
>> - if ( position[0] + position[3] < 0) mask |= (1<<1);
>> - if (-position[1] + position[3] < 0) mask |= (1<<2);
>> - if ( position[1] + position[3] < 0) mask |= (1<<3);
>> + if (!(-position[0] + position[3] >= 0)) mask |= (1<<0);
>> + if (!( position[0] + position[3] >= 0)) mask |= (1<<1);
>> + if (!(-position[1] + position[3] >= 0)) mask |= (1<<2);
>> + if (!( position[1] + position[3] >= 0)) mask |= (1<<3);
>> }
>>
>> /* Clip Z planes according to full cube, half cube or none.
>> */
>> if (flags & DO_CLIP_FULL_Z) {
>> - if ( position[2] + position[3] < 0) mask |= (1<<4);
>> - if (-position[2] + position[3] < 0) mask |= (1<<5);
>> + if (!( position[2] + position[3] >= 0)) mask |= (1<<4);
>> + if (!(-position[2] + position[3] >= 0)) mask |= (1<<5);
>> }
>> else if (flags & DO_CLIP_HALF_Z) {
>> - if ( position[2] < 0) mask |= (1<<4);
>> - if (-position[2] + position[3] < 0) mask |= (1<<5);
>> + if (!( position[2] >= 0)) mask |= (1<<4);
>> + if (!(-position[2] + position[3] >= 0)) mask |= (1<<5);
>> }
>>
>> if (flags & DO_CLIP_USER) {
>> @@ -146,7 +147,7 @@ static boolean TAG(do_cliptest)( struct pt_post_vs
>> *pvs,
>> if (clipdist < 0 || util_is_inf_or_nan(clipdist))
>> mask |= 1 << plane_idx;
>> } else {
>> - if (dot4(clipvertex, plane[plane_idx]) < 0)
>> + if (!(dot4(clipvertex, plane[plane_idx]) >= 0))
>> mask |= 1 << plane_idx;
>> }
>> }
>> @@ -192,7 +193,6 @@ static boolean TAG(do_cliptest)( struct pt_post_vs
>> *pvs,
>>
>> out = (struct vertex_header *)( (char *)out + info->stride );
>> }
>> -
>> return need_pipeline != 0;
>> }
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c
>> b/src/gallium/auxiliary/draw/draw_llvm.c
>> index 8435991..dad523a 100644
>> --- a/src/gallium/auxiliary/draw/draw_llvm.c
>> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
>> @@ -1200,6 +1200,10 @@ generate_clipmask(struct draw_llvm *llvm,
>> cv_w = pos_w;
>> }
>>
>> + /*
>> + * Be careful with the comparisons and NaNs (using llvm's unordered
>> + * comparisons here).
>> + */
>> /* Cliptest, for hardwired planes */
>> if (clip_xy) {
>> /* plane 1 */
>>
>
> Looks OK to me, but I'm not sure I understand what'll happen when we
> find a vertex with a NaN coordinate. Does the whole triangle get
> culled? Otherwise, what would be the result of computing the
> intersection point on the clip plane?
>
> Reviewed-by: Brian Paul <brianp at vmware.com>
>
If we set the clip mask bit when there's a NaN, this simply ensures
we're entering the clip stage (and the clip stage will also actually
perform clipping against the affected clip plane).
We won't actually cull such a tri directly, but the math in the clip
stage should ensure it will eventually be discarded. Because
getclipdist() then should return a NaN too, which will cause the code to
immediately return without emitting the tri (or any already generated
tri due to clipping with other planes) immediately. As far as I can see,
this logic should work reliably (the nans cannot disappear even if we
performed some math on them due to clipping with other planes).
There's more bugs in the clip stage but I don't quite understand it.
Though I spotted some issue which could be problematic, there's some
DIFFERENT_SIGNS logic (which is also used to avoid division by zero)
which doesn't look bullet-proof. This is defined as:
DIFFERENT_SIGNS(x, y) ((x) * (y) <= 0.0F && (x) - (y) != 0.0F)
But for sufficiently small x and y (in the order of 2^-63) this will be
true even if both x and y have the same sign (as the product would be
zero). But there's probably more to it which causes it to generate bogus
tris in the end...
Roland
More information about the mesa-dev
mailing list