[PATCH weston v10 01/61] Clip co-ordinates in surface->buffer translation

[Pekka Paalanen]

On Tue,  4 Apr 2017 17:54:19 +0100
Daniel Stone <daniels at collabora.com> wrote:

> Since performing the surface -> buffer translation may introduce
> rounding error taking our desired co-ordinates out of bounds, introduce
> a hard clip to the bounds specified by the client's viewport.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> ---
>  libweston/compositor.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> v10: New.
> 
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index e8dd151d..b2f4c610 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -777,7 +777,16 @@ viewport_surface_to_buffer(struct weston_surface *surface,
>  	}
>  
>  	*bx = sx * src_width / surface->width + src_x;
> +	if (*bx < src_x)
> +		*bx = src_x;
> +	if (*bx > src_x + src_width)
> +		*bx = src_x + src_width;
> +
>  	*by = sy * src_height / surface->height + src_y;
> +	if (*by < src_y)
> +		*by = src_y;
> +	if (*by > src_y + src_height)
> +		*by = src_y + src_height;
>  }
>  
>  WL_EXPORT void

Hi,

I feel that this is a wrong place to do this. This function was never
intended to clamp (see the deliberate floor/ceil in
weston_surface_to_buffer_rect()). This changes the behaviour of other
functions:
- weston_surface_to_buffer_region()
- weston_surface_to_buffer_rect()
- weston_surface_to_buffer_float()

Clamping here makes it silent, there is no way for the caller to detect
that clamping happened and how much the adjustment was. Usually the
expected magnitude of clamping would +/- 0.5 off hand, with larger
adjustments to be considered as bugs (we never check for them yet).

weston_surface_to_buffer_rect() does flooring and ceil'ing after
calling viewport_surface_to_buffer() and seems to be aimed at damage
tracking where not clamping is not a problem. It intentionally rounds
the rect to be never smaller than the unrounded result.

weston_surface_to_buffer_rect() is used by gl-renderer to compute the
rectangles for glTexSubImage2D(), where it is crucial to not exceed the
buffer boundaries. That was arguably buggy and needs the clamping
*after* the call to weston_transformed_rect().

weston_surface_to_buffer_region() internally calls
weston_surface_to_buffer_rect(). It is used by pixman-renderer for
transforming the source clip region in case it cannot be transformed
into the global coordinate system. There
weston_surface_to_buffer_rect()'s round-towards-larger behaviour might
not be right (it should guarantee we cannot get gaps between rects,
though), but it also does not require clamping as sampling from outside
the buffer is normal.

weston_surface_to_buffer_float() is used in gl-renderer's
texture_region() where it is used to produce texture coordinates. The
lack of clamping was not serious here, and clamping shouldn't change
anything.

Given all that, this patch fails to explain why to clamp and why exactly
here.

I see "[PATCH weston v10 38/61] compositor-drm: Extract buffer->plane
co-ord translation" is the reason for this. I would prefer the clamping
to happen in drm_plane_state_coords_for_view() as the final step, not
in the middle of floating point computations like this patch is doing.

gl-renderer would probably need its own clamping fix in
gl_renderer_flush_damage().

I recommend keeping the clamping in the DRM backend where the final
parameters are computed, so we don't have to start shaving this yak.

If we had an end-to-end transformation matrix, we would need to clamp
in the DRM backend anyway, so it's also better for future refactorings.


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/20170406/2734f2b1/attachment.sig>