Enable Opus ML improvements

[ann0see]

Related to: https://github.com/orgs/jamulussoftware/discussions/3244

Short description of changes

Tries to enable Opus ML improvements to improve quality on packet loss.

Untested.

CHANGELOG: Opus: Enable ML improvements for handling packet loss

Context: Fixes an issue?

Related to: https://github.com/orgs/jamulussoftware/discussions/3244

Does this change need documentation? What needs to be documented and how?

Probably not.

Status of this Pull Request

To be tested in reality. Probably works best if client and server are up to date.

What is missing until this pull request can be merged?

Real world testing, testing with local tools to simulate packet loss.

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[Copilot]

Copilot wasn't able to review any files in this pull request.

Files not reviewed (3)

• Jamulus.pro: Language not supported
• src/client.cpp: Language not supported
• src/server.cpp: Language not supported

[softins]

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?

[pljones]

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.)

[ann0see]

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.

[softins]

Comment as above

[softins]

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.

[ann0see]

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

[softins]

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.

[softins]

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.

[ann0see]

Feel free to push it here.

[ann0see]

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.

[ann0see]

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.

[ann0see]

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.

[ann0see]

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

[ann0see]

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.