Re: [patch] gl: create BGRA texture and avoid conversion

ctx->supports_bgra is more meaningful, it means it supports both creating BGRA texture and can read that back into BGRA format.  The GL_EXT_texture_format_BGRA888 is requirement of cairo, as checked at line 223 in cairo-gl-device.c .  There is no need to check it again.

ctx-supports_bgra == FALSE on desktop is to prevent create BGRA texture on GL drivers, OpenGL does not allow to create texture of internal format of BGRA.

The last point is something to do with stride difference of PIXMAN_a8b8g8r8 vs. PIXMAN_x8b8g8r8.  At least I know some gles driver does not upload PIXMAN_x8b8g8r8 format correctly.

Thanks for commenting.

Henry
________________________________________
From: [email protected] [[email protected]] on behalf of Martin Robinson [[email protected]]
Sent: Wednesday, January 30, 2013 12:29 PM
To: [email protected]
Subject: Re: [cairo] [patch] gl: create BGRA texture and avoid conversion

On 01/30/2013 11:48 AM, Henry (Yu) Song - SISA wrote:
> @@ -316,6 +298,16 @@ _cairo_gl_context_init (cairo_gl_context_t *ctx)
>      ctx->max_textures = 0;
>      glGetIntegerv (GL_MAX_TEXTURE_IMAGE_UNITS, &ctx->max_textures);
>
> +    ctx->supports_bgra = FALSE;
> +#if CAIRO_HAS_GLESV2_SURFACE

I think it's slightly more correct to check the context flavor here. At
least with EGL you can select the API you want to access, though I don't
think we support that at the moment.

> +    /* if the driver supports both extensions, we are ensured that we can
> +     * create texture, upload image and read texture with BGRA format
> +     */
> +    if (_cairo_gl_has_extension ("GL_EXT_read_format_bgra")) {
> +     ctx->supports_bgra = TRUE;
> +    }
> +#endif

A few things:
1. You mention two extensions, but are only checking one.
2. Why delete the helper function?
3. This means that ctx->supports_bgra = FALSE for Desktop GL, even
though Desktop GL supports that format. It seems that it makes sense to
either set the flag properly or to hide it behind an #ifdef.
4. I felt like the old flag name was more descriptive.

> @@ -1079,17 +1170,14 @@ _cairo_gl_surface_map_to_image (void      *abstract_surface,
>        * interacting with other image surfaces. For ALPHA, GLES2 does not
>        * support GL_PACK_ROW_LENGTH anyway, and this makes sure that the
>        * pixman image that is created has row_stride = row_width * bpp. */
> -     if (surface->base.content == CAIRO_CONTENT_ALPHA || !ctx->can_read_bgra) {
> +     if (surface->base.content == CAIRO_CONTENT_ALPHA || !ctx->supports_bgra) {
>           cairo_bool_t little_endian = _cairo_is_little_endian ();
>           format = GL_RGBA;
>
> -         if (surface->base.content == CAIRO_CONTENT_COLOR) {
> -             pixman_format = little_endian ?
> -                 PIXMAN_x8b8g8r8 : PIXMAN_r8g8b8x8;
> -         } else {
> -             pixman_format = little_endian ?
> -                 PIXMAN_a8b8g8r8 : PIXMAN_r8g8b8a8;
> -         }
> +         /* FIXME: for CAIRO_CONTENT_COLOR, using 4-byte aligned
> +          * GL_RGBA is more efficient */
> +         pixman_format = little_endian ?
> +             PIXMAN_a8b8g8r8 : PIXMAN_r8g8b8a8;

I'm not sure I understand this change. Isn't it better to represent
CONTENT_COLOR images as having no alpha? My understanding is that this
can allow optimizations in pixman.

--Martin

--
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo