From 6bd17f1e9861693262fa88bfeff5d3279b3f6e7d Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=B8ren=20Sandmann=20Pedersen?= Date: Mon, 20 Jul 2009 23:46:06 -0400 Subject: [PATCH] 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. --- pixman/pixman-bits-image.c | 1 + pixman/pixman-utils.c | 39 ++++++-------------------- pixman/pixman.c | 68 +++++++++++++++++++++++++++++++++++++++++++++- test/window-test.c | 33 ++++++++++++++++------ 4 files changed, 102 insertions(+), 39 deletions(-) diff --git a/pixman/pixman-bits-image.c b/pixman/pixman-bits-image.c index b53630c..be28ebc 100644 --- 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 --- 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 --- 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 --- 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 (®ion, dx, dy, dx + width, dy + height); + if (!src) + { + /* And add the bogus clip region */ + pixman_region32_init_rect (®ion, dx, dy, dx + width, dy + height); - pixman_image_set_clip_region32 (image, ®ion); + pixman_image_set_clip_region32 (image, ®ion); + } 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); + } } } -- 2.7.4