Re: [PATCH weston] compositor: change rounding in weston_surface_to_buffer_rect()
On Mon, 30 Nov 2015 13:33:16 -0600 Derek Foremanwrote: > Rounding both corners of the rectangle down can result in a 0 > width/height rectangle before passing to weston_transformed_rect. > > This showed up as missing damage in weston-simple-damage (the > bouncing ball would leave green trails when --use-viewport was > used) > > Also, add a big fat warning for users of the function, since > some of its operation may not be obvious at a glance. > > Signed-off-by: Derek Foreman > --- > src/compositor.c | 18 -- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/src/compositor.c b/src/compositor.c > index 4895bd6..bf59fa8 100644 > --- a/src/compositor.c > +++ b/src/compositor.c > @@ -932,6 +932,20 @@ weston_surface_to_buffer(struct weston_surface *surface, > *by = floorf(byf); > } > > +/* Users of weston_surface_to_buffer_rect() need to be > + * careful - it converts to integer as an intermediate > + * step, and rounds off at that time - the boundary may > + * not be exactly as expected. It works fine when used > + * for damage tracking since a little extra coverage is > + * not a problem. > + * Ok. This could be a proper doxygen doc. > + * Also, since the rectangles are specified by 2 corners, > + * if the input is not axis aligned and the surface to > + * buffer transform includes a rotation that isn't a > + * multiple of 90 degrees, the output rectangle won't > + * have the same area as the input (in fact it could have > + * none at all) This path is not using matrices anywhere, is it? So it's actually impossible to have unexpected transformations. In your transforms branch, you convert weston_surface_to_buffer_rect() to use a matrix, but even then this warning isn't necessary, because a) the matrix is read from weston_surface so it better be legal, and b) you could check the matrix is constrained enough. This warning is more applicable to weston_matrix_transform_region(). > + */ > WL_EXPORT pixman_box32_t > weston_surface_to_buffer_rect(struct weston_surface *surface, > pixman_box32_t rect) > @@ -945,8 +959,8 @@ weston_surface_to_buffer_rect(struct weston_surface > *surface, > rect.y1 = floorf(yf); > > scaler_surface_to_buffer(surface, rect.x2, rect.y2, , ); > - rect.x2 = floorf(xf); > - rect.y2 = floorf(yf); > + rect.x2 = ceilf(xf); > + rect.y2 = ceilf(yf); > > return weston_transformed_rect(surface->width_from_buffer, > surface->height_from_buffer, The code is anyway: Reviewed-by: Pekka Paalanen Thanks, pq pgpRtskYpU9to.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] compositor: change rounding in weston_surface_to_buffer_rect()
On 01/12/15 03:48 AM, Pekka Paalanen wrote: > On Mon, 30 Nov 2015 13:33:16 -0600 > Derek Foremanwrote: > >> Rounding both corners of the rectangle down can result in a 0 >> width/height rectangle before passing to weston_transformed_rect. >> >> This showed up as missing damage in weston-simple-damage (the >> bouncing ball would leave green trails when --use-viewport was >> used) >> >> Also, add a big fat warning for users of the function, since >> some of its operation may not be obvious at a glance. >> >> Signed-off-by: Derek Foreman >> --- >> src/compositor.c | 18 -- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/src/compositor.c b/src/compositor.c >> index 4895bd6..bf59fa8 100644 >> --- a/src/compositor.c >> +++ b/src/compositor.c >> @@ -932,6 +932,20 @@ weston_surface_to_buffer(struct weston_surface *surface, >> *by = floorf(byf); >> } >> >> +/* Users of weston_surface_to_buffer_rect() need to be >> + * careful - it converts to integer as an intermediate >> + * step, and rounds off at that time - the boundary may >> + * not be exactly as expected. It works fine when used >> + * for damage tracking since a little extra coverage is >> + * not a problem. >> + * > > Ok. This could be a proper doxygen doc. > >> + * Also, since the rectangles are specified by 2 corners, >> + * if the input is not axis aligned and the surface to >> + * buffer transform includes a rotation that isn't a >> + * multiple of 90 degrees, the output rectangle won't >> + * have the same area as the input (in fact it could have >> + * none at all) > > This path is not using matrices anywhere, is it? So it's actually > impossible to have unexpected transformations. > > In your transforms branch, you convert weston_surface_to_buffer_rect() > to use a matrix, but even then this warning isn't necessary, because a) > the matrix is read from weston_surface so it better be legal, and b) > you could check the matrix is constrained enough. > > This warning is more applicable to weston_matrix_transform_region(). Ok - for some reason I thought it was possible to have an arbitrary transform there (not that it makes any sense for surface to buffer) I'll remove that comment, and add doxygen. Thanks, Derek >> + */ >> WL_EXPORT pixman_box32_t >> weston_surface_to_buffer_rect(struct weston_surface *surface, >>pixman_box32_t rect) >> @@ -945,8 +959,8 @@ weston_surface_to_buffer_rect(struct weston_surface >> *surface, >> rect.y1 = floorf(yf); >> >> scaler_surface_to_buffer(surface, rect.x2, rect.y2, , ); >> -rect.x2 = floorf(xf); >> -rect.y2 = floorf(yf); >> +rect.x2 = ceilf(xf); >> +rect.y2 = ceilf(yf); >> >> return weston_transformed_rect(surface->width_from_buffer, >> surface->height_from_buffer, > > The code is anyway: > > Reviewed-by: Pekka Paalanen > > > Thanks, > pq > > > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] compositor: change rounding in weston_surface_to_buffer_rect()
On 30/11/15 03:21 PM, Daniel Stone wrote: > Hi, > > On 30 November 2015 at 19:33, Derek Foremanwrote: >> Rounding both corners of the rectangle down can result in a 0 >> width/height rectangle before passing to weston_transformed_rect. >> >> This showed up as missing damage in weston-simple-damage (the >> bouncing ball would leave green trails when --use-viewport was >> used) >> >> Also, add a big fat warning for users of the function, since >> some of its operation may not be obvious at a glance. >> >> Signed-off-by: Derek Foreman >> --- >> src/compositor.c | 18 -- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/src/compositor.c b/src/compositor.c >> index 4895bd6..bf59fa8 100644 >> --- a/src/compositor.c >> +++ b/src/compositor.c >> @@ -932,6 +932,20 @@ weston_surface_to_buffer(struct weston_surface *surface, >> *by = floorf(byf); >> } >> >> +/* Users of weston_surface_to_buffer_rect() need to be >> + * careful - it converts to integer as an intermediate >> + * step, and rounds off at that time - the boundary may >> + * not be exactly as expected. It works fine when used >> + * for damage tracking since a little extra coverage is >> + * not a problem. >> + * >> + * Also, since the rectangles are specified by 2 corners, >> + * if the input is not axis aligned and the surface to >> + * buffer transform includes a rotation that isn't a >> + * multiple of 90 degrees, the output rectangle won't >> + * have the same area as the input (in fact it could have >> + * none at all) >> + */ >> WL_EXPORT pixman_box32_t >> weston_surface_to_buffer_rect(struct weston_surface *surface, >> pixman_box32_t rect) >> @@ -945,8 +959,8 @@ weston_surface_to_buffer_rect(struct weston_surface >> *surface, >> rect.y1 = floorf(yf); >> >> scaler_surface_to_buffer(surface, rect.x2, rect.y2, , ); >> - rect.x2 = floorf(xf); >> - rect.y2 = floorf(yf); >> + rect.x2 = ceilf(xf); >> + rect.y2 = ceilf(yf); > > This seems to make sense, but the comment above is a bit jarring. How > could we go from a non-zero input area to a zeroed output area? I > guess we'd have to be scaling down so hard that the extents > disappeared into the noise? Rotating a square by 45 degrees will "break" this function. Since we only have top left and bottom right corner, we can rotate them so they're both along the same horizontal or vertical line - then the remaining rectangle has no area at all. > Reviewed-by: Daniel Stone Thanks > Cheers, > Daniel > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] compositor: change rounding in weston_surface_to_buffer_rect()
Hi, On 30 November 2015 at 19:33, Derek Foremanwrote: > Rounding both corners of the rectangle down can result in a 0 > width/height rectangle before passing to weston_transformed_rect. > > This showed up as missing damage in weston-simple-damage (the > bouncing ball would leave green trails when --use-viewport was > used) > > Also, add a big fat warning for users of the function, since > some of its operation may not be obvious at a glance. > > Signed-off-by: Derek Foreman > --- > src/compositor.c | 18 -- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/src/compositor.c b/src/compositor.c > index 4895bd6..bf59fa8 100644 > --- a/src/compositor.c > +++ b/src/compositor.c > @@ -932,6 +932,20 @@ weston_surface_to_buffer(struct weston_surface *surface, > *by = floorf(byf); > } > > +/* Users of weston_surface_to_buffer_rect() need to be > + * careful - it converts to integer as an intermediate > + * step, and rounds off at that time - the boundary may > + * not be exactly as expected. It works fine when used > + * for damage tracking since a little extra coverage is > + * not a problem. > + * > + * Also, since the rectangles are specified by 2 corners, > + * if the input is not axis aligned and the surface to > + * buffer transform includes a rotation that isn't a > + * multiple of 90 degrees, the output rectangle won't > + * have the same area as the input (in fact it could have > + * none at all) > + */ > WL_EXPORT pixman_box32_t > weston_surface_to_buffer_rect(struct weston_surface *surface, > pixman_box32_t rect) > @@ -945,8 +959,8 @@ weston_surface_to_buffer_rect(struct weston_surface > *surface, > rect.y1 = floorf(yf); > > scaler_surface_to_buffer(surface, rect.x2, rect.y2, , ); > - rect.x2 = floorf(xf); > - rect.y2 = floorf(yf); > + rect.x2 = ceilf(xf); > + rect.y2 = ceilf(yf); This seems to make sense, but the comment above is a bit jarring. How could we go from a non-zero input area to a zeroed output area? I guess we'd have to be scaling down so hard that the extents disappeared into the noise? Reviewed-by: Daniel Stone Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston] compositor: change rounding in weston_surface_to_buffer_rect()
Rounding both corners of the rectangle down can result in a 0 width/height rectangle before passing to weston_transformed_rect. This showed up as missing damage in weston-simple-damage (the bouncing ball would leave green trails when --use-viewport was used) Also, add a big fat warning for users of the function, since some of its operation may not be obvious at a glance. Signed-off-by: Derek Foreman--- src/compositor.c | 18 -- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/compositor.c b/src/compositor.c index 4895bd6..bf59fa8 100644 --- a/src/compositor.c +++ b/src/compositor.c @@ -932,6 +932,20 @@ weston_surface_to_buffer(struct weston_surface *surface, *by = floorf(byf); } +/* Users of weston_surface_to_buffer_rect() need to be + * careful - it converts to integer as an intermediate + * step, and rounds off at that time - the boundary may + * not be exactly as expected. It works fine when used + * for damage tracking since a little extra coverage is + * not a problem. + * + * Also, since the rectangles are specified by 2 corners, + * if the input is not axis aligned and the surface to + * buffer transform includes a rotation that isn't a + * multiple of 90 degrees, the output rectangle won't + * have the same area as the input (in fact it could have + * none at all) + */ WL_EXPORT pixman_box32_t weston_surface_to_buffer_rect(struct weston_surface *surface, pixman_box32_t rect) @@ -945,8 +959,8 @@ weston_surface_to_buffer_rect(struct weston_surface *surface, rect.y1 = floorf(yf); scaler_surface_to_buffer(surface, rect.x2, rect.y2, , ); - rect.x2 = floorf(xf); - rect.y2 = floorf(yf); + rect.x2 = ceilf(xf); + rect.y2 = ceilf(yf); return weston_transformed_rect(surface->width_from_buffer, surface->height_from_buffer, -- 2.6.2 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel