[PATCH v5 xserver 5/7] xf86Cursor: Fix xf86_crtc_rotate_coord_back using width/height wrongly

[Hans de Goede]

Hi,

On 07-09-16 03:21, Michel Dänzer wrote:
> On 06/09/16 08:31 PM, Hans De Goede wrote:
>> xf86_crtc_rotate_coord_back should be the exact inverse operation of
>> xf86_crtc_rotate_coord, but when calculating x / y for 90 / 270 degrees
>> rotation it was using height to calculate x / width to calculate y,
>> instead of the otherway around.
>
> I think it's correct as is. It's certainly symmetrical with
> xf86_crtc_rotate_coord.

Actually no, it is not symmetrical, it uses width to calculate y and
height to calculate x, where as xf86_crtc_rotate_coord uses
width when calculating x and height when calculating y as one would
expect.

Let me walk you through the steps I took when creating this patch:

First lets call xf86_crtc_rotate_coord, with 270 degree rotation,

then we do:

     case RR_Rotate_270:
         t = x_dst;
         x_dst = y_dst;
         y_dst = width - t - 1;

And now lets only look at y_dst, y_dst is:

         y_dst = width - t - 1;

and t is the original x_dst (which we want to get back in xf86_crtc_rotate_coord_back)
so y_dst is:

         y_dst = width - orig_x_dst - 1;




And now lets look at the 270 degree case in xf86_crtc_rotate_coord_back
(original version before my patch):

     case RR_Rotate_270:
         t = x_dst;
         x_dst = height - y_dst - 1;
         y_dst = t;

y_dst here is the result of xf86_crtc_rotate_coord (the _back
version is supposed to undo the normal version), so we
can substitute y_dst with the y_dst calculation from
the normal xf86_crtc_rotate_coord when trying to get back
the orginal x_dst, resulting in:

         x_dst = height - y_dst - 1;

Becoming:

         x_dst = height - (width - orig_x_dst - 1) - 1;

Remove the ():

         x_dst = height - width + orig_x_dst + 1 - 1;

Remove + 1 - 1:

         x_dst = height - width + orig_x_dst;

So iow before my patch xf86_crtc_rotate_coord is not
properly undoing xf86_crtc_rotate. With my patch this
becomes:

         x_dst = width - width + orig_x_dst;

Which can be simplified to:

         x_dst = orig_x_dst;

Which is as it should be.

>> This was likely not noticed before since xf86_crtc_rotate_coord_back
>> until now was only used with cursor_info->MaxWidth and
>> cursor_info->MaxHeight, which are usally the same.
>
> I'd say it's kind of the other way around:
> xf86_crtc_transform_cursor_position just happens to still work with your
> change because usually cursor_info->MaxWidth == cursor_info->MaxHeight .

I could be wrong, but I don't think so, see above.

Regards,

Hans