[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 <derekf at osg.samsung.com> 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 <derekf at osg.samsung.com>
>> ---
>>  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, &xf, &yf);
>> -	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 <pekka.paalanen at collabora.co.uk>
> 
> 
> Thanks,
> pq
> 
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>