[cairo] Re: new clipping change patch

[Owen Taylor]

On Wed, 2005-05-25 at 10:42 -0700, Keith Packard wrote:
> 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.

Doesn't worry me. Collisions should never happen *sooner* than 
2^32 clip operations, and most of the time not even then. 

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

Hmm, so you are getting bitten by delaying setting the clip in fear
of someone else changing the clip in the meantime. I'm not convinced
that delay is that useful, but given that, the allocate-ids scheme
should work.

> > > Perhaps we can retain
> > > this behaviour and also make the gstate_set_clip function look nicer.
> > > 
> > > Does the above suggestion make things look better?
> > 1
> > 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.

I don't really have a strong feeling here. Certainly a fair number of
operations are easier to code on boxes, but you could argue for 
rectangles on analogy to the public API. I *don't* like at all the
fact that cairo_box_t is fixed point while cairo_rectangle_t is 
integer. 

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

I think what you are saying is that a 16-bit rectangle 

 x=32757, y=32767, width=65535, height=65535

or whatever is isn't representable as a pixman_region16_t, while all
boxes are representable as pixman_region16_t.

Is that really relevant? We don't care about the parts of the coordinate
space outside of the (-32768,-32768) ... (32767,32767) range, so
clamping to that range when converting to region should be fine. 
And pixman_region_union_rect() takes integer arguments, not short 
arguments, so it should be clamping anyways.

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/812b05bd/attachment.pgp