commit 116f7be3ce7ddcf4a2b56a305bca8e5fe9a9e17d Author: Carl Worth Date: Fri Aug 4 16:06:59 2006 -0700 Fix bug 7294 by adding pixman BGR formats and internal cairo BGR formats. This approach to fixing the bug is valid since there is code in pixman for rendering to BGR images, (which is why cairo 1.0 worked with BGR X servers for example). But, since we don't want to advertise additional image formats we implement this through a new cairo_internal_format_t. This is rather fragile since we don't want to leak any internal formats nor do we ever want an internal format to be used somewhere a real format is expected, (and trigger a CAIRO_FORMAT_VALID assertion failure). More comments than code are added here to help compensate for the fragility and to give some guidance in fixing this mess in a better way in the future. diff --git a/pixman/src/icformat.c b/pixman/src/icformat.c index 62a171d..9dd1936 100644 --- a/pixman/src/icformat.c +++ b/pixman/src/icformat.c @@ -53,6 +53,18 @@ pixman_format_create (pixman_format_name 0xf800, 0x07e0, 0x001f); + case PIXMAN_FORMAT_NAME_ABGR32: + return pixman_format_create_masks (32, + 0xff000000, + 0x000000ff, + 0x0000ff00, + 0x00ff0000); + case PIXMAN_FORMAT_NAME_BGR24: + return pixman_format_create_masks (32, + 0x0, + 0x000000ff, + 0x0000ff00, + 0x00ff0000); } return NULL; diff --git a/pixman/src/pixman.h b/pixman/src/pixman.h index ca24e33..156dcf5 100644 --- a/pixman/src/pixman.h +++ b/pixman/src/pixman.h @@ -227,7 +227,9 @@ typedef enum pixman_format_name { PIXMAN_FORMAT_NAME_RGB24, PIXMAN_FORMAT_NAME_A8, PIXMAN_FORMAT_NAME_A1, - PIXMAN_FORMAT_NAME_RGB16_565 + PIXMAN_FORMAT_NAME_RGB16_565, + PIXMAN_FORMAT_NAME_ABGR32, + PIXMAN_FORMAT_NAME_BGR24 } pixman_format_name_t; typedef struct pixman_format pixman_format_t; diff --git a/src/cairo-image-surface.c b/src/cairo-image-surface.c index 4a8f52b..fc7c6b1 100644 --- a/src/cairo-image-surface.c +++ b/src/cairo-image-surface.c @@ -94,18 +94,29 @@ _cairo_format_from_pixman_format (pixman pixman_format_get_masks (pixman_format, &bpp, &am, &rm, &gm, &bm); + /* See definition of cairo_internal_format_t for an explanation of + * the CAIRO_INTERNAL_FORMAT values used here. */ switch (bpp) { case 32: - if (am == 0xff000000 && - rm == 0x00ff0000 && - gm == 0x0000ff00 && - bm == 0x000000ff) - return CAIRO_FORMAT_ARGB32; - if (am == 0x0 && - rm == 0x00ff0000 && - gm == 0x0000ff00 && - bm == 0x000000ff) - return CAIRO_FORMAT_RGB24; + if (am == 0xff000000) { + if (rm == 0x00ff0000 && + gm == 0x0000ff00 && + bm == 0x000000ff) + return CAIRO_FORMAT_ARGB32; + if (rm == 0x000000ff && + gm == 0x0000ff00 && + bm == 0x00ff0000) + return CAIRO_INTERNAL_FORMAT_ABGR32; + } else if (am == 0x0) { + if (rm == 0x00ff0000 && + gm == 0x0000ff00 && + bm == 0x000000ff) + return CAIRO_FORMAT_RGB24; + if (rm == 0x000000ff && + gm == 0x0000ff00 && + bm == 0x00ff0000) + return CAIRO_INTERNAL_FORMAT_BGR24; + } break; case 16: if (am == 0x0 && @@ -145,6 +156,10 @@ _cairo_format_from_pixman_format (pixman return (cairo_format_t) -1; } +/* XXX: This function really should be eliminated. We don't really + * want to advertise a cairo image surface that supports any possible + * format. A minimal step would be to replace this function with one + * that accepts a cairo_internal_format_t rather than mask values. */ cairo_surface_t * _cairo_image_surface_create_with_masks (unsigned char *data, cairo_format_masks_t *format, @@ -400,6 +415,8 @@ cairo_image_surface_get_format (cairo_su return 0; } + assert (CAIRO_FORMAT_VALID (image_surface->format)); + return image_surface->format; } diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c index 58ad459..8c453ba 100644 --- a/src/cairo-xlib-surface.c +++ b/src/cairo-xlib-surface.c @@ -874,6 +874,9 @@ _cairo_xlib_surface_clone_similar (void } else if (_cairo_surface_is_image (src)) { cairo_image_surface_t *image_src = (cairo_image_surface_t *)src; + if (! CAIRO_FORMAT_VALID (image_src->format)) + return CAIRO_INT_STATUS_UNSUPPORTED; + clone = (cairo_xlib_surface_t *) _cairo_xlib_surface_create_similar_with_format (surface, image_src->format, image_src->width, image_src->height); diff --git a/src/cairoint.h b/src/cairoint.h index 447c1ac..f090eec 100644 --- a/src/cairoint.h +++ b/src/cairoint.h @@ -276,6 +276,35 @@ typedef enum cairo_internal_surface_type CAIRO_INTERNAL_SURFACE_TYPE_TEST_PAGINATED } cairo_internal_surface_type_t; +/* For xlib fallbacks, we use image surfaces with formats that match + * the visual of the X server. There are a couple of common X server + * visuals for which we do not have corresponding public + * cairo_format_t values, since we do not plan on always guaranteeing + * that cairo will be able to draw to these formats. + * + * So, currently pixman does provide support for these formats. It's + * possible that in the future we will change the implementation to + * instead convert to a supported format. This would allow us to be + * able to simplify pixman to handle fewer formats. + * + * The RGB16_565 case could probably have been handled this same way, + * (and in fact we could still change it to do so, and maybe just + * leave the value in the enum but deprecate it entirely). We can't + * drop the value since it did appear in cairo 1.2.0 so it might + * appear in code, (particularly bindings which are thorough about + * things like that). But we did neglect to update CAIRO_FORMAT_VALID + * for 1.2 so we know that no functional code is out there relying on + * being able to create an image surface with a 565 format, (which is + * good since things like write_to_png are missing support for the 565 + * format. + * + * NOTE: The implementation of CAIRO_FORMAT_VALID *must* *not* + * consider these internal formats as valid. */ +typedef enum cairo_internal_format { + CAIRO_INTERNAL_FORMAT_ABGR32 = 0x1000, + CAIRO_INTERNAL_FORMAT_BGR24 +} cairo_internal_format_t; + typedef enum cairo_direction { CAIRO_DIRECTION_FORWARD, CAIRO_DIRECTION_REVERSE @@ -1894,6 +1923,36 @@ _cairo_surface_has_device_transform (cai /* cairo_image_surface.c */ +/* XXX: In cairo 1.2.0 we added a new CAIRO_FORMAT_RGB16_565 but + * neglected to adjust this macro. The net effect is that it's + * impossible to externally create an image surface with this + * format. This is perhaps a good thing since we also neglected to fix + * up things like cairo_surface_write_to_png for the new format + * (-Wswitch-enum will tell you where). Is it obvious that format was + * added in haste? + * + * The reason for the new format was to allow the xlib backend to be + * used on X servers with a 565 visual. So the new format did its job + * for that, even without being considered "valid" for the sake of + * things like cairo_image_surface_create. + * + * Since 1.2.0 we ran into the same situtation with X servers with BGR + * visuals. This time we invented cairo_internal_format_t instead, + * (see it for more discussion). + * + * The punchline is that CAIRO_FORMAT_VALID must not conside any + * internal format to be valid. Also we need to decide if the + * RGB16_565 should be moved to instead be an internal format. If so, + * this macro need not change for it. (We probably will need to leave + * an RGB16_565 value in the header files for the sake of code that + * might have that value in it.) + * + * If we do decide to start fully supporting RGB16_565 as an external + * format, then CAIRO_FORMAT_VALID needs to be adjusted to include + * it. But that should not happen before all necessary code is fixed + * to support it (at least cairo_surface_write_to_png and a few spots + * in cairo-xlib-surface.c--again see -Wswitch-enum). + */ #define CAIRO_FORMAT_VALID(format) ((format) >= CAIRO_FORMAT_ARGB32 && \ (format) <= CAIRO_FORMAT_A1)