[cairo] Re: new clipping change patch

[Keith Packard]

On Tue, 2005-05-24 at 14:48 -0400, Owen Taylor wrote:

> Hopefully this mail will get through to you one way or the other.

I had arranged for this problem in advance, having suffered through DSL
outages in the past.  My mail was neatly queued at fd.o and is slowly
trickling back in now that the link is up.

> I like this version of the patch a lot more than the last one. 

Yeah, I think it's getting cleaner each time.

> Various comments:
> 
>  * I'm not sure the name change cairo_surface_set_clip_region() =>
>    cairo_surface_clip_region() makes sense. It's still a setter...
>    there are no plans to make clip_region() intersect with the
>    current clip region.

I was just trying to preserve some symmetry with
cairo_surface_clip_path; that one can't be set_clip_path.  But, as they
are assymetrical in operation, they can be assymetrical in naming.

How about we reduce the symmetry even further:

	_cairo_surface_set_clip_region (surface,
  					    region,
					    serial)

	_cairo_surface_set_clip_none (surface);

	_cairo_surface_set_clip_paths (surface, serial);
	_cairo_surface_clip_path (surface, path);

We could reduce this to

	_cairo_surface_clip_path (surface, path, serial)

and make it reset the clip when the serial number changed, but that
feels way too mystic.  Alternatively, we could have

	_cairo_surface_reset_clip (surface);
	_cairo_surface_clip_path (surface, path, serial)

that way 'reset' would work for both no clipping and
to start path clipping.  I think I like this best.

>  * The code goes through various gymnastics to make sure that when
>    restoring to a previous clip serial, the same serial is used.

If you'll note, I also copy the serial number when I push the gstate
stack. By preserving the clip serial numbers in this way I ensure that
we don't reset the clip list on save/restore.  Perhaps we can retain
this behaviour and also make the gstate_set_clip function look nicer.

Does the above suggestion make things look better?

>    I don't see why this is useful. I'd expect 
>    _cairo_gstate_set_clip() to look like:
> 
>     if (_cairo_surface_get_clip_serial (surface) == gstate->clip.serial)
>       return CAIRO_STATUS_SUCCESS;
> 
>     /* reestablish the clip */
> 
>     gstate->clip.serial = _cairo_surface_get_clip_serial (surface);

and now save/restore will set the clip again...

>    And leave all updating of the serial internal to the surface.

Again, the serial number lets me copy the region around and avoid
hitting the surface clip function.  Seems like a useful optimization,
especiall as save/restore are supposed to be proportional to the changes
and not to the state size.

>    The "no clip gets 0 serial" optimization can still be done this
>    way.

Yeah, that seems like a useful common case.

>  * The duplication of rectangle intersection code one my time in
>    _cairo_gstate_get_clip_extents() is painful. That really should
>    be split out somewhere. (boolean return: FALSE on empty intersection)

I also really dislike the use of 'rectangles' with width/height for this
kind of computation.  The domain mismatch between rectangles and boxes
is really nasty here.

>    Or alternatively, for get_clip_extents(), do the intersection
>    on regions not rectangles.

Given a rectangle, we can't use regions to perform the intersection
because of range issues.

>  * cairo-gstate.c:_get_mask_extents() immediately intersects the 
>    result of get_clip_extents() with the clip region (as a region).
>    Which is duplication one way or the other.

What the heck is this function supposed to do?  The old code
does exactly what the patched code does, I note that in both cases the
'mask' argument (which also appears in the function name) is *unused*.

Presumably, it wants to get the extents of a surface within the mask
instead of the gstate surface.

>  
>  * I don't understand the logic in _cairo_gstate_set_target_surface()
>    calling _cairo_surface_get_next_clip_serial().

The clip serial number is relative to a specific surface; change the
surface, and you need to get a number relative to the new surface.  In
particular, the clip serial starts at 0 and until a surface is
associated, it will remain at 0.  If a clip region is applied before the
surface is associated, this 0 clip serial will inadvertantly match the
surface clip serial if that surface has no clip.

Given that we don't actually support changing surfaces, it might be
better to rework the gstate initialization code to include the surface
from the start...


>  * _cairo_xlib_surface_ensure_picture (surface, &surface->&src_picture);
> 
>   Maybe just split that into ensure_src_picture() and 
>   ensure_dst_picture()?

I suppose; this did save a function.  Would you accept a pair of macros?
Or is that too 80s?

> 
>  * In _cairo_xlib_surface_set_attributes(), the call to ensure_picture()
>    is inserted between a call that sets status and the subsequent
>    check of status. It should be inside the if (CAIRO_OK (status))

Oops.

-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/20050524/8eec4efe/attachment.pgp