Attachment 6486 Details for Bug 7294 – Make cairo work like it did in 1.0 with an internal BGR format (rev 1)

[patch] Make cairo work like it did in 1.0 with an internal BGR format (rev 1)

7294-internal-format-1.patch (text/plain), 8.79 KB, created by Carl Worth on 2006-08-07 10:16:35 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Carl Worth

Created: 2006-08-07 10:16:35 UTC

Size: 8.79 KB

patch

obsolete

>commit 59e6c1b2ba9ca03af3e2891bbd38ac9da1448037
>Author: Carl Worth <cworth@cworth.org>
>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..fdbd37c 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;
> }
> 
>@@ -491,11 +508,20 @@ _cairo_format_from_content (cairo_conten
> 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 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)
>

Actions: View | Splinter Review

Attachments on bug 7294: 6011 | 6012 | 6013 | 6014 | 6094 | 6095 | 6166 | 6191 | 6331 | 6339 | 6417 | 6418 | 6460 | 6486

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.