Re: [PATCH] gl: Add BGRA download support for GLES2

On Wed, 05 Dec 2012 16:28:03 -0800, Martin Robinson <[email protected]> wrote:
> From 18d6453c59f1a774e7a8435354e3cec9ccf1114a Mon Sep 17 00:00:00 2001
> From: Martin Robinson <[email protected]>
> Date: Mon, 3 Dec 2012 16:08:23 -0800
> Subject: [PATCH] gl: Add BGRA download support for GLES2
> 
> Some OpenGLES2 drivers support downloading BGRA data. On little-endian
> systems BGRA and GL_UNSIGNED_BYTE is equivalent to the typical
> cairo_image_t format, so this can prevent CPU bit swizzling for
> operations that involve images.

Looks good to me, one minor comment.

> ---
>  src/cairo-gl-device.c  |   10 ++++++++++
>  src/cairo-gl-private.h |    1 +
>  src/cairo-gl-surface.c |   51 ++++++++++++++++++++++++------------------------
>  3 files changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/src/cairo-gl-device.c b/src/cairo-gl-device.c
> index 6563b3b..99b10f4 100644
> --- a/src/cairo-gl-device.c
> +++ b/src/cairo-gl-device.c
> @@ -225,6 +225,16 @@ _cairo_gl_context_init (cairo_gl_context_t *ctx)
>  			   (gl_flavor == CAIRO_GL_FLAVOR_ES &&
>  			    _cairo_gl_has_extension ("GL_OES_mapbuffer")));
>  
> +    /* CAIRO_FORMAT_ARGB32 is stored based on the endianness of the CPU. For
> +     * desktop GL we can always read this format directly since glReadPixels
> +     * with GL_UNSIGNED_INT_8_8_8_8_REV + GL_BGRA will produce this. OpenGLES2
> +     * does not support GL_UNSIGNED_INT_8_8_8_8_REV and GL_UNSIGNED_BYTE orders
> +     * the components similarly regardless of endianness. Thus, GL_BGRA
> +     * is only equivalent to CAIRO_FORMAT_ARGB32 on little-endian CPUs. */
> +    ctx->can_read_bgra = gl_flavor == CAIRO_GL_FLAVOR_DESKTOP ||
> +	(_cairo_is_little_endian () && gl_flavor == CAIRO_GL_FLAVOR_ES &&
> +	    _cairo_gl_has_extension ("EXT_read_format_bgra"));

For complex conditionals like this, I have a preference for breaking the
predicate into its own little function and being a litlte more verbose:

ctx->can_read_bgra = test_can_read_bgra(gl_flavor);

static cairo_bool_t test_can_read_bgra(cairo_gl_flavor_t gl_flavor)
{
  /* Desktop GL always supports BGRA formats, but for ES we have to
   * look for a specific extension, and that only matches with cairo's
   * byte packing for litte-endian machines.
   */
  if (gl_flavor == CAIRO_GL_FLAVOR_DESKTOP)
     return true;

  if (gl_flavor != CAIRO_GL_FLAVOR_ES)
     return false;

  if (!_cairo_is_little_endian()
     return flse;

  return _cairo_gl_has_extension ("EXT_read_format_bgra"));
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo