Repository navigation
Conversation
itz4blitz
left a comment
There was a problem hiding this comment.
Thanks for digging into both of these. I checked them against the code and ran the mediad DSP tests here (17 passed).
Direct-call bitrate: right, and I'd take it on its own. On main MediaRequest::Bitrate only goes out from Calls::tiny_room and from retune when room_bitrate is set, so a direct really did sit at 32 kb/s. DIRECT_CALL_KBPS = 64 matches protocol::TINY_BITRATE_DEFAULT; consider using that constant instead of a new literal. If you split this into its own PR it can merge as soon as the gate is satisfied.
High band: not yet.
HUSH_DELAYis measured, not derived. hush-vani documents its latency as exactlyHush::LATENCY_SAMPLES(one hop, 160). The extra 13 samples comes from a cross-correlation on a signal the model attenuates by about 43 dB, so the peak sits on residue. With a longer run (120 frames instead of 60) I get a full-path peak at 557, not 519. The lag test passes at the length and seed it picked.- Because HP = δ − LP only reconstructs when the two delays match exactly, a delay error combs the 5.5–8.9 kHz crossover (a 39-sample error puts notches near 5.5, 6.8 and 8.0 kHz, about −10 dB at 6.8 kHz). The ±15-sample tolerance is roughly two cycles at 7.2 kHz, so it does not guard phase.
originalis the AEC output with NS off when isolation is on, so Enhanced now passes every noise above ~5.5 kHz untouched, while the WebRTC fallback still suppresses fullband. The hiss test was narrowed to the 500 Hz bin to allow this. Could the high band follow the isolation's own gain (the per-frame ratio of isolated to original low-band energy), so "not voice" also mutes the sparkle?
Please derive the delay from Hush::LATENCY_SAMPLES (or measure it on a signal the model passes) and tighten the lag test accordingly. Minor: the const keep shadowing KEEP in the test is leftover.
Gate. pr-gate needs the scenario on the branch, not only in the body: a fix must name e2e/scenarios/<id> under ## Proof, that <id>/scenario.json must exist on the PR head, and a file under it must be in the diff. Naming bridge-image would fail honestly, since the base already passes it. The smallest honest route is an enhanced-high-band scenario modelled on bridge-image (scenario.json plus the four drivers setting TINYCLIENT_PROVE=enhanced-high-band), a prove_enhanced_high_band() in process.rs wired in mediad/src/main.rs like the other prove_* arms, and that id under Proof.
itz4blitz
left a comment
There was a problem hiding this comment.
Thanks for the update. The gate part is sorted and the ring fix is a good catch, but the three DSP points from my last review are still open.
Resolved:
- Gate:
e2e/scenarios/high-band-keptis on the branch withscenario.jsonand the drivers, andpr-gateis green (the one cancelled run is just the duplicate that got superseded). Usinghigh-bandinstead ofenhanced-high-bandis fine. - The ring that read the slot it wrote, with the impulse test pinning the join to the sample.
Still open:
HUSH_DELAYis still the measured173, not derived fromHush::LATENCY_SAMPLES. The new comment explains the 13 extra samples but doesn't show why they are real, and I still get 557 over a longer run, not 519. Please derive it, or measure it on a signal the model passes through, and say which in the comment.- The enhanced lag test is still 60 frames with a +/-15 sample tolerance. That is about two cycles at 7.2 kHz, so it doesn't guard the crossover phase. Tighten it once the delay is derived.
- The high band still passes the isolation untouched, so Enhanced now lets everything above ~5.5 kHz through while the WebRTC fallback still suppresses it, and the hiss test was narrowed to the 500 Hz bin to allow that. Could the high band follow the isolation's gain (isolated vs original low-band energy per frame)? If you want to keep it as is, say so plainly in the PR body as a deliberate trade-off.
Small:
const keep: usize = KEEP;is still there, now in productionup(). Drop it and useKEEP.DIRECT_CALL_KBPS = 64is still a new literal.protocol::TINY_BITRATE_DEFAULTalready exists.- The
/// Joins an accepted direct...doc comment now sits onDIRECT_CALL_KBPSinstead ofjoin_direct. Move the const above it. low_zero()is added and never used.- There's a fair amount of rustfmt churn in
process.rs(theassert!reflows,ns_levelmode). It makes the DSP diff harder to read; I'd leave it out.
Process:
- No
e2e-proofstatus yet. The body says "lab run pending" and the gate needs it before merge. I'll start the lab run once the DSP points are settled. - The branch is based on
7ffa840and main is now at 0.2.63; please rebase. - Direct-call bitrate is still correct and could go in on its own if the DSP part takes a while.
…y ladder over the server's 64 kb/s room default, instead of staying at 32 kb/s
…igh band rejoins the reconstruction at the path's lag, measured and guarded by the lag tests
…lot it wrote, so its offset cancelled; it is a delay line of DELAY now, joined at the path's lag and pinned to the sample by the impulse test
…ero; a base without the hook, or one that rejoins late, fails
…s documented one hop put the rejoin at 526, the lag test holds speech the model passes at +/-4, and the high band above the cut follows the isolation's gain, so what the model silences below the cut is silenced above it like the fallback
…cond literal for 64 kb/s, with the join's doc back on join_direct
bbbd43b to
64aaecd
Compare
…kle checks assumed it passes, so the sparkle now holds the send to the isolation's own gain read per frame, whatever the model decides, and the unprovable end-to-end lag measurement is gone: the derivation stays, pinned by the bridge and impulse checks where the peak cannot wander
Direct calls keep the voice-quality ladder, and Enhanced noise keeps the high band
What changed and why
Two fixes for Tiny voice calls, plus one defect found while verifying the second:
join_directnever sentMediaRequest::Bitrateand the encoder stayed at 32 kb/s mono VOIP forever. It now encodes like a seat in a default room: the ladder over 64 kb/s (the server's room default), so Standard sounds full and High uses the Audio application.Band::upread the same slot it wrote, so its offset cancelled and every sample above the cut came back 418 samples (8.7 ms) after the low band it joins — sibilants smeared across the crossover, in the mode every call uses by default. The ring is a delay line ofDELAYnow, and an impulse test pins the join to the sample (the_high_band_joins_at_the_path_delay, also the lab'shigh-band-keptdriver). The earlier lag tests could not see this: they check that a tone survives and that the loud low band lags correctly, not where the high band lands.Type
Platforms
Proof
Scenario: e2e/scenarios/high-band-kept
Lab run: pending — this branch was cut from a machine whose loopback UDP is filtered by a VPN kill switch, so the lab scenarios cannot run here. The
mediadDSP tests, including the new impulse-lag test, run in CI on this branch; the lab run lands as a follow-up.Compatibility
No wire change. Servers and older clients are untouched; a direct call encoded at 32 kb/s still decodes everywhere.
Risk and rollback
The high band of the microphone passes isolation unfiltered by design; if its delay were wrong, the bands would comb-filter near the cut. The lag tests hold the reconstruction's delay, and the new impulse test holds the high band's join to the sample — which is exactly the guard the two lag tests alone did not provide. Rollback is reverting the commit.
Release note
Direct calls sound full again, and Enhanced noise no longer delays the sparkle above 7.2 kHz.