[cairo] pattern/glitz update merge

[David Reveman]

On Sun, 2005-02-06 at 23:16 -0500, Owen Taylor wrote: 
> On Mon, 2005-02-07 at 04:55 +0100, David Reveman wrote:
> > I did some work today to get my updates to the pattern system and to the
> > glitz backend merged with current cvs. The patch is actually too big for
> > the list so I put it here:
> > http://www.cs.umu.se/~c99drn/cairo-pattern-merge-1.diff
> > 
> > It modifies the new fall-back system slightly. I moved some of the
> > fall-back stuff into the cairo_image_surface struct, it cleaned up
> > things quite nicely and made the merge a lot easier. 
> 
> You know, at one point last weekend, I had it pretty much as you
> have it in your patch, then rewrote it all because there were
> some very difficult to deal with memory management problems.

I don't think there's any general memory management problem with this
solution. acquire_source/dest returns an image surface and when this
surface is destroyed the backend is allowed to push these bits to the
real target surface. That's the way we want this fall-back system to
work, right? However, this means that a backend that wants to push bits
back to a target surface (by using the release_image call-back) have to
return a new cairo_image_surface from acquire_dest and not just a
reference, as the image surface will otherwise never be destroyed and
release_image will not be called correctly. I don't think this is too
bad, but to avoid problems, we should probably make something like the
following change to the image backend:

cairo_image_surface_t *
_cairo_image_surface_create_with_masks (char		     *data,
		cairo_format_masks_t		*format,
		int				width,
		int				height,
		int				stride,
		cairo_image_release_func_t	release,
		void                            *extra)

and maybe add this:

/* returns a copy of an image surface without copying the actual bits */
cairo_image_surface_t *
_cairo_image_surface_copy (cairo_image_surface_t *other,
		cairo_image_release_func_t	release,
		void                            *extra);

and the only way to set the release_image call-back should be through
these.

I don't think we should avoid this solution. Having to pass around the
"extra" pointer with the image and to track if it should be released
with release_source or cairo_surface_destroy is just too painful.

> 
> Look at the win32_acquire_source_image() in your patch. When a 
> new surface is returned, if I'm not misreading the patch, the reference
> count of 'local' is 1. The reference count of 'local->image' is 2. 
> (One from the function, one from the reference count that 'local'
> holds.)
> 
> When the caller unrefs the image, the reference count drops to 
> 1 and 1 ... classic refcount cycle. 
> 
> (On the other code path, you don't reference local->image at all,
> so it's going to segfault when the caller unrefs.)

Yeah, the stuff I changed in the win32 backend is just wrong. Sorry, I
shouldn't have touched that as I can't test or even compile the win32
backend right now when I don't have access to a Windows system.

> 
> > Another thing that might be of concern is that I've done some much
> > needed restructuring of the internal cairo_pattern struct, and it's now
> > a union instead of a struct, which meant I also had to change the
> > typedef in cairo.h. I don't see any problem with this, but maybe someone
> > else does?
> 
> How can we reduce the size of the diff between your working tree and
> CVS? Are there patches you can merge in independently of the rest?

I'm afraid not :(
I actually left some things out of it already, like the overall surface
alpha acceleration using a mask surface...
I can merge some small updates to cairo_gstate.c separately, but the big
pattern update thing needs to go in at once. I could also move out some
updates to the glitz backend and commit them separately but I don't see
any point in that.

-David