Output transforms 90 and 270 are inverted

Pekka Paalanen said:

From the spec: "This describes the transform that a compositor will apply to a surface to compensate for the rotation or mirroring of an output device."

I always get confused whether you rotate the monitor or the surface. So if surface is rotated 90 degrees counter-clockwise, it means the monitor is rotated 90 degrees clockwise. I tried with Weston and you seem to be right: wl_output::transform in weston describes the monitor rotation, not surface rotation.

Because weston.ini is documented as 90 being clockwise rotation of the image, not the monitor, it matches the current Weston behaviour. We'd need to invert the rotation read from weston.ini for sending to clients, and also invert the image rotations we do in Weston and weston clients... right?

added Core compositor and removed ~227 ~226 labels

We'd need to invert the rotation read from weston.ini for sending to clients, and also invert the image rotations we do in Weston and weston clients... right?

Yes, I think so. Except we also need to do this for all existing clients and compositors.

At this point I'd say a spec fix and clarification is probably better. What do you think?

What do existing compositors other than Weston do today?

wlroots does the same as Weston.

CC @zmike @jadahl @romangg @RAOF @johanhelsing - how does your software interpret the WL_OUTPUT_TRANSFORM_* values?

I think Mir does the same as Weston and wlroots here - rotating the display image 90⁰ clockwise (so rotating the laptop 90⁰ anti-clockwise makes the screen the right way up) results in Mir advertising WL_OUTPUT_TRANSFORM_270. (I also find it difficult to really grasp what the spec is saying, so that's why I'm not sure if this is the same behaviour as Weston )

A simple way to test is running weston-transformed and pressing the arrow left/right keys. If the image doesn't change, your compositor behaves like Weston.

@RAOF That sounds like the opposite of what Weston does.

Yup! weston-transformed indicates that we're doing the opposite of what Weston does.

Tested weston-transformed on KWin, image seems to rotate on my machine. Seems like KWin implements the spec correctly?

I finally managed to test on KWin myself. I installed Plasma Wayland from Ubuntu 19.10 (KDE Plasma 5.16.5, Qt 5.12.4).

To me it seems that KWin does not handle buffer transform at all. When playing with unmodified weston-transformed, all orientations except "normal" show up wrong.

If a compositor gets only the direction of the transform wrong, then only transforms with 90 and 270 would show up wrong and the rest would show up right.

@shadeslayer @romangg Did I test an outdated version of KWin, or is this so also in master?

mentioned in commit pq/wayland@d01b7a3d

mentioned in merge request wayland!57 (closed)

For my new proposed, hopefully more clear wording, see wayland!57 (closed). Note, that the specification there has the opposite definition than the current wording.

Did a quick check, and it seems that both EFL-based compositors obey the spec as-is.

IOW, EFL-based compositors do the opposite of Weston, IIUC.

I tried weston-transformed on GNOME/wayland of Fedora 30. It works the same as on Weston.

Reaching a concensus is starting to look really hard.

So... How about the major toolkits? Do they actually use WL_OUTPUT_TRANSFORM_* for anything, and if they do, how?

The only thing GTK+ uses WL_OUTPUT_TRANSFORM_* for is debug messages.

Sorry in advance for the lengthy post. I'm worried I've misunderstood something and just want to explain my thinking in case someone else with an interest in Qt wants to double check.

TL;DR: Qt client and compositor implementations seem in line with the protocol, but the compositor API documentation is wrong.

Feel free to ignore the rest of this post if you're not interested in the details.

void QWaylandScreen::updateOutputProperties()
{
    if (mTransform >= 0) {
        bool isPortrait = mGeometry.height() > mGeometry.width();
        switch (mTransform) {
            case WL_OUTPUT_TRANSFORM_NORMAL:
                m_orientation = isPortrait ? Qt::PortraitOrientation : Qt::LandscapeOrientation;
                break;
            case WL_OUTPUT_TRANSFORM_90:
                m_orientation = isPortrait ? Qt::InvertedLandscapeOrientation : Qt::PortraitOrientation;
                break;
            case WL_OUTPUT_TRANSFORM_180:
                m_orientation = isPortrait ? Qt::InvertedPortraitOrientation : Qt::InvertedLandscapeOrientation;
                break;
            case WL_OUTPUT_TRANSFORM_270:
                m_orientation = isPortrait ? Qt::LandscapeOrientation : Qt::InvertedPortraitOrientation;
                break;
            // Ignore these ones, at least for now
            case WL_OUTPUT_TRANSFORM_FLIPPED:
            case WL_OUTPUT_TRANSFORM_FLIPPED_90:
            case WL_OUTPUT_TRANSFORM_FLIPPED_180:
            case WL_OUTPUT_TRANSFORM_FLIPPED_270:
                break;
        }

        QWindowSystemInterface::handleScreenOrientationChange(screen(), m_orientation);
        mTransform = -1;
    }

    QWindowSystemInterface::handleScreenRefreshRateChange(screen(), refreshRate());

    if (!zxdg_output_v1::isInitialized())
        QWindowSystemInterface::handleScreenGeometryChange(screen(), geometry(), geometry());
}

How I understand it:

Let's say we have a regular landscape screen (width > height at the lower levels), but it has been rotated 90 degrees clockwise. The compositor should then report WL_OUTPUT_TRANSFORM_90 as it's going to rotate the client buffers 90 degrees counter-clockwise to compensate for the physical rotation.

It looks like we're then going to set QScreen::orientation to Qt::PortraitOrientation, which is documented as: Portrait orientation, display height is greater than display width, rotated 90 degree clockwise relative to landscape.

So, assuming the Qt application is orientation aware (has called QScreen::setOrientationUpdateMask and has rendered itself rotated and called QWindow::reportContentOrientationChange with Qt::PortraitOrientation, then QWaylandWindow::handleContentOrientationChange will be called with Qt::PortraitOrientation.

void QWaylandWindow::handleContentOrientationChange(Qt::ScreenOrientation orientation)
{
    if (mDisplay->compositorVersion() < 2)
        return;

    wl_output_transform transform;
    bool isPortrait = window()->screen() && window()->screen()->primaryOrientation() == Qt::PortraitOrientation;
    switch (orientation) {
        case Qt::PrimaryOrientation:
            transform = WL_OUTPUT_TRANSFORM_NORMAL;
            break;
        case Qt::LandscapeOrientation:
            transform = isPortrait ? WL_OUTPUT_TRANSFORM_270 : WL_OUTPUT_TRANSFORM_NORMAL;
            break;
        case Qt::PortraitOrientation:
            transform = isPortrait ? WL_OUTPUT_TRANSFORM_NORMAL : WL_OUTPUT_TRANSFORM_90;
            break;
        case Qt::InvertedLandscapeOrientation:
            transform = isPortrait ? WL_OUTPUT_TRANSFORM_90 : WL_OUTPUT_TRANSFORM_180;
            break;
        case Qt::InvertedPortraitOrientation:
            transform = isPortrait ? WL_OUTPUT_TRANSFORM_180 : WL_OUTPUT_TRANSFORM_270;
            break;
        default:
            Q_UNREACHABLE();
    }
    mSurface->set_buffer_transform(transform);
    // set_buffer_transform is double buffered, we need to commit.
    mSurface->commit();
}

When handleContentOrientationChange is setting isPortrait, it's reading from QScreen::primaryOrientation, not QScreen::orientation. primaryOrientation should as far as I understand be set to landskape as width > height. So isPortrait will be false, which means QWindow::contentOrientation is going to request set_buffer_transform(WL_OUTPUT_TRANSFORM_90), which I guess seems to be in line with the spec.

Relevant documentation:

As for the compositor implementation, here's the relevant part:

void QWaylandSurfacePrivate::surface_set_buffer_transform(Resource *resource, int32_t orientation)
{
    Q_UNUSED(resource);
    Q_Q(QWaylandSurface);
    QScreen *screen = QGuiApplication::primaryScreen();
    bool isPortrait = screen->primaryOrientation() == Qt::PortraitOrientation;
    Qt::ScreenOrientation oldOrientation = contentOrientation;
    switch (orientation) {
        case WL_OUTPUT_TRANSFORM_90:
            contentOrientation = isPortrait ? Qt::InvertedLandscapeOrientation : Qt::PortraitOrientation;
            break;
        case WL_OUTPUT_TRANSFORM_180:
            contentOrientation = isPortrait ? Qt::InvertedPortraitOrientation : Qt::InvertedLandscapeOrientation;
            break;
        case WL_OUTPUT_TRANSFORM_270:
            contentOrientation = isPortrait ? Qt::LandscapeOrientation : Qt::InvertedPortraitOrientation;
            break;
        default:
            contentOrientation = Qt::PrimaryOrientation;
    }
    if (contentOrientation != oldOrientation)
        emit q->contentOrientationChanged();
}

However, the documentation is:

 * \qmlproperty enum QtWaylandCompositor::WaylandSurface::contentOrientation
 *
 * This property holds the orientation of the WaylandSurface's contents.
 *
 * \sa {WaylandOutput::transform}{WaylandOutput.transform}

Which I guess is incorrect, as it's not really the orientation of the surface, but what kind of orientation the client has already compensated for. So I would say the implementation—despite being incomplete—is correct, but the documentation is not.

So if KWin does not implement buffer transforms at all, can we then assume that probably no-one is actually using Qt with buffer transform either?

I know for a fact that Qt + Weston is quite widely used in a few environments, particularly including automotive. I don't know how they compensate for the mismatch in rotations between the two. Often the layouts are just hardcoded anyway, as they're aimed for very fixed displays.

At this point, given the relative userbases, I would argue that we should fix the spec to reflect the most popular reality, rather than fixing reality to reflect the spec.

Well, Qt + Qt Wayland Compositor is used quite widely in automotive, tvs, phones and other embedded systems as well.

Sailfish also has a fork of Qt Wayland, and seems like they're doing things with screen orientation: https://git.sailfishos.org/mer-core/lipstick/blob/master/src/compositor/lipstickcompositor.cpp

Fixing this will probably screw up a lot of setups, regardless of which option we choose. And I don't particularly like the idea of also breaking the spec.

Non Qt wayland clients are pretty rare in Sailfish, I can think of two internally which can be patched at the same time as QtWayland if need be.

And I think the output and buffer orientations are being misused at the moment. The output orientation is being used to forward the reading from an orientation sensor and the buffer orientation returns the rotation of the client UI which the homescreen UI rotates to match. So there are probably compatibility issues with non-Sailfish clients already.

@adenexter thanks for letting us know! Yeah, that's not what the protocol intended, quite creative abuse I'd say. :-)

I also understand where that comes from, when you need to dynamically adapt to physical orientation changes and the client needs to re-render before the visual orientation can change. If we did that on Wayland by the book, I think you'd add some more state to xdg_toplevel and use ack_configure or window state to trigger the visual orientation change. The wl_output and wl_buffer transforms are mostly for optimizing the pixel path once the visual orientation has been set.

Let me try to draft a summary of compositors here.

Opposite of spec	Spec as written	Not implemented
weston	Mir
wlroots	EFL	Kwin
(GNOME)	Qt

Compositors:

KWin implementation is in the works but not released
GNOME (Mutter) does not advertise transformed outputs

Toolkits:

GTK+ does not use WL_OUTPUT_TRANSFORM_*, which means anything goes.
Qt forwards transformation information as "spec as written".

I'll update this if we find something new.

mentioned in merge request !315 (merged)

In mutter, we implement it in the same way as weston, i.e. not according to spec, but we never advertise wl_output's as having any transforms. Historically, because we didn't implement buffer transforms, and we still don't for cursor surface buffers. Thus, in practice, changing mutter to implement buffer transforms according to spec would likely not cause any issues in practice for GNOME, as buffer transforms are likely mostly, if not only except for test cases, in response to wl_output transforms.

Edit: it's implemented the same way as weston

@daniels writes:

I know for a fact that Qt + Weston is quite widely used in a few environments, particularly including automotive. I don't know how they compensate for the mismatch in rotations between the two. Often the layouts are just hardcoded anyway, as they're aimed for very fixed displays.

If Qt + Weston use cases hardcode everything, then I suppose it should be fairly easy to fix the fallout if I fix Weston to honor the spec as is, right?

OTOH Qt + Qt Wayland compositor is very much unknown to me, but Johan suggests it could break badly.

KWin has no released implementation of tranforms, and GNOME/mutter seems fine fixing theirs.

I would interpret the situation like this:

keep spec as is	change spec	either way
Mir		KWin
EFL		GNOME
Qt		weston
		GTK+
		wlroots

"Keep spec as is" also has one "default" vote in my opinion, because it is the word of the spec and it is not even ambiguous, even if it is a little hard to read.

I propose that "Keep the spec as is" wins.

Yays? Nays? Does anyone want to move to a different column?

I don't think wlroots cares too much either. I can submit patches to fix it.

Thanks, I edited the table. This is starting to look like a consensus to me.

I agree. Lets keep the spec as is, fixing the bugs in the compositors and the test client.

mentioned in merge request !222 (closed)

mentioned in commit pq/weston@ba223089

wlroots patch fixing the bug: https://github.com/swaywm/wlroots/pull/2023

mentioned in issue #354

I think we also got 90 vs. FLIPPED_90 wrong...

No, wait, that's not what I was thinking...

mentioned in commit pq/weston@5328c1e5

Output transforms 90 and 270 are inverted

Submitted by emersion

Description

Child items ...

Activity

Admin message

Admin message

Output transforms 90 and 270 are inverted

Submitted by emersion

Description

Activity