[PATCH] Fix on-the-fly transparency changes in pixman-renderer

[Pekka Paalanen]

On Mon,  7 Apr 2014 11:58:45 +0200
Manuel Bachmann <manuel.bachmann at open.eurogiciel.org> wrote:

> This fixes :
> - leaking the mask used to simulate transparency ;
> - code style (definitions moved up, use of brackets) ;
> - applying an opaque region when transparency is
>  wanted (shound not happen).
> 
> Signed-off-by: Manuel Bachmann <manuel.bachmann at open.eurogiciel.org>
> ---
>  src/pixman-renderer.c |   19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c
> index eff7201..4ac4693 100644
> --- a/src/pixman-renderer.c
> +++ b/src/pixman-renderer.c
> @@ -181,6 +181,8 @@ repaint_region(struct weston_view *ev, struct weston_output *output,
>  	float view_x, view_y;
>  	pixman_transform_t transform;
>  	pixman_fixed_t fw, fh;
> +	pixman_image_t *mask_image;
> +	pixman_color_t mask;
>  
>  	/* The final region to be painted is the intersection of
>  	 * 'region' and 'surf_region'. However, 'region' is in the global
> @@ -340,12 +342,12 @@ repaint_region(struct weston_view *ev, struct weston_output *output,
>  	if (ps->buffer_ref.buffer)
>  		wl_shm_buffer_begin_access(ps->buffer_ref.buffer->shm_buffer);
>  
> -	pixman_image_t *mask_image;
>  	if (ev->alpha < 1.0) {
> -		pixman_color_t mask = { 0x0000, 0x0000, 0x0000, 0xffff*ev->alpha };
> +		mask.alpha = 0xffff*ev->alpha;

Hi,

doesn't this now leave red, green and blue of 'mask' undefined?

>  		mask_image = pixman_image_create_solid_fill(&mask);
> -	} else
> +	} else {
>  		mask_image = NULL;
> +	}
>  
>  	pixman_image_composite32(pixman_op,
>  				 ps->image, /* src */
> @@ -357,6 +359,9 @@ repaint_region(struct weston_view *ev, struct weston_output *output,
>  				 pixman_image_get_width (po->shadow_image), /* width */
>  				 pixman_image_get_height (po->shadow_image) /* height */);
>  
> +	if (mask_image)
> +		pixman_image_unref(mask_image);
> +
>  	if (ps->buffer_ref.buffer)
>  		wl_shm_buffer_end_access(ps->buffer_ref.buffer->shm_buffer);
>  
> @@ -408,12 +413,14 @@ draw_view(struct weston_view *ev, struct weston_output *output,
>  	    ev->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE) {
>  		repaint_region(ev, output, &repaint, NULL, PIXMAN_OP_OVER);
>  	} else {
> -		/* blended region is whole surface minus opaque region: */
> +		/* blended region is whole surface minus opaque region, if not transparent: */
>  		pixman_region32_init_rect(&surface_blend, 0, 0,
>  					  ev->surface->width, ev->surface->height);
> -		pixman_region32_subtract(&surface_blend, &surface_blend, &ev->surface->opaque);
> +		if (ev->alpha == 1.0) {
> +			pixman_region32_subtract(&surface_blend, &surface_blend, &ev->surface->opaque);
> +		}
>  
> -		if (pixman_region32_not_empty(&ev->surface->opaque)) {
> +		if ((ev->alpha == 1.0) && pixman_region32_not_empty(&ev->surface->opaque)) {
>  			repaint_region(ev, output, &repaint, &ev->surface->opaque, PIXMAN_OP_SRC);
>  		}
>  

You could merge the two if-statements. If the region is empty, the
subtraction won't have an effect anyway.

I wonder if the condition alpha == 1.0 is really the opposite of alpha
< 1.0 in weston... maybe it should be written as alpha >= 1.0 or
even !(alpha < 1.0) to exactly reflect the earlier test.

This looks ok, but I suppose it will have the same problem with
Xwayland that the gl-renderer is working around.

Kristian, now on the new era of Xwayland DDX and possibly glamor,
does Xwayland still produce garbage alpha channels sometimes, or could
we drop that hack?


Thanks,
pq