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.

3 files changed, 99 insertions, 11 deletions

diff --git a/src/cairo-image-surface.c b/src/cairo-image-surface.c
index 4a8f52b1..fdbd37cf 100644
--- a/src/cairo-image-surface.c
+++ b/src/cairo-image-surface.c
@@ -94,18 +94,29 @@ _cairo_format_from_pixman_format (pixman_format_t *pixman_format)
 
     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_format_t *pixman_format)
     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_surface_t *surface)
 	return 0;
     }
 
+    assert (CAIRO_FORMAT_VALID (image_surface->format));
+
     return image_surface->format;
 }
 
@@ -491,11 +508,20 @@ _cairo_format_from_content (cairo_content_t content)
 cairo_content_t
 _cairo_content_from_format (cairo_format_t format)
 {
-    switch (format) {
+    /* XXX: Use an int to avoid the warnings from mixed cairo_format_t
+     * and cairo_internal_format_t values. The warnings are extremely
+     * valuable since mixing enums can lead to subtle bugs. It's just
+     * that cairo_internal_format_t is an interim approach to getting
+     * bug #7294 fixed so we can release cairo 1.2.2 . */
+    int f = format;
+
+    switch (f) {
     case CAIRO_FORMAT_ARGB32:
+    case CAIRO_INTERNAL_FORMAT_ABGR32:
 	return CAIRO_CONTENT_COLOR_ALPHA;
     case CAIRO_FORMAT_RGB24:
     case CAIRO_FORMAT_RGB16_565:
+    case CAIRO_INTERNAL_FORMAT_BGR24:
 	return CAIRO_CONTENT_COLOR;
     case CAIRO_FORMAT_A8:
     case CAIRO_FORMAT_A1:
diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
index 58ad4599..8c453ba9 100644
--- a/src/cairo-xlib-surface.c
+++ b/src/cairo-xlib-surface.c
@@ -874,6 +874,9 @@ _cairo_xlib_surface_clone_similar (void			*abstract_surface,
     } 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 9d1c789d..f8d45026 100644
--- a/src/cairoint.h
+++ b/src/cairoint.h
@@ -278,6 +278,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
@@ -1896,6 +1925,36 @@ _cairo_surface_has_device_transform (cairo_surface_t *surface);
 
 /* 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)