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

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