Repository navigation
Conversation
srperens
left a comment
There was a problem hiding this comment.
Verdict: Request changes. The element is well built and its guard tests ran in CI. But one premise of the latency design is wrong, and the default-on build cost needs a maintainer's call.
Requested changes
- Fix the latency premise in the PR body, or argue it again. The body says "Strom never recalculates pipeline latency". Strom already does that for a live property change, in
backend/src/gst/pipeline/properties.rs:210—let _ = self.pipeline.recalculate_latency();(the Time Offset path). Voice isolation could report 60 ms only while enabled and use the same path. The remaining argument against that is narrower:audiomixerwould then hold back every channel of the mix while one channel is on. Say whether that is the reason, because it decides whether the design is right. - Re-run the failed jobs (
gh run rerun 37671616522 --failed).Check (Linux)went red onstatic_underlays_are_not_reuploaded_gpu(underlay pads hold a frame with no zone configured), in a file this diff does not touch. Because that step failed,volume_ramp_test, thewhip_*tests, and thestrom-typesandstrom-frontendsteps never ran. This diff changestypes/src/mixer.rsandfrontend/src/mixer/.
For the maintainer — default-on cost
- Every build downloads a model from another project's release.
backend/Cargo.toml:10—default = ["nvidia", "voice-isolation"]. The source isbackend/build.rs:392. The sha256 pin makes the file safe, but not available: if that asset moves, every default build breaks, offline builds included unlessSTROM_VOICE_ISOLATION_MODELis set. The binary grows by 34.5 MB. - The weights ship inside the binary under Apache-2.0 (Ceva). This PR adds no attribution or NOTICE for them. EXTERNAL: what redistribution requires here.
- 44.1 kHz mixers lose quality even with the switch off. The resample pair is always built at a non-48 kHz rate, from
backend/src/blocks/builtin/mixer/voice_isolation.rs:93—let (in_first, in_last) = convert("in", voice_isolation::SAMPLE_RATE)?;. Per the PR body that costs −12 dB above 20 kHz on every 44.1 kHz flow. One option: build the conversion only when the flow starts with the switch on.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Strom never recalculates latency | CONTRADICTED |
backend/src/gst/pipeline/properties.rs:210, above |
| Off is bit-exact passthrough | CONFIRMED |
passthrough only reads the buffer (downmix into history). disabled_passes_audio_through_unchanged printed ok in job 112964538701 |
| On writes a mono result to every channel | CONFIRMED |
backend/src/gst/voice_isolation/imp.rs:422 — *s = *s * (1.0 - g) + w * g;, with one wet sample per frame. A stereo source collapses to mono while the switch is on |
| The element is finalized after running | CONFIRMED |
element_is_finalized_after_running ran and passed in the same job |
Radius. GLOBAL. The block chain is local to the mixer, but the default feature changes every build: a network fetch in build.rs, binary size, and dev-profile opt-level overrides in the workspace Cargo.toml. Lock order is consistent (state, then settings).
Overlaps.
- #898 and #930 change the same per-channel chain in
mixer/builder.rs. Land #898 (standing Approve) first. - #1002 and #1034 both rewrite
Cargo.lock. Regenerate the lock against whichever lands first.
Tests & CI. All 6 voice_isolation tests executed and passed in Check (Linux), and both Linux builds are green. macOS and Windows were skipped here. The Windows evidence is the author's fork dispatch, so ask for the ci:windows label before merge, for the build-time curl and compile time.
Confidence: HIGH
Adds stromvoiceisolation, a native element that runs the DPDFNet speech enhancement model (DPDFNet-2, 48 kHz, Apache-2.0) through onnxruntime, and puts it in every mixer channel between the HPF and the gate. Off by default; switching it on or off while the flow runs crossfades over 10 ms. The element reports no latency: Strom never recalculates pipeline latency, so reporting the model's delay would hold back every channel. Audio on an enabled channel is instead 60 ms late within unchanged timestamps. The model is downloaded and checksummed at build time and compiled into the binary; onnxruntime is linked statically from the ort prebuilts. Both sit behind the default `voice-isolation` feature. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ort prebuilt onnxruntime needs libstdc++ and glibc 2.38 symbols, so the zigbuild Linux release builds (glibc 2.31, libc++) cannot link it. tract is pure Rust and builds on every target. Debug builds optimise the tract and FFT crates, since unoptimised inference cannot keep up with live audio. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The disabled test now runs for 3 s, long enough for a model load that should never start. The odd-buffer test runs at a 0 dB limit, where the wet signal is the input itself, and checks every output sample against the input exactly CONTENT_DELAY_SAMPLES earlier, so running short of wet samples fails it. The element and engine use the shared VOICE_ISOLATION_NO_LIMIT_DB instead of a literal 100. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The disabled test ran for a fixed 3 s, and a slow CI host can take longer than that to load the model, so an element ignoring `enabled` could pass. An enabled element fed the same audio now marks when a load completes on this host, and the check runs until one second after that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eb76ed7 to
3f66f15
Compare
|
Claude here, replying for @wagenet. Thanks for the review. I rebased onto main ( 1. Latency premise. You're right: 2. CI. Re-running on the rebased head. 3. 44.1 kHz with the switch off. Agreed that a disabled feature shouldn't cost quality. Two ways to fix it:
I have no strong preference. Is losing the live switch at 44.1 kHz acceptable, or worth the extra code to keep? Attribution. I can add a Build-time download and default-on feature. Also the maintainer's call. If an external asset in the default build is a blocker, the options are to make |
srperens
left a comment
There was a problem hiding this comment.
Verdict: Comment. This replaces my review at eb76ed7 and moves it from Request changes to Comment. Both requested changes are dealt with: the body now gives the right latency argument, and CI ran again. The diff itself is unchanged since eb76ed7. Comparing the two heads over this PR's files shows only lines that came from main in the rebase. Two things still keep this from an Approve: a red Check (Linux) that this diff did not cause, and the maintainer's decisions below.
Requested changes
- Re-run the failed job (
gh run rerun 37813364018 --failed).Check (Linux)failed instinger_prores_test(a_prores_clip_after_another_format_still_airs,left: 29 right: 30frames). That is a stinger timing test this diff does not touch, from the same family as #1055. The cost is thatRun tests on shared typesandRun tests on frontendwereskipped, and this diff changestypes/src/mixer.rsandfrontend/src/mixer/. For two runs now, those steps have not executed against this change.
For the maintainer, still open, and the author asked you directly:
- 44.1 kHz with the switch off.
backend/src/blocks/builtin/mixer/voice_isolation.rs:93—let (in_first, in_last) = convert("in", voice_isolation::SAMPLE_RATE)?;is always built at a non-48 kHz rate. The author offers two fixes: build the conversion only when the flow starts with the switch on, which loses the live switch at 44.1 kHz, or resample inside the element only while it is enabled, which needs more code and a resampler crate. - Default-on build-time download from another project's release:
backend/build.rs:392—"https://github.com/k2-fsa/sherpa-onnx/releases/download/speech-enhancement-models/dpdfnet2_48khz_hr.onnx";withbackend/Cargo.toml:10—default = ["nvidia", "voice-isolation"]. The options are to keep it, make the feature off by default, or host the model somewhere Strom controls. - Attribution for the embedded weights (Ceva, Apache-2.0). There is still no NOTICE in the tree, and the author has offered to add one.
EXTERNAL: what redistribution requires.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Strom already recalculates latency on a live change (corrected body) | CONFIRMED |
backend/src/gst/pipeline/properties.rs:210 — let _ = self.pipeline.recalculate_latency(); |
| Reporting 60 ms would delay every sink, video included | EXTERNAL |
Assumes standard audiomixer and pipeline latency aggregation. This is the reason the body now gives for keeping the delay inside the channel |
The 6 voice_isolation tests ran in CI at this head |
CONFIRMED |
All six print ... ok in job 113435663380 at 3f66f15. pipeline_lifecycle_test also ran before the stinger failure |
Radius. GLOBAL, unchanged from my previous review. The feature is on by default, so it changes every build: a network fetch, 34.5 MB more binary, and dev-profile opt-level overrides.
Overlaps. #898 and #930 change the same per-channel chain in mixer/builder.rs. #1002 and #1017 rewrite Cargo.lock. Whichever of these lands second needs a rebase. None of them owns the decisions above.
Tests & CI. At 3f66f15: Build (Linux x86_64/ARM64), Check & Build (WASM) and API Contract Check are green. Check (Linux) is red, as described above. macOS and Windows were skipped. The Windows evidence is still the author's fork dispatch, so add the ci:windows label before merge to check the build-time download and the compile time on that runner.
Confidence: HIGH
Superseded by my review at 3f66f15: both requested changes are addressed, and the verdict is now Comment.
Eyevinn#1036 puts the element on every mixer channel; Eyevinn#898's tests build the mixer without registering it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
No issue.
Problem
A remote guest sends whatever is around them to air: music, a TV, fans, kitchen noise. The HPF and gate can't remove it, because that sound overlaps speech in level and frequency.
Change
A per-channel Voice isolation switch in the mixer, between the HPF and the gate. The properties are
chN_voice_isolationandchN_voice_isolation_limit(maximum cut in dB); both are live and appear in the channel detail panel.The new element,
stromvoiceisolation, runs DPDFNet (dpdfnet2_48khz_hr, Ceva, Apache-2.0 code and weights) with tract, a pure-Rust ONNX runtime. Off, it passes audio through untouched. On, it runs the model on the channel's mono downmix and writes the result to every channel. Switching it on or off crossfades over 10 ms. The model loads off the streaming thread on first enable; if loading fails, the element posts a warning and stays dry. The model runs only at 48 kHz, so a mixer at another rate getsaudioresample+capsfilteraround the element.Latency. The element reports none. An enabled channel's audio is 60 ms late inside unchanged timestamps instead: 50 ms for the model plus one hop of buffering, so buffers that are not whole hops are always answered in full. Reporting it is possible (Strom already recalculates latency when a Time Offset changes live), but
audiomixerreports the largest latency among its inputs, and the pipeline applies the highest latency of any sink to every sink, video included. One enabled channel would then delay every output of the flow by 60 ms, the conversation return included, and each toggle would re-sync every sink. This way only the enabled channel pays, and its audio trails its video by 60 ms, which is at the EBU R37 limit.Build.
build.rsdownloads the 10.6 MB model, checks its sha256, and embeds it in the binary;STROM_VOICE_ISOLATION_MODELpoints at a local copy for offline builds. Everything sits behind the default-onvoice-isolationfeature. Debug builds compile the tract and FFT crates atopt-level = 3, because unoptimised inference can't keep up with live audio.Rejected: DeepFilterNet3 (dormant, unclear weight licence); RNNoise (about 3 dB off the music); onnxruntime via
ort(about half the CPU, but its prebuilt needs glibc 2.38 and libstdc++, so the zigbuild Linux release builds against glibc 2.31 cannot link it).Evidence
Model, offline on a real 47-minute, four-guest recording, through the Python reference that tract matches to −66 dB: about 40 dB off an in-room instrument, with that guest's voice changed by −0.15 dB; clean speech changed 0.06–0.15 dB (median); voiced laughter kept, breaths and knocks reduced 40–95 dB. Judged good enough by ear. Long or overlapping laughter is untested.
Element. Each test below fails when the behaviour it guards is removed; each removal was run on this head:
enabled_suppresses_noise_and_switches_back_cleanly: on cuts noise by at least 20 dB with sizes and timestamps unchanged, and off returns the exact dry signal. Without the wet mix:noise only reduced 0.0 dB. Without the fade back:buffer 0 after disabling is not the dry signal.odd_buffer_sizes_come_back_whole_and_60_ms_late: at a 0 dB limit the wet signal is the input itself, so every output sample must equal the input exactly 2880 samples earlier, for buffers of 1 to 1024 frames. Without the padding hop:output never became the delayed input.disabled_passes_audio_through_unchanged: bit-exact while off, until an enabled element fed the same audio has been running for 1 s, so the window outlasts a model load however slow the host. Withenabledignored, under background priority (taskpolicy -b):buffer 40 changed while disabled.element_is_finalized_after_running: with a leaked reference:voice isolation element still alive after drop.test_voice_isolation_converts_around_a_non_48k_mixer: without the conversion:link mx:hpf_0 -> mx:voiceiso_0: Noformat.test_voice_isolation_sits_between_hpf_and_gate: with the element left unlinked, the placement assertion fails.Tests
Ran, on macOS arm64 at eb76ed7, before the rebase onto main:
cargo test --workspace voice_isolation(6 passed),mixer(84 passed, 2 already ignored),pipeline_lifecycle_test, clippy-D warningswith and without default features,fmt --check. After the rebase, at 3f66f15:cargo check --all-targetsandcargo test --workspace voice_isolation(6 passed).CI: at eb76ed7, before the rebase, Check (Linux) ran and passed all 6
voice_isolationtests, and both Linux zigbuild release builds passed. Check (Linux) then failed onstatic_underlays_are_not_reuploaded_gpu, a GPU vision mixer test this PR does not touch; a fork run of the same commit passed it. Windows tests passed in a fork dispatch (wagenet/strom run 37671643619; the upstream job needs theci:windowslabel), but that run's release build hit the job's 45-minute limit; see For the reviewer.Not run: the Docker build and macOS CI (label-gated); the GUI section in a running Strom; CPU inside Strom.
For the reviewer
opt-level = 3adds about 5 minutes to tests and 2–3 to the release build on runs without sccache. Fork dispatches have no cache and include the release build, so they reach the job's 45-minute limit (two of three were cut off there, after tests passed; the release build passed on 66c8913 in 41 minutes). Upstream PR runs skip the release build; main runs use sccache, so after the first one tract should come from the cache (unmeasured).STROM_VOICE_ISOLATION_MODELis set.Known limits
🤖 Generated with Claude Code