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

Create OpusCodec class, similar to CeltCodec, in order to load Opus' functions from a shared library #3431

Merged

Conversation

@davidebeatrici
Copy link
Member

commented Jul 1, 2018

In the past we already encountered a problem where we couldn't link statically to both Opus and CELT at the same time, because there are multiple symbols (functions) shared across the two libraries, as Opus is the successor of CELT.

Initially this was solved by building Opus in C++ mode. which provides name mangling for the symbols, but since version 1.1 this isn't possible anymore:

# Before Opus 1.1 we used to be able to build Opus
# as C++ code to get C++ name mangling for free,
# allowing us to statically build both libcelt
# and libopus into the same binary while avoiding
# symbol clashes between the two libraries.
#
# Stock Opus 1.1 doesn't build in C++ mode, so error
# out for now.

We currently build Opus as static library and CELT as shared, loading it through the CELTCodec class.

#3427 adds support for RNNoise, an advanced noise suppression library, which has some functions shared with Opus, resulting in a linker error.

This pull request adds a new class called OpusCodec, similar to CELTCodec, in order to load Opus' functions from a shared library, so that we can link statically to RNNoise.

@davidebeatrici davidebeatrici requested review from mkrautz, Kissaki, bontibon and hacst Jul 1, 2018

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:opuscodec-shared-library branch 4 times, most recently from 7e4ce62 to 512fa3d Jul 1, 2018

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

Please provide context so I don’t have to work through and think about an entire other PR to make sense of this. This adds a ton of overhead for me when you already have the context and could describe it.

What is CeltCodec? How is Opus implemented differently? Why would that make sense, or make sense to change it?

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2018

I updated the pull request message.

@@ -288,14 +291,17 @@ bool AudioOutputSpeech::needSamples(unsigned int snum) {
memset(pOut, 0, sizeof(float) * iFrameSize);
} else if (umtType == MessageHandler::UDPVoiceOpus) {
#ifdef USE_OPUS
decodedSamples = opus_decode_float(opusState,
if (opusState) {

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 2, 2018

Member

This was not here before? What is this?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 2, 2018

Author Member

Additional check: if opusState is not initialized, the function makes the program crash.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 3, 2018

Member

The function opus_decoder_create does not seem to define when it returns NULL, and what it does in error states. Can we do that?
Maybe we should be checking the error code that the function returns and set NULL ourselves?

https://opus-codec.org/docs/html_api-1.0.1/group__opus__decoder.html#ga753f6fe0b699c81cfd47d70c8e15a0bd

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 3, 2018

Author Member

Maybe we should be checking the error code that the function returns and set NULL ourselves?

Yeah, it would be better.

Do you prefer to do it in this pull request or in a dedicated one?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 4, 2018

Member

I would prefer this added check not to be in the commit that introduces OpusCodec anyway. That should not change logic.

That is, unless our introduction of OpusCodec would make opusState being NULL any more likely? But I don't think that's the case?

So it's adding the check either in a separate commit in this PR, or in a new one. And in that place, we should probably check the error code instead.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 4, 2018

Author Member

Let's do it in another pull request.

@@ -288,14 +291,17 @@ bool AudioOutputSpeech::needSamples(unsigned int snum) {
memset(pOut, 0, sizeof(float) * iFrameSize);
} else if (umtType == MessageHandler::UDPVoiceOpus) {
#ifdef USE_OPUS
decodedSamples = opus_decode_float(opusState,
if (opusState) {
decodedSamples = oCodec->opus_decode_float(opusState,

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 2, 2018

Member

Wrong indent

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 2, 2018

Author Member

Fixed.

#endif

#ifdef PLUGIN_PATH
qlOpus.setFileName(QLatin1String(MUMTEXT(PLUGIN_PATH) "/") + lib);

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 2, 2018

Member

What is this? What is PLUGIN_PATH?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 2, 2018

Author Member

Copied from CELTCodec, it looks for the library in the plugins folder.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 3, 2018

Member

Our PA plugins? How does that make sense for either CELT or Opus?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 3, 2018

Author Member

On Debian the directory is /usr/lib/mumble: https://packages.debian.org/stable/amd64/mumble/filelist

The positional audio plugins and all the libraries are stored there.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 3, 2018

Member

I’m a little confused where PLUGIN_PATH is defined though. But ok.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 3, 2018

Author Member

It's defined by the package maintainer.

#define __cdecl
#endif

class OpusCodec {

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 2, 2018

Member

Please add a class comment

I shouldn't have to read git log/blame to know what this class is about

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 2, 2018

Author Member

Also CELTCodec doesn't have it.

I think that we should add a comment for both in a separate pull request.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 3, 2018

Member

No. This PR introduces this class. This class should be documented. This PR documents what the class is for. This PR should document this class.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 3, 2018

Member

e,g, The OpusCodec class handles loading the shared Opus library and is a wrapper for that shared librarys functions.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 3, 2018

Author Member
The OpusCodec class loads Opus from a shared library and acts as a wrapper for its functions.

What do you think?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 3, 2018

Member

We only seem to have 3 @brief comments, and none of those use the style "The X type".

So maybe just use

Loads Opus from a shared library and acts as a wrapper for its functions.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 3, 2018

Author Member

I agree.

# Stock Opus 1.1 doesn't build in C++ mode, so error
# out for now.
error(Mumble cannot be built in SBCELT mode with Opus 1.1 - aborting build.)
}

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 2, 2018

Member

I don't get the logic of this block. Is sbcelt from mumbles qmake build parameters? It bleeds into here?

We always statically link celt 0.7 which doesn't have conflicts? Is this about celt 0.11? Is that yet another lib?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 2, 2018

Author Member

Is sbcelt from mumbles qmake build parameters? It bleeds into here?

Yes.

We always statically link celt 0.7 which doesn't have conflicts? Is this about celt 0.11? Is that yet another lib?

We dynamically link to both CELT 0.7 and CELT 0.11.

The sbcelt build option enables a special version of CELT, statically linked.

https://github.com/mumble-voip/sbcelt

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

/spend 40min

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:opuscodec-shared-library branch from 512fa3d to 9d72b73 Jul 2, 2018

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

What is your reasoning for making Opus a shared library rather than RNNoise? Opus is rather more central to Mumble.

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2018

  • To maintain consistency, as we load CELT dynamically;
  • To be able to load a more recent version of Opus without recompiling Mumble;
  • RNNoise is not released in any distribution yet.
@Kissaki
Copy link
Member

left a comment

The commit message should be improved. The summary line holds too much information, and ends up being too long.

e.g.

Introduce OpusCodec for use of Opus as shared library

The OpusCodec class handles loading the shared Opus library and is a wrapper for that shared librarys functions.

We use equivalent logic for Celt in CeltCodec.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

Missing class comment and too long commit title, otherwise LGTM

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

/spend 15min

@Kissaki Kissaki removed request for mkrautz, bontibon and hacst Jul 3, 2018

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:opuscodec-shared-library branch from 9d72b73 to 9d29c3d Jul 3, 2018

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2018

Done.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:opuscodec-shared-library branch from 9d29c3d to 6803a90 Jul 3, 2018

#endif

// Loads Opus from a shared library and acts as a wrapper for its functions.
class OpusCodec {

This comment has been minimized.

Copy link
@Kissaki

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:opuscodec-shared-library branch 2 times, most recently from 5348950 to 08a05fa Jul 3, 2018

oCodec->report();
g.oCodec = oCodec;
} else {
delete oCodec;

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 4, 2018

Member

We should probably log/display some information if the opus codec should be used but is not available/found?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 4, 2018

Author Member

Done.

@@ -149,8 +149,9 @@ AudioInput::~AudioInput() {
wait();

#ifdef USE_OPUS
if (opusState)
opus_encoder_destroy(opusState);
if (opusState) {

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 4, 2018

Member

Oh, here we actually did have an opusState check. That doesn't make much sense if we simply use opusState above as if it were always usable...

We should probably add the error check and set null in a commit before this one

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 4, 2018

Author Member

I think it would be ideal to do it in a dedicated pull request, as this one should just make the shared library work.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 6, 2018

Member

On one access of oCodec we check oCodec, on another we check opusState. I don’t like that asymmetry. Definitely not "ideal". But w/e

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 6, 2018

Author Member

I don't like it either, but we will fix it in a dedicated pull request.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:opuscodec-shared-library branch 2 times, most recently from b5fb0cf to d522ae4 Jul 4, 2018

oCodec->report();
g.oCodec = oCodec;
} else {
qWarning("CodecInit: failed to load Opus, it will not be available for encoding/decoding");

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 6, 2018

Member

Should probably be encapsulated? Failed to

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 6, 2018

Member

I think the message could be more elaborative. "encoding/decoding audio". Maybe "load" could be improved. But it's okay.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jul 6, 2018

Author Member

Done.

@@ -149,8 +149,9 @@ AudioInput::~AudioInput() {
wait();

#ifdef USE_OPUS
if (opusState)
opus_encoder_destroy(opusState);
if (opusState) {

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 6, 2018

Member

On one access of oCodec we check oCodec, on another we check opusState. I don’t like that asymmetry. Definitely not "ideal". But w/e

@Kissaki
Kissaki approved these changes Jul 6, 2018
Introduce OpusCodec to use Opus as shared library
The OpusCodec class loads Opus from a shared library and acts as a wrapper for its functions.

We use equivalent logic for CELT in CELTCodec.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:opuscodec-shared-library branch from d522ae4 to 8ca51d0 Jul 6, 2018

@davidebeatrici davidebeatrici merged commit e0ee016 into mumble-voip:master Jul 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.