[cairo] Re: new clipping change patch

[Keith Packard]

On Wed, 2005-05-25 at 09:49 -0400, Owen Taylor wrote:

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

Ok, I like that name better.  Note that I'm not doing anything to ensure
the serial numbers are actually unique, just trusting that 32 bits is
'big enough' that collisions are unlikely.


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

No, the case in question is just:

a	< change clip state >
b	cairo_save ();
c	< change anything but clip state, draw >
d	cairo_restore ();
e	< do some other drawing >

This is the case where preserving the clip serial numbers across gstate
changes is critical -- a) allocates a new clip serial, b) copies that
region and the serial number to the newly allocate gstate.  c) then sets
the clip into the surface.  At e), the same serial number will be in the
preserved gstate, so drawing there will not need to reset the clip.

> > 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);

Again, we want all identical regions to have the same serial number, so
if the region is copied before set into the surface, the copy will have
a serial of -1 and hence will also get a separate number allocated.  By
allocating the numbers along with the region value, we ensure that
future copies will be marked as identical.  See the case above.

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

Oh, that's certainly true, the question I have is whether we should use
boxes everywhere inside the API so we don't have this weird case where
the clip bounds representation covers a larger value space than the
coordinates do.

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

Using boxes everywhere guarantees this will be right.
> 
> > >  * 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
> 
> _______________________________________________
> cairo mailing list
> cairo at cairographics.org
> http://lists.freedesktop.org/mailman/listinfo/cairo
-------------- 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/98d8414e/attachment.pgp