[PATCH v14 20/41] compositor-drm: Fully account for buffer transformation

[Pekka Paalanen]

On Wed, 20 Dec 2017 12:26:37 +0000
Daniel Stone <daniels at collabora.com> wrote:

> In our new and improved helper to determine the src/dest values for a
> buffer on a given plane, make sure we account for all buffer
> transformations, including viewport clipping.
> 
> Rather than badly open-coding it ourselves, just use the helper which
> does exactly this.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Reported-by: Tiago Gomes <tiago.gomes at codethink.co.uk>
> ---
>  libweston/compositor-drm.c | 55 ++++++++++------------------------------------
>  1 file changed, 12 insertions(+), 43 deletions(-)
> 

Hi,

the idea here is good, it looks like wp_viewport settings were
completely missed in drm_output_prepare_overlay_view() so they would
get through. This will handle them.

I believe ignoring scissor is ok in drm_output_prepare_overlay_view(),
because the boundingbox already includes its effect, assuming the
boundingbox was exact for the cases we pass through.

> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 71bf94edd..001eb5278 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -1188,10 +1188,9 @@ drm_plane_state_coords_for_view(struct drm_plane_state *state,
>  				struct weston_view *ev)
>  {
>  	struct drm_output *output = state->output;
> -	struct weston_buffer_viewport *viewport = &ev->surface->buffer_viewport;
>  	pixman_region32_t dest_rect, src_rect;
>  	pixman_box32_t *box, tbox;
> -	wl_fixed_t sx1, sy1, sx2, sy2;
> +	float sxf1, syf1, sxf2, syf2;
>  
>  	/* Update the base weston_plane co-ordinates. */
>  	box = pixman_region32_extents(&ev->transform.boundingbox);
> @@ -1217,52 +1216,22 @@ drm_plane_state_coords_for_view(struct drm_plane_state *state,
>  	state->dest_h = tbox.y2 - tbox.y1;
>  	pixman_region32_fini(&dest_rect);
>  
> -	/* Now calculate the source rectangle, by finding the points in the
> -	 * view and working backwards to source co-ordinates. */
> +	/* Now calculate the source rectangle, by finding the extents of the
> +	 * view, and working backwards to source co-ordinates. */
>  	pixman_region32_init(&src_rect);
>  	pixman_region32_intersect(&src_rect, &ev->transform.boundingbox,
>  				  &output->base.region);
>  	box = pixman_region32_extents(&src_rect);
> -
> -	/* Accounting for any transformations made to this particular surface
> -	 * view, find the source rectangle to use. */
> -	weston_view_from_global_fixed(ev,
> -				      wl_fixed_from_int(box->x1),
> -				      wl_fixed_from_int(box->y1),
> -				      &sx1, &sy1);
> -	weston_view_from_global_fixed(ev,
> -				      wl_fixed_from_int(box->x2),
> -				      wl_fixed_from_int(box->y2),
> -				      &sx2, &sy2);
> -
> -	/* XXX: How is this possible ... ? */
> -	if (sx1 < 0)
> -		sx1 = 0;
> -	if (sy1 < 0)
> -		sy1 = 0;
> -	if (sx2 > wl_fixed_from_int(ev->surface->width))
> -		sx2 = wl_fixed_from_int(ev->surface->width);
> -	if (sy2 > wl_fixed_from_int(ev->surface->height))
> -		sy2 = wl_fixed_from_int(ev->surface->height);

(I think these checks should have actually been after the
weston_transformed_rect() call.)

> -
> -	tbox.x1 = sx1;
> -	tbox.y1 = sy1;
> -	tbox.x2 = sx2;
> -	tbox.y2 = sy2;
> -
> -	/* Apply viewport transforms in reverse, to get the source co-ordinates
> -	 * in buffer space. */
> -	tbox = weston_transformed_rect(wl_fixed_from_int(ev->surface->width),
> -				       wl_fixed_from_int(ev->surface->height),
> -				       viewport->buffer.transform,
> -				       viewport->buffer.scale,
> -				       tbox);
> -
> -	state->src_x = tbox.x1 << 8;
> -	state->src_y = tbox.y1 << 8;
> -	state->src_w = (tbox.x2 - tbox.x1) << 8;
> -	state->src_h = (tbox.y2 - tbox.y1) << 8;
> +	weston_view_from_global_float(ev, box->x1, box->y1, &sxf1, &syf1);
> +	weston_surface_to_buffer_float(ev->surface, sxf1, syf1, &sxf1, &syf1);
> +	weston_view_from_global_float(ev, box->x2, box->y2, &sxf2, &syf2);
> +	weston_surface_to_buffer_float(ev->surface, sxf2, syf2, &sxf2, &syf2);
>  	pixman_region32_fini(&src_rect);
> +
> +	state->src_x = wl_fixed_from_double(sxf1) << 8;
> +	state->src_y = wl_fixed_from_double(syf1) << 8;
> +	state->src_w = wl_fixed_from_double(sxf2 - sxf1) << 8;
> +	state->src_h = wl_fixed_from_double(syf2 - syf1) << 8;

I would assume that KMS would not be happy about negative or zero width
or height, or negative x or y, or x+w or y+h going over the buffer
dimensions. Should we check those?

This is based on the boundingbox again being the smallest possible but
still in global integer coordinates fully containing the view, which
due to rounding might be larger than the view, therefore exceeding the
buffer dimensions.

drm_view_transform_supported() will let fractional translations pass.

>  }
>  
>  /**

Other than the corner-cases, this patch does look good.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180124/69f6f14a/attachment.sig>