<div dir="ltr"><div>Awesome ! Thank you both once again.</div><div> </div><div>Regards,</div><div>Manuel</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-04-07 18:46 GMT+02:00 Kristian Høgsberg <span dir="ltr"><<a href="mailto:hoegsberg@gmail.com" target="_blank">hoegsberg@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Apr 07, 2014 at 04:02:11PM +0300, Pekka Paalanen wrote:<br>
> On Mon, 7 Apr 2014 11:58:45 +0200<br>
> Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>> wrote:<br>
><br>
> > This fixes :<br>
> > - leaking the mask used to simulate transparency ;<br>
> > - code style (definitions moved up, use of brackets) ;<br>
> > - applying an opaque region when transparency is<br>
> > wanted (shound not happen).<br>
> ><br>
> > Signed-off-by: Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>><br>
> > ---<br>
> > src/pixman-renderer.c | 19 +++++++++++++------<br>
> > 1 file changed, 13 insertions(+), 6 deletions(-)<br>
> ><br>
> > diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c<br>
> > index eff7201..4ac4693 100644<br>
> > --- a/src/pixman-renderer.c<br>
> > +++ b/src/pixman-renderer.c<br>
> > @@ -181,6 +181,8 @@ repaint_region(struct weston_view *ev, struct weston_output *output,<br>
> > float view_x, view_y;<br>
> > pixman_transform_t transform;<br>
> > pixman_fixed_t fw, fh;<br>
> > + pixman_image_t *mask_image;<br>
> > + pixman_color_t mask;<br>
> ><br>
> > /* The final region to be painted is the intersection of<br>
> > * 'region' and 'surf_region'. However, 'region' is in the global<br>
> > @@ -340,12 +342,12 @@ repaint_region(struct weston_view *ev, struct weston_output *output,<br>
> > if (ps->buffer_ref.buffer)<br>
> > wl_shm_buffer_begin_access(ps->buffer_ref.buffer->shm_buffer);<br>
> ><br>
> > - pixman_image_t *mask_image;<br>
> > if (ev->alpha < 1.0) {<br>
> > - pixman_color_t mask = { 0x0000, 0x0000, 0x0000, 0xffff*ev->alpha };<br>
> > + mask.alpha = 0xffff*ev->alpha;<br>
><br>
> Hi,<br>
><br>
> doesn't this now leave red, green and blue of 'mask' undefined?<br>
<br>
</div></div>It does, but for the mask argument, pixman only uses the alpha component.<br>
Regardless, I added an initializer for mask to set it to zero. Mainly to<br>
make sure valgrind won't complain about.<br>
<div><div class="h5"><br>
> > mask_image = pixman_image_create_solid_fill(&mask);<br>
> > - } else<br>
> > + } else {<br>
> > mask_image = NULL;<br>
> > + }<br>
> ><br>
> > pixman_image_composite32(pixman_op,<br>
> > ps->image, /* src */<br>
> > @@ -357,6 +359,9 @@ repaint_region(struct weston_view *ev, struct weston_output *output,<br>
> > pixman_image_get_width (po->shadow_image), /* width */<br>
> > pixman_image_get_height (po->shadow_image) /* height */);<br>
> ><br>
> > + if (mask_image)<br>
> > + pixman_image_unref(mask_image);<br>
> > +<br>
> > if (ps->buffer_ref.buffer)<br>
> > wl_shm_buffer_end_access(ps->buffer_ref.buffer->shm_buffer);<br>
> ><br>
> > @@ -408,12 +413,14 @@ draw_view(struct weston_view *ev, struct weston_output *output,<br>
> > ev->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE) {<br>
> > repaint_region(ev, output, &repaint, NULL, PIXMAN_OP_OVER);<br>
> > } else {<br>
> > - /* blended region is whole surface minus opaque region: */<br>
> > + /* blended region is whole surface minus opaque region, if not transparent: */<br>
> > pixman_region32_init_rect(&surface_blend, 0, 0,<br>
> > ev->surface->width, ev->surface->height);<br>
> > - pixman_region32_subtract(&surface_blend, &surface_blend, &ev->surface->opaque);<br>
> > + if (ev->alpha == 1.0) {<br>
> > + pixman_region32_subtract(&surface_blend, &surface_blend, &ev->surface->opaque);<br>
> > + }<br>
> ><br>
> > - if (pixman_region32_not_empty(&ev->surface->opaque)) {<br>
> > + if ((ev->alpha == 1.0) && pixman_region32_not_empty(&ev->surface->opaque)) {<br>
> > repaint_region(ev, output, &repaint, &ev->surface->opaque, PIXMAN_OP_SRC);<br>
> > }<br>
> ><br>
><br>
> You could merge the two if-statements. If the region is empty, the<br>
> subtraction won't have an effect anyway.<br>
><br>
> I wonder if the condition alpha == 1.0 is really the opposite of alpha<br>
> < 1.0 in weston... maybe it should be written as alpha >= 1.0 or<br>
> even !(alpha < 1.0) to exactly reflect the earlier test.<br>
<br>
</div></div>We cap the alpha at 1 in the animation code. I changed the tests above to<br>
just:<br>
<br>
if (ev->alpha != 1.0 ||<br>
(ev->transform.enabled &&<br>
ev->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE)) {<br>
<div> repaint_region(ev, output, &repaint, NULL, PIXMAN_OP_OVER);<br>
<br>
</div>and left the else case unchanged.<br>
<div><br>
> This looks ok, but I suppose it will have the same problem with<br>
> Xwayland that the gl-renderer is working around.<br>
<br>
</div>Yeah, I guess it would. I think we can create an XRGB pixman image for the<br>
same data and use that as source when we renderthe opaque regions.<br>
<div><br>
> Kristian, now on the new era of Xwayland DDX and possibly glamor,<br>
> does Xwayland still produce garbage alpha channels sometimes, or could<br>
> we drop that hack?<br>
<br>
</div>We'll fix it but currently we still get undefined alpha. Before the new<br>
Xwayland approach, the problem was that each driver implements part of the<br>
rendering architecture and we'd have to audit every driver to make sure they<br>
generates opaque alpha. With Xwayland using glamor, we only have to audit<br>
the glamor paths (which should already do the right thing) and fix the sw<br>
fallbacks (in pixman and fb/ in the server).<br>
<br>
Kristian<br>
<br>
><br>
> Thanks,<br>
> pq<br>
> _______________________________________________<br>
> wayland-devel mailing list<br>
> <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
</blockquote></div><br><br clear="all"><br>-- <br><div dir="ltr"><font>Regards,<br>
<br>
<i><b>Manuel BACHMANN</b><br>
Tizen Project<br>
VANNES-FR</i><br>
</font></div>
</div>