Re: [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