cogl: Support any format in cogl_read_pixels

cogl_read_pixels() no longer asserts that the format passed in is
RGBA_8888 but instead accepts any format. The appropriate GL enums for
the format are passed to glReadPixels so OpenGL should be perform a
conversion if neccessary.

It currently assumes glReadPixels will always give us premultiplied
data. This will usually be correct because the result of the default
blending operations for Cogl ends up with premultiplied data in the
framebuffer. However it is possible for the framebuffer to be in
whatever format depending on what CoglMaterial is used to render to
it. Eventually we may want to add a way for an application to inform
Cogl that the framebuffer is not premultiplied in case it is being
used for some special purpose.

If the requested format is not premultiplied then Cogl will convert
it. The tests have been changed to read the data as premultiplied so
that they won't be affected by the conversion. Picking in Clutter has
been changed to use COGL_PIXEL_FORMAT_RGB_888 because it doesn't need
the alpha component. clutter_stage_read_pixels is left unchanged
because the application can't specify a format for that so it seems to
make most sense to store unpremultiplied values.

http://bugzilla.openedhand.com/show_bug.cgi?id=1959

clutter/clutter-main.c

@@ -499,15 +499,15 @@ read_pixels_to_file (char *filename_stem,
   data = g_malloc (4 * width * height);
   cogl_read_pixels (x, y, width, height,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGB_888,
                     data);
   pixbuf = gdk_pixbuf_new_from_data (data,
                                      GDK_COLORSPACE_RGB,
-                                     TRUE, /* has alpha */
+                                     FALSE, /* has alpha */
                                      8, /* bits per sample */
                                      width, /* width */
                                      height, /* height */
-                                     width * 4, /* rowstride */
+                                     width * 3, /* rowstride */
                                      pixbuf_free, /* callback to free data */
                                      NULL); /* callback data */
   if (pixbuf)
@@ -549,7 +549,7 @@ _clutter_do_pick (ClutterStage   *stage,
 		  ClutterPickMode mode)
 {
   ClutterMainContext *context;
-  guchar              pixel[4] = { 0xff, 0xff, 0xff, 0xff };
+  guchar              pixel[3] = { 0xff, 0xff, 0xff };
   CoglColor           stage_pick_id;
   guint32             id;
   GLboolean           dither_was_on;
@@ -634,7 +634,7 @@ _clutter_do_pick (ClutterStage   *stage,
   CLUTTER_TIMER_START (_clutter_uprof_context, pick_read);
   cogl_read_pixels (x, y, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGB_888,
                     pixel);
   CLUTTER_TIMER_STOP (_clutter_uprof_context, pick_read);
 

clutter/cogl/cogl/cogl.c

@@ -41,6 +41,9 @@
 #include "cogl-framebuffer-private.h"
 #include "cogl-matrix-private.h"
 #include "cogl-journal-private.h"
+#include "cogl-bitmap-private.h"
+#include "cogl-texture-private.h"
+#include "cogl-texture-driver.h"
 
 #if defined (HAVE_COGL_GLES2) || defined (HAVE_COGL_GLES)
 #include "cogl-gles2-wrapper.h"
@@ -630,20 +633,20 @@ cogl_read_pixels (int x,
 {
   CoglHandle framebuffer;
   int        framebuffer_height;
-  int        rowstride = width * 4;
+  int        bpp;
-  guint8    *temprow;
+  CoglBitmap bmp;
+  GLenum     gl_intformat;
+  GLenum     gl_format;
+  GLenum     gl_type;
 
   _COGL_GET_CONTEXT (ctx, NO_RETVAL);
 
-  g_return_if_fail (format == COGL_PIXEL_FORMAT_RGBA_8888);
   g_return_if_fail (source == COGL_READ_PIXELS_COLOR_BUFFER);
 
   /* make sure any batched primitives get emitted to the GL driver before
    * issuing our read pixels... */
   cogl_flush ();
 
-  temprow = g_alloca (rowstride * sizeof (guint8));
-
   framebuffer = _cogl_get_framebuffer ();
 
   _cogl_framebuffer_flush_state (framebuffer, 0);
@@ -659,21 +662,47 @@ cogl_read_pixels (int x,
   if (!cogl_is_offscreen (framebuffer))
     y = framebuffer_height - y - height;
 
+  /* Initialise the CoglBitmap */
+  bpp = _cogl_get_format_bpp (format);
+  bmp.format = format;
+  bmp.data = pixels;
+  bmp.width = width;
+  bmp.height = height;
+  bmp.rowstride = bpp * width;
+
+  if ((format & COGL_A_BIT))
+    {
+      /* FIXME: We are assuming glReadPixels will always give us
+         premultiplied data so we'll set the premult flag on the
+         bitmap format. This will usually be correct because the
+         result of the default blending operations for Cogl ends up
+         with premultiplied data in the framebuffer. However it is
+         possible for the framebuffer to be in whatever format
+         depending on what CoglMaterial is used to render to
+         it. Eventually we may want to add a way for an application to
+         inform Cogl that the framebuffer is not premultiplied in case
+         it is being used for some special purpose. */
+      bmp.format |= COGL_PREMULT_BIT;
+    }
+
   /* Setup the pixel store parameters that may have been changed by
      Cogl */
-  GE (glPixelStorei (GL_PACK_ALIGNMENT, 1));
+  _cogl_texture_driver_prep_gl_for_pixels_download (bmp.rowstride, bpp);
-#ifdef HAVE_COGL_GL
-  GE (glPixelStorei (GL_PACK_ROW_LENGTH, 0));
-  GE (glPixelStorei (GL_PACK_SKIP_PIXELS, 0));
-  GE (glPixelStorei (GL_PACK_SKIP_ROWS, 0));
-#endif /* HAVE_COGL_GL */
 
-  GE (glReadPixels (x, y, width, height, GL_RGBA, GL_UNSIGNED_BYTE, pixels));
+  _cogl_pixel_format_to_gl (format, &gl_intformat, &gl_format, &gl_type);
+
+  GE (glReadPixels (x, y, width, height, gl_format, gl_type, pixels));
+
+  /* Convert to the premult format specified by the caller
+     in-place. This will do nothing if the premult status is already
+     correct. */
+  _cogl_bitmap_convert_premult_status (&bmp, format);
 
   /* NB: All offscreen rendering is done upside down so there is no need
    * to flip in this case... */
-  if (cogl_is_offscreen (framebuffer))
+  if (!cogl_is_offscreen (framebuffer))
-    return;
+    {
+      guint8 *temprow = g_alloca (bmp.rowstride * sizeof (guint8));
 
       /* TODO: consider using the GL_MESA_pack_invert extension in the future
        * to avoid this flip... */
@@ -684,12 +713,13 @@ cogl_read_pixels (int x,
           if (y != height - y - 1) /* skip center row */
             {
               memcpy (temprow,
-                  pixels + y * rowstride, rowstride);
+                      pixels + y * bmp.rowstride, bmp.rowstride);
-          memcpy (pixels + y * rowstride,
+              memcpy (pixels + y * bmp.rowstride,
-                  pixels + (height - y - 1) * rowstride, rowstride);
+                      pixels + (height - y - 1) * bmp.rowstride, bmp.rowstride);
-          memcpy (pixels + (height - y - 1) * rowstride,
+              memcpy (pixels + (height - y - 1) * bmp.rowstride,
                       temprow,
-                  rowstride);
+                      bmp.rowstride);
+            }
         }
     }
 }

clutter/cogl/cogl/cogl.h

@@ -936,7 +936,13 @@ typedef enum { /*< prefix=COGL_READ_PIXELS >*/
  *
  * This reads a rectangle of pixels from the current framebuffer where
  * position (0, 0) is the top left. The pixel at (x, y) is the first
- * read, and the data is returned with a rowstride of (width * 4)
+ * read, and the data is returned with a rowstride of (width * 4).
+ *
+ * Currently Cogl assumes that the framebuffer is in a premultiplied
+ * format so if @format is non-premultiplied it will convert it. To
+ * read the pixel values without any conversion you should either
+ * specify a format that doesn't use an alpha channel or use one of
+ * the formats ending in PRE.
  */
 void
 cogl_read_pixels (int x,

tests/conform/test-cogl-backface-culling.c

@@ -50,7 +50,7 @@ validate_part (int xnum, int ynum, gboolean shown)
                     TEXTURE_RENDER_SIZE - TEST_INSET * 2,
                     TEXTURE_RENDER_SIZE - TEST_INSET * 2,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixels);
 
   /* Make sure every pixels is the appropriate color */

tests/conform/test-cogl-blend-strings.c

@@ -135,7 +135,7 @@ test_blend (TestState *state,
 
   cogl_read_pixels (x_off, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     {
@@ -265,7 +265,7 @@ test_tex_combine (TestState *state,
 
   cogl_read_pixels (x_off, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     {

tests/conform/test-cogl-materials.c

@@ -53,7 +53,7 @@ check_pixel (TestState *state, int x, int y, guint32 color)
 
   cogl_read_pixels (x_off, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("  result = %02x, %02x, %02x, %02x\n",

tests/conform/test-cogl-multitexture.c

@@ -31,7 +31,7 @@ assert_region_color (int x,
   guint8 *data = g_malloc0 (width * height * 4);
   cogl_read_pixels (x, y, width, height,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     data);
   for (y = 0; y < height; y++)
     for (x = 0; x < width; x++)

tests/conform/test-cogl-offscreen.c

@@ -96,28 +96,28 @@ on_paint (ClutterActor *actor, void *state)
   /* red, top right */
   cogl_read_pixels (FRAMEBUFFER_WIDTH - 1, 0, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   g_assert (pixel[RED] == 0xff && pixel[GREEN] == 0x00 && pixel[BLUE] == 0x00);
 
   /* green, top left */
   cogl_read_pixels ((FRAMEBUFFER_WIDTH/2), 0, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   g_assert (pixel[RED] == 0x00 && pixel[GREEN] == 0xff && pixel[BLUE] == 0x00);
 
   /* blue, bottom right */
   cogl_read_pixels (FRAMEBUFFER_WIDTH - 1, (FRAMEBUFFER_HEIGHT/2) - 1, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   g_assert (pixel[RED] == 0x00 && pixel[GREEN] == 0x00 && pixel[BLUE] == 0xff);
 
   /* white, bottom left */
   cogl_read_pixels ((FRAMEBUFFER_WIDTH/2), (FRAMEBUFFER_HEIGHT/2) - 1, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   g_assert (pixel[RED] == 0xff && pixel[GREEN] == 0xff && pixel[BLUE] == 0xff);
 

tests/conform/test-cogl-premult.c

@@ -126,7 +126,7 @@ check_texture (TestState *state,
 
   cogl_read_pixels (x_off, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     {

tests/conform/test-cogl-readpixels.c

@@ -25,6 +25,7 @@ on_paint (ClutterActor *actor, void *state)
   CoglHandle tex;
   CoglHandle offscreen;
   guint32 *pixels;
+  guint8 *pixelsc;
 
   /* Save the Clutter viewport/matrices and load identity matrices */
 
@@ -70,7 +71,7 @@ on_paint (ClutterActor *actor, void *state)
   pixels = g_malloc0 (FRAMEBUFFER_WIDTH * 4 * FRAMEBUFFER_HEIGHT);
   cogl_read_pixels (0, 0, FRAMEBUFFER_WIDTH, FRAMEBUFFER_HEIGHT,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     (guchar *)pixels);
 
   g_assert_cmpint (pixels[0], ==, 0xff0000ff);
@@ -91,7 +92,7 @@ on_paint (ClutterActor *actor, void *state)
   pixels = g_malloc0 (FRAMEBUFFER_WIDTH * 4 * FRAMEBUFFER_HEIGHT);
   cogl_read_pixels (0, 0, FRAMEBUFFER_WIDTH, FRAMEBUFFER_HEIGHT,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     (guchar *)pixels);
 
   g_assert_cmpint (pixels[0], ==, 0xff0000ff);
@@ -100,6 +101,27 @@ on_paint (ClutterActor *actor, void *state)
   g_assert_cmpint (pixels[(FRAMEBUFFER_HEIGHT - 1) * FRAMEBUFFER_WIDTH + FRAMEBUFFER_WIDTH - 1], ==, 0xffffffff);
   g_free (pixels);
 
+  /* Verify using BGR format */
+
+  cogl_set_source_texture (tex);
+  cogl_rectangle (-1, 1, 1, -1);
+
+  pixelsc = g_malloc0 (FRAMEBUFFER_WIDTH * 3 * FRAMEBUFFER_HEIGHT);
+  cogl_read_pixels (0, 0, FRAMEBUFFER_WIDTH, FRAMEBUFFER_HEIGHT,
+                    COGL_READ_PIXELS_COLOR_BUFFER,
+                    COGL_PIXEL_FORMAT_BGR_888,
+                    (guchar *)pixelsc);
+
+  g_assert_cmpint (pixelsc[0], ==, 0x00);
+  g_assert_cmpint (pixelsc[1], ==, 0x00);
+  g_assert_cmpint (pixelsc[2], ==, 0xff);
+
+  g_assert_cmpint (pixelsc[(FRAMEBUFFER_WIDTH - 1) * 3 + 0], ==, 0x00);
+  g_assert_cmpint (pixelsc[(FRAMEBUFFER_WIDTH - 1) * 3 + 1], ==, 0xff);
+  g_assert_cmpint (pixelsc[(FRAMEBUFFER_WIDTH - 1) * 3 + 2], ==, 0x00);
+
+  g_free (pixelsc);
+
   cogl_handle_unref (tex);
 
 

tests/conform/test-cogl-texture-mipmaps.c

@@ -82,7 +82,7 @@ on_paint (ClutterActor *actor, TestState *state)
   /* Read back the two pixels we rendered */
   cogl_read_pixels (0, 0, 2, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixels);
 
   /* The first pixel should be just one of the colors from the

tests/conform/test-cogl-vertex-buffer-contiguous.c

@@ -43,7 +43,7 @@ validate_result (TestState *state)
   /* Should see a blue pixel */
   cogl_read_pixels (10, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("pixel 0 = %x, %x, %x\n", pixel[RED], pixel[GREEN], pixel[BLUE]);
@@ -52,7 +52,7 @@ validate_result (TestState *state)
   /* Should see a red pixel */
   cogl_read_pixels (110, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("pixel 1 = %x, %x, %x\n", pixel[RED], pixel[GREEN], pixel[BLUE]);
@@ -61,7 +61,7 @@ validate_result (TestState *state)
   /* Should see a blue pixel */
   cogl_read_pixels (210, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("pixel 2 = %x, %x, %x\n", pixel[RED], pixel[GREEN], pixel[BLUE]);
@@ -70,7 +70,7 @@ validate_result (TestState *state)
   /* Should see a green pixel, at bottom of 4th triangle */
   cogl_read_pixels (310, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("pixel 3 = %x, %x, %x\n", pixel[RED], pixel[GREEN], pixel[BLUE]);
@@ -79,7 +79,7 @@ validate_result (TestState *state)
   /* Should see a red pixel, at top of 4th triangle */
   cogl_read_pixels (310, y_off - 70, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("pixel 4 = %x, %x, %x\n", pixel[RED], pixel[GREEN], pixel[BLUE]);

tests/conform/test-cogl-vertex-buffer-interleved.c

@@ -47,7 +47,7 @@ validate_result (TestState *state)
   /* Should see a blue pixel */
   cogl_read_pixels (10, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("pixel 0 = %x, %x, %x\n", pixel[RED], pixel[GREEN], pixel[BLUE]);

tests/conform/test-cogl-vertex-buffer-mutability.c

@@ -34,7 +34,7 @@ validate_result (TestState *state)
   /* Should see a red pixel */
   cogl_read_pixels (110, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("pixel 0 = %x, %x, %x\n", pixel[RED], pixel[GREEN], pixel[BLUE]);
@@ -43,7 +43,7 @@ validate_result (TestState *state)
   /* Should see a green pixel */
   cogl_read_pixels (210, y_off, 1, 1,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     pixel);
   if (g_test_verbose ())
     g_print ("pixel 1 = %x, %x, %x\n", pixel[RED], pixel[GREEN], pixel[BLUE]);

tests/conform/test-cogl-viewport.c

@@ -27,7 +27,7 @@ assert_region_color (int x,
   guint8 *data = g_malloc0 (width * height * 4);
   cogl_read_pixels (x, y, width, height,
                     COGL_READ_PIXELS_COLOR_BUFFER,
-                    COGL_PIXEL_FORMAT_RGBA_8888,
+                    COGL_PIXEL_FORMAT_RGBA_8888_PRE,
                     data);
   for (y = 0; y < height; y++)
     for (x = 0; x < width; x++)