[cairo] some fixes and improvements to current cvs version of
cairo
David Reveman
davidr at novell.com
Fri Jan 28 13:45:10 PST 2005
On Fri, 2005-01-28 at 15:37 -0500, Kristian Høgsberg wrote:
> David Reveman wrote:
> > I ran some test on current cvs version of cairo today and found a number
> > of problems. Most problems where simple or obvious to fix, I've attached
> > my current diff. It includes a lot of important fixes to the pattern
> > system, some clipping fixes, some improvements to the glitz backend and
> > an up to date version of the xcb backend.
> >
> > I'll probably not be able to write changelog entries and commit this
> > until next week, but you're very welcome to pick fixes from my diff and
> > commit them if you like. Most of them are obvious fixes so you should be
> > able to fill out changelog entries yourself.
>
> > Index: src/cairo_gstate.c
> > ===================================================================
> > RCS file: /cvs/cairo/cairo/src/cairo_gstate.c,v
> > retrieving revision 1.77
> > diff -u -r1.77 cairo_gstate.c
> > --- src/cairo_gstate.c 28 Jan 2005 01:21:13 -0000 1.77
> > +++ src/cairo_gstate.c 28 Jan 2005 15:41:04 -0000
> > @@ -392,11 +392,9 @@
> > if (pattern == NULL)
> > return CAIRO_STATUS_NULL_POINTER;
> >
> > - if (gstate->pattern)
> > - cairo_pattern_destroy (gstate->pattern);
> > -
> > - gstate->pattern = pattern;
> > cairo_pattern_reference (pattern);
> > + cairo_pattern_destroy (gstate->pattern);
> > + gstate->pattern = pattern;
> >
> > return CAIRO_STATUS_SUCCESS;
> > }
> > @@ -1419,14 +1417,10 @@
> > if (dest->y < src->y)
> > dest->y = src->y;
> >
> > - if (dest->x + dest->width < src->x + src->width)
> > - dest->width = dest->x + dest->width - dest->x;
> > - else
> > + if (dest->x + dest->width > src->x + src->width)
> > dest->width = src->x + src->width - dest->x;
> >
> > - if (dest->y + dest->height < src->y + src->height)
> > - dest->height = dest->y + dest->height - dest->y;
> > - else
> > + if (dest->y + dest->height > src->y + src->height)
> > dest->height = src->y + src->height - dest->y;
> >
> > if (dest->width <= 0 || dest->height == 0) {
>
> I've fixed this in CVS, using a temporary cairo_rectangle_t.
>
> > @@ -1521,8 +1515,6 @@
> > }
> >
> > _cairo_pattern_init_solid (&pattern, 1.0, 1.0, 1.0);
> > - _cairo_gstate_create_pattern (gstate, &pattern);
> > - /* Override the alpha set from gstate. */
> > _cairo_pattern_set_alpha (&pattern, 1.0);
> >
> > status = _cairo_surface_composite_trapezoids (CAIRO_OPERATOR_ADD,
>
> Oh, right, makes sense. I've updated all occurences of this pattern.
>
> > @@ -1989,10 +1980,10 @@
> > cairo_pattern_t pattern;
> > cairo_box_t pattern_extents;
> > cairo_rectangle_t extents;
> > + int xoff, yoff;
> >
> > cairo_surface_get_matrix (surface, &user_to_image);
> > cairo_matrix_multiply (&device_to_image, &gstate->ctm_inverse, &user_to_image);
> > - cairo_surface_set_matrix (surface, &device_to_image);
> >
> > image_to_user = user_to_image;
> > cairo_matrix_invert (&image_to_user);
> > @@ -2006,8 +1997,12 @@
> > &device_width, &device_height);
> >
> > _cairo_pattern_init_for_surface (&pattern, surface);
> > - _cairo_gstate_create_pattern (gstate, &pattern);
> >
> > + if (!_cairo_matrix_is_integer_translation (&device_to_image, &xoff, &yoff))
> > + pattern.matrix = device_to_image;
> > +
> > + _cairo_pattern_set_alpha (&pattern, gstate->alpha);
> > +
>
> Isn't this an optimization that belongs in the backend?
I don't think any backend would mind getting an integer translation as a
source offset rather than a transformation matrix. But if you like to
move this into the backends instead that's fine with me. You should know
that this optimization makes a pretty big difference on xrender and
image backends, so it's probably worth keeping, in some form.
>
> > pattern_extents.p1.x = _cairo_fixed_from_double (device_x);
> > pattern_extents.p1.y = _cairo_fixed_from_double (device_y);
> > pattern_extents.p2.x = _cairo_fixed_from_double (device_x + device_width);
> > @@ -2017,27 +2012,41 @@
> > if (gstate->clip.surface)
> > {
> > _cairo_rectangle_intersect (&extents, &gstate->clip.rect);
> > -
> > - /* Shortcut if empty */
> > - if (_cairo_rectangle_empty (&extents)) {
> > - status = CAIRO_STATUS_SUCCESS;
> > - goto BAIL1;
> > + if (!_cairo_rectangle_empty (&extents))
> > + {
> > + status = _cairo_surface_composite (gstate->operator,
> > + &pattern,
> > + gstate->clip.surface,
> > + gstate->surface,
> > + extents.x + xoff,
> > + extents.y + yoff,
> > + 0, 0,
> > + extents.x, extents.y,
> > + extents.width, extents.height);
> > }
> > -
> > - status = _cairo_surface_composite (gstate->operator,
> > - &pattern,
> > - gstate->clip.surface,
> > - gstate->surface,
> > - extents.x, extents.y,
> > - 0, 0,
> > - extents.x, extents.y,
> > - extents.width, extents.height);
> > -
> > - BAIL1:
> > - ;
> > + else
> > + status = CAIRO_STATUS_SUCCESS;
> > }
> > else
> > {
> > + cairo_surface_t *mask = NULL;
> > +
> > + /* alpha as mask surface */
> > + if (pattern.color.alpha != 1.0)
> > + {
> > + mask = _cairo_surface_create_similar_solid (gstate->surface,
> > + CAIRO_FORMAT_A8, 1, 1,
> > + &pattern.color);
> > + if (!mask)
> > + {
> > + _cairo_pattern_fini (&pattern);
> > + return CAIRO_STATUS_NO_MEMORY;
> > + }
> > +
> > + pattern.color.alpha = 1.0;
> > + cairo_surface_set_repeat (mask, 1);
> > + }
> > +
>
> Hmm, this wouldn't handle the case with clipping and alpha != 1.0. I
> comitted an alternate fix yesterday that should handle both cases. The
> code is in _cairo_pattern_get_surface in the CAIRO_PATTERN_SURFACE case.
check my new patch, I've got a better solution to this in there.
>
>
> > static void
> > Index: src/cairo_image_surface.c
> > ===================================================================
> > RCS file: /cvs/cairo/cairo/src/cairo_image_surface.c,v
> > retrieving revision 1.22
> > diff -u -r1.22 cairo_image_surface.c
> > --- src/cairo_image_surface.c 28 Jan 2005 03:57:32 -0000 1.22
> > +++ src/cairo_image_surface.c 28 Jan 2005 15:41:04 -0000
> > @@ -395,6 +395,8 @@
> > if (generic_mask && (generic_mask->backend != dst->base.backend))
> > return CAIRO_INT_STATUS_UNSUPPORTED;
> >
> > + _cairo_pattern_prepare_surface (pattern, &src->base);
> > +
> > pixman_composite (_pixman_operator (operator),
> > src->pixman_image,
> > mask ? mask->pixman_image : NULL,
> > @@ -404,6 +406,8 @@
> > dst_x, dst_y,
> > width, height);
> >
> > + _cairo_pattern_restore_surface (pattern, &src->base);
> > +
> > cairo_surface_destroy (&src->base);
>
> Yeah, these are missing a few places, I think, but I was planning on
> moving _cairo_pattern_prepare_surface back into
> _cairo_gstate_create_pattern, which should then be renamed to
> _cairo_gstate_prepare_pattern. _cairo_pattern_restore_surface should be
> renamed _cairo_gstate_restore_pattern or moved back into
> _cairo_pattern_fini (which is sort of ugly, but works because we always
> use temporary patterns which we fini immediately after use).
>
> ...
>
> > Index: src/cairo_pattern.c
> > ===================================================================
> > RCS file: /cvs/cairo/cairo/src/cairo_pattern.c,v
> > retrieving revision 1.15
> > diff -u -r1.15 cairo_pattern.c
> > --- src/cairo_pattern.c 28 Jan 2005 05:14:06 -0000 1.15
> > +++ src/cairo_pattern.c 28 Jan 2005 15:41:04 -0000
> > @@ -64,6 +64,9 @@
> > sizeof (cairo_color_stop_t) * other->n_stops);
> > }
> >
> > + if (pattern->type == CAIRO_PATTERN_SURFACE)
> > + cairo_surface_reference (pattern->u.surface.surface);
> > +
> > return CAIRO_STATUS_SUCCESS;
> > }
>
> Comitted this.
>
>
> > @@ -810,16 +811,17 @@
> > }
> >
> > image = _cairo_surface_get_image (src);
> > -
> > - surface = cairo_surface_create_similar (dst,
> > - CAIRO_FORMAT_ARGB32,
> > - width, height);
> > +
> > + surface = cairo_surface_create_similar (dst, CAIRO_FORMAT_ARGB32,
> > + image->width,
> > + image->height);
>
> What the difference here?
It makes a difference if x or y are negative, and when using
cairo_surface_set_image, surface dimensions must match image dimensions.
With width and height, this might not always be true.
>
> > if (surface == NULL)
> > return NULL;
> >
> > cairo_surface_set_filter (surface, cairo_surface_get_filter(src));
> > _cairo_surface_set_image (surface, image);
> > cairo_surface_set_matrix (surface, &(image->base.matrix));
> > + cairo_surface_set_repeat (surface, image->base.repeat);
> > cairo_surface_destroy (&image->base);
> >
> > return surface;
>
> Ooh, thanks. Comitted.
>
> > Index: src/cairo_png_surface.c
> > ===================================================================
> > RCS file: /cvs/cairo/cairo/src/cairo_png_surface.c,v
> > retrieving revision 1.12
> > diff -u -r1.12 cairo_png_surface.c
> > --- src/cairo_png_surface.c 27 Jan 2005 18:46:20 -0000 1.12
> > +++ src/cairo_png_surface.c 28 Jan 2005 15:41:05 -0000
> > @@ -53,7 +53,7 @@
> > int height)
> > {
> > cairo_surface_t *surface;
> > -
> > +
> > surface = cairo_png_surface_create (file, format,
> > width, height);
> >
> > @@ -239,7 +239,7 @@
> >
> > static cairo_int_status_t
> > _cairo_png_surface_composite (cairo_operator_t operator,
> > - cairo_surface_t *generic_src,
> > + cairo_pattern_t *pattern,
> > cairo_surface_t *generic_mask,
> > void *abstract_dst,
> > int src_x,
>
> Check, comitted.
>
> (XCB and Xlib changes omitted)
-David
More information about the cairo
mailing list