Skip to content

Enable Opus ML improvements #3487

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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Enable Opus ML improvements #3487

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f31a9bd
Enable Opus ML improvements
ann0see Apr 6, 2025
5097306
Undo change to Jamulus.pro
softins May 1, 2025
e320601
Add Deep PLC and DRED files for OPUS
softins Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Jamulus.pro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,7 @@ contains(QT_ARCH, armeabi-v7a) | contains(QT_ARCH, arm64-v8a) {
contains(QT_ARCH, x86_64):DEFINES_OPUS += OPUS_X86_PRESUME_SSE=1 OPUS_X86_PRESUME_SSE2=1
DEFINES_OPUS += CPU_INFO_BY_C
}
DEFINES_OPUS += OPUS_BUILD=1 USE_ALLOCA=1 OPUS_HAVE_RTCD=1 HAVE_LRINTF=1 HAVE_LRINT=1
DEFINES_OPUS += OPUS_BUILD=1 USE_ALLOCA=1 OPUS_HAVE_RTCD=1 HAVE_LRINTF=1 HAVE_LRINT=1 OPUS_DRED=1
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this extra define actually does anything. Grepping for OPUS_DRED in the Opus sources doesn't turn it up as a tested preprocessor symbol in any source file. It only appears in libs/opus/CMakeLists.txt, and we don't currently use configure or cmake to build Opus, but rather compile the Opus sources directly in the Jamulus build from Jamulus.pro.

In CMakeLists.txt, it appears that OPUS_DRED as a cmake option controls the inclusion of certain source files in the build, and the addition of preprocessor symbols ENABLE_DEEP_PLC and ENABLE_DRED.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. But the question would then be how to add the arguments in another way.

Copy link
Member

Choose a reason for hiding this comment

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

I did some tests on this a week or two ago while reviewing this PR. That was when I discovered the missing Makefile.in files for which I raised #3488

I tested it by going into libs/opus and doing firstly a plain ./configure, then a ./configure --enable-deep-plc or ./configure --enable-dred. I then compared the config.h and Makefile produced in each case. I did it both on ARM (RPi) and x86 (Debian VM).

For --enable-deep-plc, config.h includes #define ENABLE_DEEP_PLC 1, and the Makefile includes the following extra source file groups:

  • $(DEEP_PLC_SOURCES)
  • $(DNN_SOURCES_ARM_RTCD) (ARM only)
  • $(DNN_SOURCES_DOTPROD) (ARM only)
  • $(DNN_SOURCES_NEON) (ARM only)
  • $(DNN_SOURCES_X86_RTCD) (x86 only)
  • $(DNN_SOURCES_SSE2) (x86 only)
  • $(DNN_SOURCES_SSE4_1) (x86 only)
  • $(DNN_SOURCES_AVX2) (x86 only)
  • $(DEEP_PLC_HEAD)

It also adds something like -Idnn to the compilation so that the extra headers can be found (particularly lpcnet.h).

For --enable-dred, it does all of the above, but config.h also includes #define ENABLE_DRED 1, and the Makefile also includes these extra source file groups:

  • $(DRED_SOURCES)
  • $(DRED_HEAD)

So the above changes need to be incorporated into our Jamulus.pro instead. Probably controlled by testing a CONFIG option so that they can be turned off or on.

It was my plan to expand the above file groups and suggest updates to Jamulus.pro, but I haven't had the time yet (my available time is currently quite limited).

Certainly, I tried just adding DEFINES_OPUS += ENABLE_DEEP_PLC=1 ENABLE_DRED=1 to Jamulus.pro, but compilation then failed because the conditionally-included lpcnet.h could not be found (existing lists of files and include directories do not mention /dnn/). And all the extra files above were not included in the compilation.

Copy link
Member

Choose a reason for hiding this comment

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

I have now implemented the changes to Jamulus.pro to incorporate the PLC and DRED files in the build. See my branch at https://github.com/softins/jamulus/tree/opus-ml which has only one commit, 15f0800. @ann0see, I could either push this commit to your branch here, or you could cherry-pick it yourself. You would need to undo your existing one-line change to Jamulus.pro first, to avoid a conflict.

To build with just the deep PLC, qmake Jamulus.pro "CONFIG+=opus_deep_plc"

To build with Deep Redundancy too, qmake Jamulus.pro "CONFIG+=opus_dred". This always implies deep PLC.

I have only tested that it compiles successfully, with and without the above options, on both ARM (RPi 4) and x86 (Debian VM). I have not done any run-time tests, not even to check the binaries run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to push it here.

Copy link
Member Author

@ann0see ann0see May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Compiling on debian 12 with qmake Jamulus.pro "CONFIG+=opus_dred" brings warnings. Connecting to a server seems to crash the app... Exit code 139, so a segfault.

Copy link
Member Author

@ann0see ann0see May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems to segfault at line 664 in celt_decoder.c.

The Qt Creator debugger shows that there's a variable i = -264587904 which looks suspicious.

Copy link
Member Author

@ann0see ann0see May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nvm. i is not used. lpcnet seems like a null pointer.

After skipping through the stack trace, it seems as if indeed celt_decode_with_ec_dred is called with NULL as *lpcnet parameter. This seems a bit odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. We might need to call int celt_decode_with_ec_dred() in the custom encoder somehow...

Copy link
Member Author

@ann0see ann0see May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok. So in celt_decoder.c the opus_custom_decode function calls celt_decode_with_ec, which then calls celt_decode_with_ec_dred but with the lpcnet parameter set to NULL. I believe this is the cause of the segfault.

opus_decode_frame in opus_decoder.c adds the lpcnet once it calls celt_decode_with_ec_dred.

Most likely, the custom encoder does not support the NN yet.


DISTFILES += ChangeLog \
COMPILING.md \
Expand Down
4 changes: 4 additions & 0 deletions src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ CClient::CClient ( const quint16 iPortNumber,
opus_custom_encoder_ctl ( OpusEncoderMono, OPUS_SET_COMPLEXITY ( 1 ) );
opus_custom_encoder_ctl ( OpusEncoderStereo, OPUS_SET_COMPLEXITY ( 1 ) );

// enable lowest decoder complexity for Opus64 decoders to enable DRED
opus_custom_decoder_ctl ( Opus64DecoderMono, OPUS_SET_COMPLEXITY ( 5 ) );
opus_custom_decoder_ctl ( Opus64DecoderStereo, OPUS_SET_COMPLEXITY ( 5 ) );
Comment on lines +109 to +111
Copy link
Member

Choose a reason for hiding this comment

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

Is it correct that these lines are for decoder when all the lines above them are for encoder?

Also, as this only specifies Opus64, it looks like it only enables complexity 5 for when "Small Network Buffers" is ticked in the client.

I've never been exactly clear on the benefits or not of that setting, from an audio perspective, and usually run without it.

Is there a reason not to set complexity 5 for the original 128 Opus too?

Copy link
Collaborator

@pljones pljones Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Small Network Buffers, as I understand it, has no impact on audio -- excepting that the network audio buffer size is 64 frames rather than 128. That, in itself, could affect a lossy compression algorithm (i.e. due to less information being received to reconstruct the compressed waveform in each block) - it's a trade off against "smoother networking" (I hesitate to say "lower latency" as the overall latency isn't really affected).

(Of course, if the OPUS output buffer size is 128 and the network buffer size is 64 - i.e. splitting the compressed buffer in two regardless of audio buffer size - the compression isn't affected at all.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it correct that these lines are for decoder when all the lines above them are for encoder?

Yes. As far as I understand the documentation this is correct:

When building Opus, using --enable-deep-plc will compile in the deep PLC code at a cost of about 1 MB in binary size. To actually enable it at run time, you will need to set the decoder complexity to 5 or more. Previously, only the encoder had a complexity knob, but the decoder is now getting one too. It can be set with the -dec_complexity option to opus_demo, or OPUS_SET_COMPLEXITY() in the API (like for the encoder). The extra complexity from running PLC at a high loss rate is about 1% of a laptop CPU core. Because deep PLC only affects the decoder, turning it on does not have any compatibility implications.

Also, as this only specifies Opus64, it looks like it only enables complexity 5 for when "Small Network Buffers" is ticked in the client.

For now, this choice was for testing only. But it's still to be discussed/tested.


// Connections -------------------------------------------------------------
// connections for the protocol mechanism
QObject::connect ( &Channel, &CChannel::MessReadyForSending, this, &CClient::OnSendProtMessage );
Expand Down
4 changes: 4 additions & 0 deletions src/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ CServer::CServer ( const int iNewMaxNumChan,
opus_custom_encoder_ctl ( OpusEncoderMono[i], OPUS_SET_COMPLEXITY ( 1 ) );
opus_custom_encoder_ctl ( OpusEncoderStereo[i], OPUS_SET_COMPLEXITY ( 1 ) );

// enable lowest decoder complexity for Opus64 decoders to enable DRED
opus_custom_decoder_ctl ( Opus64DecoderMono[i], OPUS_SET_COMPLEXITY ( 5 ) );
opus_custom_decoder_ctl ( Opus64DecoderStereo[i], OPUS_SET_COMPLEXITY ( 5 ) );
Comment on lines +119 to +121
Copy link
Member

Choose a reason for hiding this comment

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

Comment as above


// init double-to-normal frame size conversion buffers -----------------
// use worst case memory initialization to avoid allocating memory in
// the time-critical thread
Expand Down