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

[bug] Wireplumber module crashes on audio context change #1907

Closed
Kaylier opened this issue Dec 27, 2022 · 5 comments · Fixed by #1942
Closed

[bug] Wireplumber module crashes on audio context change #1907

Kaylier opened this issue Dec 27, 2022 · 5 comments · Fixed by #1942
Labels
bug Something isn't working

Comments

@Kaylier
Copy link

Kaylier commented Dec 27, 2022

The wireplumber module causes waybar to crash when the wireplumber context changes.
This is probably due to the fact that node.id is not a stable identifier for nodes.

How to replicate

Sway v1.7
Waybar v0.9.16
libwireplumber v0.4.13

config.conf:

{ "modules-center": ["wireplumber"] }

style.css empty

There are several ways to trigger the issue.

Disconnect sink

If you have a USB soundcard, a bluetooth headset or anything that you can disconnect:

  1. plug it
  2. set it as default sink with wpctl set-default <id>
  3. start waybar
  4. unplug it
  5. waybar will crash on next refresh of the module, you can force it by changing your volume for example

Change TTY

For some reasons, wireplumber reassign a new id to every node when I open a new tty.

  1. start waybar
    (opt) check nodes id with wpctl status
  2. switch to a new tty (no need to log in)
  3. switch back to sway
    (opt) check nodes id again

Output

> waybar -c config.conf -s style.css -l trace
[2022-12-27 10:14:43.318] [info] Using configuration file config.conf
[2022-12-27 10:14:43.318] [info] Using CSS file style.css
[2022-12-27 10:14:43.324] [debug] Output detection done: eDP-1 (Sharp Corporation 0x1453 0x00000000)
[2022-12-27 10:14:43.345] [debug] GTK widget tree:
window#waybar.background.eDP-1..mode-default:dir(ltr)
  decoration:dir(ltr)
  box.horizontal:dir(ltr)
    box.horizontal.modules-left:dir(ltr)
    box.horizontal.modules-center:dir(ltr)
      widget:dir(ltr)
        label#wireplumber:dir(ltr)
    box.horizontal.modules-right:dir(ltr)

[2022-12-27 10:14:43.346] [info] Bar configured (width: 1920, height: 19) for output: eDP-1
[2022-12-27 10:14:47.439] [error] Node 31 does not support volume
> echo $?
1

Note 1: I triggered the error by switching tty2 at ~10:14:47.4
Note 2: Node 31 was the default Audio/Sink node when waybar started

Related issue

Suggestion

  • Re-fetch the default node each time the module is updated
  • Add a callback on relevant WP signal (maybe Object Manager objects-changed?)
  • Use the node.name instead of node.id (doesn't fix the related issue though)
@smoak
Copy link
Contributor

smoak commented Jan 13, 2023

Can you see if this patch fixes it for you? It has in my testing

https://github.com/Alexays/Waybar/compare/master...smoak:Waybar:fix-wireplumber-crashes.patch

@Alexays Alexays added the bug Something isn't working label Jan 13, 2023
@Kaylier
Copy link
Author

Kaylier commented Jan 13, 2023

It doesn't crash anymore, but volume is not updated.

After some investigation, the issue is with "get-default-configured-node-name" which returns the name of the sink configured to be the default, but not the current selected sink. So in my case, it always gets my configured default aka my bluez output, even when it's not connected.

I changed it to get the name from wp_pipewire_object_get_property instead, like you do in onMixerChanged (cf
node_name.patch.txt). It works well after that.

Also, I understand better what happens during tty changes. When I switch to a tty which doesn't have a session of the same user opened, it "unplugs" devices from the user and "plugs" it to the new user.
If I run wpctl status in the user session while I am in the unlogged tty, it shows no devices, and a single sink (nick="Dummy Output", name="auto_null").
Once I get back to sway, it reaffect/plugs everything back to the user session. This results in new node ids.

Now that the module handle hotplug of a soundcard, it also handle "hotplug" of internal devices. So it should work with node.id as well as node.name. The difference is that the name is likely to stay the same one session to an other.
For curiosity, I quickly tried to remove/swap default_node_name_ by node_id_ (cf node_id.patch.txt), and indeed it seems to work. (I might have broken something else though)

@smoak
Copy link
Contributor

smoak commented Jan 13, 2023

After some investigation, the issue is with "get-default-configured-node-name" which returns the name of the sink configured to be the default, but not the current selected sink. So in my case, it always gets my configured default aka my bluez output, even when it's not connected.

Thanks. That explanation makes sense. I have updated my branch with what you have stated (ie getting the default node name via wp_pipewire_object_get_property) and put it some extra defensive code. I'll submit a PR for this since I believe it should fix everything (including the volume update).

@Kaylier
Copy link
Author

Kaylier commented Jan 14, 2023

I tested it, it works perfectly after I plug/unplug devices. But there is an issue at startup, onObjectManagerInstalled still uses the configured default.

@smoak
Copy link
Contributor

smoak commented Jan 16, 2023

But there is an issue at startup, onObjectManagerInstalled still uses the configured default.

I don't think this is possible. There is only the default nodes api to use. Otherwise, this module would have to be a full blown manager application in order to access the internals of Pipewire. I could add a config option (e.g. node_name) to override the default node, but that seems like a new feature request that shouldn't hold up the PR merge to fix this particular crash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants