werman / noise-suppression-for-voice

Noise suppression plugin based on Xiph's RNNoise
GNU General Public License v3.0
4.79k stars 229 forks source link

Add target.object to capture.props in exemplary pipewire config #162

Open jkhsjdhjs opened 1 year ago

jkhsjdhjs commented 1 year ago

The readme currently gives an exemplary pipewire config using the filter-chain module, which really helps when setting this up. The filter chain simply listens on the default source, which might not be the source you actually want it to use. To get around this, you can specify a target source as input for the filter-chain (thanks to @licentiapoetica for figuring this out):

capture.props = {
    node.name =  "capture.rnnoise_source"
    node.passive = true
    audio.rate = 48000
    target.object = "alsa_input.usb-Logitech_Logitech_G930_Headset-00.mono-fallback"
}

I propose to either add this to the example or at least note it somewhere, so that other users don't face the same issue as I did.

See also: https://github.com/werman/noise-suppression-for-voice/issues/162#issuecomment-1677979108

pallaswept commented 1 year ago

I've just been setting my pipewire up like this, and thought I should mention that I'd noticed node.target is deprecated and target.object is the new way to do this, so you'd want

capture.props = {
    node.name =  "capture.rnnoise_source"
    node.passive = true
    audio.rate = 48000
    target.object = "alsa_input.usb-Logitech_Logitech_G930_Headset-00.mono-fallback"
}

The only issue I can see with this is that if you set it up like this then you can't dynamically switch the default source, which might actually be desirable (ie you have multiple mics and want to be able to toggle which one goes to a single instance of rnnoise, thus avoiding the need to set up multiple rnnoise instances)

jkhsjdhjs commented 1 year ago

Thanks for the info! Do you happen to have a source on the deprecation?

The only issue I can see with this is that if you set it up like this then you can't dynamically switch the default source, which might actually be desirable (ie you have multiple mics and want to be able to toggle which one goes to a single instance of rnnoise, thus avoiding the need to set up multiple rnnoise instances)

That's true, or at least it's not as easy anymore. You can still do this via tools like qpwgraph or pw-cli.

pallaswept commented 1 year ago

Thanks for the info! Do you happen to have a source on the deprecation?

Sure, it's in the official docs https://docs.pipewire.org/group__pw__keys.html#ga33441b02b8e14e14451272c2b497a5c9 There's more talk of it elsewhere where they explain why the new way is better because it allows you to use a more unique target identifier. I really wish the search engine on those pages actually worked, sorry I couldn't find it right away. (IIRC the old way can take object .name or object .id, and the new way takes object .name or object .serial? I might be mis-remembering here but I'm pretty sure that was it.

fwiw, node.target still does actually work, but it's pretty clear from the docs that it won't be that way forever and they encourage the use of the new way in a few places.

That's true, or at least it's not as easy anymore. You can still do this via tools like qpwgraph or pw-cli.

Yeh and you could always set up the input device that way too, but it's not a nice way to go about it which is why we like target designators :). My point is that if this hard-wiring a mic to the rnnoise input is your intended use case (like it is for me and obviously you) then this seems like a good idea... but it doesn't just add something, it takes something away, and for anyone with the other use-case as their intention, this change would cause things to not work. As in, this fixes a problem for some people and creates a problem for others. And now that I'm thinking about it, there may well be people who want both approaches... So either way you win some and you lose some.

Perhaps it would be best to give both alternatives, and an explanation of the behaviour in the comments.

jkhsjdhjs commented 1 year ago

Sure, it's in the official docs https://docs.pipewire.org/group__pw__keys.html#ga33441b02b8e14e14451272c2b497a5c9 There's more talk of it elsewhere where they explain why the new way is better because it allows you to use a more unique target identifier. I really wish the search engine on those pages actually worked, sorry I couldn't find it right away. (IIRC the old way can take object .name or object .id, and the new way takes object .name or object .serial? I might be mis-remembering here but I'm pretty sure that was it.

Ah, interesting! Thanks for linking that. I've always found the pipewire docs hard to navigate and sadly wasn't able to locate this myself. I wonder why pipewire doesn't print a deprecation warning when using node.target though. Anyway, I just switched my configs to target.object and it works fine!

Yeh and you could always set up the input device that way too, but it's not a nice way to go about it which is why we like target designators :). My point is that if this hard-wiring a mic to the rnnoise input is your intended use case (like it is for me and obviously you) then this seems like a good idea... but it doesn't just add something, it takes something away, and for anyone with the other use-case as their intention, this change would cause things to not work. As in, this fixes a problem for some people and creates a problem for others. And now that I'm thinking about it, there may well be people who want both approaches... So either way you win some and you lose some.

Perhaps it would be best to give both alternatives, and an explanation of the behaviour in the comments.

Yeah, I get your point, and I agree. If this is added the (dis-)advantages and usecases of either method should be stated.

pallaswept commented 1 year ago

I've always found the pipewire docs hard to navigate

You are not alone. A working search engine would go a long way. I just stumbled across this when I was setting up some node-targets myself.

I wonder why pipewire doesn't print a deprecation warning

Good point, I'm about to file an issue that it doesn't, because it should. There we go