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

[Olivier Fourdan]

Hi,

On Mon, Jul 16, 2018 at 10:47 AM Simon Ser <contact at emersion.fr> wrote:
>
> 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.

Yeap, agreed.

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

Agreed, the variable name could be better, I've never been very good
at picking good function and variable names :)

> > > +        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.

Right

> > > +
> > > +    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
> >

So this is

Reviewed-by: Olivier Fourdan <ofourdan at redhat.com>

Cheers,
Olivier