search

marioortizmanero / polybar-pulseaudio-control

A feature-full Polybar module to control PulseAudio
MIT License
466 stars 49 forks source link

Add support for sources (microphone) #64

Closed Aerion closed 2 years ago

Aerion commented 2 years ago

A new flag --microphone changes the behavior of the script to handle sources instead of sinks.

Sinks and sources almost share the same commands, with the main difference being the command name (i.e. sink for Sinks).

Thus, this commit adds a simple and hacky way to handle sources: a variable SINK_OR_SOURCE holds either ink or ource so that commands that were using Sink Sinks sinks sink can be transformed to Source Sources sources source easily.

Again, hacky, but does the job.

marioortizmanero commented 2 years ago

I've tried it out and it seems like we need to add on client to the listen grep. Otherwise that command doesn't work properly.

Aerion commented 2 years ago

I would also change the SINK_NICKNAMES, ICON_SINK, and SINK_BLACKLIST variables to avoid confusion. Unfortunately, that will be a breaking change, but I would say it's worth it? Perhaps we could declare the old variables at the top as dummies with the value "Please update your variable names. See ". That way everyone would know about it, rather than breaking mysteriously. We can then remove them in a future version. Also the command next-sink.

Sure thing!

For the documentation, we're going to need to explain how it works in the README and update the --help message. I think the explanation in the --help message could elaborate a bit more as well.

Updated, I changed the flag to be --node-type, please let me know if additional clarifications should be done.

Maybe we could come up with a word to treat both sinks and sources to update the outdated comments, functions or variables, like artefact or device?

Agreed, changed it to node, taking inspiration from AudioNode

Since it's tricky, could you please also comment out this solution carefully when introducing the variable, so that future developers can understand it easily, and why this was the best option?

The tests are broken now

On it

Looks like you're using tabs and we used spaces in the script

Whoops, should be fixed :)

Aerion commented 2 years ago

I've tried it out and it seems like we need to add on client to the listen grep. Otherwise that command doesn't work properly.

Fixed: it seems that listening on server does the trick.

marioortizmanero commented 2 years ago

Also, the listen command is still broken for microphone swapping. This is what I get when changing the default one:

$ pactl subscribe
Event 'new' on client #35269
Event 'remove' on client #35269

The "on s${SINK_OR_SOURCE}" part is wrong, since there are no on source events on my machine.

marioortizmanero commented 2 years ago

And one additional problem I've found: for some reason I also get sinks when running pactl list sources short? I think I don't understand how pactl works correctly, because our script tries to swap to sinks when using sources because of this, as far as I know. I'm a bit confused.

$ pactl list sinks short
81  alsa_output.usb-Kingston_HyperX_Virtual_Surround_Sound_00000000-00.analog-stereo    PipeWire    s16le 2ch 48000Hz   SUSPENDED
83  alsa_output.pci-0000_00_1f.3.analog-stereo  PipeWire    s32le 2ch 48000Hz   SUSPENDED
128 bluez_output.F8_5C_7D_B5_D7_97.a2dp-sink    PipeWire    s16le 2ch 48000Hz   RUNNING

$ pactl list sources short
81  alsa_output.usb-Kingston_HyperX_Virtual_Surround_Sound_00000000-00.analog-stereo.monitor    PipeWire    s16le 2ch 48000Hz   SUSPENDED
82  alsa_input.usb-Kingston_HyperX_Virtual_Surround_Sound_00000000-00.analog-stereo PipeWire    s16le 2ch 48000Hz   SUSPENDED
83  alsa_output.pci-0000_00_1f.3.analog-stereo.monitor  PipeWire    s32le 2ch 48000Hz   SUSPENDED
84  alsa_input.pci-0000_00_1f.3.analog-stereo   PipeWire    s32le 2ch 48000Hz   SUSPENDED
128 bluez_output.F8_5C_7D_B5_D7_97.a2dp-sink.monitor    PipeWire    s16le 2ch 48000Hz   IDLE
Aerion commented 2 years ago

Also, the listen command is still broken for microphone swapping. This is what I get when changing the default one:

$ pactl subscribe
Event 'new' on client #35269
Event 'remove' on client #35269

The "on s${SINK_OR_SOURCE}" part is wrong, since there are no on source events on my machine.

On my side (pipewire), here is the output when switching the microphone once. Note that there are lots of clients, but only one server event: Event 'change' on server #4294967295.

I'm fine with changing the event criteria if it doesn't work on your end of course. I didn't dig deeper as this event was always generated on my end.

Event 'new' on client #2889
Event 'remove' on client #2889
Event 'new' on client #2890
Event 'remove' on client #2890
Event 'new' on client #2891
Event 'remove' on client #2891
Event 'new' on client #2892
Event 'remove' on client #2892
Event 'new' on client #2893
Event 'remove' on client #2893
Event 'change' on server #4294967295
Event 'new' on client #2894
Event 'remove' on client #2894
Event 'new' on client #2895
Event 'new' on client #2896
Event 'new' on client #2897
Event 'new' on client #2898
Event 'remove' on client #2896
Event 'remove' on client #2897
Event 'remove' on client #2895
Event 'remove' on client #2898
Event 'new' on client #2899
Event 'remove' on client #2899
Event 'new' on client #2900
Event 'remove' on client #2900
Event 'new' on client #2901
Event 'new' on client #2902
Event 'remove' on client #2901
Event 'remove' on client #2902
Event 'new' on client #2903
Event 'remove' on client #2903
Event 'new' on client #2904
Event 'new' on client #2905
Event 'remove' on client #2904
Event 'remove' on client #2905
Event 'new' on client #2906
Event 'remove' on client #2906
Event 'new' on client #2907
Event 'new' on client #2908
Event 'new' on client #2909
Event 'remove' on client #2907
Event 'remove' on client #2908
Event 'remove' on client #2909
Event 'new' on client #2910
Event 'remove' on client #2910
Event 'new' on client #2911
Event 'new' on client #2912
Event 'new' on client #2913
Event 'remove' on client #2912
Event 'remove' on client #2911
Event 'remove' on client #2913
Event 'new' on client #2914
Event 'remove' on client #2914
Event 'new' on client #2915
Event 'new' on client #2916
Event 'new' on client #2917
Event 'new' on client #2918
Event 'remove' on client #2918
Event 'remove' on client #2916
Event 'remove' on client #2917
Event 'remove' on client #2915
Event 'new' on client #2919
Event 'new' on client #2920
Event 'new' on client #2921
Event 'new' on client #2922
Event 'remove' on client #2919
Event 'remove' on client #2920
Event 'remove' on client #2921
Event 'remove' on client #2922
Aerion commented 2 years ago

I've fixed a couple nitpicks and deprecated SINK_NICKNAME as well. Let me know if it's okay for merging and I will do so! Thanks a lot for the help, the documentation looks great as well.

The only feedback I have is that I find 100 ms for the latency in read to be too much, personally. It lags a bit and seems smoother with 50ms. What do you think? Should we modify the default value to 50ms or do we make it possible to customize? Do you agree with me here?

I agree, thinking about it I think this fix would solve the issue without changing the latency: https://github.com/marioortizmanero/polybar-pulseaudio-control/pull/67

If you feel it doesn't solve the issue, we can change the 100 ms value to lower, and configurable too!

Aerion commented 2 years ago

And one additional problem I've found: for some reason I also get sinks when running pactl list sources short? I think I don't understand how pactl works correctly, because our script tries to swap to sinks when using sources because of this, as far as I know. I'm a bit confused.

I'm a novice regarding PulseAudio, but the monitor for sinks are displayed as source (same can be seen in pavucontrol), in my case I ignored them via --node-blacklist

marioortizmanero commented 2 years ago

I agree, thinking about it I think this fix would solve the issue without changing the latency: https://github.com/marioortizmanero/polybar-pulseaudio-control/pull/67

If you feel it doesn't solve the issue, we can change the 100 ms value to lower, and configurable too!

That does fix it! Great job.

I'm a novice regarding PulseAudio, but the monitor for sinks are displayed as source (same can be seen in pavucontrol), in my case I ignored them via --node-blacklist

Perhaps we should add that --node-blacklist to the config example? That way it's used by ""default"", which I imagine is what everyone wants anyway.

On my side (pipewire), here is the output when switching the microphone once. Note that there are lots of clients, but only one server event: Event 'change' on server #4294967295.

I'm fine with changing the event criteria if it doesn't work on your end of course. I didn't dig deeper as this event was always generated on my end.

Seems like adding the blacklist fixed that issue! No worries, I think.

Aerion commented 2 years ago

Perhaps we should add that --node-blacklist to the config example? That way it's used by ""default"", which I imagine is what everyone wants anyway.

Good idea, it's done :)

A cleaner way would be to ignore within the script the .monitor sources, but I feel it's a good enough way as-is.

marioortizmanero commented 2 years ago

Ok, I'd say that we can revert this commit and merge: https://github.com/marioortizmanero/polybar-pulseaudio-control/pull/64/commits/a75f392c8bbbbf3e62eccd0a58102b01d62c158f

I do get source events now, when ignoring the .monitor sources. Sorry for bothering.

Aerion commented 2 years ago

The switch is not detected anymore on my end I don't have the server events, only the client ones…

i.e.

$ pactl subscribe
Event 'new' on client #130403
Event 'remove' on client #130403
Event 'new' on client #130404
Event 'remove' on client #130404
Event 'new' on client #130405
Event 'remove' on client #130405
Event 'new' on client #130406
Event 'remove' on client #130406
Event 'new' on client #130407
Event 'remove' on client #130407
Event 'new' on client #130408
Event 'remove' on client #130408
Aerion commented 2 years ago

To be safe, and support every scenario, I have pushed a new commit where changes are updated on multiple pactl events. (After a reboot, I only had client events when I switched the default sink or source, until I played some sound)

marioortizmanero commented 2 years ago

Yeah it's quite unpredictable, not sure how it works nor where to look it up, so we can just leave it like this. Though I wonder if at this point it's worth having the grep, ha. I don't think I've ever seen an event that doesn't have one of these patterns when running pactl subscribe.

Edit: nevermind, I guess we wouldn't listen for source events when using sinks. So that's enough. Merging this PR finally! Thanks a bunch for your help. I'll release a new version shortly.