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

celt-0.11.x compatibility #184

Closed
wants to merge 1 commit into from
Closed

Conversation

ei-grad
Copy link

@ei-grad ei-grad commented Nov 5, 2013

There is no CELT_SET_VBR_RATE in celt-0.11, so main.pro build fails if system has celt-0.11 installed.

@ei-grad
Copy link
Author

ei-grad commented Nov 5, 2013

With this patch it successfully builds on linux system (ArchLinux) with celt-0.11.3 installed.

@mkrautz
Copy link
Contributor

mkrautz commented Nov 10, 2013

Hi Andrew,

First of all, thanks for the patch.

Can you elaborate a bit on why you needed this?

Were you building Mumble from source on Arch, and ran into this problem?

The thing is, to get a Mumble build that's compatible across all 1.2.x versions, you need CELT 0.7.x support. It seems to me like you are only building against the system libcelt (which is 0.11.x). The result of that is a Mumble client that supports
these codecs:

Speex, CELT 0.11.x, Opus

Or perhaps even fewer.

Such a client is not guaranteed to be able to communicate amongst all Mumble 1.2.x versions because version 1.2.0 shipped with CELT 0.7.x as its only codec. That codec is our baseline codec, and as long as your client has CELT 0.7.x, you're compatible, and can speak to everyone (because every client should, in theory at least, include a CELT 0.7.x codec).

We do have a build option to help with this "mess", though. It is CONFIG+=bundled-celt, and passing it to qmake will build a Mumble client against Mumble's own copies of CELT 0.7.x and CELT 0.11.x, ensuring compatibility regardless of which version of CELT is shipped with your distro. This is also how the mumble packages in Arch are built. See for example the package contents on https://www.archlinux.org/packages/community/x86_64/mumble/. It has its own local CELT copies in /usr/lib/mumble.

More and more people/distros should probably be using CONFIG+=bundled-celt now the CELT has been consumed into Opus. Finding packaged CELT libraries to live up to Mumble's requirement of a 0.7.x baseline is probably going to be really, really hard, and very few packages depend on CELT 0.11.x as far as I'm aware.

I'd also like to touch on the <celt.h> include in CELTCodec.h, which is a problem when building against system CELT libraries.

When building against bundled CELT we ensure that <celt.h> resolves to CELT 0.7.x's copy. However if your system only has 0.11.x, we just get build errors, like you've encountered here.

Perhaps we should just #error in that case, and describe the situation.

@vorot93
Copy link
Contributor

vorot93 commented Nov 28, 2013

Is it necessary to support CELT at this point? I am very sure that the overwhelming majority have upgraded to Mumble 1.2.4 by now. It would be better to focus on Opus, would it not?

@hacst
Copy link
Contributor

hacst commented Nov 30, 2013

We are focusing on Opus. If the clients support it and the server is capable Opus will be preferred. However we promised backwards compatibility throughout the 1.2.X series and we intent to honor that promise as good as we can (which means supporting CELT and Speex). For the future the plan definitely is to get rid of all other codecs and use Opus exclusively.

@hacst
Copy link
Contributor

hacst commented Nov 30, 2013

As for this PR: Mumble needs CELT 0.7.X. All other CELT versions can only be used in addition to that. As we adopted CELT pre-release we willingly accepted that we would have to support specific versions of the codec while it's developing. @mkrautz described the situation very well and since there has been no follow-up I'm assuming this PR can be closed. I created issue #1080 for adding explicit compiler output for this case.

@hacst hacst closed this Nov 30, 2013
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

4 participants