fix: allocation-free steady-state echo cancellation (builds on #39) - #40
Open
dignifiedquire wants to merge 15 commits into
Open
dignifiedquire wants to merge 15 commits into
dignifiedquire wants to merge 15 commits into
Conversation
Echo cancellation runs in real-time audio callbacks, where allocating memory can block. With AEC3 enabled, processing one 10 ms frame allocated about 40 times: - EchoRemover::process_capture created eleven per-channel working vectors per 4 ms block. They now live in a CaptureScratch struct that is allocated once and reset to the same zero/default state per block. - EchoCanceller3 built a Vec<Vec<&[f32]>> view per sub-frame for the frame blocker (capture and render). FrameBlocker::insert_sub_frame_and_extract_block now accepts owned buffers as well, so no view is built. - AudioBuffer::copy_to_buffer resampled into a temporary vector; it now resamples straight into the destination channel. The output is bit-identical to 0.2.0 (same SHA-256 of 60 s of double-talk capture output). A new test, tests/no_allocation.rs, counts allocations in steady-state processing (40,500 before, 0 after). Co-Authored-By: Claude <noreply@anthropic.com>
AudioBuffer still allocated a temporary vector per 10 ms frame when downmixing float input (copy_from_float) and in the int16 conversions (copy_from_interleaved_i16, copy_to_interleaved_i16). With echo cancellation on, that is 1 allocation per frame for int16 at 16 kHz, 3 for int16 at 48 kHz and 2 for float stereo in, mono out. The C API's wap_process_stream_i16 takes this path. These temporaries now use one scratch buffer that AudioBuffer allocates at construction, sized for the longer of the input and output frames. The averaging downmix in copy_from_float writes into it; selecting one channel reads that channel directly, as upstream does, instead of copying it first. copy_to_buffer resamples straight into the destination, which matches the old copy only while the resampler writes exactly as many samples as the destination holds. A debug assertion on resample's return value now documents that. The output is bit-identical: the capture and render output and the statistics of 46 configurations (f32 and int16; 16, 32, 44.1 and 48 kHz; rate conversion; mono, downmixed and stereo capture; both downmix methods; 32 kHz maximum processing rate; unused capture output; echo delay change) hash the same before and after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
… reset EchoRemover::process_capture moved its scratch buffers out of self with mem::take and put them back only at the end of the call. sonora-ffi catches panics and keeps using the instance, so a panic in between left empty vectors behind, and every later call panicked on them. The buffers are now borrowed in place by destructuring &mut self.scratch, which Rust allows next to borrows of the other fields of self. The take, the reassembly, the Default derive that only mem::take needed, and the comment that described them are gone. A new test panics midway through a call and checks that the next call works; it fails on the old code. The scratch was also cleared at the start of every 4 ms block, about 5.7 KB per capture channel. On every path through process_capture, each element is written before it is read: - subtractor_output: Subtractor::process writes all fields for every channel (prediction_error, compute_metrics, zero_padded_fft, spectrum) before its own first read. - e: form_linear_filter_output (signal_transition writes all 64). - y_fft, e_fft: padded_fft, whose copy_from_packed_array writes every re and im bin. - s2_linear: linear_echo_power; y2, e2: FftData::spectrum (all 65 bins). - r2, r2_unbounded: read only when the capture output is used, and in that branch ResidualEchoEstimator::estimate first overwrites them on all four paths (copy of Y2, linear_estimate or non_linear_estimate). - comfort_noise, high_band_comfort_noise: generate_comfort_noise writes all re bins and im[1..64]. Nothing writes im[0] and im[64], so they keep the zeros from construction with or without the reset. Upstream does not clear these arrays either. The output is bit-identical: 60 configurations, including saturated echo on the linear and the non-linear residual echo paths and an unused or toggled capture output, hash the same before and after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
FrameBlocker::insert_sub_frame_and_extract_block became generic over two AsRef parameters, mainly so that a test helper building borrowed views kept compiling. Every production caller passes owned per-band, per-channel buffers, and BlockFramer::insert_block_and_extract_sub_frame takes those as a concrete &mut [Vec<Vec<f32>>]. The blocker now takes a concrete &[Vec<Vec<f32>>], and the tests pass their buffers directly instead of through make_sub_frame_view, which is gone. FrameBlocker::new built its buffers with vec![vec![Vec::with_capacity(BLOCK_SIZE); n]; m]. vec! clones its element, and a cloned Vec does not keep the capacity, so all but one buffer started empty and grew in the audio callback: 4 and then 2 allocations in the first two frames at 48 kHz mono, 8 and 4 at 48 kHz stereo, and again whenever initialize() rebuilds the render blocker after the stereo detection changes. Each buffer now gets its own Vec::with_capacity. A new test checks the capacity and fails on the old construction. No other vec![Vec::with_capacity(..); n] remains in sonora-aec3 or sonora; BlockFramer fills its buffers with zeros, so its clones keep a full block of capacity. The output is bit-identical (the same 60 configurations as before). BREAKING CHANGE: sonora-aec3's public FrameBlocker::insert_sub_frame_and_extract_block now takes sub_frame: &[Vec<Vec<f32>>]. In the published sonora-aec3 0.2.0 it took &[Vec<&[f32]>], so callers that pass borrowed views no longer compile. The next release must bump sonora-aec3's minor version (0.2 to 0.3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
ResidualEchoEstimator::update_render_noise_power summed the render channels into a stack array and then copied it into a Vec, once per 4 ms block whenever there is more than one render channel: 2.5 allocations per 10 ms frame, 2,500 in 10 s of 48 kHz stereo. Found while extending tests/no_allocation.rs to stereo; it predates PR #39 and is in the published sonora-aec3 0.2.0. It now uses the array directly, as update_reverb in the same file and the upstream C++ do. The output is bit-identical (the same 60 configurations as before). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
…anges in no_allocation tests/no_allocation.rs covered only 48 kHz mono float input with a constant echo delay, and it never checked that the echo canceller did anything. It now runs 12 scenarios, each in its own test: - float at 16, 32 and 48 kHz, mono, at the default 32 kHz maximum processing rate (one and two bands); - float at 48 kHz, mono, with a 48 kHz maximum processing rate (three bands); - int16 at 16 and 48 kHz, mono (the C API's wap_process_stream_i16); - float stereo in, mono out (downmix); - float stereo in and out, with stereo render; - int16 at 44.1 kHz, stereo in and mono out, and stereo in and out (downmix and multi-channel conversion with resampling both ways); - set_capture_output_used(false); - an echo delay change from 60 ms to 100 ms while counting. Each run is 15 s of far end with pauses, a delayed echo, double talk and noise. The far-end pauses keep a low noise floor rather than digital silence. AEC3 treats render as stereo only after the channels have differed for 2 s without a break; with exact zeros in the pauses the stereo scenarios passed only because of subnormal differences between the channels, and with the flush-to-zero of PR #38 they ran on mono render and failed. Allocations are counted after a 5 s warm-up, on the current thread only, as before. Each test then checks that the stream ran at the intended processing rate and that the echo canceller converged on the real echo: ERLE above 10 dB, the echo on the output attenuated by more than 10 dB (when the output is used) and the delay estimate within 10 ms of the true delay, which also shows that the delay change was tracked. Measured values are above 16 dB. On the PR #39 head, 6 of the 12 tests fail (per 10 s: 1000 allocations for int16 16 kHz, 3000 for int16 48 kHz, 2000 for float stereo in mono out, 2500 for float stereo, 3000 and 6500 for int16 44.1 kHz). Without the multi-channel render power fix, the two stereo tests fail with 2500 allocations, also when merged with PR #38. The test takes about 4 s wall time (about 32 s CPU time) in a debug build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
The fullband copy in process_capture_stream_locked moved capture_audio out of its Option with take(), called copy_to_buffer and put it back afterwards. If copy_to_buffer panicked, for example at its debug_assert_eq! on the resampled length or at an assert in PushSincResampler, capture_audio stayed None. sonora-ffi catches panics and keeps using the instance, so every later capture call would then panic at capture_audio.as_mut().unwrap(). capture_audio and capture_fullband_audio are separate fields, so both can be borrowed mutably at the same time and the take is not needed. Nothing changes when no panic occurs. The pattern predates PR #39; it is the one the echo remover's capture scratch had. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
==========================================
+ Coverage 93.64% 94.34% +0.69%
==========================================
Files 146 146
Lines 31130 31208 +78
Branches 31130 31208 +78
==========================================
+ Hits 29153 29443 +290
+ Misses 1753 1577 -176
+ Partials 224 188 -36
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This reverts the signature change of d417d08. FrameBlocker::insert_sub_frame_and_extract_block is generic again over Band: AsRef<[Channel]> and Channel: AsRef<[f32]>, as b10927e (#39) had it. The call form of the published sonora-aec3 0.2.0, &[Vec<&[f32]>], compiles again, and EchoCanceller3 still passes its owned &[Vec<Vec<f32>>] buffers without building a view on each call. d417d08's capacity fix in FrameBlocker::new and its test stay. The tests now exercise both call forms: block_bitexactness passes borrowed views built by make_sub_frame_view (restored), and blocker_and_framer passes owned buffers. With d417d08's concrete signature, the view test does not compile. No API break remains, so the BREAKING CHANGE footer of d417d08 no longer applies. Against the published 0.2.0 sources (a024d6e, the commit that the 0.2.0 crates in the cargo cache were packaged from): - A rustdoc JSON listing of the public API of all eight crates differs only in this method, which is now generic. - cargo-semver-checks 0.46.0 (--baseline-rev a024d6e, offline) flags method_requires_different_generic_type_params here and nothing else. The Cargo SemVer guide classifies this change as minor ("generalizing a function to use generics (supporting original type)"): the 0.2.0 type is the instantiation Band = Vec<&[f32]>, Channel = &[f32]. As the guide notes, a caller that relied on the parameter type to infer a type, for example the target of a collect() passed straight in, may need an annotation. The output is unchanged: the same code runs for owned buffers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
The no_allocation tests run 12 scenarios of 15 s of audio each. In an
unoptimized build they take most of the test time; on the emulated
Android CI targets, the binary alone took 541 to 1,021 s. The sonora-aec3
unit tests are the other slow binary.
The test profile now builds sonora, sonora-aec3, sonora-common-audio,
sonora-fft and sonora-simd with opt-level 3, through
[profile.test.package.<name>] overrides. cargo test uses the test
profile, and so do cross test (the Android jobs), cargo nextest and
cargo llvm-cov nextest, which build through cargo test. dev builds
(cargo build, cargo run, the examples) stay unoptimized and debuggable;
[profile.dev.package.<name>] would have optimized those too. Debug
assertions and overflow checks stay on: the test profile inherits them
from dev, cargo passes -C debug-assertions=on with the new opt-level, and
rustc enables overflow checks with debug assertions. A probe test
confirmed that both still fire in these crates.
Measured on an M-series Mac (16 cores, other jobs running; two samples
each, without the kache wrapper, clean target directory):
opt-level 0 opt-level 3
cargo test --workspace --no-run 6.6-6.7 s wall 10.3-12.0 s wall
(clean build) 58 s CPU 91-120 s CPU
cargo test --workspace (run) 21.9-22.0 s wall 1.9-2.0 s wall
55-57 s CPU 4.2-4.5 s CPU
no_allocation binary 4.1-5.7 s wall 0.1 s wall
32-38 s CPU 1.1-1.2 s CPU
cargo nextest run --workspace 19.4-21.9 s wall 1.0-1.2 s wall
cargo llvm-cov nextest (clean) 28.6 s wall 13.6 s wall
The results do not change: 774 tests pass with cargo test (17 binaries)
and 768 with nextest, as before, and the coverage report is identical
for all 144 files. opt-level 1 builds no faster here and runs the tests
about three times slower than opt-level 3.
Trade-offs: a clean test build costs about 4-5 s more wall time. These
crates no longer share their compiled artifacts between cargo build and
cargo test. Tests in these crates run optimized code under a debugger;
cargo test --profile dev builds them without the override.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
The header now says that the allocator counts alloc and realloc but not frees. In the two scenarios with stereo render (f32_48k_stereo and i16_44k_stereo), the echo canceller rebuilds its block processor (echo_canceller3.rs, initialize) once it detects stereo render. 1000 blocks later, the new comfort noise generator drops its initial noise estimate (comfort_noise_generator.rs:170-172), which frees one 520-byte buffer (260 bytes per capture channel) inside the counted window. A probe that also counted frees found exactly this one free in those two scenarios and none in the others. Upstream frees the same buffer with N2_initial_.reset() (comfort_noise_generator.cc:178-180). The comment on the attenuation check said that without echo removal the attenuation is about 0 dB. Measured with the echo canceller disabled and the split-band high-pass filter that it enforces kept on (so that the processing rate is unchanged), the attenuation reaches 6.39 dB in f32_48k_stereo_in_mono_out: 3.5 dB because the downmix carries less echo than the first microphone, which is the one measured, and 2.9 dB because 48 kHz audio is processed at 32 kHz. With the echo canceller, the lowest value is 16.8 dB, so the 10 dB threshold still separates the two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
Noise suppression runs in the same capture callback as the echo canceller, but no scenario enabled it. f32_48k_stereo_noise_suppression runs the f32_48k_stereo layout with noise suppression: two suppressors, one per capture channel, each over two bands. It allocates nothing today, and the test now asserts that. With an allocation added to NoiseSuppressor::process, this scenario fails and the other 12 pass. The attenuation check still needs the echo canceller here. A probe measured 7.3 dB at most with noise suppression alone (echo canceller disabled, its split-band high-pass filter kept), against 16.9 dB with both in this scenario. The comment on the check now gives that figure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
The C API catches panics and returns WapError::Internal, but the docs did not say what a caller should do next. The call stops partway, so the processing state may be inconsistent. The docs now say to call wap_initialize(), and to destroy and recreate the instance if that also fails. What wap_initialize() resets, as read in the code: it sets all four stream formats (functions.rs:144-163), then initialize_locked (audio_processing_impl.rs:1052-1142) rebuilds the render, capture and full-band capture audio buffers. It clears or reallocates the residual echo detector's render queue and force-resets the high-pass filter. It re-initializes the residual echo detector and rebuilds the echo controller, AGC2, the noise suppressors and the capture levels adjuster from the current configuration, which it keeps. Since ba20000, a panic in the echo remover no longer leaves it unusable, and that commit's test covers the case. The header that build.rs generates with cbindgen carries the same text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
b10927e added fmt to the existing ptr import as use std::{fmt, ptr};, the only grouped std import in sonora-aec3. Every other file imports one std path per line. (#39 review, F10.) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
…method 800c96f made FrameBlocker::insert_sub_frame_and_extract_block generic over Band: AsRef<[Channel]> and Channel: AsRef<[f32]>, and its message says "No API break remains". That is wrong. A generic method is not source-compatible with every caller of the published 0.2.0 signature. Of 10 call patterns that compile against the 0.2.0 sources (a024d6e), 6 fail against 800c96f's signature (checked at dbcd920, Rust 1.91.1): - coercion to fn(&mut FrameBlocker, &[Vec<&[f32]>], &mut Block): E0308; - passing the method where FnMut over those types is required: "implementation of FnMut is not general enough"; - arguments whose type only the parameter fixed: &[iter.map(|c| c.as_slice()).collect()], &[vec![a.as_ref(), ..]] and &[vec![a, b].into()] (E0283), and .map(AsRef::as_ref).collect::<Vec<_>>() (E0790). cargo-semver-checks 0.46.0 rates that signature major (method_requires_different_generic_type_params). insert_sub_frame_and_extract_block now has the exact 0.2.0 signature, sub_frame: &[Vec<&[f32]>], again. The new method insert_owned_sub_frame_and_extract_block takes owned buffers, sub_frame: &[Vec<Vec<f32>>], and EchoCanceller3 uses it for capture and render, so it still builds no view per call. Both methods delegate to one private generic helper, so the copy code exists once and is the code that ran before. d417d08's capacity fix in FrameBlocker::new and its test stay. The change is additive. All 10 call patterns compile against this commit. cargo-semver-checks 0.46.0 (--baseline-rev a024d6e, offline, 1.91.1 rustdoc) reports "no semver update required" for all eight crates; the same run flags 800c96f's signature as major, so the check detects the break. Neither d417d08's BREAKING CHANGE footer nor 800c96f's generic signature applies to the next release. A new test coerces the method to the 0.2.0 fn-pointer type, so making it generic again breaks the test build (checked: E0308). blocker_and_framer now calls the owned method; block_bitexactness still passes borrowed views. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
…cenario The header of no_allocation.rs says each test checks that a test cannot pass because the processing it covers was skipped. The checks cover the echo canceller only, so f32_48k_stereo_noise_suppression (102369d) still passed when noise suppression did not run: with initialize_noise_suppressor creating no suppressors, all 13 tests pass. The scenario now also runs without noise suppression and requires the echo attenuation of the two runs to differ by more than 1 dB. The input is deterministic, so skipped noise suppression gives the same output and the same attenuation. Measured on aarch64: 16.85 dB with noise suppression, 18.97 dB without. The check does not assume a direction: with the echo canceller in front, noise suppression raises the output energy here. Under the mutation above the test now fails (18.97 dB both ways). check() returns its Outcome so the test can reuse the first run. The header now names both checks. The extra run takes about 0.1 s with the optimized test profile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx
dignifiedquire
changed the title
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
3 participants
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.
Summary
This PR builds on #39 by Roman Ernst and fixes every finding from its review. The first commit is the contributor's commit
b10927e, unchanged and with its authorship. The rest of the PR adds six commits on top. It makes steady-state processing with echo cancellation allocation-free across the f32 and i16 APIs, every channel layout, and the 16, 32, and 48 kHz rates. Output is byte-identical tomain.Merge this instead of #39. It contains #39's commit as is, so #39 can be closed when this merges.
What #39 did
EchoRemoverkeeps its per-channel working buffers in a scratch struct instead of allocating 11 vectors per 4 ms block.EchoCanceller3no longer builds a view per sub-frame for the frame blocker.AudioBuffer::copy_to_bufferresamples directly into the destination.tests/no_allocation.rs, counts allocations on the current thread: 40,500 onmain, 0 after the change.What this PR adds
8fd36b6AudioBuffer. A channel-select downmix now reads the channel directly, as upstream does. Adebug_assertdocuments the direct-resample invariant.ba20000mem::take. Before, one panic left the C API instance broken for good, because the FFI layer catches panics and reuses the instance. A regression test covers this. Also drops the per-blockreset(): every buffer is fully written before it is read, checked buffer by buffer in the commit message and empirically. Upstream doesn't zero them either.d417d08FrameBlocker::insert_sub_frame_and_extract_blocktakes a concrete&[Vec<Vec<f32>>]. Each buffer gets its own capacity:vec![Vec::with_capacity(..); n]dropped the capacity on clone. The signature change is reverted below (800c96f,99591ae); its BREAKING CHANGE footer no longer applies.59225300cbfee6no_allocationto 12 scenarios (see below).558b694copy_to_buffercan't be triggered without injecting a fault.800c96f,99591aeinsert_sub_frame_and_extract_blockkeeps its exact 0.2.0 signature (&[Vec<&[f32]>]), and the newinsert_owned_sub_frame_and_extract_block(&[Vec<Vec<f32>>])takes owned buffers. Both share one private generic body, andEchoCanceller3uses the owned method, so no views are built per call. A test pins the 0.2.0 signature.cargo-semver-checks(against the 0.2.0 tags) reports "no semver update required" for all 8 crates.d10b2deopt-level = 3, through[profile.test.package.*]. Debug assertions and overflow checks stay on.cargo test --workspace(already built) drops from about 22 s to 2 s, andno_allocationfrom about 35 s CPU to 1.2 s. A clean build takes about 4 s longer. Results and coverage are unchanged.74aabed,102369d,7cfea3f9e10a0e,dbcd920WapError::Internal. Onestdimport per line inecho_remover.rs.Verification
The allocation test now covers 13 scenarios:
Each scenario asserts:
no_allocationon this branchb10927emainmain(8 scenarios: HPF+AEC3+NS+AGC2, 16/32/48 kHz, mono and stereo)main(new harness)cargo test --workspacecargo-semver-checks)-D warnings(both CI commands), MSRV 1.91.1, docs, fuzz checkWhen this branch is merged with #38's flush-to-zero guard, two stereo scenarios failed at first. The cause was the test signal, not the library: pauses of exact digital silence left only subnormal differences between the stereo channels, and flush-to-zero erased them. The far-end pauses now carry low-level noise, and the scenarios pass with and without #38.
Not in this PR
BlockProcessor. Upstream reallocates there too.GainController2::processand the limiter). That predates this PR. Hence the title says echo cancellation.wap_process_stream_f32andwap_process_reverse_stream_f32(sonora-ffi/src/functions.rs:193, 224) build two small slice tables per call. Worth a follow-up.d10b2de. Before it, the extended test roughly doubled the Android jobs, which run under emulation (16–25 min, up from 8–10). Check the job times on this PR's latest CI run.🤖 Generated with Claude Code
https://claude.ai/code/session_01H29DamSLugosGXJSz1e5Yx