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

Add support for sources (microphone) #64

Merged
merged 5 commits into from
Jul 3, 2022

Conversation

Aerion
Copy link
Contributor

@Aerion Aerion commented Jun 28, 2022

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.

Aerion added a commit to Aerion/dotfiles-public that referenced this pull request Jun 28, 2022
Copy link
Owner

@marioortizmanero marioortizmanero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the contribution!

Haha, it is indeed quite hacky but it does the trick, and that's what shell scripts are for, after all :P This will finally close #25.

  • 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 <link to release>". 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.
  • For the documentation, we're going to need to explain how it works in the README and update the --help message.
  • 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?
  • 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?
  • I think the explanation in the --help message could elaborate a bit more as well.
  • The tests are broken now
  • Looks like you're using tabs and we used spaces in the script

pulseaudio-control.bash Show resolved Hide resolved
@marioortizmanero
Copy link
Owner

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
Copy link
Contributor Author

Aerion commented Jun 29, 2022

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
Copy link
Contributor Author

Aerion commented Jun 29, 2022

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.

Aerion added a commit to Aerion/dotfiles-public that referenced this pull request Jun 29, 2022
Copy link
Owner

@marioortizmanero marioortizmanero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@marioortizmanero
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

Aerion commented Jul 2, 2022

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
Copy link
Contributor Author

Aerion commented Jul 2, 2022

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: #67

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

@Aerion
Copy link
Contributor Author

Aerion commented Jul 2, 2022

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
Copy link
Owner

marioortizmanero commented Jul 2, 2022

I agree, thinking about it I think this fix would solve the issue without changing the latency: #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 and others added 3 commits July 2, 2022 15:02
Before this commit, pulseaudio-control supported solely sinks. After
this commit, it supports sinks and sources, under a new name "node".

A new flag `--node-type` changing the behavior of the script to
handle sources instead of sinks.

All the existing mentions of sinks have been changed to node when
appropriate.

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.
@Aerion
Copy link
Contributor Author

Aerion commented Jul 2, 2022

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
Copy link
Owner

Ok, I'd say that we can revert this commit and merge: a75f392

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

README.md Outdated Show resolved Hide resolved
@Aerion
Copy link
Contributor Author

Aerion commented Jul 2, 2022

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

Sometimes, only the client event is present, to be safe, update when
it's triggered.
@Aerion
Copy link
Contributor Author

Aerion commented Jul 2, 2022

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
Copy link
Owner

marioortizmanero commented Jul 3, 2022

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.

@marioortizmanero marioortizmanero merged commit 399b7e8 into marioortizmanero:master Jul 3, 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