Rework the workaround for bogus X server images.

Bug 22844 demonstrates that it is not sufficient to play tricks with
the clip regions to work around the bogus images from the X
server. The problem there is that if the operation hits the general
path and the destination has a different format than a8r8g8b8, the
destination pixels will be fetched into a temporary array. But because
those pixels would be outside the clip region, they would be fetched
as black. The previous workaround was relying on fast paths fetching
those pixels without checking the clip region.

In the new scheme we work around the problem at the
pixman_image_composite() level. If an image is determined to need a
work around, we translate both the bits pointer, the coordinates, and
the clip region, thus effectively undoing the X server's broken
computation.

diff --git a/pixman/pixman-bits-image.c b/pixman/pixman-bits-image.c
index b53630c..be28ebc 100644 (file)
--- a/pixman/pixman-bits-image.c
+++ b/pixman/pixman-bits-image.c
@@ -806,6 +806,7 @@ source_image_needs_out_of_bounds_workaround (bits_image_t *image)
 {
     if (image->common.clip_sources                      &&
         image->common.repeat == PIXMAN_REPEAT_NONE      &&
+       image->common.have_clip_region                  &&
         out_of_bounds_workaround)
     {
        const pixman_box32_t *boxes;

diff --git a/pixman/pixman-utils.c b/pixman/pixman-utils.c
index 3dfacde..2c34d02 100644 (file)
--- a/pixman/pixman-utils.c
+++ b/pixman/pixman-utils.c
@@ -84,20 +84,13 @@ clip_source_image (pixman_region32_t * region,
                    int                 dx,
                    int                 dy)
 {
-    /* The workaround lets certain fast paths run even when they
-     * would normally be rejected because of out-of-bounds access.
-     * We need to clip against the source geometry in that case
+    /* Source clips are ignored, unless they are explicitly turned on
+     * and the clip in question was set by an X client. (Because if
+     * the clip was not set by a client, then it is a hierarchy
+     * clip and those should always be ignored for sources).
      */
-    if (!picture->common.need_workaround)
-    {
-       /* Source clips are ignored, unless they are explicitly turned on
-        * and the clip in question was set by an X client. (Because if
-        * the clip was not set by a client, then it is a hierarchy
-        * clip and those should always be ignored for sources).
-        */
-       if (!picture->common.clip_sources || !picture->common.client_clip)
-           return TRUE;
-    }
+    if (!picture->common.clip_sources || !picture->common.client_clip)
+       return TRUE;
 
     return clip_general_image (region,
                                &picture->common.clip_region,
@@ -133,21 +126,8 @@ pixman_compute_composite_region32 (pixman_region32_t * region,
 
     region->extents.x1 = MAX (region->extents.x1, 0);
     region->extents.y1 = MAX (region->extents.y1, 0);
-
-    /* Some X servers rely on an old bug, where pixman would just believe the
-     * set clip_region and not clip against the destination geometry. So,
-     * since only X servers set "source clip", we don't clip against
-     * destination geometry when that is set and when the workaround has
-     * not been explicitly disabled by
-     *
-     *      pixman_disable_out_of_bounds_workaround();
-     *
-     */
-    if (!(dst_image->common.need_workaround))
-    {
-       region->extents.x2 = MIN (region->extents.x2, dst_image->bits.width);
-       region->extents.y2 = MIN (region->extents.y2, dst_image->bits.height);
-    }
+    region->extents.x2 = MIN (region->extents.x2, dst_image->bits.width);
+    region->extents.y2 = MIN (region->extents.y2, dst_image->bits.height);
 
     region->data = 0;
 
@@ -746,8 +726,7 @@ _pixman_run_fast_path (const pixman_fast_path_t *paths,
 
            if (sources_cover (
                    src, mask, extents,
-                   src_x, src_y, mask_x, mask_y, dest_x, dest_y) ||
-               src->common.need_workaround)
+                   src_x, src_y, mask_x, mask_y, dest_x, dest_y))
            {
                walk_region_internal (imp, op,
                                      src, mask, dest,

diff --git a/pixman/pixman.c b/pixman/pixman.c
index 94675d2..12b4c5a 100644 (file)
--- a/pixman/pixman.c
+++ b/pixman/pixman.c
@@ -55,6 +55,8 @@ static const optimized_operator_info_t optimized_operators[] =
     { PIXMAN_OP_NONE }
 };
 
+static pixman_implementation_t *imp;
+
 /*
  * Check if the current operator could be optimized
  */
@@ -105,7 +107,50 @@ pixman_optimize_operator (pixman_op_t     op,
 
 }
 
-static pixman_implementation_t *imp;
+static void
+apply_workaround (pixman_image_t *image,
+                 int16_t *       x,
+                 int16_t *       y,
+                 uint32_t **     save_bits,
+                 int *           save_dx,
+                 int *           save_dy)
+{
+    /* Some X servers generate images that point to the
+     * wrong place in memory, but then set the clip region
+     * to point to the right place. Because of an old bug
+     * in pixman, this would actually work.
+     *
+     * Here we try and undo the damage
+     */
+    int bpp = PIXMAN_FORMAT_BPP (image->bits.format) / 8;
+    pixman_box32_t *extents;
+    uint8_t *t;
+    int dx, dy;
+
+    extents = pixman_region32_extents (&(image->common.clip_region));
+    dx = extents->x1;
+    dy = extents->y1;
+
+    *save_bits = image->bits.bits;
+
+    *x -= dx;
+    *y -= dy;
+    pixman_region32_translate (&(image->common.clip_region), -dx, -dy);
+
+    t = (uint8_t *)image->bits.bits;
+    t += dy * image->bits.rowstride * 4 + dx * bpp;
+    image->bits.bits = (uint32_t *)t;
+
+    *save_dx = dx;
+    *save_dy = dy;
+}
+
+static void
+unapply_workaround (pixman_image_t *image, uint32_t *bits, int dx, int dy)
+{
+    image->bits.bits = bits;
+    pixman_region32_translate (&image->common.clip_region, dx, dy);
+}
 
 PIXMAN_EXPORT void
 pixman_image_composite (pixman_op_t      op,
@@ -121,6 +166,13 @@ pixman_image_composite (pixman_op_t      op,
                         uint16_t         width,
                         uint16_t         height)
 {
+    uint32_t *src_bits;
+    int src_dx, src_dy;
+    uint32_t *mask_bits;
+    int mask_dx, mask_dy;
+    uint32_t *dest_bits;
+    int dest_dx, dest_dy;
+
     /*
      * Check if we can replace our operator by a simpler one
      * if the src or dest are opaque. The output operator should be
@@ -137,12 +189,26 @@ pixman_image_composite (pixman_op_t      op,
     if (!imp)
        imp = _pixman_choose_implementation ();
 
+    if (src->common.need_workaround)
+       apply_workaround (src, &src_x, &src_y, &src_bits, &src_dx, &src_dy);
+    if (mask && mask->common.need_workaround)
+       apply_workaround (mask, &mask_x, &mask_y, &mask_bits, &mask_dx, &mask_dy);
+    if (dest->common.need_workaround)
+       apply_workaround (dest, &dest_x, &dest_y, &dest_bits, &dest_dx, &dest_dy);
+
     _pixman_implementation_composite (imp, op,
                                       src, mask, dest,
                                       src_x, src_y,
                                       mask_x, mask_y,
                                       dest_x, dest_y,
                                       width, height);
+
+    if (src->common.need_workaround)
+       unapply_workaround (src, src_bits, src_dx, src_dy);
+    if (mask && mask->common.need_workaround)
+       unapply_workaround (mask, mask_bits, mask_dx, mask_dy);
+    if (dest->common.need_workaround)
+       unapply_workaround (dest, dest_bits, dest_dx, dest_dy);
 }
 
 PIXMAN_EXPORT pixman_bool_t

diff --git a/test/window-test.c b/test/window-test.c
index b2b00b6..bbaa3e2 100644 (file)
--- a/test/window-test.c
+++ b/test/window-test.c
@@ -73,16 +73,22 @@ make_image (int width, int height, pixman_bool_t src, int *rx, int *ry)
     dx = get_rand (500);
     dy = get_rand (500);
 
-    /* Now simulate the bogus X server translations */
-    bits -= (dy * stride + dx) * bpp;
+    if (!src)
+    {
+       /* Now simulate the bogus X server translations */
+       bits -= (dy * stride + dx) * bpp;
+    }
 
     image = pixman_image_create_bits (
        format, width, height, (uint32_t *)bits, stride * bpp);
 
-    /* And add the bogus clip region */
-    pixman_region32_init_rect (&region, dx, dy, dx + width, dy + height);
+    if (!src)
+    {
+       /* And add the bogus clip region */
+       pixman_region32_init_rect (&region, dx, dy, dx + width, dy + height);
 
-    pixman_image_set_clip_region32 (image, &region);
+       pixman_image_set_clip_region32 (image, &region);
+    }
 
     pixman_image_set_source_clipping (image, TRUE);
 
@@ -112,8 +118,15 @@ make_image (int width, int height, pixman_bool_t src, int *rx, int *ry)
        pixman_image_set_repeat (image, PIXMAN_REPEAT_PAD);
     }
 
-    *rx = dx;
-    *ry = dy;
+    if (!src)
+    {
+       *rx = dx;
+       *ry = dy;
+    }
+    else
+    {
+       *rx = *ry = 0;
+    }
 
     return image;
 }
@@ -148,7 +161,11 @@ main ()
            uint8_t *pixel =
                bits + (i + dest_y) * stride + (j + dest_x) * bpp;
 
-           assert (*(uint16_t *)pixel == 0x788f);
+           if (*(uint16_t *)pixel != 0x788f)
+           {
+               printf ("bad pixel %x\n", *(uint16_t *)pixel);
+               assert (*(uint16_t *)pixel == 0x788f);
+           }
        }
     }