From 7da0a02b3a17b734d40c718a54a3df75db30cdfb Mon Sep 17 00:00:00 2001 From: Talon Date: Wed, 17 Jun 2026 01:51:50 +0200 Subject: [PATCH] fix(audio): buffer capture frames to Opus encoder's fixed frame size AudioEngine::on_capture() was passing miniaudio's hardware callback period (commonly 480 samples / 10 ms on WASAPI shared mode) directly to opus_encode(), which requires exactly frame_samples_ (960 for 20 ms @ 48 kHz). The mismatch returned OPUS_BAD_ARG and silently dropped every real mic frame, while screen share and injected test frames happened to be correctly sized and worked fine. Fix: accumulate PCM in a pre-allocated CaptureAccum buffer (mirroring the existing RemoteStream::ring fix on the playback side) and only call capture_cb_ when a full frame_samples_ chunk is ready. Same pattern applied to on_loopback(). Add test_capture_frame_accumulation() to verify the accumulator fires exactly the right number of callbacks for misaligned chunk sizes (480, 240+720, 1920 samples). Co-Authored-By: Claude Sonnet 4.6 --- core/src/audio/audio_engine.cpp | 72 +++++++++++++++++++++++++++------ core/src/audio/audio_engine.h | 39 ++++++++++++++++++ tests/test_vad_ptt_devices.cpp | 57 ++++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 13 deletions(-) diff --git a/core/src/audio/audio_engine.cpp b/core/src/audio/audio_engine.cpp index 7aacdce..a4b9323 100644 --- a/core/src/audio/audio_engine.cpp +++ b/core/src/audio/audio_engine.cpp @@ -123,6 +123,16 @@ bool AudioEngine::start(const AudioParams& p, CaptureCallback capture_cb) { frame_samples_ = static_cast(p.sample_rate / 1000 * p.frame_ms); running_.store(true, std::memory_order_release); +#ifdef VOICECAT_HAS_AUDIO + // Pre-allocate capture accumulators before the devices start so on_capture / on_loopback + // never allocate on the RT thread. count=0 means "empty"; the buf is sized to exactly one + // encoder frame so a memcpy into it can never overrun. + capture_accum_.buf.assign(static_cast(frame_samples_), 0); + capture_accum_.count = 0; + loopback_accum_.buf.assign(static_cast(frame_samples_), 0); + loopback_accum_.count = 0; +#endif + #ifdef VOICECAT_HAS_AUDIO // ── Capture device ────────────────────────────────────────────────────── ma_device_id cap_id{}; @@ -130,12 +140,16 @@ bool AudioEngine::start(const AudioParams& p, CaptureCallback capture_cb) { hex_decode_device_id(p.capture_device_id, &cap_id); ma_device_config cap_cfg = ma_device_config_init(ma_device_type_capture); - cap_cfg.capture.format = ma_format_s16; - cap_cfg.capture.channels = p.capture_channels; - cap_cfg.sampleRate = p.sample_rate; - cap_cfg.dataCallback = capture_data_cb; - cap_cfg.pUserData = this; - cap_cfg.capture.pDeviceID = have_cap_id ? &cap_id : nullptr; // null = default device + cap_cfg.capture.format = ma_format_s16; + cap_cfg.capture.channels = p.capture_channels; + cap_cfg.sampleRate = p.sample_rate; + cap_cfg.dataCallback = capture_data_cb; + cap_cfg.pUserData = this; + cap_cfg.capture.pDeviceID = have_cap_id ? &cap_id : nullptr; // null = default device + // Hint: request the encoder's frame size as the callback period. WASAPI shared mode may + // not honor this (the hardware period is fixed), but when it is honored the accumulator + // below becomes a zero-copy passthrough rather than a copy every two callbacks. + cap_cfg.periodSizeInFrames = static_cast(frame_samples_); if (ma_device_init(nullptr, &cap_cfg, &capture_device_) == MA_SUCCESS) { if (ma_device_start(&capture_device_) == MA_SUCCESS) { @@ -347,10 +361,26 @@ void AudioEngine::capture_data_cb(ma_device* dev, void* /*out*/, } void AudioEngine::on_capture(const int16_t* pcm, ma_uint32 frames) { - // The real hardware capture device is always the "primary" tap (kind 0 / MIC). A second - // concurrent local stream (e.g. SCREEN_AUDIO) is fed via inject_capture() in M3 — there is - // only one real capture device. - if (capture_cb_) capture_cb_(0, pcm, static_cast(frames)); + // Accumulate samples until we have exactly frame_samples_ (e.g. 960 for 20 ms @ 48 kHz), + // then fire capture_cb_. WASAPI shared mode commonly delivers 480-sample (10 ms) callbacks + // regardless of the periodSizeInFrames hint above; passing a sub-frame chunk directly to + // opus_encode() returns OPUS_BAD_ARG (negative), silently dropping every mic frame. + if (!capture_cb_ || frame_samples_ <= 0) return; + const int16_t* src = pcm; + auto remaining = static_cast(frames); + while (remaining > 0) { + int space = frame_samples_ - capture_accum_.count; + int copy = std::min(remaining, space); + std::memcpy(capture_accum_.buf.data() + capture_accum_.count, src, + static_cast(copy) * sizeof(int16_t)); + capture_accum_.count += copy; + src += copy; + remaining -= copy; + if (capture_accum_.count == frame_samples_) { + capture_cb_(0, capture_accum_.buf.data(), frame_samples_); + capture_accum_.count = 0; + } + } } void AudioEngine::playback_data_cb(ma_device* dev, void* out, @@ -444,13 +474,29 @@ void AudioEngine::loopback_data_cb(ma_device* dev, void* /*out*/, const void* in } void AudioEngine::on_loopback(const int16_t* pcm, ma_uint32 frames) { - // Same pattern as the real mic capture callback (on_capture) — call capture_cb_ directly, - // NOT through inject_capture()'s test-only ring (see audio_engine.h). - if (capture_cb_) capture_cb_(loopback_kind_, pcm, static_cast(frames)); + // Same accumulation as on_capture — the WASAPI loopback render endpoint's callback + // period is also hardware-driven and may not match frame_samples_. + if (!capture_cb_ || frame_samples_ <= 0) return; + const int16_t* src = pcm; + auto remaining = static_cast(frames); + while (remaining > 0) { + int space = frame_samples_ - loopback_accum_.count; + int copy = std::min(remaining, space); + std::memcpy(loopback_accum_.buf.data() + loopback_accum_.count, src, + static_cast(copy) * sizeof(int16_t)); + loopback_accum_.count += copy; + src += copy; + remaining -= copy; + if (loopback_accum_.count == frame_samples_) { + capture_cb_(loopback_kind_, loopback_accum_.buf.data(), frame_samples_); + loopback_accum_.count = 0; + } + } } bool AudioEngine::start_loopback_capture(int kind) { if (loopback_started_) return false; // already running; stop_loopback_capture() first + loopback_accum_.count = 0; // discard any partial frame from a previous loopback session ma_device_config cfg = ma_device_config_init(ma_device_type_loopback); cfg.capture.format = ma_format_s16; diff --git a/core/src/audio/audio_engine.h b/core/src/audio/audio_engine.h index cc0faed..28087eb 100644 --- a/core/src/audio/audio_engine.h +++ b/core/src/audio/audio_engine.h @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -168,6 +169,30 @@ class AudioEngine { // stereo mixing end-to-end (no audio hardware needed). Same logic the real playback // callback uses; safe to call any time after start() (no ma_device touched). void mix_for_test(int16_t* out, uint32_t frames) { on_playback(out, frames); } + + // TEST-ONLY — drives the capture-side frame accumulator directly with an explicit + // callback, bypassing capture_cb_. Call after engine.start(p) WITHOUT a capture + // callback so the real mic (if any) never touches capture_accum_ (on_capture returns + // early when capture_cb_ is null). The explicit `cb` is invoked only when a full + // frame_samples_ chunk is ready — that is the invariant under test. + void feed_capture_for_test(const int16_t* pcm, int frames, const CaptureCallback& cb) { + if (frame_samples_ <= 0 || !cb || capture_accum_.buf.empty()) return; + const int16_t* src = pcm; + auto remaining = frames; + while (remaining > 0) { + int space = frame_samples_ - capture_accum_.count; + int copy = std::min(remaining, space); + std::memcpy(capture_accum_.buf.data() + capture_accum_.count, src, + static_cast(copy) * sizeof(int16_t)); + capture_accum_.count += copy; + src += copy; + remaining -= copy; + if (capture_accum_.count == frame_samples_) { + cb(0, capture_accum_.buf.data(), frame_samples_); + capture_accum_.count = 0; + } + } + } #endif private: @@ -199,6 +224,20 @@ class AudioEngine { CaptureCallback capture_cb_; std::atomic running_{false}; + // Capture-side frame accumulators: miniaudio fires the capture (and loopback) callback at + // whatever period the hardware/driver chooses — commonly 480 samples (10 ms) on WASAPI + // shared mode, while the Opus encoder requires exactly frame_samples_ per call (960 for + // 20 ms @ 48 kHz). Accumulate incoming PCM until a full frame is ready, then call + // capture_cb_. This mirrors the RemoteStream::ring fix on the playback side. Both + // accumulators are pre-allocated once in start(); never resized from the RT callback + // thread (satisfies architecture.md §3 — no allocation on RT threads). + struct CaptureAccum { + std::vector buf; // pre-sized to frame_samples_ in start() + int count = 0; + }; + CaptureAccum capture_accum_; // mic / real capture device (on_capture) + CaptureAccum loopback_accum_; // screen-audio WASAPI loopback (on_loopback) + // Inject ring(s): stores raw int16 PCM written by inject_capture(), one ring per local // stream kind so e.g. MIC and SCREEN_AUDIO can each be fed independently in tests. // The encode thread reads from these (no real capture device needed in tests). diff --git a/tests/test_vad_ptt_devices.cpp b/tests/test_vad_ptt_devices.cpp index 2666386..eae635d 100644 --- a/tests/test_vad_ptt_devices.cpp +++ b/tests/test_vad_ptt_devices.cpp @@ -231,6 +231,60 @@ static void test_stereo_mix() { } #endif // VOICECAT_HAS_AUDIO && VOICECAT_HAS_OPUS +// ── 5. Capture-frame accumulation (white-box, no audio hardware needed) ────────── +// Regression for the capture-side analogue of the playback ring fix: miniaudio's capture +// callback fires at the hardware period (commonly 480 samples on WASAPI shared mode), while +// opus_encode() requires exactly frame_samples_ (960). Sub-frame chunks must be accumulated; +// the callback must receive exactly 960-sample frames regardless of input chunk size. +#ifdef VOICECAT_HAS_AUDIO +static void test_capture_frame_accumulation() { + voicecat::audio::AudioEngine engine; + voicecat::audio::AudioParams p; + p.sample_rate = 48000; + p.capture_channels = 1; + p.frame_ms = 20; // frame_samples_ = 960 + + std::atomic call_count{0}; + std::atomic wrong_size{false}; + constexpr int kExpected = 960; + + // Start WITHOUT a capture callback: the real mic (if any) fires on_capture(), but + // on_capture() returns immediately when capture_cb_ is null, so capture_accum_ is + // never touched by the hardware thread. feed_capture_for_test() bypasses capture_cb_ + // and drives the same accumulator directly with the explicit `cb` below — no races. + CHECK(engine.start(p)); + + auto cb = [&](int /*kind*/, const int16_t* /*pcm*/, int samples) { + ++call_count; + if (samples != kExpected) wrong_size.store(true); + }; + + // 480-sample (10 ms) input — WASAPI's common hardware period on modern Windows. + // Two 480-chunk inputs → exactly one callback at 960. + std::vector h(480, 1000); + engine.feed_capture_for_test(h.data(), 480, cb); + CHECK(call_count.load() == 0); // half a frame — no callback yet + engine.feed_capture_for_test(h.data(), 480, cb); + CHECK(call_count.load() == 1); // one full frame — callback fired once + + // Mis-aligned split: 240 then 720 → still exactly one callback. + std::vector s(240, 500), l(720, 500); + engine.feed_capture_for_test(s.data(), 240, cb); + CHECK(call_count.load() == 1); + engine.feed_capture_for_test(l.data(), 720, cb); + CHECK(call_count.load() == 2); + + // 1920-sample input (two Opus frames) → exactly two callbacks. + std::vector d(1920, 800); + engine.feed_capture_for_test(d.data(), 1920, cb); + CHECK(call_count.load() == 4); + + CHECK(!wrong_size.load()); + engine.stop(); + std::printf("test_capture_frame_accumulation: ok (callbacks=%d)\n", call_count.load()); +} +#endif // VOICECAT_HAS_AUDIO + // ── 2/3. VAD + PTT gate, through the real ABI against a real server ───────────── static void test_vad_and_ptt_gate() { auto tmp = std::filesystem::temp_directory_path() / @@ -385,6 +439,9 @@ int main() { test_device_enumeration(); #if defined(VOICECAT_HAS_AUDIO) && defined(VOICECAT_HAS_OPUS) test_stereo_mix(); +#endif +#ifdef VOICECAT_HAS_AUDIO + test_capture_frame_accumulation(); #endif test_vad_and_ptt_gate();