[cairo] Re: new clipping change patch

[Owen Taylor]

On Tue, 2005-05-24 at 21:46 -0700, Keith Packard wrote:

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

Blech.

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

That's not bad. In combination with renaming 
_cairo_surface_get_next_clip_serial() to 
_cairo_surface_allocate_clip_serial(), things would be pretty
comprehensible.

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

Ah. For those following along, the combination being referred to here 
is:

 cairo_save();
 <something else changes the clip state>
 draw(); /* restores the clip state */
 cairo_restore(); /* should not re-restore the clip state */

> Perhaps we can retain
> this behaviour and also make the gstate_set_clip function look nicer.
> 
> Does the above suggestion make things look better?

Definitely. 

Another possibility would be to leave serial allocation
to the surface (every clip operation increments the serial) but have a
function to set the clip serial to a  specific value. Then
_cairo_gstate_clip() would set gstate->clip.serial to an invalid value
(-1, say), and the end of _cairo_gstate_set_clip() would look like:

 if (gstate->clip.serial == (unsigned int)-1)
    gstate->clip.serial = _cairo_surface_get_clip_serial (gstate->surface);
 else
    _cairo_surface_set_clip_serial (gstate->surface, gstate->clip.serial);

That strike me as a little simpler .. I'm not sure if it's better 
or not.

[...]

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

All the reason to encapsulate intersection into a function.

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

We are working in device space coordinates which are already limited to
16-bits. (by the use of 16.16 fixed point as well as regions.) If
pixman_region_union_rect() doesn't clamp the rectangle passed in,
that should be fixed.

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

What the function does is gets the extents of the mask operation.
(mask in the name is a verb, not a noun.) The mask argument isn't 
currently used, but would be needed for the useful optimization:

 - If the mask is a surface pattern, with extend=NONE and the 
   operator does nothing for transparent source (OVER, say)
   then the size of the intermediate surface is bounded by the
   size of the (transformed mask)

(This optimization can be done for the source as well)

Yes, your patch doesn't change the end result of the function, my
comment was that with the patch we end up doing:
 
 rect = get_surface_extents (gstate->surface);
 clip_rect = get_extents (gstate->clip.region);
 rect = intersect (rect, clip_rect);

 region = region_rect (rect);
 region = intersect (region, gstate->clip_region);

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

Could do with a comment :-). 

 /* allocate a new serial to represent our current state. Each 
    surface has its own set of serials */

say. I think my suggestion above with surface_set_clip_serial() does
come out a bit better here, since you can just reset to 
(unsigned int)-1.

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

Maybe leave the set_target_surface() code around until we see if we
need it for groups or not.

> >  * _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?

I think GCC is going to do better with the pair of functions... if
inlining makes sense, it has the power to do it.

Regards,
						Owen

-------------- 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/20050525/133ad218/attachment.pgp