[cairo] Workaround patch for Xserver repeating pattern bug
Carl Worth
cworth at redhat.com
Mon Jun 20 15:24:17 PDT 2005
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
More information about the cairo
mailing list