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

[Manuel Bachmann]

Awesome ! Thank you both once again.

Regards,
Manuel


2014-04-07 18:46 GMT+02:00 Kristian Høgsberg <hoegsberg at gmail.com>:

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



-- 
Regards,



*Manuel BACHMANN Tizen Project VANNES-FR*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140407/2e0e1be2/attachment.html>