Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libobs: Bind to only one wayland seat #7514

Closed
wants to merge 1 commit into from

Conversation

markbolhuis
Copy link

Description

Bind to only one wl_seat.

Motivation and Context

Compositors like Sway are mulit-seat aware. This means the registry will send multiple global events for each wl_seat.
This cause an issue where obs will leak every seat other than the last one sent, and will try to get the keyboard for a seat that doesn't support it, causing a protocol error.

Relevant Wayland log:

[2453323.467] wl_seat@30.name("seat0")
[2453323.508] wl_seat@30.capabilities(3)
[2453323.516]  -> wl_seat@31.get_keyboard(new id wl_keyboard@34)
[2453323.526] wl_seat@31.name("seat1")
[2453323.533] wl_seat@31.capabilities(0)
[2453323.539]  -> wl_keyboard@34.release()
[2453323.719] wl_display@1.error(wl_seat@31, 0, "wl_seat.get_keyboard called when no keyboard capability has existed")
wl_seat@31: error 0: wl_seat.get_keyboard called when no keyboard capability has existed
The Wayland connection experienced a fatal error: Protocol error

How Has This Been Tested?

  • ArchLinux: 5.19.12.arch1-1
  • Sway: 1.7 (configured with 2 seats)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

Wayland compositors may advertise more than one wl_seat on the
registry, therefore only bind to the first wl_seat.

This prevents a protocol error where a wl_seat.capabilities event
triggers a wl_seat.get_keyboard request for a wl_seat that has been
replaced by a subsequent one, which does not support keyboard input.
@kkartaltepe
Copy link
Collaborator

It looks like this will break if the first seat provided is not the one with the keyboard.

Sorry im not familiar enough with precisely what a seal is intended to represent, but probably we should either listen to every keyboard or pick one. Since we need to bind the seat to check its capabilities we still need to bind to all of them.

@markbolhuis
Copy link
Author

A seat is an event aggregator. Basically it represents a collection of input devices that all logically belong to one person sitting at the computer.

‐--------------------

If the first seat doesn't have a keyboard capability then the keyboard object isn't created:

if (kb_present && plat->keyboard == NULL) {

The issue wasn't that there was no keyboard at all, but that the capabilities event triggered after binding to the first seat has the keyboard bit set, but by then any other seat had replaced that one, so attempting to get the keyboard terminated the connection if that new seat didn't support it.


We should not try to be clever and pick different seats just because they have a keyboard capability. As I said a seat represents a person, so picking a different one would mean a completely different collection of physical keyboards intended for a different person.

Ideally OBS should be multiseat aware, but for my first commit I decided to not be too clever and just fix it crashing.
Proper multiseat support is much more involved. For single seat apps picking the first one is fine.

@kkartaltepe
Copy link
Collaborator

kkartaltepe commented Oct 2, 2022

We should not try to be clever and pick different seats just because they have a keyboard capability. As I said a seat represents a person, so picking a different one would mean a completely different collection of physical keyboards intended for a different person.

We already (try, and fail as you show) pick the seat with a keyboard. Please provide protocol & compositor reference that we shouldn't bind to the most appropriate seat given to us if you think it should be changed.

@markbolhuis
Copy link
Author

Please provide protocol & compositor reference that we shouldn't bind to the most appropriate seat given to us if you think it should be changed.

Unfortunately we don't know which seat is the most appropriate. A seat may gain or lose the keyboard capability at any time. So just because one of the seats has a keyboard on the first capabilities event doesn't mean it will persist, or another won't gain it later.
wayland.xml:1817

We also cannot use the name of the seat, since there is no naming convention.
wayland.xml:1891


The only compositors that I'm aware of that handle multiple seats are Sway and Weston, and each handle it differently.

Weston's first seat is named "default" but more can be added with udev rules.
Tizen Docs
Weston Docs

Sway's first seat is named "seat0", and more are added by attaching devices in .config/sway/config.
Sway seat configuration


Ideally we need to know which seats Qt is using.
From a brief loot at their source code it seems Qt binds to all seats. Although I'm not sure how it handles multiple focus.

@kkartaltepe
Copy link
Collaborator

Thanks the references are really helpful.

The only compositors that I'm aware of that handle multiple seats are Sway and Weston, and each handle it differently.

I see then probably we dont want to deal with multiple seats as application usage of this is both undefined and different compositors have different expectations of applications. Binding to the first seat seems fine since so far we have been broken if any compositor reported multiple seats anyway.

@kkartaltepe
Copy link
Collaborator

sway developers suggest we listen to every keyboard, so that can be an enhancement to support multi-seat setups.

@markbolhuis
Copy link
Author

sway developers suggest we listen to every keyboard, so that can be an enhancement to support multi-seat setups.

Ok, I'll close this MR and move the discussion to #7517

@markbolhuis markbolhuis closed this Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants