[cairo] Bug in _cairo_xlib_surface_create_similar

[Carl Worth]

On Thu, 18 May 2006 14:27:59 -0700, Carl Worth wrote:
> On Fri, 19 May 2006 08:26:08 +1200, Robert O'Callahan wrote:
> > My response was (and is) that the patch doesn't do that. If we can't
> > match the visual and depth, we fall back to
> > _cairo_xlib_surface_create_similar_with_format, i.e. the existing
> > behaviour.
> 
> Sorry about that. I should have looked closer at the patch and seen
> that that is obviously the case.

Part of what confused me here was the language "if the display doesn't
have COMPOSITE" which looks like we're making a choice based on the
presence of the COMPOSITE extension, which would be totally bogus.

In fact, though, the code is using a macro:

	CAIRO_SURFACE_RENDER_HAS_COMPOSITE

which is checking whether the Composite request within the RENDER
extension is available. That macro name is definitely not as clear as
it could be, but in my defense, I think I wrote it before the
COMPOSITE extension existed.

Owen's criticism still stands in a general sense---we shouldn't
prevent the X server from choosing how it will perform rendering. But
what we're detecting here is an X server that doesn't have the RENDER
extension at all, so by definition it can't make smart decisions about
how to implement RENDER operations.

Returning the image surface might still prevent some cairo
optimizations from working well, (such as the XCopyArea stuff), but I
think the non-RENDER X server case is uninteresting enough that I
don't care.

Although I do think I'd prefer to not make special cases for things
like this if haven't tested whether they actually help. That clutters
the code and makes a misleading impression that we're doing something
intentionally. (I checked and the original logic to return an image
surface for a non-Render X server is mine, but I called it a "pretty
lame heuristic" while Owen recast it as a "good first approximation
later, (see below)).

-Carl

commit 26148a1d15b710b8f2ce850f8704bf44b0c3d484
Author: Owen Taylor <otaylor at redhat.com>
Date:   Mon Jan 31 08:50:22 2005 +0000

[...]
    In the absence of of RENDER, make cairo_xlib_surface_create_similar() return an image surface.
[...]

 #define CAIRO_SURFACE_RENDER_HAS_FILL_RECTANGLE(surface)
  CAIRO_SURFACE_RENDER_AT_LEAST((surface), 0, 1)
 #define CAIRO_SURFACE_RENDER_HAS_FILL_RECTANGLES(surface)
  CAIRO_SURFACE_RENDER_AT_LEAST((surface), 0, 1)
@@ -157,18 +159,13 @@ _cairo_xlib_surface_create_similar (void
     Pixmap pix;
     cairo_xlib_surface_t *surface;

-    /* XXX: There's a pretty lame heuristic here. This assumes that
-     * all non-Render X servers do not support depth-32 pixmaps, (and
-     * that they do support depths 1, 8, and 24). Obviously, it would
-     * be much better to check the depths that are actually
-     * supported. */
-    if (!dpy
-       || (!CAIRO_SURFACE_RENDER_HAS_COMPOSITE (src)
-           && format == CAIRO_FORMAT_ARGB32))
-    {
-       return NULL;
+    /* As a good first approximation, if the display doesn't have
-    COMPOSITE,
+     * we're better off using image surfaces for all temporary
-    operations
+     */
+    if (!CAIRO_SURFACE_RENDER_HAS_COMPOSITE(src)) {
+       return cairo_image_surface_create (format, width, height);
     }
-------------- 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/20060518/034c9fce/attachment.pgp