Awesome ! Thank you both once again. Regards, Manuel
2014-04-07 18:46 GMT+02:00 Kristian Høgsberg <hoegsb...@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.bachm...@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.bachm...@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > -- Regards, *Manuel BACHMANN Tizen Project VANNES-FR*
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel