[cairo] Corrupt image rendering in 1.5.18

[Owen Taylor]

On Mon, 2008-04-07 at 09:36 -0700, Carl Worth wrote:
> Brown paper bag time...
> 
> Just before I made the 1.5.18 snapshot, Owen pointed out an
> interesting new bug[*] and it looked like it had an easy fix, so I
> threw it in.

[...]

> So I tweaked _cairo_pattern_get_extents to expand the pre-transformed
> extents by 0.5 in the case of a BILINEAR filter.
> 
> Of course, we know now that this has led to some nasty bugs. For
> example, mozilla is getting corrupted results like this:
> 
> 	http://people.mozilla.com/~vladimir/misc/broken-xlib-rgb24-out.png

[...]

> One option would be to back out the change I committed. Another option
> is to actually fix the bug. Owen has started chasing the bug down, and
> he has found problems in _cairo_pattern_acquire_surface. He's put some
> good writeups about the problem into bugzilla, and I'm inviting him to
> follow up here on the list, (Owen, you can just quote what you've
> written already).

The relevant part of what I said in 
http://bugs.freedesktop.org/show_bug.cgi?id=15349
is:

====
What's going on here is that with the current version, 
_cairo_pattern_acquire_surface() is being called:

 - with an integer translation or identity matrix
 - with a bounds bigger than the source surface

I think this could happen previously, but only when
!_cairo_operator_bounded_by_source(op).

When the above two things are true, there's an obvious bug in 
_cairo_pattern_acquire_surface_for_surface()

            /* Otherwise, we first transform the rectangle to the
             * coordinate space of the source surface so that we can
             * clone only that portion of the surface that will be
             * read. */
            [...]
            if (! _cairo_matrix_is_identity (&attr->matrix)) {
                double x1 = x;
                double y1 = y;
                double x2 = x + width;
                double y2 = y + height;
                cairo_bool_t is_tight;

                _cairo_matrix_transform_bounding_box  (&attr->matrix,
                                                       &x1, &y1, &x2, &y2,
                                                       &is_tight);

                /* The transform_bounding_box call may have resulted
                 * in a region larger than the surface, but we never
                 * want to clone more than the surface itself, (we
                 * know we're not repeating at this point due to the
                 * above.
                 *
                 * XXX: The one padding here is to account for filter
                 * radius.  It's a workaround right now, until we get a
                 * proper fix. (see bug #10508)
                 */
                x = MAX (0, floor (x1) - 1);
                y = MAX (0, floor (y1) - 1);
                width = MIN (extents.width, ceil (x2) + 1) - x;
                height = MIN (exts.height, ceil (y2) + 1) - y;
            }

So the clamp to the bounds of the surface only occurs when the matrix
is not the identity.
===

The patch I'm suggesting is:

 https://bugs.freedesktop.org/attachment.cgi?id=15740

It's a little bigger than I like for a tiny targeted fix ... more
opportunity to mix x/y together,etc. But I didn't want to move
around and modify the manually coded version of
cairo_rectangle_intersect. But the actual logic change is very small.

Assuming that testing doesn't reveal further problems, my instinct is
that we should make this work rather than backing it out, but that
may just be biased self-interest. (I'm trying to get stuff fixed for
repeat NONE in the X server, and it's hard to use cairo based
test cases if I have to tell people to use a custom cairo.)

- Owen

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.cairographics.org/archives/cairo/attachments/20080407/c6cce8a3/attachment-0001.pgp