[cairo] pattern/glitz update merge

[Owen Taylor]

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.

> > 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.

Regards,
						Owen