[cairo] Workaround patch for Xserver repeating pattern bug

[Carl Worth]

On Mon, 20 Jun 2005 14:07:40 -0400, Owen Taylor wrote:
>  * There are cases with this patch where we go ahead and create
>    Xlib surfaces for patterns and then realize that we'll trigger
>    the bug and end up throwing them away.

More on this below.

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

>  * I've made the out parameters to 
>   _cairo_matrix_is_integer_translation() optional.

I've committed that part (in a slightly different style).

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

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;

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

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050620/dd21f6e9/attachment.pgp