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

[Kristian Høgsberg]

On Mon, Apr 07, 2014 at 04:02:11PM +0300, Pekka Paalanen wrote:
> 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?

It does, but for the mask argument, pixman only uses the alpha component.
Regardless, I added an initializer for mask to set it to zero.  Mainly to
make sure valgrind won't complain about.

> >  		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.

We cap the alpha at 1 in the animation code.  I changed the tests above to
just:

        if (ev->alpha != 1.0 ||
            (ev->transform.enabled &&
             ev->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE)) {
                repaint_region(ev, output, &repaint, NULL, PIXMAN_OP_OVER);

and left the else case unchanged.

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

Yeah, I guess it would.  I think we can create an XRGB pixman image for the
same data and use that as source when we renderthe opaque regions.
 
> 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?

We'll fix it but currently we still get undefined alpha.  Before the new
Xwayland approach, the problem was that each driver implements part of the
rendering architecture and we'd have to audit every driver to make sure they
generates opaque alpha.  With Xwayland using glamor, we only have to audit
the glamor paths (which should already do the right thing) and fix the sw
fallbacks (in pixman and fb/ in the server).

Kristian

> 
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel