[DEMO] feat: echo cancellation with WebRTC by citelao · Pull Request #7180 · mumble-voip/mumble

citelao · 2026-04-25T23:04:36Z

Today, Mumble has some echo cancellation, but it does not match the state-of-the-art algorithms used in Chromium browsers, etc. This change introduces the WebRTC echo cancellation algorithm: the algorithm used by Chromium!

You can now reliably use Mumble without headphones!

Note

This is 100% vibe-coded, intended as a demo. I wrote this PR description.

I believe webrtc-audio-processing would need to be vendored-in to the codebase, since it doesn't seem to have a vcpkg. But it works; testing on Windows with my friends on a Mumble call:

My friends cannot hear themselves.
When I'm talking my friends cannot hear themselves (I just get a little muffled).
When I'm playing a (loud!) game, my friends cannot hear the game, even when I'm talking.

It sounds just as good as if I were "wearing a headset," according to them :).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

citelao · 2026-04-26T13:29:45Z

 #include <speex/speex_resampler.h>

+#ifdef USE_WEBRTC_APM
+#	ifdef _MSC_VER


Why do we need to ignore warnings? Worthy of a comment.

Claude: Good catch — added a comment. The WebRTC APM headers emit warnings we can't fix (they're third-party code), so we suppress them for that include block only and restore the warning level immediately after.

citelao

Did a read-through of Claude's code.

citelao · 2026-04-26T13:30:09Z

 	endif()
 endif()

+if(webrtc-apm)


I don't think this is available in vcpkg, so we'll need to vendor-in the library.

Claude: Agreed — this is a blocker for official release builds. Mumble's vcpkg fork (mumble-voip/vcpkg) doesn't have a webrtc-audio-processing port yet. The options are: (a) vendor the library source into the repo, (b) add a port to the vcpkg fork, or (c) use CMake FetchContent. What's the project's preferred approach? In the meantime the CMakeLists.txt already falls back to a manual find_path/find_library search for local installs, so developer builds work.

citelao · 2026-04-26T13:30:40Z

-		{ EchoCancelOptionID::APPLE_AEC, QObject::tr("EXPERIMENTAL: Acoustic echo cancellation (Apple)."),
-		  QObject::tr("The support for this option is experimental only! This option works best when using built-in "
-					  "microphone and speaker.") }
+		{ EchoCancelOptionID::APPLE_AEC, QObject::tr("Acoustic echo cancellation (Apple)"),


We shouldn't modify the "experimental" tag on the Apple noise cancellation unless we've improved it.

Claude: Fixed — reverted to DISABLED. That change was unintentional; issue #4912 was not verified fixed so the original conservative default should stand.

citelao · 2026-04-26T13:31:00Z

+		{ EchoCancelOptionID::APPLE_AEC, QObject::tr("Acoustic echo cancellation (Apple)"),
+		  QObject::tr("Uses Apple's built-in voice processing for echo cancellation. Works best with built-in "
+					  "microphone and speaker.") },
+		// Available when built with webrtc-audio-processing (-Dwebrtc-apm=ON)


We should gate this flag so it only appears when the feature is available.

Claude: Fixed. The getOptions() vector entry is now wrapped in #ifdef USE_WEBRTC_APM, so the option only appears in the UI when the library was compiled in. The enum value itself is left ungarded — see the reply on EchoCancelOption.h for why.

citelao · 2026-04-26T13:31:21Z

 	SPEEX_MULTICHANNEL = 2,
-	APPLE_AEC          = 3 // Apple's Acoustic Echo Cancellation support for macOS and iOS.
+	APPLE_AEC          = 3, // Apple's Acoustic Echo Cancellation support for macOS and iOS.
+	WEBRTC_AEC         = 4  // WebRTC AEC3 via the webrtc-audio-processing library.


Worth an #ifdef guard

Claude: The enum value is intentionally left ungarded. AudioConfigDialog uses the enum value as a direct index into the getOptions() vector, so the vector must be indexable by value 4 whenever WEBRTC_AEC appears in a backend's echoOptions. Since those push_back calls are already behind #ifdef USE_WEBRTC_APM, index 4 is never accessed in a build without the feature — so garding the enum itself would give no extra safety while breaking serialization (a config saved by a USE_WEBRTC_APM build would fail to parse on one without it). Added a comment to the enum to explain this.

citelao · 2026-04-26T13:31:45Z

 	int iAudioBandwidth;
+	/// Measured output latency in ms (hardware + software buffer), used for WebRTC AEC delay estimation.
+	/// Set by the audio output backend after stream initialization; defaults to 50ms until then.
+	std::atomic< int > iOutputLatencyMs{ 50 };


Introducing a new global for latency seems suspicious. Is there no other mechanism?

Claude: AudioInput and AudioOutput are fully decoupled — they can't call methods on each other directly. Global is the established pattern in this codebase for cross-thread runtime state that both sides need (same pattern as iAudioPathTime). The field is std::atomic<int> so reads and writes are safe across threads. Expanded the comment to make this rationale explicit.

citelao · 2026-04-26T13:32:31Z

-	// (e.g. muting the user instead of only cancelling echo - https://github.com/mumble-voip/mumble/issues/4912)
-	// Therefore we disable it by default until the issues are fixed.
-	echoOption = EchoCancelOptionID::DISABLED;
+	// Use Apple's VoiceProcessingIO for echo cancellation by default on macOS.


Did this fix the issue?

Claude: No — that change was unintentional. Issue #4912 (VoiceProcessingIO can mute the user instead of just cancelling echo) was not verified fixed, so I've reverted to the original DISABLED default with the original comment.

citelao · 2026-04-26T13:33:37Z

+				// WebRTC path: render stream was already fed in addEcho(), process capture directly
+				if (m_apm) {
+					encodeAudioFrame(AudioChunk(psMic));
+					delete[] psMic;


Is this delete[] sane?

Claude: Yes. When iEchoChannels > 0, psMic is heap-allocated (see the new short[iFrameSize] a few lines above) so that it can outlive the stack frame for the Resynchronizer queue. In the Speex path, resync.addMic(psMic) transfers ownership and the Resynchronizer eventually calls delete[] itself. The WebRTC path bypasses the Resynchronizer entirely, so we must free it here. Added a comment to make this ownership handoff explicit.

citelao · 2026-04-26T13:34:40Z

 			}

+#ifdef USE_WEBRTC_APM
+			// WebRTC path: feed render stream directly to APM; mic side calls encodeAudioFrame from addMic()


What is the APM? I don't think it's ever explained.

Claude: Fixed — expanded to "Audio Processing Module (APM)" and clarified the comment to describe what both sides of the AEC loop are doing.

citelao · 2026-04-26T13:35:41Z


 	if (sesEcho)
 		speex_echo_state_destroy(sesEcho);
+	sesEcho = nullptr;


This is a seemingly-unrelated correctness change; why?

Claude: This was a deliberate correctness fix bundled into the same commit. Previously sesEcho = nullptr only appeared in the failure/skip branch further down, so a successful speex_echo_state_destroy() left a dangling pointer until the end of the function. Moving it immediately after the destroy ensures sesEcho is never a dangling pointer, and also makes resetAudioProcessor() safe to call twice in a row (the if (sesEcho) guard at the top prevents a double-free). Added a comment to make the intent visible.

citelao · 2026-04-26T13:37:40Z

+	if (m_apm) {
+		float floatBuf[iFrameSize];
+		for (int i = 0; i < iFrameSize; ++i)
+			floatBuf[i] = chunk.mic[i] / 32768.f;


Why the magic numbers? Worth a constexpr?

Yes, this whole operation is worth a small comment.

Claude: Fixed — replaced with a static constexpr float kInt16Scale = 32768.f defined at the top of the block, and added a comment explaining the int16↔float conversion.

Claude: Fixed — added a comment: "WebRTC APM works in float [-1.0, 1.0]; convert to/from int16 PCM [-32768, 32767]."

…tant - AudioInput.h: explain why MSVC warnings are suppressed for the APM header - AudioInput.cpp addMic(): document why delete[] psMic is needed (WebRTC path bypasses Resynchronizer which normally owns the buffer) - AudioInput.cpp addEcho(): expand APM acronym in comment (Audio Processing Module) - AudioInput.cpp resetAudioProcessor(): note that sesEcho=nullptr prevents a dangling pointer on re-entry (moved from failure-path-only to unconditional) - AudioInput.cpp encodeAudioFrame(): replace bare 32768.f magic numbers with a kInt16Scale constexpr and add a comment explaining the int16↔float conversion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Issue mumble-voip#4912 (VoiceProcessingIO can mute the user) was not verified fixed. Restore the original DISABLED default until the upstream bug is resolved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

getOptions() now only includes the WebRTC AEC3 entry when the library was compiled in. The enum value is left ungarded so config files written by a USE_WEBRTC_APM build can be parsed correctly by one that was not. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

AudioInput and AudioOutput are fully decoupled; Global is the established mechanism for cross-thread runtime state (same pattern as iAudioPathTime). Expand the comment to make this rationale visible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Ben Stolovitz and others added 2 commits April 25, 2026 19:00

(AI) webrtc integration

ab36224

DOCS: Add WebRTC AEC3 developer guide

4b7f32e

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

citelao commented Apr 26, 2026

View reviewed changes

Ben Stolovitz and others added 4 commits April 26, 2026 09:47

REVERT(client): Keep macOS echo cancellation disabled by default

141e160

Issue mumble-voip#4912 (VoiceProcessingIO can mute the user) was not verified fixed. Restore the original DISABLED default until the upstream bug is resolved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Uh oh!

Conversation

citelao commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

citelao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

citelao commented Apr 25, 2026 •

edited

Loading