-
Notifications
You must be signed in to change notification settings - Fork 29
layer-shell: introduce ack for new outputs #86
base: master
Are you sure you want to change the base?
Conversation
59c5857
to
032cbfe
Compare
If wonder if it would be desirable to change this to a ping/pong mechanism similar to xdg-shell. |
032cbfe
to
a8f190b
Compare
Clarified that all clients must ack and rebased onto #87 to avoid conflicts.
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:
This seems like it would work just as well and could be more generally useful than the new output specific language. |
a8f190b
to
4aae5a1
Compare
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. |
Opened up #90 to explore the ping/pong alternative approach. |
Yeah. That would work similarly to This also has more use-cases, like detecting unresponsive layer-shell clients (like xdg-shell). |
4aae5a1
to
28390ec
Compare
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. |
created by the client take the new output(s) into account. | ||
|
||
A client may make multiple ack_new_output requests before committing. | ||
The last request made before the commit indicates which new_output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if this is a stupid question, but: what does this last sentence refer to? Aren't next commits taking into account all acked outputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea with this paragraph was to parallel the configure/ack behavior of the xdg-shell protocol. However, I think it may be over-complicating things. There are two paths we could take here:
- State that new layer surfaces created on a new output before acking that new output are associated only with that change of state.
- In the case that a second new output is created before the first commit on a new layer surface which was created in response to a previous
new_output
event and the client acks the second new output before committing the layer surface, the layer surface then becomes part of the ack sequence for the second new output.
In it's current state this paragraph is, I believe, trying to explain option 2. I'd suggest that we switch this over to option 1 though as it's simpler and I believe more ideal behavior. Creation of a second new output wouldn't prolong the waiting period for the first frame on a prior new output.
Hopefully that all makes sense, it's rather late here and I don't feel like I've done a terribly good job of putting it into words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO 1. makes more sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched this to option 1, let me know if the wording is unclear.
28390ec
to
63bf3ea
Compare
To make sure I understand correctly:
Is this the intended sequence? Also, another question. Why do we choose this design over the following:
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). |
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.
There's no guessing here unless I'm missing something. Only new surfaces created on the new output(s) before the client sends the |
Ok, thanks for the explanation. |
Overall looks good. Would be nice to see a client & compositor implementation! |
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, allowing frame perfection to be maintained.
63bf3ea
to
34b5e3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 |
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 swaywm/swaylock#16