From 694c4fbb7822dda765e18b1d0af9aa609ea4a363 Mon Sep 17 00:00:00 2001 From: Robert Adam Date: Fri, 29 Dec 2023 18:50:21 +0100 Subject: [PATCH] CHANGE(client): Exclude and discourage RNNoise Due to RNNoise being unmaintained and the library's code being in very poor shape, this commit excludes the RNNoise feature from Mumble by default and discourages its use. The library contains Opus and CELT symbols (probably due to copy&paste of code) that can end up being called from Opus instead of its own versions of these functions. This can lead to completely unforeseen behavior, including crashes. An example of this is as of writing this, enabling RNNoise on macOS leads to a crash of Mumble pretty much as soon as it starts up with an "invalid instruction" error. The reason being that part of RNNoise's implementation of one of Opus's symbols contains a code path that produces an invalid instruction in optimized builds (and a segfault in debug builds) and this code path is taken when Opus (wrongly) uses this function instead of its own. Fixes #6041 --- docs/dev/build-instructions/cmake_options.md | 2 +- installer/ClientInstaller.cs | 15 +++++++++++++-- src/mumble/CMakeLists.txt | 9 ++++++++- .../generate_test_case.py | 2 +- 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/docs/dev/build-instructions/cmake_options.md b/docs/dev/build-instructions/cmake_options.md index c95244d3f..85151ebbe 100644 --- a/docs/dev/build-instructions/cmake_options.md +++ b/docs/dev/build-instructions/cmake_options.md @@ -192,7 +192,7 @@ Build redacted (outdated) plugins as well ### rnnoise Use RNNoise for machine learning noise reduction. -(Default: ON) +(Default: OFF) ### server diff --git a/installer/ClientInstaller.cs b/installer/ClientInstaller.cs index 778ea6845..779d38a33 100644 --- a/installer/ClientInstaller.cs +++ b/installer/ClientInstaller.cs @@ -18,6 +18,7 @@ using WixSharp.CommonTasks; public struct Features { public bool overlay; public bool g15; + public bool rnnoise; } public class ClientInstaller : MumbleInstall { @@ -86,11 +87,14 @@ public class ClientInstaller : MumbleInstall { // 64 bit this.Platform = WixSharp.Platform.x64; binaries = new List() { - "rnnoise.dll", "speexdsp.dll", "mumble.exe", }; + if (features.rnnoise) { + binaries.Add("rnnoise.dll"); + } + if (features.overlay) { binaries.Add("mumble_ol.dll"); binaries.Add("mumble_ol_helper.exe"); @@ -105,11 +109,14 @@ public class ClientInstaller : MumbleInstall { // 32 bit this.Platform = WixSharp.Platform.x86; binaries = new List() { - "rnnoise.dll", "speexdsp.dll", "mumble.exe", }; + if (features.rnnoise) { + binaries.Add("rnnoise.dll"); + } + if (features.overlay) { binaries.Add("mumble_ol.dll"); binaries.Add("mumble_ol_helper.exe"); @@ -214,6 +221,10 @@ class BuildInstaller if (args[i] == "--overlay") { features.overlay = true; } + + if (args[i] == "--rnnoise") { + features.rnnoise = true; + } } if (version != null && arch != null) { diff --git a/src/mumble/CMakeLists.txt b/src/mumble/CMakeLists.txt index 957ebd4f9..1b167e5c2 100644 --- a/src/mumble/CMakeLists.txt +++ b/src/mumble/CMakeLists.txt @@ -21,7 +21,7 @@ option(translations "Include languages other than English." ON) option(bundle-qt-translations "Bundle Qt's translations as well" ${static}) option(bundled-speex "Build the included version of Speex instead of looking for one on the system." ON) -option(rnnoise "Use RNNoise for machine learning noise reduction." ON) +option(rnnoise "Use RNNoise for machine learning noise reduction." OFF) option(bundled-rnnoise "Build the included version of RNNoise instead of looking for one on the system." ${rnnoise}) option(bundled-json "Build the included version of nlohmann_json instead of looking for one on the system" ON) @@ -726,6 +726,7 @@ else() endif() if(rnnoise) + message(WARNING "Using RNNoise is discouraged and can lead to unforeseen behavior, including crashes at runtime due to the RNNoise library exposing Opus/CELT symbols") target_compile_definitions(mumble_client_object_lib PRIVATE "USE_RNNOISE") if(bundled-rnnoise) @@ -1165,6 +1166,12 @@ if(packaging AND WIN32) ) endif() + if(rnnoise) + list(APPEND installer_vars + "--rnnoise" + ) + endif() + file(COPY ${CMAKE_SOURCE_DIR}/installer/MumbleInstall.cs ${CMAKE_SOURCE_DIR}/installer/ClientInstaller.cs diff --git a/src/tests/TestSettingsJSONSerialization/generate_test_case.py b/src/tests/TestSettingsJSONSerialization/generate_test_case.py index 2e185a848..f54a09bda 100755 --- a/src/tests/TestSettingsJSONSerialization/generate_test_case.py +++ b/src/tests/TestSettingsJSONSerialization/generate_test_case.py @@ -139,7 +139,7 @@ def getDefaultValueForType(dataType): elif dataType in ["IdleAction"]: return "Settings::Deafen" elif dataType in ["NoiseCancel"]: - return "Settings::NoiseCancelBoth" + return "Settings::NoiseCancelOff" elif dataType in ["EchoCancelOptionID"]: return "EchoCancelOptionID::SPEEX_MULTICHANNEL" elif dataType in ["QuitBehavior"]: