[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