Add GCC-managed encoded Opus audio track

[lkang-nuro]

Context

The audio stream is effectively fixed bitrate: audio is pre-encoded outside the sender and pushed as a passive track via AddAudioTrack, so the GCC bandwidth estimator — which dynamically drives video bitrate — never touches it. When the network degrades, video scales down but audio holds its bandwidth.

This makes audio a first-class, GCC-managed encoded Opus track inside the sender, so the same control loop (updateBitrate → updateEncoderBitrate) that drives VP8 also drives Opus, sharing the GCC bitrate pool. Callers push raw PCM instead of pre-encoded Opus.

The change is additive: the legacy passive AddAudioTrack path is left untouched.

Changes

• Generalize EncodedTrack to carry audio (isAudio, audioTrack, audioSource); the existing local track and encodedReader/bitrateTracker/mimeType are shared by both kinds.
• AddEncodedAudioTrack(trackID, opus.Params) — builds the Opus codec selector, AudioBuffer source, mediadevices AudioTrack, and encoded reader; registers the track in s.tracks so it flows through GCC allocation.
• SendAudioFrame(trackID, pcm, sampleRate, channels) — pushes raw interleaved PCM.
• New sender/audio_buffer.go (AudioBuffer) — the audio counterpart to FrameBuffer: same initialized-gate non-blocking Read and drop-oldest bounded queue.
• updateEncoderBitrate gains a codec.BitRateController branch for Opus, clamped to [8000, 32000] bps.
• processEncodedFrames uses a 20 ms duration for audio and skips the VP8 keyframe sniff; ForceKeyFrame/recreateEncoder/Close guard on isAudio.
• Extracted attachTrackToPeerConnection helper (de-dupes AddVideoTrack).

Testing

go build ./..., go vet ./..., golangci-lint run ./... (0 issues), and go test ./... all pass. New tests in sender/audio_buffer_test.go cover buffer behavior, track creation/validation, a full PCM→Opus encode path, and the GCC bitrate-driven path.

Note

Downstream consumer wiring (cgo PushAudioPCM export, C++ raw-PCM push, Bazel .so re-pin) lives in a separate repo and is out of scope here. For GCC to genuinely drive audio, the encoded audio track must be AddTrack'd on the PeerConnection this sender owns so transport-cc feedback reaches the estimator.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 79.42122% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.59%. Comparing base (d6fa118) to head (8536302).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
sender/rtc_sender.go	72.35%	34 Missing and 13 partials ⚠️
sender/audio_buffer.go	89.24%	8 Missing and 2 partials ⚠️
receiver/receiver.go	50.00%	5 Missing ⚠️
receiver/capture_latency.go	90.90%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
+ Coverage   48.79%   52.59%   +3.79%     
==========================================
  Files          19       21       +2     
  Lines        1916     2181     +265     
==========================================
+ Hits          935     1147     +212     
- Misses        908      947      +39     
- Partials       73       87      +14     

Flag	Coverage Δ
go	52.59% <79.42%> (+3.79%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jayli-nuro]

Nice work — a lot of this is carefully done. Two things I want to credit up front because I checked them specifically:

• GCC pool is genuinely shared, not additive: videoPoolAndEqualShare reserves audio off the top (videoPool = max(0, target - reservedAudio)) and the encoder gets that same reserved value, so video + audio == target. The encoded audio track is also actually AddTrack'd on the PC (so transport-cc feedback reaches the estimator) — the PR-body caveat is satisfied for AddEncodedAudioTrack.
• The RTP-timestamp encoding correctly uses the negotiated clock rate (info.ClockRate, reduced — 6/125 for 48 kHz Opus, 9/100 for VP8), verified against pion rtpsender.go. So it is not mis-applying the 90 kHz mapping to audio. Video path is unchanged.

That said, there are a few real issues — one I'd consider blocking (a libopus data race). Details inline, plus a couple that don't map to a changed line:

Not-in-diff (so noted here):

• [Medium] GetEncodeFrameOk video-stall watchdog is defeated by the audio track. It returns true if any track in s.tracks encoded recently; audio flows continuously, so a stalled video encoder no longer trips the watchdog once an encoded audio track exists. Consider scoping the health check to video tracks (!isAudio).
• [Med-low] SetBitrateAllocation accepts an audio track ID. Its weight is summed into total but updateBitrate continues past audio, so a caller passing the audio ID double-counts it — video gets under-allocated and the GCC target is under-utilized. Reject/ignore isAudio IDs in the validation loop.
• [Low] sub-8 kbps congestion: when the GCC target drops below kAudioMinBps, audio clamps up to 8 kbps while videoPool floors at 0 — the one regime where total > target (audio overshoots by up to ~8 kbps, video starved). Fine to accept, worth a comment.
• [Low] capture-latency is Debug-log-only (return value unused in the read loops) — confirm that's intended vs feeding a metric.
• Test gaps: shared-pool asserted only for single-video-track; no multi-video + audio, no SetBitrateAllocation+audio, encoder-received value unchecked; e2e capture-ts tests bypass the Opus encoder so the buffering skew below is untested.

[jayli-nuro]

[High — data race / UB] This runs on the GCC path (Start's 100ms ticker / UpdateBitrate) with no encoderMu, while the per-track encode goroutine is inside opus_encode. Unlike vpx (whose SetBitRate locks the encoder's e.mu, and whose enc_config_set is applied inside the locked Read), mediadevices' opus SetBitRate calls opus_encoder_ctl(OPUS_SET_BITRATE) with no lock (opus.go — Read locks e.mu around opus_encode, SetBitRate does not). Calling opus_encoder_ctl concurrently with opus_encode on the same *OpusEncoder is undefined behavior in libopus → -race failure / corrupted encoder state / crash, and it's guaranteed in steady state (audio always flowing + GCC always ticking). Fix here by holding track.encoderMu.Lock() around the audio SetBitRate (the encode goroutine only takes RLock, so a write lock serializes them), or fix opus.go upstream to lock e.mu in SetBitRate.

[lkang-nuro]

Good catch — fixed in 4e217d3. updateEncoderBitrate now takes track.encoderMu.Lock() around the Opus SetBitRate.

The per-track encode goroutine already holds encoderMu.RLock() across encodedReader.Read() (the opus_encode call) in encodeAndSendTrack, and it releases tracksMu before acquiring encoderMu, so the write lock here serializes the bitrate change against encoding with no lock-ordering deadlock (updateEncoderBitrate is only ever called from updateBitrate, which holds tracksMu.RLock, never encoderMu).

Left the underlying mediadevices opus.go SetBitRate locking as a separate upstream fix.

[jayli-nuro]

[High — audio playout/jitter] Overwriting the audio RTP timestamp is semantically wrong even at the correct 48 kHz rate. An Opus receiver (browser NetEq, and pion's own RTCP-jitter computation) requires the audio RTP timestamp to advance by exactly the sample count per frame (960 ticks / 20 ms). After this, the timestamp is captureUs*48000/1e6, so capture-side jitter/gaps map straight into the RTP timestamp instead of a clean +960 → receiver jitter buffer mis-schedules playout, RTCP Jitter is corrupted, PLC/DTX gap detection misfires. Related: because the value derives from LastCaptureTSUs (not a sample counter), if the Opus encoder emits ≥2 frames for one dequeued chunk they get identical RTP timestamps (and can go backward if the capture clock isn't monotonic) — an RFC 3550 violation. This trick was a defensible tradeoff for video; for audio I'd gate it off (keep the packetizer timestamp) and carry capture-time only via abs-capture-time, or restrict the RTP-timestamp overwrite to video tracks.

[lkang-nuro]

Thanks — this is the sharpest concern, and the analysis is right. Decision for now is to keep the audio RTP-timestamp overwrite, for two reasons:

• The whole point of encoding capture time into the RTP timestamp (rather than relying only on abs-capture-time) is to survive an SFU that strips header extensions on egress — gating audio off loses that for audio.
• This harness's receiver is synthetic (no NetEq / real jitter buffer / DTX), so the playout, RTCP-Jitter, and PLC/DTX corruption you describe isn't exercised on this path.

The ≥2-frames-per-chunk → identical/backward timestamps caveat is noted. If we later drive a real Opus decoder through this path, the plan is exactly your suggestion: gate the RTP-timestamp overwrite to video and carry audio capture-time via abs-capture-time only. Happy to make that change now instead if you'd prefer it on principle.

[jayli-nuro]

[Medium — clock skew] The candidate > nowTicks -> -= 1<<32 correction assumes capture always precedes receipt, which fails under cross-machine wall-clock skew. If the sender's clock is ahead of the receiver's by more than the true one-way latency, captureUs > nowUnixUs, the spliced candidate lands one wrap in the future, gets pulled back a full 2^32, and GlassToGlassLatency returns ~the wrap period (~24.9 h @ 48 kHz) instead of a small/negative value. Every test uses now = capture + lat with lat >= 0, so this is unexercised. Consider: if candidate is only slightly ahead (within a small guard band), treat it as ~0/negative rather than subtracting a full wrap.

[lkang-nuro]

Fixed in 4e217d3. CaptureTimeUsFromRTP now snaps to the wrap nearest nowTicks using a half-period (2^31-tick) guard band, instead of pulling back a full wrap whenever candidate > nowTicks:

switch {
case candidate-nowTicks > 1<<31:
    candidate -= 1 << 32
case nowTicks-candidate > 1<<31:
    candidate += 1 << 32
}

So a sender clock running slightly ahead now maps to a small negative latency rather than collapsing a full 2^32 wrap (~24.9 h @ 48 kHz) onto it. Added the symmetric += 1<<32 case for completeness.

[jayli-nuro]

[Medium] No validation that sampleRate/channels match the track's fixed Opus encoder. The encoder's rate/channels are locked at track creation (NewAudioBuffer(48000, 2) + silence-chunk detection), but this forwards arbitrary sampleRate/channels straight into PushPCMWithCaptureTS. Mono input, or any sampleRate != 48000, is accepted silently → wrong-pitch/garbled audio (and the 48 kHz RTP-timestamp math then mis-scales), with no error returned. Recommend rejecting sampleRate != 48000 / a channel count that differs from the buffer's.

[lkang-nuro]

Fixed in 4e217d3. SendAudioFrameWithCaptureTS now rejects PCM whose sampleRate/channels don't match the track's fixed encoder (NewAudioBuffer(48000, 2)), returning a new ErrPCMFormatMismatch:

if sampleRate != track.audioSource.sampleRate || channels != track.audioSource.channels {
    return fmt.Errorf("%w: got %d Hz/%d ch, track expects %d Hz/%d ch", ...)
}

Put the check at the public SendAudioFrame* entry point rather than inside PushPCMWithCaptureTS, so the buffer's unit tests (which pass a dummy rate to exercise the length/channel-count validation) keep working.

[jayli-nuro]

[Low] Read fast path doesn't prioritize closeChan. FrameBuffer.Read was deliberately written to check closeChan in its own select before the non-blocking chunkChan read (Go picks ready cases pseudo-randomly), so a closed buffer always reports ErrBufferClosed. Here chunkChan/closeChan/default share one select, so after Close() with a buffered chunk it can return the chunk instead of ErrBufferClosed — re-introducing the exact race the sibling type was patched to avoid. Minor (loop still terminates next iteration), but worth matching FrameBuffer.

[lkang-nuro]

Fixed in 4e217d3. AudioBuffer.Read's initialized fast path now checks closeChan in its own select before the non-blocking chunkChan read, matching FrameBuffer.Read, so a closed buffer always reports ErrBufferClosed instead of racing the buffered-chunk/default branches.

[lkang-nuro]

Thanks for the thorough pass — and for verifying the shared-pool and negotiated-clock-rate behavior up front. Addressing the not-in-diff items in 4e217d3:

• [Medium] GetEncodeFrameOk video-stall watchdog defeated by audio — scoped the health check to video tracks (isAudio skipped). No-video (including the no-tracks case) still returns healthy, so continuously-flowing audio no longer masks a stalled video encoder.
• [Med-low] SetBitrateAllocation accepts an audio track ID — now rejects audio IDs with a new ErrAudioTrackNotAllocatable, since audio is GCC-allocated independently (updateBitrate skips it via audioTargetBitrate) and a weight there would be summed into total but never applied.
• [Low] sub-kAudioMinBps overshoot — documented the accepted audio+video > target regime in videoPoolAndEqualShare.
• [Low] capture-latency is Debug-log-only — intended for now; the recovered latency is consumed only by the round-trip tests, not yet fed to a metric.
• Test gaps — noted as follow-ups: multi-video + audio shared-pool, SetBitrateAllocation+audio rejection, and asserting the encoder-received bitrate value.

The one item left as-is is the audio RTP-timestamp overwrite (replied inline on that thread) — keeping it for the SFU-extension-stripping survival reason given the synthetic receiver here, but happy to gate it to video if you'd rather not carry the tradeoff.

[jayli-nuro]

Re-reviewed after the "Address PR review feedback" commits — this addresses essentially everything, cleanly. Verified each:

• [High] opus SetBitRate/opus_encode race — FIXED & verified deadlock-free. updateEncoderBitrate now takes track.encoderMu.Lock() around the audio SetBitRate, which serializes against the encode goroutine's encoderMu.RLock. I checked it doesn't introduce a lock-ordering deadlock: the hierarchy is consistently tracksMu → encoderMu everywhere (updateBitrate holds tracksMu.RLock then takes encoderMu.Lock; recreateEncoder holds tracksMu.Lock then encoderMu.Lock), and encodeAndSendTrack releases tracksMu.RLock before taking encoderMu.RLock — so no thread holds encoderMu while waiting on tracksMu. Good.
• [Medium] receiver clock-skew wrap — FIXED. The half-period nearest-wrap guard maps a slightly-ahead sender clock to a small negative latency instead of collapsing a full 2^32 (~24.9 h). Correct.
• [Medium] SendAudioFrame format validation — FIXED (ErrPCMFormatMismatch on rate/channel mismatch).
• [Medium] GetEncodeFrameOk watchdog — FIXED (skips isAudio, returns !hasVideo, so audio can't mask a stalled video encoder).
• [Med-low] SetBitrateAllocation audio double-count — FIXED (ErrAudioTrackNotAllocatable).
• [Low] AudioBuffer.Read closeChan priority — FIXED (checked first, mirrors FrameBuffer).
• [Low] sub-8kbps overshoot — documented as an accepted tradeoff. Fine.
• Nice cleanup folding captureSrc/signaler into one source trackBuffer.

One residual, and I think it's an intentional design choice — just flagging the downstream caveat: the audio RTP timestamp is still overwritten with capture time (and the new receiver code recovers it), so within this harness it's coherent. But for the production consumer (real Opus receiver / browser NetEq), a capture-jittered / non-monotonic audio RTP timestamp still perturbs jitter-buffer + RTCP jitter, and duplicate timestamps can occur if >1 Opus frame is emitted per pushed chunk. Not a bwe-test blocker; worth gating the RTP-timestamp overwrite to video on the Nuro consumer side (or carrying audio capture-time only via abs-capture-time). Confirming that's the intent.

Otherwise LGTM — the fixes are solid.