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

Mumble bridge locks audio down to CELT #1750

Closed
mweinelt opened this issue Feb 23, 2022 · 7 comments
Closed

Mumble bridge locks audio down to CELT #1750

mweinelt opened this issue Feb 23, 2022 · 7 comments

Comments

@mweinelt
Copy link

Describe the bug
The mumble bridge connects without advertising Opus support, which downgrades the audio codec to CELT for everyone.

To Reproduce
Configure matterbridge to bridge a mumble room, notice how when the bridge is connected the server codec under Server -> Information switches to CELT 0.7.0, while if it isn't connected it uses Opus.

Expected behavior
Matterbridge should advertise Opus support when connecting.

Screenshots/debug logs
20220223_18h56m54s_grim

Use logs from running matterbridge -debug if possible.
Deferring to @kmein

Environment (please complete the following information):

  • OS: NixOS 21.11
  • Matterbridge version: 1.22.3
  • If self compiled: output of git rev-parse HEAD

Additional context
Please add your configuration file (be sure to exclude or anonymize private data (tokens/passwords))
Deferering to @kmein

@mweinelt mweinelt added the bug label Feb 23, 2022
@mweinelt
Copy link
Author

Sorry, wrong version. We were using 1.19.0 and it appears to be fixed with 1.22.3.

@mweinelt mweinelt reopened this Feb 23, 2022
@mweinelt
Copy link
Author

mweinelt commented Feb 23, 2022

Correction. It worked because we forced the server to use Opus and that worked fine. When we stop forcing Opus then Matterbridge forces everyone back to CELT. This is on 1.22.3 as stated in the OP.

20220223_23h32m03s_grim

@42wim 42wim added the mumble label Mar 12, 2022
@42wim
Copy link
Owner

42wim commented Mar 12, 2022

@s3lph do you have an idea?

@s3lph
Copy link
Contributor

s3lph commented Mar 15, 2022

Hm it appears that the only currently available option to enable Opus support is to actually enable the Opus codec in gumble, which pulls in an entirely new dependency (github.com/layeh/gopus, a Go wrapper around the Opus C implementation). Enabling Opus support is as simple as adding a single import:

diff --git a/bridge/mumble/mumble.go b/bridge/mumble/mumble.go
index 2281d1c2..7c48c991 100644
--- a/bridge/mumble/mumble.go
+++ b/bridge/mumble/mumble.go
@@ -12,6 +12,7 @@ import (

        "layeh.com/gumble/gumble"
        "layeh.com/gumble/gumbleutil"
+       _ "layeh.com/gumble/opus"

        "github.com/42wim/matterbridge/bridge"
        "github.com/42wim/matterbridge/bridge/config"

I'm not quite sure whether I'm doing something wrong, but building the Opus library seems to not work properly with vendoring (the opus-1.1.2 source is checked into the gopus repo):

~/go/src/github.com/42wim/matterbridge $ go build
# layeh.com/gopus
vendor/layeh.com/gopus/opus_nonshared.go:9:11: fatal error: opus-1.1.2/config.h: No such file or directory
    9 | // #include "opus-1.1.2/config.h"
      |             ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.

I see 3 potential ways going forward. Personally, I'm not a huge fan of introducting this actually completely unnecessary dependency (since we're not processing any audio). :

  • We get gopus to build. @42wim do you have any ideas how to fix this? I'm not really familiar with Go's build system. If this is easy to fix, this is the most simple solution for this issue.
  • We don't use gopus, but provide a stub codec that claims to implement Opus. Rather ugly, but I implemented this as a POC and it's working.
  • We open a PR on Gumble adding an option that would allow us to explicitly set the "Opus supported" flag without requiring to pull in the Opus dependency.

@42wim any thoughts?

@42wim
Copy link
Owner

42wim commented Mar 15, 2022

@s3lph thanks for looking into this, as gopus is requiring CGO, this is something I rather not do, want to keep matterbridge pure go as possible.

I think option 3 is the cleanest if you're willing to do this, but I'm also fine with option 2 if that's less work :)

@s3lph
Copy link
Contributor

s3lph commented Mar 15, 2022

layeh/gumble#61

I hope this gets merged soon. If not we could still resort to option 2.

s3lph added a commit to s3lph/matterbridge that referenced this issue Mar 19, 2022
42wim pushed a commit that referenced this issue Mar 19, 2022
* Mumble: Implement a workaround to signal Opus support without pulling in the CGO gopus dependency.

* mumble: lowercase error messages

* mumble: Add link to #1750 in bridge/mumble/codec.go
@42wim
Copy link
Owner

42wim commented Mar 19, 2022

Fixed by #1764

@42wim 42wim closed this as completed Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants