[cairo] Workaround patch for Xserver repeating pattern bug

[Owen Taylor]

On Mon, 2005-06-20 at 15:24 -0700, Carl Worth wrote:

> >  * I haven't tried to use the Xlib fallback code for the case where
> >    we don't have RENDER at all, though it is equally applicable
> >    there.
> 
> That one seems worth a comment in the code to me.

Added one.

> >  * A big problem with the core protocol fallback is that the
> >    screens must match for the source and destination drawables,
> >    but we don't have that information, since the screen or
> >    root window isn't a parameter to the Xlib surface constructor.
> [...]
> >    Rather than round-tripping, I've just disabled the optimization
> >    for multi-head displays.
> 
> Ouch. We should figure out how to fix this. Obviously we could add
> cairo_xlib_surface_set_screen, but then the user would have no way of
> knowing whether it was important to call this function in order to
> avoid a round-trip. All we could say in the documentation is that the
> user should _always_ call it just in case. But then, aren't we just
> documenting the presence of a performance trap.
> 
> As much as I hate to rev. it again, and as much as I hate to add yet
> another parameter to cairo_xlib_surface_create, perhaps that's what's
> forced on us here, (by what I consider a protocol defect). Keith,
> Owen?

Certainly most of the same arguments apply here as width/height.
I sort of gave up on fighting over the interface then, so I'll leave
it to you and Keith.

(Just like the width/height case, we can frequently avoid the
roundtrip if we are sufficiently smart, but not always.)

> Heck, if nothing else, changing cairo_xlib_surface_create will fix one
> thing I noticed when porting fdclock this morning. The current
> prototype:
> 
> 	(Display*, Drawable, Visual*, int width, int height)
> 
> just so happens to be type-compatible with an old prototype:
> 
> 	(Display*, Drawable, Visual*, cairo_format_t, Colormap)
> 
> Which, as you might imagine, led to some not-too-useful results when
> porting.
> 
> > In general, I'm not terribly happy with the patch though I think
> > it should work OK and I've tried to keep it pretty clean and
> > well documented: the whole way that _cairo_pattern_acquire_surfaces()
> > works makes it hard to do better.
> 
> I agree. But I don't have any quick fix for acquire_surfaces. The
> current patch looks quite functional to me, and is well-commented. The
> redundancy is unfortunate, (both in readability in performance), but I
> think it's clear enough to guide us through a better implementation
> later.
> 
> So, given the urgency of this workaround, I think the patch is worth
> committing, (with the important fix noted below).
> 
> 
> > +	    if (!(!have_mask &&
> > +		  (operator == CAIRO_OPERATOR_SOURCE ||CAIRO_OPERATOR_OVER)))
> > +		return DO_UNSUPPORTED;
> 
> That test is obviously broken. I assume it should instead be:
> 
> 	    if (!(!have_mask &&
> 		  (operator == CAIRO_OPERATOR_SOURCE ||
> 		   operator == CAIRO_OPERATOR_OVER)))
> 		return DO_UNSUPPORTED;

Yeah, fixed.

> The tripled negative does still make it hard to read, but I don't see
> how to do much better.

I applied one level of DeMorgan's to get:

            if (have_mask ||
                !(operator == CAIRO_OPERATOR_SOURCE || 
                  operator == CAIRO_OPERATOR_OVER))
                return DO_UNSUPPORTED;

I'm not sure that's clearer, but it's at least prettier.

Patch committed.

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/20050620/1ba8aabd/attachment.pgp