[cairo] patch for using XCopyArea when possible

[Owen Taylor]

Some comments on the patch:

====
+2005-07-15  Vladimir Vukicevic  <vladimir at pobox.com>
+
+	* src/cairo-xlib-surface.c: (_cairo_xlib_surface_composite,
+	_recategorize_composite_repeat): Use XCopyArea when
+	possible, for optimization and bug workaround.
====

"bug workaround" can you expand on this? I'm not aware of any bugs
that this patch would work around.

A patch like this the is always going to be a bit of a balancing
game - should we do something slower and more complex on the
client side to work around bad implementation in some servers?

In fact, in many recent servers, the CopyArea will actually go
through exactly the same code paths (fbCopyAreammx) as calling
RenderComposite..

I think this patch makes sense ... but I don't think we necessarily
want to do anything we can possibly do with Xlib with Xlib ..
if an operation can be expressed more simply with RENDER it may
be more likely to be optimized with RENDER in the long term.

Probably the right way of deciding is really seeing what ends
up being a problem in practice and optimizing that.

Index: src/cairo-xlib-surface.c
===================================================================
RCS file: /cvs/cairo/cairo/src/cairo-xlib-surface.c,v
retrieving revision 1.88
diff -u -8 -p -r1.88 cairo-xlib-surface.c
--- src/cairo-xlib-surface.c	14 Jul 2005 23:56:08 -0000	1.88
+++ src/cairo-xlib-surface.c	15 Jul 2005 18:51:32 -0000
@@ -833,17 +833,18 @@ _surface_has_alpha (cairo_xlib_surface_t
  * source pattern when the source is in off-screen video memory. When
that
  * bug could be triggered, we need a fallback: in the common case where
we have no
  * transformation and the source and destination have the same
format/visual,
  * we can do the operation using the core protocol, otherwise, we need
  * a software fallback.
  */

A whole bunch of comments in this section pretty carefully describe
what is going on. They need modification now that you've made the code
paths do something different.

 typedef enum {
     DO_RENDER,			/* use render */
-    DO_XLIB,			/* core protocol fallback */
+    DO_XCOPYAREA,		/* core protocol optimization/fallback */
+    DO_XTILE,			/* core protocol optimization/fallback */

Shouldn't these have different comments, since they do mean different
things?

@@ -895,29 +896,42 @@ _categorize_composite_repeat (cairo_xlib
  */
 static composite_operation_t
 _recategorize_composite_repeat (cairo_xlib_surface_t

Needs renaming, since there really isn't anything "repeating" about this
any more.

+    if (!have_mask &&
+	is_integer_translation &&
+	src_attr->extend == CAIRO_EXTEND_NONE &&
+	(operator == CAIRO_OPERATOR_SOURCE ||
+	 (operator == CAIRO_OPERATOR_OVER && (!_surface_has_alpha (src)))) &&

I think:

+	(operator == CAIRO_OPERATOR_SOURCE ||
+	 (operator == CAIRO_OPERATOR_OVER && (!_surface_has_alpha (src))))

Should be pulled out into a function since it is repeated multiple
places;
and in fact could be extended (ATOP and IN can get the same treatment
as OVER, maybe others?)

+    else if (operation == DO_XCOPYAREA)

Would strike me as a bit more natural to use a switch here than an if
chain.

+    cairo_rectangle (cr, 0, 0, SIZE-OFFSET, SIZE-OFFSET);

Test cases have a problem of not having spaces around operators
in a bunch of places.

-------------- 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/20050715/ec44f0d7/attachment.pgp