On Tue, 4 Apr 2017 17:54:19 +0100 Daniel Stone <[email protected]> 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 <[email protected]> > --- > 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
pgp7wMWtp3HS4.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
