backend/x11: only send frames when the window is visible #2682

imirkin · 2021-01-23T16:42:16Z

Tested with WindowMaker. When switching to another workspace,
minimizing, or rolling up the window, we get an UnmapNotify event, and a
corresponding MapNotify event when the window appears on screen again.

Fixes #2675

emersion · 2021-01-25T09:42:57Z

Hm. It seems like the UnmapNotify event doesn't happen on i3. Instead, it seems like i3 uses _NET_WM_STATE_HIDDEN:

https://github.com/i3/i3/blob/dcd6079c9bb5e06716a53b90f8539715bb194eeb/src/x.c#L751

Does WindowMaker do this as well?

emersion · 2021-01-25T10:44:56Z

backend/x11/output.c

@@ -579,7 +579,8 @@ void handle_x11_present_event(struct wlr_x11_backend *x11,
 		};
 		wlr_output_send_present(&output->wlr_output, &present_event);

-		wlr_output_send_frame(&output->wlr_output);
+		if (!output->hidden)


Style nit: braces are mandatory

imirkin · 2021-01-25T21:42:40Z

Hm. It seems like the UnmapNotify event doesn't happen on i3. Instead, it seems like i3 uses _NET_WM_STATE_HIDDEN:

https://github.com/i3/i3/blob/dcd6079c9bb5e06716a53b90f8539715bb194eeb/src/x.c#L751

Does WindowMaker do this as well?

According to the code, yes. But I don't see it in x11trace, perhaps I haven't enabled something. Here's what the ICCCM spec says:

There are at least two options for implementing virtual desktops. The first is to use multiple virtual roots (see the section called “Implementation note”) and change the current desktop by manipulating the stacking order of the virtual roots. This is completely ICCCM compliant, but has the issues outlined in the section called “Implementation note”

The second option is to keep all managed windows as children of the root window and unmap the frames of those which are not on the current desktop. Unmapped windows should be placed in IconicState, according to the ICCCM. Windows which are actually iconified or minimized should have the _NET_WM_STATE_HIDDEN property set, to communicate to pagers that the window should not be represented as "onscreen."

So we should probably listen to both. That said, I can't quite figure out how to listen to _NET_WM_STATE stuff. Will require some digging into X APIs.

emersion · 2021-01-25T21:51:40Z

Have you added XCB_EVENT_MASK_PROPERTY_CHANGE to the window's event mask? We should receive some XCB_PROPERTY_NOTIFY events.

imirkin · 2021-01-25T22:01:11Z

Have you added XCB_EVENT_MASK_PROPERTY_CHANGE to the window's event mask? We should receive some XCB_PROPERTY_NOTIFY events.

Ah I see. So then I get _NET_WM_STATE notifications, and then I have to run XGetWindowProperty to get the hidden state? Delightful...

imirkin · 2021-01-25T22:59:29Z

OK, so experiments suggest that _NET_WM_STATE_HIDDEN is only set when miniaturizing the window in WindowMaker. When switching workspaces, I only see UnmapNotify. When "rolling up" the window, I do see a _NET_WM_STATE change, but HIDDEN is not set.

Happy to include this logic though.

imirkin · 2021-01-26T00:24:55Z

Looks like we have to keep track of the Map/Unmap state separate from the STATE_HIDDEN state, and "and" them. In the roll-up case, I get an unmap followed by some sort of _NET_WM_STATE change which does not include the HIDDEN atom. So we have to be careful not to decide to start displaying stuff...

Tested with WindowMaker. When switching to another workspace, minimizing, or rolling up the window, we get an UnmapNotify event, and a corresponding MapNotify event when the window appears on screen again. Fixes swaywm#2675

Some window managers don't unmap windows when they're hidden. Also check the _NET_WM_STATE property for _NET_WM_STATE_HIDDEN. This way if the window is hidden or unmapped, then we won't send updates.

imirkin · 2021-02-06T03:43:17Z

I believe this should work correctly now. Mapped and hidden state are now kept track of separately.

emersion

Logic looks good to me, here are a few comments.

emersion · 2021-02-08T10:19:26Z

backend/x11/output.c

@@ -740,7 +743,8 @@ void handle_x11_present_event(struct wlr_x11_backend *x11,
 		};
 		wlr_output_send_present(&output->wlr_output, &present_event);

-		wlr_output_send_frame(&output->wlr_output);
+		if (output->mapped && !output->hidden)


Nit: braces are mandatory

emersion · 2021-02-08T10:19:48Z

backend/x11/output.c

@@ -600,6 +601,8 @@ struct wlr_output *wlr_x11_output_create(struct wlr_backend *backend) {
 	xcb_map_window(x11->xcb, output->win);
 	xcb_flush(x11->xcb);

+	output->mapped = true;


I wonder if we should wait for MapNotify here?

We won't get it. We only insert the window into the list after this happens.

emersion · 2021-02-08T10:35:44Z

backend/x11/backend.c

+					ev->atom, XCB_ATOM_ATOM, 0, 32);
+			xcb_get_property_reply_t *reply =
+				xcb_get_property_reply(x11->xcb, cookie, NULL);
+			if (reply->type != XCB_ATOM_ATOM) {


We should probably check reply != NULL too. And log an error message if something goes wrong, to make sure we don't miss these kind of errors.

emersion · 2021-02-08T10:36:45Z

backend/x11/backend.c

+			xcb_get_property_cookie_t cookie =
+				xcb_get_property(x11->xcb, false, ev->window,
+					ev->atom, XCB_ATOM_ATOM, 0, 32);
+			xcb_get_property_reply_t *reply =


The spec says:

The Window Manager should remove the property whenever a window is withdrawn

So we probably need to handle the case where the property is removed, and reset hidden to false. xcb_property_notify_event_t.state indicates whether a property is deleted.

emersion · 2021-02-08T10:37:30Z

backend/x11/backend.c

+			for (int i = 0; i < xcb_get_property_value_length(reply); i++) {
+				if (atoms[i] == x11->atoms.net_wm_state_hidden) {
+					wlr_log(WLR_DEBUG, "Window hidden, stopping updates");
+					output->hidden = true;


Nit: can add a break here.

emersion · 2021-04-06T08:57:41Z

Gentle ping: do you have plans to update this PR?

imirkin · 2021-04-06T14:48:16Z

Gentle ping: do you have plans to update this PR?

To be honest, I don't know -- this is all getting into the territory of stuff I can't really test. My WM doesn't really do these things. I'd actually rather go back to just doing UnmapNotify/MapNotify and leaving the WM state stuff to someone who can really test that out properly. What do you think?

psychon · 2021-04-06T15:34:18Z

Might I suggest using VisibilityNotify events instead? See https://www.x.org/releases/X11R7.6/doc/xproto/x11protocol.html#events:VisibilityNotify

These events contain a window and one of "Unobscured", "PartiallyObscured" or "FullyObscured". You need to select for VisibilityChange on the window to get these events.

Relevant names for xcb seem to be:

$ grep VISIBILIT /usr/include/xcb/xproto.h
    XCB_EVENT_MASK_VISIBILITY_CHANGE = 65536,
    XCB_VISIBILITY_UNOBSCURED = 0,
    XCB_VISIBILITY_PARTIALLY_OBSCURED = 1,
    XCB_VISIBILITY_FULLY_OBSCURED = 2
#define XCB_VISIBILITY_NOTIFY 15

imirkin · 2021-04-06T15:43:23Z

Might I suggest using VisibilityNotify events instead? See https://www.x.org/releases/X11R7.6/doc/xproto/x11protocol.html#events:VisibilityNotify

These events contain a window and one of "Unobscured", "PartiallyObscured" or "FullyObscured". You need to select for VisibilityChange on the window to get these events.

That sounds nice in theory. In practice, I don't get these. See #2675 (comment) .

psychon · 2021-04-06T15:51:28Z

Hm, okay... I only found https://gitlab.freedesktop.org/xorg/xserver/-/issues/922 which is about "when a compositor is running". I guess this does not apply to your case (or are you running WindowManager plus a compositing manager?)

imirkin · 2021-04-06T15:53:13Z

I'm only running WindowMaker. No compositors, no funny business. Same setup as I had in ~1998 :)

imirkin · 2021-04-06T20:30:02Z

I suspect there's no VisibilityNotify since the window is just plain unmapped. It's not obscured -- it's just gone.

emersion · 2021-04-06T20:32:52Z

I'd actually rather go back to just doing UnmapNotify/MapNotify and leaving the WM state stuff to someone who can really test that out properly. What do you think?

This looks fine to me!

psychon · 2021-04-08T16:10:51Z

I won't (and can't) stop you, but... in a reparenting WM, I wouldn't expect any UnmapNotify events since the WM can just unmap its frame window and keep the actual child window mapped.

I just tried running /usr/bin/xev under i3. When I switch to another workspace and back, xev gets the following events:

VisibilityNotify event, serial 35, synthetic NO, window 0x2a00001,
    state VisibilityFullyObscured

PropertyNotify event, serial 35, synthetic NO, window 0x2a00001,
    atom 0x186 (WM_STATE), time 490341, state PropertyNewValue

PropertyNotify event, serial 35, synthetic NO, window 0x2a00001,
    atom 0x186 (WM_STATE), time 490844, state PropertyNewValue

VisibilityNotify event, serial 35, synthetic NO, window 0x2a00001,
    state VisibilityUnobscured

Expose event, serial 35, synthetic NO, window 0x2a00001,
    (0,0), width 178, height 10, count 3

Expose event, serial 35, synthetic NO, window 0x2a00001,
    (0,10), width 10, height 58, count 2

Expose event, serial 35, synthetic NO, window 0x2a00001,
    (68,10), width 110, height 58, count 1

Expose event, serial 35, synthetic NO, window 0x2a00001,
    (0,68), width 178, height 110, count 0

(That PropertyNotify is IMO incorrect; i3 changed the window state to Withdrawn which I would interpret to mean "minimised"; I doubt this happens on other WMs.)

I tried finding something relevant in ICCCM, but... yeah.

i3 violates § 4.2.5 by not unmapping the window when going to iconic state:

It [the client] will receive a UnmapNotify event when it goes Iconic and a MapNotify event when it goes Normal.

§ 4.1.3.1 seems to say that a window in Normal state and in Withdrawn state can still be visible / be able to redraw itself:

In either state, clients should be prepared to handle exposure events from either window.

TL;DR: I don't really have anything useful to say and X11 is complicated.

imirkin · 2021-04-08T16:18:18Z

Without commenting on how window managers are supposed to work, here is how WindowMaker works on e.g. a workspace change:

https://repo.or.cz/wmaker-crm.git/blob/HEAD:/src/workspace.c#l510

As you can see it unmaps all the current workspace's windows. (And wWindowUnmap is what you'd expect - just a XUnmapWindow wrapper.)

psychon · 2021-04-08T16:36:10Z

Without commenting on how window managers are supposed to work,

Sorry :-(

I took another look at Xorg's source code and I think I found some missing piece of information:

https://gitlab.freedesktop.org/xorg/xserver/-/blob/3e4e70db10a4b2d531ccd649c18317ea2a49d256/mi/mivaltree.c#L489-494

        if (pChild->viewable) {
            oldVis = pChild->visibility;
            if (oldVis != (pChild->visibility = VisibilityFullyObscured) &&
                ((pChild->
                  eventMask | wOtherEventMasks(pChild)) & VisibilityChangeMask))
                SendVisibilityNotify(pChild);

This code completely ignores windows where ->viewable is false. Assuming viewable corresponds to map-state being viewable in GetWindowAttributes, this would mean something like this:

While a window is unmapped, it obviously cannot be shown anywhere. It gets a MapNotify when leaving this state.
While a window is mapped, it gets VisibilityNotify events that update its visibility state. It gets an UnmapNotify when leaving this state.

This would explain the behaviour you are seeing.

I'd actually rather go back to just doing UnmapNotify/MapNotify and leaving the WM state stuff to someone who can really test that out properly. What do you think?

This looks fine to me!

This is also fine with me. If you @psychon me in your PR about UnmapNotify/MapNotify, I volunteer to look into adding the handling of VisibilityNotify events after your PR is in (but I cannot promise to do it immediately). Since i3 does not seem to unmap windows, I should be able to test this.

emersion · 2021-04-08T16:40:15Z

So X11 clients are supposed to check 3 different pieces of information (map state, visibility, _net_wm_hidden) to figure out whether they need to continue rendering new frames?

emersion · 2021-11-01T16:00:21Z

wlroots has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/2682

emersion reviewed Jan 25, 2021

View reviewed changes

imirkin added 2 commits February 5, 2021 09:25

backend/x11: only send frames when the window is mapped

04d4483

Tested with WindowMaker. When switching to another workspace, minimizing, or rolling up the window, we get an UnmapNotify event, and a corresponding MapNotify event when the window appears on screen again. Fixes swaywm#2675

backend/x11: listen to _NET_WM_STATE changes for determining hidden

df8d12f

Some window managers don't unmap windows when they're hidden. Also check the _NET_WM_STATE property for _NET_WM_STATE_HIDDEN. This way if the window is hidden or unmapped, then we won't send updates.

imirkin force-pushed the pause-render branch from f6bf0fb to df8d12f Compare February 6, 2021 03:42

emersion suggested changes Feb 8, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backend/x11: only send frames when the window is visible #2682

backend/x11: only send frames when the window is visible #2682

imirkin commented Jan 23, 2021

emersion commented Jan 25, 2021

emersion Jan 25, 2021

imirkin commented Jan 25, 2021

emersion commented Jan 25, 2021

imirkin commented Jan 25, 2021

imirkin commented Jan 25, 2021

imirkin commented Jan 26, 2021

imirkin commented Feb 6, 2021

emersion left a comment

emersion Feb 8, 2021

emersion Feb 8, 2021

imirkin Feb 8, 2021

emersion Feb 8, 2021

emersion Feb 8, 2021

emersion Feb 8, 2021

emersion commented Apr 6, 2021

imirkin commented Apr 6, 2021

psychon commented Apr 6, 2021

imirkin commented Apr 6, 2021

psychon commented Apr 6, 2021

imirkin commented Apr 6, 2021

imirkin commented Apr 6, 2021

emersion commented Apr 6, 2021

psychon commented Apr 8, 2021

imirkin commented Apr 8, 2021

psychon commented Apr 8, 2021 •

edited

Loading

emersion commented Apr 8, 2021

emersion commented Nov 1, 2021

backend/x11: only send frames when the window is visible #2682

Are you sure you want to change the base?

backend/x11: only send frames when the window is visible #2682

Conversation

imirkin commented Jan 23, 2021

emersion commented Jan 25, 2021

emersion Jan 25, 2021

Choose a reason for hiding this comment

imirkin commented Jan 25, 2021

emersion commented Jan 25, 2021

imirkin commented Jan 25, 2021

imirkin commented Jan 25, 2021

imirkin commented Jan 26, 2021

imirkin commented Feb 6, 2021

emersion left a comment

Choose a reason for hiding this comment

emersion Feb 8, 2021

Choose a reason for hiding this comment

emersion Feb 8, 2021

Choose a reason for hiding this comment

imirkin Feb 8, 2021

Choose a reason for hiding this comment

emersion Feb 8, 2021

Choose a reason for hiding this comment

emersion Feb 8, 2021

Choose a reason for hiding this comment

emersion Feb 8, 2021

Choose a reason for hiding this comment

emersion commented Apr 6, 2021

imirkin commented Apr 6, 2021

psychon commented Apr 6, 2021

imirkin commented Apr 6, 2021

psychon commented Apr 6, 2021

imirkin commented Apr 6, 2021

imirkin commented Apr 6, 2021

emersion commented Apr 6, 2021

psychon commented Apr 8, 2021

imirkin commented Apr 8, 2021

psychon commented Apr 8, 2021 • edited Loading

emersion commented Apr 8, 2021

emersion commented Nov 1, 2021

psychon commented Apr 8, 2021 •

edited

Loading