[PATCH xserver v2] xwayland: rotate logical size for RRMode

[Simon Ser]

On July 16, 2018 8:50 AM, Olivier Fourdan <ofourdan at redhat.com> wrote:
> Hi,
>
> Thanks for your follow up on that!
>
> On Fri, Jul 13, 2018 at 9:51 PM Simon Ser <contact at emersion.fr> wrote:
> >
> > From: emersion <contact at emersion.fr>
> >
> > The logical size is the size of the output in the global compositor
> > space. The mode width/height should be scaled as in the logical
> > size, but shouldn't be transformed. Thus we need to rotate back
> > the logical size to be able to use it as the mode width/height.
> >
> > This fixes issues with pointer input on transformed outputs.
> >
> > Signed-Off-By: Simon Ser <contact at emersion.fr>
> > ---
> >
> > Changes from v1 to v2:
> > - Fixed rotation when xdg-output isn't available
> >
> > I've made sure this works on both rootston (with xdg-output) and
> > Weston (without xdg-output).
> >
> >  hw/xwayland/xwayland-output.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> > index 379062549..0d2ec7890 100644
> > --- a/hw/xwayland/xwayland-output.c
> > +++ b/hw/xwayland/xwayland-output.c
> > @@ -213,6 +213,7 @@ apply_output_change(struct xwl_output *xwl_output)
> >  {
> >      struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
> >      struct xwl_output *it;
> > +    int mode_width, mode_height;
> >      int width = 0, height = 0, has_this_output = 0;
> >      RRModePtr randr_mode;
> >      Bool need_rotate;
> > @@ -224,7 +225,16 @@ apply_output_change(struct xwl_output *xwl_output)
> >      /* xdg-output sends output size in compositor space. so already rotated */
> >      need_rotate = (xwl_output->xdg_output == NULL);
> >
> > -    randr_mode = xwayland_cvt(xwl_output->width, xwl_output->height,
> > +    /* We need to rotate back the logical size for the mode */
> > +    if (need_rotate || xwl_output->rotation & (RR_Rotate_0 | RR_Rotate_180)) {
>
> But is it `need_rotate` or `!need_rotate` here?
>
> `need_rotate` being `TRUE` means we don't have xdg-output available,
> in which case we shouldn't have to do this. Basically, we need to
> revert to the original width/height only if we have xdg-output (which
> has already applied the rotation), so I reckon it should be
> `!need_rotate` here.

Yes, except this branch handles the "don't do anything" case: width = width,
height = height. The other branch is when we actually need to rotate.

This variable could probably be renamed to something more sensible (like
`is_logical_size` and invert conditions?).

> > +        mode_width = xwl_output->width;
> > +        mode_height = xwl_output->height;
> > +    } else {
> > +        mode_width = xwl_output->height;
> > +        mode_height = xwl_output->width;
> > +    }
>
> So if we use `(!need_rotate)`, this addition becomes completely
> similar to the code found in `output_get_new_size()` it might be
> interesting to move that to a small helper function, e.g.
> `output_get_transform_size()` that would return the swapped
> width/height depending on the output rotation, so we don't duplicate
> that small portion of code. Nit picking, I know :)

The problem is, `output_get_new_size` needs the logical size and we need the
mode size, so one is swapped and the other isn't. We could still factor those in
a separate function, but I'm afraid this will make things more complicated than
they already are.

> > +
> > +    randr_mode = xwayland_cvt(mode_width, mode_height,
> >                                xwl_output->refresh / 1000.0, 0, 0);
> >      RROutputSetModes(xwl_output->randr_output, &randr_mode, 1, 1);
> >      RRCrtcNotify(xwl_output->randr_crtc, randr_mode,
> > --
> > 2.18.0
>
> Also, this is something I noticed the hard way some time ago
> (completely unrelated to your patch, though), the width/height
> parameters for `output_get_new_size()` are inverted (`height, width`
> instead of `width, height` as usual) which is error prone (don't ask
> how I noticed...).
>
> I can't see a reason for this everywhere else in the code we use
> `width, height`, I reckon it would be great if we could fix that (in a
> separate patch) as well, while at it... Because things are already
> complicated enough that we don't need to add more confusion by
> changing the well established order of parameters just for the sake of
> it :)

Indeed, will do!

> Cheers,
> Olivier