[Mesa-dev] [PATCH] draw: fix clip test with NaNs
Roland Scheidegger
sroland at vmware.com
Thu Dec 17 11:56:53 PST 2015
Am 17.12.2015 um 20:22 schrieb Matt Turner:
> On Thu, Dec 17, 2015 at 11:19 AM, Roland Scheidegger <sroland at vmware.com> wrote:
>> 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...
>
> I changed the implementation of DIFFERENT_SIGNS in src/mesa/main/macros.h to be
>
> return signbit(x) != signbit(y);
>
> a while ago. Perhaps you could just pull that into draw_pipe_clip.c?
>
I'm not sure it's a good fit as it sounds like we really need the result
to be false for negative zero / positive zero comparison. The existing
code does that too (which is the x-y != 0 part).
(Rather interestingly, the old definition in macros.h would actually
have returned different values for this depending on USE_IEEE or not...)
That said, the only user of the macro is in tnl/t_vb_cliptmp.h so I
guess it actually would want a different definition than it now is too
(the code is pretty much the same there as the one in draw) though I
don't think this code there generaly handles "crazy" clip conditions
right in any case.
And I'd have to pull that macro into util code.
Roland
More information about the mesa-dev
mailing list