swaywm / wlr-protocols

Wayland protocols designed for use in wlroots (and other compositors)
122 stars 29 forks source link

layer-shell: introduce ack for new outputs #86

Open ifreund opened 4 years ago

ifreund commented 4 years ago

This ack sequence eliminates the race between a client creating a new layer surface and the compositor rendering the first frame of a new output.

This race is especially important for screenlockers using layer-shell such as swaylock https://github.com/swaywm/swaylock/issues/16

emersion commented 4 years ago

If wonder if it would be desirable to change this to a ping/pong mechanism similar to xdg-shell.

ifreund commented 4 years ago

Clarified that all clients must ack and rebased onto #87 to avoid conflicts.

If wonder if it would be desirable to change this to a ping/pong mechanism similar to xdg-shell.

If I understand correctly, this would just make the ability to use this to achieve atomicity of new outputs implicit instead of explicit. The logic would stay the same from the server's point of view:

  1. post new output global
  2. send ping to all layer-shells
  3. if a new layer surface is requested on the new output before receiving a pong from that layer_shell, wait for a committed buffer before enabling/drawing the first frame of the new output.

This seems like it would work just as well and could be more generally useful than the new output specific language.

ifreund commented 4 years ago

The disadvantage to solving this race implicitly through a ping/pong is that clients which for whatever reason make the pong request before get_layer_surface will still be racy. We could certainly explain how compositors could use ping/pong to solve this in the protocol and encourage clients to respond to the events in order though.

ifreund commented 4 years ago

Opened up #90 to explore the ping/pong alternative approach.

emersion commented 4 years ago

If I understand correctly, this would just make the ability to use this to achieve atomicity of new outputs implicit instead of explicit.

Yeah. That would work similarly to wl_display_roundtrip: the ping-pong roundtrip guarantees that the client has processed all events until the ping request.

This also has more use-cases, like detecting unresponsive layer-shell clients (like xdg-shell).

ifreund commented 4 years ago

Tweaked the descriptions to clarify that the compositor may create several new outputs at once before sending this event. Also bumped the protocol version to 4 as there was a wlroots release with version 3 since this PR was opened.

ammen99 commented 4 years ago

To make sure I understand correctly:

  1. Server sends new_output
  2. Client creates layer-surface but does not commit
  3. Client sends ack_output
  4. Server waits for client to "map" its surface(s) and then starts rendering

Is this the intended sequence? Also, another question. Why do we choose this design over the following:

  1. Server sends new_output
  2. Client creates layer-surface
  3. Client commits all of its surfaces for the new output
  4. Client sends ack_output, at this point compositor just starts operating as usual

It seems to me this second variant will be easier to implement because compositor doesn't have to guess when a client is ready, although I may be missing something.

BTW, wayfire does something similar with a private protocol: https://github.com/WayfireWM/wayfire/blob/master/proto/wayfire-shell-unstable-v2.xml#L47-L64, which is used for ex. for fade-in animation when an output is plugged in or at startup. (Note that this mechanism in wayfire-shell-unstable is racy, I'd switch to the ack_output once it is implemented).

ifreund commented 4 years ago

Is this the intended sequence? Also, another question. Why do we choose this design over the following:

Yes, that's the intended sequence. This was chosen mostly for consistency with other established ack sequences (e.g. in xdg_shell) where the ack sent in response to a configure indicates that the next commit takes the change in state into account.

It seems to me this second variant will be easier to implement because compositor doesn't have to guess when a client is ready, although I may be missing something.

There's no guessing here unless I'm missing something. Only new surfaces created on the new output(s) before the client sends the ack_new_output request are waited on. Each surface is "ready" exactly when it has attached and committed its first buffer, which will then be part of the first frame rendered on the new output.

ammen99 commented 4 years ago

Is this the intended sequence? Also, another question. Why do we choose this design over the following:

Yes, that's the intended sequence. This was chosen mostly for consistency with other established ack sequences (e.g. in xdg_shell) where the ack sent in response to a configure indicates that the next commit takes the change in state into account.

It seems to me this second variant will be easier to implement because compositor doesn't have to guess when a client is ready, although I may be missing something.

There's no guessing here unless I'm missing something. Only new surfaces created on the new output(s) before the client sends the ack_new_output request are waited on. Each surface is "ready" exactly when it has attached and committed its first buffer, which will then be part of the first frame rendered on the new output.

Ok, thanks for the explanation.

emersion commented 4 years ago

Overall looks good. Would be nice to see a client & compositor implementation!

emersion commented 2 years ago

wlr-protocols has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlr-protocols/-/merge_requests/86