[PATCH weston 7/8] pixman-renderer: implement source clipping

[Pekka Paalanen]

On Fri, 06 Mar 2015 13:53:46 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 06/03/15 05:04 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > Implement a way to do composition clipping with a region32 given in
> > source image space.
> > 
> > Pixman does not directly support this kind of operation at all. If you
> > pixman_image_set_clip_region32() on a source image, it will be ignored
> > unless you also
> > 	pixman_image_set_source_clipping(image, 1);
> > 	pixman_image_set_has_client_clip(image, 1);
> > but then it takes the region from source image and still uses it in the
> > destination coordinate space. For reference:
> > http://lists.freedesktop.org/archives/pixman/2015-March/003501.html
> > That is actually the intended behaviour in Pixman.
> > 
> > This patch implements source clipping by taking each rectangle of the
> > source clip region, wrapping that sub-rect of the source image in a new
> > pixman_image_t, and compositing it separately. This might be very heavy as
> > we are painting the whole damage the number of rectangles times, but
> > practically always the number of rectangles is one.
> > 
> > An alternative solution would be to use mask images of type PIXMAN_a1,
> > render the source clip region in it, and set the transformation. You'd
> > probably also want to cache those images. And because we use the mask to
> > apply view->alpha, you'd have to use PIXMAN_a8 in those cases.
> > 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  src/pixman-renderer.c | 166 +++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 150 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c
> > index fc7613c..d3f1342 100644
> > --- a/src/pixman-renderer.c
> > +++ b/src/pixman-renderer.c
> > @@ -26,6 +26,7 @@
> >  
> >  #include <errno.h>
> >  #include <stdlib.h>
> > +#include <assert.h>
> >  
> >  #include "pixman-renderer.h"
> >  
> > @@ -326,6 +327,101 @@ region_intersect_only_translation(pixman_region32_t *result_global,
> >  	pixman_region32_intersect(result_global, result_global, global);
> >  }
> >  
> > +static void
> > +composite_whole(pixman_op_t op,
> > +		pixman_image_t *src,
> > +		pixman_image_t *mask,
> > +		pixman_image_t *dest,
> > +		const pixman_transform_t *transform,
> > +		pixman_filter_t filter)
> > +{
> > +	int32_t dest_width;
> > +	int32_t dest_height;
> > +
> > +	dest_width = pixman_image_get_width(dest);
> > +	dest_height = pixman_image_get_height(dest);
> > +
> > +	pixman_image_set_transform(src, transform);
> > +	pixman_image_set_filter(src, filter, NULL, 0);
> > +
> > +	pixman_image_composite32(op, src, mask, dest,
> > +				 0, 0, /* src_x, src_y */
> > +				 0, 0, /* mask_x, mask_y */
> > +				 0, 0, /* dest_x, dest_y */
> > +				 dest_width, dest_height);
> > +}
> > +
> > +static void
> > +composite_clipped(pixman_image_t *src,
> > +		  pixman_image_t *mask,
> > +		  pixman_image_t *dest,
> > +		  const pixman_transform_t *transform,
> > +		  pixman_filter_t filter,
> > +		  pixman_region32_t *src_clip)
> > +{
> > +	int n_box;
> > +	pixman_box32_t *boxes;
> > +	int32_t dest_width;
> > +	int32_t dest_height;
> > +	int src_stride;
> > +	int bitspp;
> > +	pixman_format_code_t src_format;
> > +	void *src_data;
> > +	int i;
> > +
> > +	/* Hardcoded to use PIXMAN_OP_OVER, because sampling outside of
> > +	 * a Pixman image produces (0,0,0,0) instead of discarding the
> > +	 * fragment.
> > +	 */
> > +
> > +	dest_width = pixman_image_get_width(dest);
> > +	dest_height = pixman_image_get_height(dest);
> > +	src_format = pixman_image_get_format(src);
> > +	src_stride = pixman_image_get_stride(src);
> > +	bitspp = PIXMAN_FORMAT_BPP(src_format);
> > +	src_data = pixman_image_get_data(src);
> > +
> > +	assert(src_format);
> > +
> > +	/* This would be massive overdraw, except n_box is 1. */
> 
> except WHEN n_box is 1?  comment seems to declare that n_box is 1, so
> why the for loop?

Adding "when". I'd almost consider it a bug (but not fatal) to have
more than one box.

It's just that n_box should always be 1 in our current code, yet since
we are dealing with a pixman_region32_t, might as well handle it
completely.

> > +	boxes = pixman_region32_rectangles(src_clip, &n_box);
> > +	for (i = 0; i < n_box; i++) {
> > +		uint8_t *ptr = src_data;
> > +		pixman_image_t *boximg;
> > +		pixman_transform_t adj = *transform;
> > +
> > +		ptr += boxes[i].y1 * src_stride;
> > +		ptr += boxes[i].x1 * bitspp / 8;
> > +		boximg = pixman_image_create_bits_no_clear(src_format,
> > +					boxes[i].x2 - boxes[i].x1,
> > +					boxes[i].y2 - boxes[i].y1,
> > +					(uint32_t *)ptr, src_stride);
> > +
> > +		pixman_transform_translate(&adj, NULL,
> > +					   pixman_int_to_fixed(-boxes[i].x1),
> > +					   pixman_int_to_fixed(-boxes[i].y1));
> > +		pixman_image_set_transform(boximg, &adj);
> > +
> > +		pixman_image_set_filter(boximg, filter, NULL, 0);
> > +		pixman_image_composite32(PIXMAN_OP_OVER, boximg, mask, dest,
> > +					 0, 0, /* src_x, src_y */
> > +					 0, 0, /* mask_x, mask_y */
> > +					 0, 0, /* dest_x, dest_y */
> > +					 dest_width, dest_height);
> > +
> > +		pixman_image_unref(boximg);
> > +	}
> > +
> > +	if (n_box > 1) {
> > +		static bool warned = false;
> > +
> > +		if (!warned)
> > +			weston_log("Pixman-renderer warning: %dx overdraw\n",
> > +				   n_box);
> > +		warned = true;
> > +	}
> > +}

> > @@ -475,12 +599,22 @@ draw_view(struct weston_view *ev, struct weston_output *output,
> >  		zoom_logged = 1;
> >  	}
> >  
> > -	/* TODO: Implement repaint_region_complex() using pixman_composite_trapezoids() */
> 
> I guess this patch doesn't do what the removed commend suggested - is
> that comment still valid?

It's not valid anymore from rendering correctness point of view.

It might be valid from performance point of view, I'm not sure.

The comment was added in f4a457fe0ff3b0a049c3fe80daeca22b5769564c and
I'm not sure if it meant only for correctness or also performance. So
I'm assuming correctness. I have a vague feeling that I might have
myself somehow suggested that comment, but I'm not going to dig that up.


Thanks,
pq

> >  	if (view_transformation_is_translation(ev)) {
> > +		/* The simple case: The surface regions opaque, non-opaque,
> > +		 * etc. are convertible to global coordinate space.
> > +		 * There is no need to use a source clip region.
> > +		 * It is possible to paint opaque region as PIXMAN_OP_SRC.
> > +		 */
> >  		draw_view_translated(ev, output, &repaint);
> >  	} else {
> > -		region_global_to_output(output, &repaint);
> > -		repaint_region(ev, output, &repaint, NULL, PIXMAN_OP_OVER);
> > +		/* The complex case: the view transformation does not allow
> > +		 * converting opaque etc. regions into global coordinate space.
> > +		 * Therefore we need source clipping to avoid sampling from
> > +		 * unwanted source image areas, unless the source image is
> > +		 * to be used whole. Source clipping does not work with
> > +		 * PIXMAN_OP_SRC.
> > +		 */
> > +		draw_view_source_clipped(ev, output, &repaint);
> >  	}
> >  
> >  out:
> > 
>