[cairo] pattern/glitz update merge

[David Reveman]

On Mon, 2005-02-07 at 14:01 -0500, Owen Taylor wrote: 
> On Mon, 2005-02-07 at 17:57 +0100, David Reveman wrote:
> > 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. 
> 
> If I understand your argument, what you are saying is that:
> 
>  - In the case where we are creating a new win32_surface/image_surface
>    pair, the image_surface needs to reference the win32_surface, but
>    not vice-versa since the win32_surface is never exposed.
> 
>  - In the case where we returning the image_surface for an existing
>    win32_surface/image_surface pair, the win32_surface already
>    references the image surface, but we don't need the reverse link
>    since the win32_surface is going to be kept around until the 
>    image_surface is freed.
> 
> While that is true, it introduces to very ugly things:
> 
> In the backend:
> 
>  Whether or not win32_surface->image should be destroyed when the
>  win32_surface is destroyed will depend on how the win32_surface
>  was created.
> 
> In the calling code:
> 
>  The caller must make sure that all references to the acquired
>  image are dropped before it returns (and the win32_surface might
>  be destroyed.) So the "freedom" of having replacing
>  acquire/release_source_image() with a get_image() is largely
>  illusory, but it's much easier to slip up.
> 
> That's something I don't want to see. If you badly need a 
> get_source_image() rather than acquire/release_source_image()
> pair, I do know how to implement it in the win32 backend. What
> you do is replace
>  
>                  => image_surface
>  win32_surface
>                  => HDC,DIB
> 
> With:
> 
>  win32_surface => image_surface => HDC,DIB
> 
> That is, you attach the DIB to the image_surface using 
> cairo_surface_set_data (or your ad-hoc system), so that we
> can free the win32 surface but keep the image_surface around.
> 
> That actually allows you to have a get_source_image() which really
> delivers the promise of "here is an image, do what you want with
> it". On the other hand, it is some rewriting to change things
> to work that way. What 
> particular problems did acquire/release_source_image cause for you?
> 
> [...]
> 
> > 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.
> 
> The pain is largely intentional ... it forces the discipline needed
> to get the memory management right. We can get rid of the extra
> pointer easily enough by introducing cairo_surface_set_data(), but
> the need for rigorous pairing of acquire/release is still there unless
> we change the win32 backend (and similar backends, though there are 
> none currently) as outlined above.

ok, I see you point and as you've already went back and forth on this,
you're probably right. I'll see if I can get the pairing of
acquire/release working within my pattern updates.

> 
> > > 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.
> 
> Well, the advantage would be that it would be easier to review your 
> patch and see what it is doing. Right now, I look at it, and have
> thoughts like "Oh yeah, this is the gstate pattern initialization stuff
> we discussed earlier".
> 
> The problem with just accumulating patches is that I'm afraid that
> we're in a situation where we go from
> 
>  XXXXXXXXXYYYYYYYYYYY
>  ^^^^^^^^^-----------
>     OK      NOT OK
> 
> To:
> 
>   XXXXXXXXXYYYYYYYYYYZZZZZZZZZZZ
>   ^^^^^^^^^^^^^^^^^^^
>          OK            NOT OK
> 
> That we'll never come to a consensus. It would be a lot easier if 
> there was a patch that was only XXXXXXX even if the YYYYYYY patch
> depended upon it.

yeah, I know, you should never allow a patch to grow this big.

The patch includes: 
1. gstate pattern initialization fix - this should go in separately.

2. general pattern update - includes changes to the pattern struct and
changes to cairo_pattern_get_surface. this propagates a lot of changes
and is going to be hard to split up, but if that's necessary, I'll see
what I can do.

3. glitz backend update - a lot of stuff, almost a rewrite of the glitz
backend, depends on the pattern update.

I can separate these three, quite easily. Is that enough? Or do you
think we need to split up number 2 as well?

-David