[cairo] Clip region problems

[Keith Packard]

On Wed, 2005-05-18 at 12:27 -0400, Owen Taylor wrote:

> I assume you have application locking in place here? Or are you
> proposing we add explicit locking for the internal state of
> cairo_surface_t and the various backends.

Yes, of course.

> (Note that we do things like temporarily change the picture 
> attributes of surfaces when using them as sources... the need
> to lock *two* surfaces for a drawing operation would definitely
> raise some fun deadlock issues)
> 
> > Before the BAD_NESTING state, this worked just fine.  
> 
> Actually, no it didn't, not if you were using clipping :-)

Right, and in the cases I'm using, that's not a problem.  In fact,
another non-threaded case that another example uses just wanted to use
different graphic states on the same surface in an interleaved fashion:

	cr1 = cairo_create (surface);
	cr2 = cairo_create (surface);
	...
	cairo_stroke (cr1);
	...
	cairo_stroke (cr2);

In this case, as long as there isn't any surface state changes that must
survive a single cairo call, things should "just work".

> 
> > Now I get an assertion failure and my program aborts.
> 
> I'm actually curious why you got an assertion failure rather than
> a silent status (I have the suspicion that there might be some bugs in 
> cairo in this area)

Yes, I noticed a bug -- the assertion is checking for valid error values
and BAD_NESTING isn't one of those.

> But in any case, it seems that the BAD_NESTING error handling is
> achieving its purpose ... to catch problems and make people think
> about better ways of handling the situation :-)

Indeed -- useful programming strategies (not the threaded example, which
is somewhat contrived, but the parallel cairo_t example above) are not
possible with a strictly nested model.

> 
> > I think we should revisit the discussion which prompted the current
> > state and see if we really want to keep things like this.
> > 
> > To me, a surface is an object without any state aside from the image
> > drawn there. 
> 
> Any *public* state presumably. But this model really only applies at
> most to "pixel" surfaces. There is obviously other state in, for 
> example, a paginated surface.

Right, I think it's this hidden state which is at issue; the public
state (page number, pixel contents, etc) is visible to the application
and managed through direct application calls.

> 
> > Ok, so I believe the BAD_NESTING status was added to allow the cairo_t
> > to place clipping information directly into the backend for efficient
> > rectangular pixel-aligned clipping.  A key requirement for efficient
> > cairo usage in a windowed environment.
> 
> Well, that's a bit ahistorical ... the clipping was placed into the
> backend, and then later the clipping stack (and thus the BAD_NESTING
> error) was added to make it actually work.

Right, I'm just hoping to move it back out of the surface and up into
the cairo_t where it is already nicely stacked and make the surface
responsible only for setting the current clipping in an efficient
fashion.

> (Clipping isn't usable in 0.4.0, because it leaks out onto the next
> user of the surface. This was a huge problem for GTK+.)

I'm certainly not proposing that cairo_t state reflected as private
state in the surface be visible to users; that is admittedly far worse
than the current limitation.

> One of the understandings of adding nesting restrictions is that they
> could always be loosened later if we figured out better ways of doing
> things.

I would like to try and see if I can't do that then.

> > The cairo_surface_t clip region is usually just a copy of the gstate
> > clip region, at which times it would be more efficient to just use the
> > gstate clip_region directly and not store it in the cairo_surface_t as
> > well.  The only unusual case is when compositing trapezoids, the
> > cairo_surface_t clip region is temporarily set to the trapezoid region.
> > 
> > It seems to me what we want is instead of making the surface push/pop
> > clip lists, we just want the gstate to tell the surface which clip list
> > is in use for each operation and then have the surface cache updates to
> > the backend itself.
> 
> One thing to be aware of here is that we almost certainly need clip
> *paths* in the surface vtable for the metasurface and the PDF/PS
> backends. The interface that I've been discussing with Kristian is
> along the lines of:
> 
>  surface.intersect_clip_with_path (path);
>  surface.reset_clip ();
> 
> (The reason it's intersect_clip_with_path() rather than set_clip_path()
> is to allow us to avoid doing complex computation geometry for 
> path intersection)
> 
> So any solution that is specific to rectangular clip lists (for example,
> the COW pixman_region objects that were discussed earlier), isn't going
> to work.

Right, so clip objects are polymorphic arrays of rectangle-sets and
paths.  The key is to keep them stored entirely in the cairo_t and only
cache them in the surface.

> > We just need a way to globally identify region contents and the surface
> > can tell when the backend must be informed about a new clip list.  In a
> > similar situation, the X server uses 'serial numbers' -- integers
> > created by a global counter.  This will be hard in cairo without
> > locking, but as a cairo_t (and gstate, by associatio) can only be used
> > with a single surface_t, it seems like we can place a serial number
> > generator in each surface and use that.
> > 
> > Surely I'm missing something here; this seems like it will be both more
> > general (allowing parallel cairo_t objects to reference the same
> > surface) and more efficient (by eliminating duplicate regions in the
> > cairo_surface_t).
> 
> So, what you are proposing is basically, that before every drawing
> operation, we do:
> 
>  if (gstate->clip_serial != gstate->surface->clip_serial)
>     gstate_reestablish_clip (gstate);

Yes.

> 
> (With some available invalid value used initially for gstate-
> >clip_serial)
> 
> It's an approach I hadn't thought of, and might in fact be workable.

Cool.  I'm glad you haven't already figured out that it's impossible...

> 
> * It does remove some information from the backend. It's going to 
> be harder to optimize sequences like:
> 
>  - Clip to one path
>  - Save
>  - Clip to another path
>  - draw
>  - Restore
>  - draw 
> 
> In the PS or PDF backend, because the surface no longer sees the
> stack of clips. While in PS/PDF it would be possible to encode the
> above with only two clips, with the clip serial approach, the
> end result is:
> 
>  - Save
>  - Clip to one path
>  - Clip to another path
>  - draw
>  - Restore
>  - Save
>  - Clip to one path
>  - draw
>  - Restore

Yes, this looks correct.  Note that for multiple cairo_t objects to be
usable in a single-threaded application, every cairo function call must
appear atomic wrt the surface.  Is this possible?

> 
> Nested clipping when drawing to paper isn't probably common enough
> to make this a big worry.

And, if it does, we could consider making either the interface or the
backend smarter, but I suspect that's unlikely to happen...

> 
> * Using a surface as a source for drawing to itself is going to be a
> problem, but it's already a problem, so that's nothing new.

Yes, clipping in this environment is ill-defined.  X actually defines
the clip in this case only for output; input is not affected by
clipping.

> 
> In general, it seems like a promising idea.

I'll see if I can't make it work then.

-keith

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050518/f49f275a/attachment.pgp