i965/blorp: Improve precission of blitting coordinates when clipping

We do this in two steps: first we clip the dst rect and adjust the src
rect accordingly. Then we do it the other way around. In both passes
the adjustment part involves multiplying by a scale factor that can lead
to a small precision loss. This is breaking a few dEQP tests.

Specifically, the problem happens when we need to clip the same coordinate
twice. For example, if srcX0 and dstX0 need both to be clipped we want to
avoid the situation where we clip srcX0 first, then adjust dstX0 accordingly
but then we realize that the resulting dstX0 still needs to be clipped, so
we clip dstX0 and adjust srcX0 again. Each of these two passes can lead
to precission loss. What we want to do here is detect the rect that leads
to the largest clip (accounting for the scale factor involved), clip that
rect and adjust the other one. With this we ensure that the adjusted
coordinate does not need to be clipped again and we can skip a second pass,
improving precision.

Fixes the following 4 dEQP tests:
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_x_nearest
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_src_x_linear
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_dst_x_nearest
dEQP-GLES3.functional.fbo.blit.rect.out_of_bounds_reverse_dst_x_linear

Reviewed-by: Kenneth Graunke <[email protected]>
Tested-by: Mark Janes <[email protected]>

1 files changed, 163 insertions, 61 deletions

diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.c b/src/mesa/drivers/dri/i965/brw_meta_util.c
index a3b060413e5..b250d8a8146 100644
--- a/src/mesa/drivers/dri/i965/brw_meta_util.c
+++ b/src/mesa/drivers/dri/i965/brw_meta_util.c
@@ -41,65 +41,111 @@ fixup_mirroring(bool *mirror, float *coord0, float *coord1)
 }
 
 /**
- * Adjust {src,dst}_x{0,1} to account for clipping and scissoring of
- * destination coordinates.
+ * Compute the number of pixels to clip for each side of a rect
  *
- * Return true if there is still blitting to do, false if all pixels got
- * rejected by the clip and/or scissor.
+ * \param x0 The rect's left coordinate
+ * \param y0 The rect's bottom coordinate
+ * \param x1 The rect's right coordinate
+ * \param y1 The rect's top coordinate
+ * \param min_x The clipping region's left coordinate
+ * \param min_y The clipping region's bottom coordinate
+ * \param max_x The clipping region's right coordinate
+ * \param max_y The clipping region's top coordinate
+ * \param clipped_x0 The number of pixels to clip from the left side
+ * \param clipped_y0 The number of pixels to clip from the bottom side
+ * \param clipped_x1 The number of pixels to clip from the right side
+ * \param clipped_y1 The number of pixels to clip from the top side
  *
- * For clarity, the nomenclature of this function assumes we are clipping and
- * scissoring the X coordinate; the exact same logic applies for Y
- * coordinates.
- *
- * Note: this function may also be used to account for clipping of source
- * coordinates, by swapping the roles of src and dst.
+ * \return false if we clip everything away, true otherwise
  */
 static inline bool
-clip_or_scissor(bool mirror,
-                GLfloat *src_x0, GLfloat *src_x1,
-                GLfloat *dst_x0, GLfloat *dst_x1,
-                GLfloat fb_xmin, GLfloat fb_xmax)
+compute_pixels_clipped(float x0, float y0, float x1, float y1,
+                       float min_x, float min_y, float max_x, float max_y,
+                       float *clipped_x0, float *clipped_y0, float *clipped_x1, float *clipped_y1)
 {
-   float scale = (float) (*src_x1 - *src_x0) / (*dst_x1 - *dst_x0);
-   /* If we are going to scissor everything away, stop. */
-   if (!(fb_xmin < fb_xmax &&
-         *dst_x0 < fb_xmax &&
-         fb_xmin < *dst_x1 &&
-         *dst_x0 < *dst_x1)) {
+   /* If we are going to clip everything away, stop. */
+   if (!(min_x <= max_x &&
+         min_y <= max_y &&
+         x0 <= max_x &&
+         y0 <= max_y &&
+         min_x <= x1 &&
+         min_y <= y1 &&
+         x0 <= x1 &&
+         y0 <= y1)) {
       return false;
    }
 
-   /* Clip the destination rectangle, and keep track of how many pixels we
-    * clipped off of the left and right sides of it.
-    */
-   int pixels_clipped_left = 0;
-   int pixels_clipped_right = 0;
-   if (*dst_x0 < fb_xmin) {
-      pixels_clipped_left = fb_xmin - *dst_x0;
-      *dst_x0 = fb_xmin;
-   }
-   if (fb_xmax < *dst_x1) {
-      pixels_clipped_right = *dst_x1 - fb_xmax;
-      *dst_x1 = fb_xmax;
-   }
+   if (x0 < min_x)
+      *clipped_x0 = min_x - x0;
+   else
+      *clipped_x0 = 0;
+   if (max_x < x1)
+      *clipped_x1 = x1 - max_x;
+   else
+      *clipped_x1 = 0;
 
-   /* If we are mirrored, then before applying pixels_clipped_{left,right} to
-    * the source coordinates, we need to flip them to account for the
-    * mirroring.
-    */
-   if (mirror) {
-      int tmp = pixels_clipped_left;
-      pixels_clipped_left = pixels_clipped_right;
-      pixels_clipped_right = tmp;
-   }
+   if (y0 < min_y)
+      *clipped_y0 = min_y - y0;
+   else
+      *clipped_y0 = 0;
+   if (max_y < y1)
+      *clipped_y1 = y1 - max_y;
+   else
+      *clipped_y1 = 0;
+
+   return true;
+}
 
-   /* Adjust the source rectangle to remove the pixels corresponding to those
-    * that were clipped/scissored out of the destination rectangle.
+/**
+ * Clips a coordinate (left, right, top or bottom) for the src or dst rect
+ * (whichever requires the largest clip) and adjusts the coordinate
+ * for the other rect accordingly.
+ *
+ * \param mirror true if mirroring is required
+ * \param src the source rect coordinate (for example srcX0)
+ * \param dst0 the dst rect coordinate (for example dstX0)
+ * \param dst1 the opposite dst rect coordinate (for example dstX1)
+ * \param clipped_src0 number of pixels to clip from the src coordinate
+ * \param clipped_dst0 number of pixels to clip from the dst coordinate
+ * \param clipped_dst1 number of pixels to clip from the opposite dst coordinate
+ * \param scale the src vs dst scale involved for that coordinate
+ * \param isLeftOrBottom true if we are clipping the left or bottom sides
+ *        of the rect.
+ */
+static inline void
+clip_coordinates(bool mirror,
+                 float *src, float *dst0, float *dst1,
+                 float clipped_src0,
+                 float clipped_dst0,
+                 float clipped_dst1,
+                 float scale,
+                 bool isLeftOrBottom)
+{
+   /* When clipping we need to add or subtract pixels from the original
+    * coordinates depending on whether we are acting on the left/bottom
+    * or right/top sides of the rect respectively. We assume we have to
+    * add them in the code below, and multiply by -1 when we should
+    * subtract.
     */
-   *src_x0 += pixels_clipped_left * scale;
-   *src_x1 -= pixels_clipped_right * scale;
+   int mult = isLeftOrBottom ? 1 : -1;
 
-   return true;
+   if (!mirror) {
+      if (clipped_src0 >= clipped_dst0 * scale) {
+         *src += clipped_src0 * mult;
+         *dst0 += clipped_src0 / scale * mult;
+      } else {
+         *dst0 += clipped_dst0 * mult;
+         *src += clipped_dst0 * scale * mult;
+      }
+   } else {
+      if (clipped_src0 >= clipped_dst1 * scale) {
+         *src += clipped_src0 * mult;
+         *dst1 -= clipped_src0 / scale * mult;
+      } else {
+         *dst1 -= clipped_dst1 * mult;
+         *src += clipped_dst1 * scale * mult;
+      }
+   }
 }
 
 bool
@@ -121,23 +167,79 @@ brw_meta_mirror_clip_and_scissor(const struct gl_context *ctx,
    fixup_mirroring(mirror_y, srcY0, srcY1);
    fixup_mirroring(mirror_y, dstY0, dstY1);
 
-   /* If the destination rectangle needs to be clipped or scissored, do so. */
-   if (!(clip_or_scissor(*mirror_x, srcX0, srcX1, dstX0, dstX1,
-                         draw_fb->_Xmin, draw_fb->_Xmax) &&
-         clip_or_scissor(*mirror_y, srcY0, srcY1, dstY0, dstY1,
-                         draw_fb->_Ymin, draw_fb->_Ymax))) {
-      /* Everything got clipped/scissored away, so the blit was successful. */
+   /* Compute number of pixels to clip for each side of both rects. Return
+    * early if we are going to clip everything away.
+    */
+   float clip_src_x0;
+   float clip_src_x1;
+   float clip_src_y0;
+   float clip_src_y1;
+   float clip_dst_x0;
+   float clip_dst_x1;
+   float clip_dst_y0;
+   float clip_dst_y1;
+
+   if (!compute_pixels_clipped(*srcX0, *srcY0, *srcX1, *srcY1,
+                               0, 0, read_fb->Width, read_fb->Height,
+                               &clip_src_x0, &clip_src_y0, &clip_src_x1, &clip_src_y1))
       return true;
-   }
 
-   /* If the source rectangle needs to be clipped or scissored, do so. */
-   if (!(clip_or_scissor(*mirror_x, dstX0, dstX1, srcX0, srcX1,
-                         0, read_fb->Width) &&
-         clip_or_scissor(*mirror_y, dstY0, dstY1, srcY0, srcY1,
-                         0, read_fb->Height))) {
-      /* Everything got clipped/scissored away, so the blit was successful. */
+   if (!compute_pixels_clipped(*dstX0, *dstY0, *dstX1, *dstY1,
+                               draw_fb->_Xmin, draw_fb->_Ymin, draw_fb->_Xmax, draw_fb->_Ymax,
+                               &clip_dst_x0, &clip_dst_y0, &clip_dst_x1, &clip_dst_y1))
       return true;
-   }
+
+   /* When clipping any of the two rects we need to adjust the coordinates in
+    * the other rect considering the scaling factor involved. To obtain the best
+    * precision we want to make sure that we only clip once per side to avoid
+    * accumulating errors due to the scaling adjustment.
+    *
+    * For example, if srcX0 and dstX0 need both to be clipped we want to avoid
+    * the situation where we clip srcX0 first, then adjust dstX0 accordingly
+    * but then we realize that the resulting dstX0 still needs to be clipped,
+    * so we clip dstX0 and adjust srcX0 again. Because we are applying scaling
+    * factors to adjust the coordinates in each clipping pass we lose some
+    * precision and that can affect the results of the blorp blit operation
+    * slightly. What we want to do here is detect the rect that we should
+    * clip first for each side so that when we adjust the other rect we ensure
+    * the resulting coordinate does not need to be clipped again.
+    *
+    * The code below implements this by comparing the number of pixels that
+    * we need to clip for each side of both rects  considering the scales
+    * involved. For example, clip_src_x0 represents the number of pixels to be
+    * clipped for the src rect's left side, so if clip_src_x0 = 5,
+    * clip_dst_x0 = 4 and scaleX = 2 it means that we are clipping more from
+    * the dst rect so we should clip dstX0 only and adjust srcX0. This is
+    * because clipping 4 pixels in the dst is equivalent to clipping
+    * 4 * 2 = 8 > 5 in the src.
+    */
+
+   float scaleX = (float) (*srcX1 - *srcX0) / (*dstX1 - *dstX0);
+   float scaleY = (float) (*srcY1 - *srcY0) / (*dstY1 - *dstY0);
+
+   /* Clip left side */
+   clip_coordinates(*mirror_x,
+                    srcX0, dstX0, dstX1,
+                    clip_src_x0, clip_dst_x0, clip_dst_x1,
+                    scaleX, true);
+
+   /* Clip right side */
+   clip_coordinates(*mirror_x,
+                    srcX1, dstX1, dstX0,
+                    clip_src_x1, clip_dst_x1, clip_dst_x0,
+                    scaleX, false);
+
+   /* Clip bottom side */
+   clip_coordinates(*mirror_y,
+                    srcY0, dstY0, dstY1,
+                    clip_src_y0, clip_dst_y0, clip_dst_y1,
+                    scaleY, true);
+
+   /* Clip top side */
+   clip_coordinates(*mirror_y,
+                    srcY1, dstY1, dstY0,
+                    clip_src_y1, clip_dst_y1, clip_dst_y0,
+                    scaleY, false);
 
    /* Account for the fact that in the system framebuffer, the origin is at
     * the lower left.