[Mesa-dev] [PATCH] draw: fix clip test with NaNs

[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?