cogl: Don't assume that CoglBitmaps are allocated to height*rowstride

Cogl no longer has any code that assumes the buffer in a CoglBitmap is
allocated to the full size of height*rowstride. We should comment that
this is the case so that we remember to keep it that way. This is
important for cogl_texture_new_from_data because the application may
have created the data from a sub-region of a larger image and in that
case it's not safe to read the full rowstride of the last row when the
sub region contains the last row of the larger image.

http://bugzilla.clutter-project.org/show_bug.cgi?id=2491

diff --git a/clutter/cogl/cogl/cogl-bitmap-pixbuf.c b/clutter/cogl/cogl/cogl-bitmap-pixbuf.c
index 80ed00c..f0bd0ee 100644 (file)
--- a/clutter/cogl/cogl/cogl-bitmap-pixbuf.c
+++ b/clutter/cogl/cogl/cogl-bitmap-pixbuf.c
@@ -198,13 +198,8 @@ _cogl_bitmap_from_file (const char   *filename,
   int               width;
   int               height;
   int               rowstride;
-  int               aligned_rowstride;
   int               bits_per_sample;
   int               n_channels;
-  guint8           *pixels;
-  guint8           *out_data;
-  guint8           *out;
-  int               r;
 
   g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
 
@@ -247,55 +242,18 @@ _cogl_bitmap_from_file (const char   *filename,
       return FALSE;
     }
 
-  /* Work out what the rowstride would be if it was packing the data
-     but aligned to 4 bytes */
-  aligned_rowstride = (width * n_channels + 3) & ~3;
-
-  /* The documentation for GdkPixbuf states that is not safe to read
-     all of the data as height*rowstride because the buffer might not
-     be allocated to include the full length of the rowstride for the
-     last row so arguably we should always copy the buffer when
-     rowstride != width*bpp because some places in Cogl assume that it
-     can memcpy(height*rowstride). However that rule is probably only
-     in place so that GdkPixbuf can implement gdk_pixbuf_new_subpixbuf
-     by just sharing the data and setting a large rowstride. That does
-     not apply in this case because we are just creating a new buffer
-     for a file. It seems very unlikely that GdkPixbuf would not
-     allocate the full rowstride in this case and it is highly
-     desirable to avoid copying the buffer. This instead just assumes
-     that whatever buffer pixbuf points into will always be allocated
-     to a 4-byte aligned buffer so we can avoid copying unless the
-     rowstride is unusually large */
-  if (rowstride <= aligned_rowstride)
-    return _cogl_bitmap_new_from_data (gdk_pixbuf_get_pixels (pixbuf),
-                                       pixel_format,
-                                       width,
-                                       height,
-                                       rowstride,
-                                       _cogl_bitmap_unref_pixbuf,
-                                       pixbuf);
-
-  pixels   = gdk_pixbuf_get_pixels (pixbuf);
-  out_data = g_malloc (aligned_rowstride * height);
-  out      = out_data;
-
-  for (r = 0; r < height; ++r)
-    {
-      memcpy (out, pixels, n_channels * width);
-      pixels += rowstride;
-      out += aligned_rowstride;
-    }
-
-  /* Destroy GdkPixbuf object */
-  g_object_unref (pixbuf);
-
-  return _cogl_bitmap_new_from_data (out_data,
+  /* We just use the data directly from the pixbuf so that we don't
+     have to copy to a seperate buffer. Note that Cogl is expected not
+     to read past the end of bpp*width on the last row even if the
+     rowstride is much larger so we don't need to worry about
+     GdkPixbuf's semantics that it may under-allocate the buffer. */
+  return _cogl_bitmap_new_from_data (gdk_pixbuf_get_pixels (pixbuf),
                                      pixel_format,
                                      width,
                                      height,
-                                     aligned_rowstride,
-                                     (CoglBitmapDestroyNotify) g_free,
-                                     NULL);
+                                     rowstride,
+                                     _cogl_bitmap_unref_pixbuf,
+                                     pixbuf);
 }
 
 #else

diff --git a/clutter/cogl/cogl/cogl-bitmap-private.h b/clutter/cogl/cogl/cogl-bitmap-private.h
index bf21a57..a1f3066 100644 (file)
--- a/clutter/cogl/cogl/cogl-bitmap-private.h
+++ b/clutter/cogl/cogl/cogl-bitmap-private.h
@@ -180,6 +180,14 @@ _cogl_bitmap_get_height (CoglBitmap *bitmap);
 int
 _cogl_bitmap_get_rowstride (CoglBitmap *bitmap);
 
+/* Maps the bitmap so that the pixels can be accessed directly or if
+   the bitmap is just a memory bitmap then it just returns the pointer
+   to memory. Note that the bitmap isn't guaranteed to allocated to
+   the full size of rowstride*height so it is not safe to read up to
+   the rowstride of the last row. This will be the case if the user
+   uploads data using gdk_pixbuf_new_subpixbuf with a sub region
+   containing the last row of the pixbuf because in that case the
+   rowstride can be much larger than the width of the image */
 guint8 *
 _cogl_bitmap_map (CoglBitmap *bitmap,
                   CoglBufferAccess access,