Fix audio protocol bugs: robot voice, clipped syllables, spurious PLC - #2
Open
bnfone wants to merge 7 commits into
Open
Fix audio protocol bugs: robot voice, clipped syllables, spurious PLC#2bnfone wants to merge 7 commits into
bnfone wants to merge 7 commits into
Conversation
…clients The Mumble protocol defines frame_number (sequence number) as a count of 10 ms audio units, not a count of packets. This is documented in docs/dev/network-protocol/voice_data.md: "The sequence number might increase by more than one between subsequent audio packets in case the audio packets contain multiple audio segments." The authoritative formula is in AudioOutputSpeech.cpp:216: jbp.timestamp = iFrameSize * audioData.frameNumber where iFrameSize = 480 samples = 10 ms @ 48 kHz. A 20 ms Opus frame therefore requires frame_number to increment by 2 per packet. Effect on outbound (CleanMumble → remote): CleanMumble sent seq 0, 1, 2, 3 … The remote jitter buffer computed timestamps as 480 × frameNumber, so each packet appeared to represent only 10 ms instead of 20 ms → playback at 2× speed → robot voice on iOS, Linux, and every other Mumble client receiving from CleanMumble. Effect on inbound (remote → CleanMumble): Linux/iOS Mumble sends seq 0, 2, 4, 6 … (step 2 for 20 ms frames). JitterBuffer expected step 1, so it saw a gap at every odd sequence number and attempted Opus FEC for each apparent "missing" packet. 50 % of all frames were decoded as FEC → audio stretched 2× in duration → robot voice + ~25 % apparent packet loss in the stats UI. Why iOS → iOS worked: both ends used the same (wrong) convention, so the timestamps were at least internally consistent. Fix: - RealMumbleClient: increment audioSequence by opusFrameMs / 10 per packet (= 2 for the default 20 ms frame size). Applied to both sendAudioFrame and sendTerminator. - JitterBuffer: auto-detect the sender's sequence step from the first two distinct sequence numbers received in an utterance. The detected step is used for nextSeq advancement (play / FEC / PLC paths), the FEC candidate lookup (packets[want + seqStep]), and the ancient-packet drop threshold (nxt - seq > 8 * seqStep). If the drain timer has already advanced nextSeq by 1 before the step is known, nextSeq is corrected immediately in push() so the next tick finds the real packet instead of triggering spurious FEC. State resets on each reset() call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the Mumble server TCP-batches audio frames and a terminator in the same flush, the terminator can arrive before the buffered audio frames have been drained. Calling reset() immediately discards those frames and the last syllable of the utterance is silently dropped. onTerminator() checks whether frames are still queued: - If yes: sets pendingTerminator=true and returns. The drain loop detects this flag once the queue empties and resets the decoder cleanly at that point, without emitting spurious PLC for the "missing" next packet. - If no: resets immediately (same behaviour as before for the common case where the terminator arrives after all frames have already been played). reset() is kept for hard teardown (user leaves channel / disconnect). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mumble audio travels over TCP, so the server batches several frames into one flush. CleanMumble drains the batch faster than real-time (the drain timer runs every 10 ms, frames are 20 ms each), after which the buffer looks empty and the previous stale check — based on lastArrivalHostTime — immediately fires PLC because "no packet arrived in the last frame period". nextFrameDueTime anchors the stale deadline to the playback timeline: after each emitted frame (play, FEC, or PLC) the deadline is set to now + one frame interval. A missing packet is only considered stale when that deadline has passed, which gives the next TCP batch time to arrive before PLC kicks in. Before the first frame of an utterance is played the check falls back to lastArrivalHostTime (same as before). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When a sender stops speaking without a clean terminator (network drop, app crash), the jitter buffer emits PLC silence until maxConsecutivePLC is reached, then parks the playhead. At 25 frames × 10 ms = 250 ms this tail was audibly long. 8 frames = 80 ms is enough to bridge real packet loss without producing a noticeable silence artifact at utterance end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The fixed threshold of 1.5 × frameMs (= 30 ms for 20 ms frames) fired PLC whenever a packet was more than 30 ms late. Over a TCP connection this is too aggressive: server-side batching, OS scheduling jitter, and the occasional retransmit can all delay a packet by 30–60 ms without it being truly lost. The new threshold is max(2 × frameMs, 0.75 × targetDepthMs). targetDepthMs is the jitter buffer's exponentially-smoothed depth estimate: it grows automatically when the network is jittery and shrinks on clean paths. At the default 60 ms jitter target this gives 45 ms of headroom; at 200 ms (very jittery path) it gives 150 ms. The 2 × frameMs floor ensures we always wait at least one extra frame period before declaring a packet lost. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Nagle's algorithm coalesces small TCP writes into larger segments to improve throughput. For bulk data this is desirable, but for real-time audio it adds latency: the OS holds a 20 ms Opus frame until either the previous segment is acknowledged or enough data accumulates to fill an MSS (~1460 bytes). On a LAN the ACK usually arrives within a few ms, but on paths with higher RTT this can delay frames by up to 40 ms — an entire frame period — before they are even sent. With TCP_NODELAY each sendFrame() call results in an immediate TCP segment, keeping end-to-end audio latency at the network RTT rather than RTT + Nagle coalescing delay. The slight increase in TCP overhead (one segment per frame instead of potentially batched) is negligible for Opus frame sizes (20–150 bytes) compared to IP/TCP header overhead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The original 200 ms (9 600 samples @ 48 kHz) cap was chosen to bound
latency in a specific failure mode: the JitterBuffer's 10 ms drain timer
emits one 20 ms frame per tick when PLC is running, filling the ring at
2× real-time. A 200 ms ring would overflow after ~100 ms of continuous
PLC and drop the oldest audio — so keeping it small made the worst-case
latency predictable.
That reasoning no longer applies after two fixes introduced earlier in
this branch:
1. nextFrameDueTime ("Fix spurious PLC between TCP bursts", f744c9b):
stale detection is anchored to the playback timeline rather than
packet arrival time. When the server TCP-batches several frames they
drain faster than real-time, but the due-time deadline advances one
frame per play — so PLC does not fire between batches. Continuous PLC
now only occurs during genuine packet loss, which on a Mumble TCP
connection is rare.
2. Adaptive stale threshold ("Use adaptive stale threshold…", 76ca76d):
delayed packets on high-jitter paths that previously triggered a PLC
cycle now wait longer for the real packet to arrive. This further
reduces spurious PLC.
With PLC bursts effectively eliminated the 2× fill-rate hazard is gone.
The remaining risk is a TCP retransmit: a single retransmit on a path
with RTT > 100 ms can deliver 300–500 ms of queued audio all at once.
The JitterBuffer drains that burst faster than real-time and all decoded
PCM lands in the ring before CoreAudio can consume it. A 200 ms ring
overflows in that scenario; the oldest frames are dropped and the
listener hears an audible glitch.
1 s (48 000 samples) absorbs a worst-case retransmit burst while keeping
the maximum latency at 1 s + JB target depth — well within the 2–3 s
that is tolerable for voice conversation.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CleanMumble produced robot voice on all non-CleanMumble clients (Linux,
standard iOS Mumble) and showed ~25 % apparent inbound packet loss when
receiving from any standard Mumble client.
Root cause
The Mumble protocol defines
frame_numberas a count of 10 ms audiounits, not packets. This is documented in
docs/dev/network-protocol/voice_data.mdand the authoritative receiverformula in
AudioOutputSpeech.cpp:216:jbp.timestamp = iFrameSize * audioData.frameNumber
where
iFrameSize = 480 samples = 10 ms @ 48 kHz. A 20 ms Opus framemust therefore increment
frame_numberby 2 per packet.Outbound (CleanMumble → remote): CleanMumble incremented by 1.
Remote jitter buffers computed timestamps at 2× speed → robot voice on
every receiving client.
Inbound (remote → CleanMumble): Standard clients send seq 0, 2, 4, 6…
The JitterBuffer expected step 1 and saw a gap at every odd sequence
number → 50 % of frames decoded as FEC → audio stretched 2× in duration
→ robot voice + ~25 % apparent packet loss in the stats UI.
Changes
Core protocol fix
RealMumbleClient: incrementaudioSequencebyopusFrameMs / 10per packet (= 2 for 20 ms frames). Applied to both
sendAudioFrameand
sendTerminator.JitterBuffer: auto-detect the sender's sequence step from thefirst two distinct sequence numbers received. The detected step is used
for
nextSeqadvancement, FEC candidate lookup, and the ancient-packetdrop threshold. Corrects
nextSeqimmediately if the drain timeralready advanced it before the step was known.
Audio quality improvements
pendingTerminator: when the sender's terminator arrives in thesame TCP batch as the last audio frames, defer the reset until the
drain loop has played all buffered frames. Previously
reset()wascalled immediately, silently dropping the last syllable of every
utterance.
nextFrameDueTime: anchor stale detection to the playback timelinerather than packet arrival time. TCP-batched bursts drain faster than
real-time, but the deadline advances one frame per play — preventing
false PLC fires between batches.
max(2 × frameMs, 0.75 × targetDepthMs)instead of the fixed
1.5 × frameMs(30 ms). Scales with observedjitter so high-latency paths get more headroom before PLC fires.
maxConsecutivePLC250 ms → 80 ms: shorter PLC tail when a senderstops without a clean terminator.
immediately rather than being coalesced, removing up to 40 ms of
artificial head-of-line delay.
to bound latency when PLC ran at 2× real-time. With
nextFrameDueTimefixing the spurious-PLC root cause, that hazard is gone. The larger
ring absorbs TCP retransmit bursts (which can deliver 300–500 ms of
audio at once) without dropping the oldest frames.
Testing
Tested on macOS 15 (Sequoia) against: