[PATCH weston v2 05/20] pixman-renderer: Add a weston_matrix_to_pixman_transform function and simplify the buffer-to-output matrix computation

[Derek Foreman]

Thanks for looking at this!

On 09/01/15 03:15 PM, Giulio Camuffo wrote:
> One comment below, otherwise looks fine
> 
> 2014-10-16 18:55 GMT+03:00 Derek Foreman <derekf at osg.samsung.com>:
>> From: Jason Ekstrand <jason at jlekstrand.net>
>>
>> Now that we have a buffer-to-surface matrix and the global-to-output matrix
>> is in pixels, we can remove a large chunk of confusing code from the pixman
>> renderer.  Hopefully, having this stuff in weston core will keep the pixman
>> renderer from gettin broken quite as often.
>> ---
>>  src/pixman-renderer.c | 155 +++++++-------------------------------------------
>>  1 file changed, 20 insertions(+), 135 deletions(-)
>>
>> diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c
>> index 2c26c3a..18b6476 100644

<snip>

>>  static void
>> @@ -180,7 +168,7 @@ repaint_region(struct weston_view *ev, struct weston_output *output,
>>         pixman_region32_t final_region;
>>         float view_x, view_y;
>>         pixman_transform_t transform;
>> -       pixman_fixed_t fw, fh;
>> +       struct weston_matrix matrix;
> 
> This new matrix isn't being initialized anywhere, or am i blind?

It's used as a target further down...

>>         pixman_image_t *mask_image;
>>         pixman_color_t mask = { 0, };
>>
>> @@ -217,121 +205,18 @@ repaint_region(struct weston_view *ev, struct weston_output *output,
>>         /* Set up the source transformation based on the surface
>>            position, the output position/transform/scale and the client
>>            specified buffer transform/scale */
>> -       pixman_transform_init_identity(&transform);
>> -       pixman_transform_scale(&transform, NULL,
>> -                              pixman_double_to_fixed ((double)1.0/output->current_scale),
>> -                              pixman_double_to_fixed ((double)1.0/output->current_scale));
>> -
>> -       fw = pixman_int_to_fixed(output->width);
>> -       fh = pixman_int_to_fixed(output->height);
>> -       switch (output->transform) {
>> -       default:
>> -       case WL_OUTPUT_TRANSFORM_NORMAL:
>> -       case WL_OUTPUT_TRANSFORM_FLIPPED:
>> -               break;
>> -       case WL_OUTPUT_TRANSFORM_90:
>> -       case WL_OUTPUT_TRANSFORM_FLIPPED_90:
>> -               pixman_transform_rotate(&transform, NULL, 0, -pixman_fixed_1);
>> -               pixman_transform_translate(&transform, NULL, 0, fh);
>> -               break;
>> -       case WL_OUTPUT_TRANSFORM_180:
>> -       case WL_OUTPUT_TRANSFORM_FLIPPED_180:
>> -               pixman_transform_rotate(&transform, NULL, -pixman_fixed_1, 0);
>> -               pixman_transform_translate(&transform, NULL, fw, fh);
>> -               break;
>> -       case WL_OUTPUT_TRANSFORM_270:
>> -       case WL_OUTPUT_TRANSFORM_FLIPPED_270:
>> -               pixman_transform_rotate(&transform, NULL, 0, pixman_fixed_1);
>> -               pixman_transform_translate(&transform, NULL, fw, 0);
>> -               break;
>> -       }
>> -
>> -       switch (output->transform) {
>> -       case WL_OUTPUT_TRANSFORM_FLIPPED:
>> -       case WL_OUTPUT_TRANSFORM_FLIPPED_90:
>> -       case WL_OUTPUT_TRANSFORM_FLIPPED_180:
>> -       case WL_OUTPUT_TRANSFORM_FLIPPED_270:
>> -               pixman_transform_scale(&transform, NULL,
>> -                                      pixman_int_to_fixed (-1),
>> -                                      pixman_int_to_fixed (1));
>> -               pixman_transform_translate(&transform, NULL, fw, 0);
>> -               break;
>> -       }
>> -
>> -        pixman_transform_translate(&transform, NULL,
>> -                                  pixman_double_to_fixed (output->x),
>> -                                  pixman_double_to_fixed (output->y));
>> +       weston_matrix_invert(&matrix, &output->matrix);

Right here - the inverse of output->matrix is placed in matrix... though
if the output->matrix is singular matrix will be left uninitialized.  I
don't think this can happen...

>>         if (ev->transform.enabled) {
>> -               /* Pixman supports only 2D transform matrix, but Weston uses 3D,
>> -                * so we're omitting Z coordinate here
>> -                */
>> -               pixman_transform_t surface_transform = {{
>> -                               { D2F(ev->transform.matrix.d[0]),

<snip>