Re: [PATCH weston] compositor: change rounding in weston_surface_to_buffer_rect()

[Pekka Paalanen]

On Mon, 30 Nov 2015 13:33:16 -0600
Derek Foreman  wrote:

> 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()

[Derek Foreman]

On 01/12/15 03:48 AM, Pekka Paalanen wrote:
> On Mon, 30 Nov 2015 13:33:16 -0600
> Derek Foreman  wrote:
> 
>> 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()

[Derek Foreman]

On 30/11/15 03:21 PM, Daniel Stone wrote:
> Hi,
> 
> On 30 November 2015 at 19:33, Derek Foreman  wrote:
>> 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()

[Daniel Stone]

Hi,

On 30 November 2015 at 19:33, Derek Foreman  wrote:
> 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()

[Derek Foreman]

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