GdkPixbuf: Formalize the difference between pixels/bytes storage

Instead of checking the bytes vs. pixels fields all over the place, we
introduce an enum that says which mode the pixbuf is in.  We have a
central downgrade_to_pixels() function to turn the pixbuf from
immutable to mutable.

This commit only does the gdk-pixbuf core; we'll deal with pixdata and
the loaders separately.

3 files changed, 133 insertions, 41 deletions

diff --git a/gdk-pixbuf/gdk-pixbuf-data.c b/gdk-pixbuf/gdk-pixbuf-data.c
index 769ead337..cd968d4ae 100644
--- a/gdk-pixbuf/gdk-pixbuf-data.c
+++ b/gdk-pixbuf/gdk-pixbuf-data.c
@@ -77,9 +77,9 @@ gdk_pixbuf_new_from_data (const guchar *data, GdkColorspace colorspace, gboolean
 			       "rowstride", rowstride,
 			       "pixels", data,
 			       NULL);
-        
-	pixbuf->destroy_fn = destroy_fn;
-	pixbuf->destroy_fn_data = destroy_fn_data;
+	g_assert (pixbuf->storage == STORAGE_PIXELS);
+	pixbuf->s.pixels.destroy_fn = destroy_fn;
+	pixbuf->s.pixels.destroy_fn_data = destroy_fn_data;
 
 	return pixbuf;
 }
diff --git a/gdk-pixbuf/gdk-pixbuf-private.h b/gdk-pixbuf/gdk-pixbuf-private.h
index cd26895f5..fc63d8f90 100644
--- a/gdk-pixbuf/gdk-pixbuf-private.h
+++ b/gdk-pixbuf/gdk-pixbuf-private.h
@@ -55,6 +55,26 @@ typedef struct _GdkPixbufClass GdkPixbufClass;
 /* Default fill color */
 #define DEFAULT_FILL_COLOR 0x979899ff
 
+typedef enum {
+        STORAGE_PIXELS,
+        STORAGE_BYTES
+} Storage;
+
+typedef struct {
+        /* The pixel array */
+        guchar *pixels;
+
+        /* Destroy notification function; it is supposed to free the pixel array */
+        GdkPixbufDestroyNotify destroy_fn;
+
+        /* User data for the destroy notification function */
+        gpointer destroy_fn_data;
+} Pixels;
+
+typedef struct {
+        GBytes *bytes;
+} Bytes;
+
 /* Private part of the GdkPixbuf structure */
 struct _GdkPixbuf {
         GObject parent_instance;
@@ -74,17 +94,12 @@ struct _GdkPixbuf {
 	/* Offset between rows */
 	int rowstride;
 
-	/* The pixel array */
-	guchar *pixels;
-
-	/* Destroy notification function; it is supposed to free the pixel array */
-	GdkPixbufDestroyNotify destroy_fn;
+        Storage storage;
 
-	/* User data for the destroy notification function */
-	gpointer destroy_fn_data;
-
-        /* Replaces "pixels" member (and destroy notify) */
-        GBytes *bytes;
+        struct {
+                Pixels pixels;
+                Bytes bytes;
+        } s;
 
 	/* Do we have an alpha channel? */
 	guint has_alpha : 1;
diff --git a/gdk-pixbuf/gdk-pixbuf.c b/gdk-pixbuf/gdk-pixbuf.c
index 8f220f44b..9afef4334 100644
--- a/gdk-pixbuf/gdk-pixbuf.c
+++ b/gdk-pixbuf/gdk-pixbuf.c
@@ -265,14 +265,42 @@ gdk_pixbuf_class_init (GdkPixbufClass *klass)
 }
 
 static void
+free_pixels (GdkPixbuf *pixbuf)
+{
+        g_assert (pixbuf->storage == STORAGE_PIXELS);
+
+        if (pixbuf->s.pixels.pixels && pixbuf->s.pixels.destroy_fn) {
+                (* pixbuf->s.pixels.destroy_fn) (pixbuf->s.pixels.pixels, pixbuf->s.pixels.destroy_fn_data);
+        }
+
+        pixbuf->s.pixels.pixels = NULL;
+}
+
+static void
+free_bytes (GdkPixbuf *pixbuf)
+{
+        g_assert (pixbuf->storage == STORAGE_BYTES);
+
+        g_clear_pointer (&pixbuf->s.bytes.bytes, g_bytes_unref);
+}
+
+static void
 gdk_pixbuf_finalize (GObject *object)
 {
         GdkPixbuf *pixbuf = GDK_PIXBUF (object);
-        
-        if (pixbuf->pixels && pixbuf->destroy_fn)
-                (* pixbuf->destroy_fn) (pixbuf->pixels, pixbuf->destroy_fn_data);
 
-        g_clear_pointer (&pixbuf->bytes, g_bytes_unref);
+        switch (pixbuf->storage) {
+        case STORAGE_PIXELS:
+                free_pixels (pixbuf);
+                break;
+
+        case STORAGE_BYTES:
+                free_bytes (pixbuf);
+                break;
+
+        default:
+                g_assert_not_reached();
+        }
         
         G_OBJECT_CLASS (gdk_pixbuf_parent_class)->finalize (object);
 }
@@ -688,6 +716,32 @@ gdk_pixbuf_get_pixels (const GdkPixbuf *pixbuf)
         return gdk_pixbuf_get_pixels_with_length (pixbuf, NULL);
 }
 
+static void
+downgrade_to_pixels (const GdkPixbuf *pixbuf)
+{
+        switch (pixbuf->storage) {
+        case STORAGE_PIXELS:
+                return;
+
+        case STORAGE_BYTES: {
+                GdkPixbuf *mut_pixbuf = (GdkPixbuf*)pixbuf;
+                gsize len;
+                Pixels pixels;
+
+                pixels.pixels = g_bytes_unref_to_data (pixbuf->s.bytes.bytes, &len);
+                pixels.destroy_fn = free_buffer;
+                pixels.destroy_fn_data = NULL;
+
+                mut_pixbuf->storage = STORAGE_PIXELS;
+                mut_pixbuf->s.pixels = pixels;
+                break;
+        }
+
+        default:
+                g_assert_not_reached();
+        }
+}
+
 /**
  * gdk_pixbuf_get_pixels_with_length: (rename-to gdk_pixbuf_get_pixels)
  * @pixbuf: A pixbuf.
@@ -710,19 +764,13 @@ gdk_pixbuf_get_pixels_with_length (const GdkPixbuf *pixbuf,
 {
 	g_return_val_if_fail (GDK_IS_PIXBUF (pixbuf), NULL);
 
-        if (pixbuf->bytes) {
-                GdkPixbuf *mut_pixbuf = (GdkPixbuf*)pixbuf;
-                gsize len;
-                mut_pixbuf->pixels = g_bytes_unref_to_data (pixbuf->bytes, &len);
-                mut_pixbuf->bytes = NULL;
-                mut_pixbuf->destroy_fn = free_buffer;
-                mut_pixbuf->destroy_fn_data = NULL;
-        }
+        downgrade_to_pixels (pixbuf);
+        g_assert (pixbuf->storage == STORAGE_PIXELS);
 
         if (length)
                 *length = gdk_pixbuf_get_byte_length (pixbuf);
 
-	return pixbuf->pixels;
+	return pixbuf->s.pixels.pixels;
 }
 
 /**
@@ -740,13 +788,20 @@ const guint8*
 gdk_pixbuf_read_pixels (const GdkPixbuf  *pixbuf)
 {
 	g_return_val_if_fail (GDK_IS_PIXBUF (pixbuf), NULL);
-        
-        if (pixbuf->bytes) {
+
+        switch (pixbuf->storage) {
+        case STORAGE_PIXELS:
+                return pixbuf->s.pixels.pixels;
+
+        case STORAGE_BYTES: {
                 gsize len;
                 /* Ignore len; callers know the size via other variables */
-                return g_bytes_get_data (pixbuf->bytes, &len);
-        } else {
-                return pixbuf->pixels;
+                return g_bytes_get_data (pixbuf->s.bytes.bytes, &len);
+        }
+
+        default:
+                g_assert_not_reached ();
+                return NULL;
         }
 }
 
@@ -766,11 +821,16 @@ gdk_pixbuf_read_pixel_bytes (const GdkPixbuf  *pixbuf)
 {
         g_return_val_if_fail (GDK_IS_PIXBUF (pixbuf), NULL);
 
-        if (pixbuf->bytes) {
-                return g_bytes_ref (pixbuf->bytes);
-        } else {
-                return g_bytes_new (pixbuf->pixels,
+        switch (pixbuf->storage) {
+        case STORAGE_PIXELS:
+                return g_bytes_new (pixbuf->s.pixels.pixels,
                                     gdk_pixbuf_get_byte_length (pixbuf));
+
+        case STORAGE_BYTES:
+                return g_bytes_ref (pixbuf->s.bytes.bytes);
+
+        default:
+                g_assert_not_reached();
         }
 }
 
@@ -879,7 +939,6 @@ gdk_pixbuf_fill (GdkPixbuf *pixbuf,
         guint w, h;
 
         g_return_if_fail (GDK_IS_PIXBUF (pixbuf));
-        g_return_if_fail (pixbuf->pixels || pixbuf->bytes);
 
         if (pixbuf->width == 0 || pixbuf->height == 0)
                 return;
@@ -1206,13 +1265,31 @@ gdk_pixbuf_set_property (GObject         *object,
                   notify = pixbuf->rowstride != g_value_get_int (value);
                   pixbuf->rowstride = g_value_get_int (value);
                   break;
+
+          /* The following two are a bit strange.  Both PROP_PIXELS and PROP_PIXEL_BYTES are
+           * G_PARAM_CONSTRUCT_ONLY properties, which means that GObject will generate default
+           * values for any missing one and call us for *both*.  So, we need to check whether the
+           * passed value is not NULL before actually setting pixbuf->storage.
+           */
           case PROP_PIXELS:
-                  notify = pixbuf->pixels != (guchar *) g_value_get_pointer (value);
-                  pixbuf->pixels = (guchar *) g_value_get_pointer (value);
+                  g_assert(pixbuf->s.pixels.pixels == NULL);
+                  notify = pixbuf->s.pixels.pixels != (guchar *) g_value_get_pointer (value);
+                  pixbuf->s.pixels.pixels = (guchar *) g_value_get_pointer (value);
+                  pixbuf->s.pixels.destroy_fn = NULL;
+                  pixbuf->s.pixels.destroy_fn_data = NULL;
+
+                  if (pixbuf->s.pixels.pixels != NULL) {
+                          pixbuf->storage = STORAGE_PIXELS;
+                  }
                   break;
           case PROP_PIXEL_BYTES:
-                  notify = pixbuf->bytes != g_value_get_boxed (value);
-                  pixbuf->bytes = g_value_dup_boxed (value);
+                  g_assert(pixbuf->s.bytes.bytes == NULL);
+                  notify = pixbuf->s.bytes.bytes != g_value_get_boxed (value);
+                  pixbuf->s.bytes.bytes = g_value_dup_boxed (value);
+
+                  if (pixbuf->s.bytes.bytes != NULL) {
+                          pixbuf->storage = STORAGE_BYTES;
+                  }
                   break;
           default:
                   G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
@@ -1258,7 +1335,7 @@ gdk_pixbuf_get_property (GObject         *object,
                   g_value_set_pointer (value, gdk_pixbuf_get_pixels (pixbuf));
                   break;
           case PROP_PIXEL_BYTES:
-                  g_value_set_boxed (value, pixbuf->bytes);
+                  g_value_set_boxed (value, gdk_pixbuf_read_pixel_bytes(pixbuf));
                   break;
           default:
                   G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);