Repository navigation
fix(voice): handle U16 sample format in the fallback capture path - #4537
Conversation
Voice capture builds the input stream for the preferred device config, and on
failure retries with the device's default config. The preferred path handles
F32, I16 and U16, but the fallback match handled only F32 and I16 — U16 fell
through to `Err("unsupported fallback format: U16")`. So when the preferred
config failed and the device's default config was U16 (common on some Windows
drivers / older hardware), recording failed outright despite U16 being fully
supported on the preferred path. Nothing marked the omission as intentional;
it's an asymmetry between two paths meant to mirror each other.
Add the missing SampleFormat::U16 fallback arm mirroring the preferred handler.
While here, extract the i16/u16 -> f32 conversions (duplicated across the
preferred and fallback closures) into pure i16_to_f32 / u16_to_f32 helpers used
at all four sites — removing the copy-paste and, unlike the inline closures
that need a live audio device, making the conversion math unit-testable.
Tests: i16_to_f32 / u16_to_f32 pin the normalisation (mid-scale -> 0.0,
endpoints -> ±1.0, order preserved). Stream building still needs a real cpal
device and stays uncovered, consistent with the rest of the module.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extracts duplicated PCM-to-f32 conversion logic into shared i16/u16 helper functions, uses them in preferred and fallback audio stream callbacks, and adds unit tests covering normalization and unsigned PCM silence handling. ChangesPCM Conversion Refactor
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -520,6 +531,22 @@ pub fn list_input_devices() -> Result<Vec<String>, String> { | |||
| } | |||
|
|
|||
| /// Convert interleaved multi-channel samples to mono by averaging channels. | |||
There was a problem hiding this comment.
Nitpick (documentation): The new helpers were inserted between to_mono's doc comment and its fn, so this line — /// Convert interleaved multi-channel samples to mono by averaging channels. — is now attached to i16_to_f32. As a result i16_to_f32 carries a stray, wrong first doc line, and to_mono (now at ~line 550) has no doc comment at all.
Move that line back down to sit immediately above pub(crate) fn to_mono:
// remove this stray line from above i16_to_f32
/// Convert interleaved signed `i16` PCM samples to normalised `f32` in
/// `[-1.0, 1.0)` (`s / 32768`). Extracted so the preferred and fallback stream
/// paths share one conversion instead of duplicating it inline.
pub(crate) fn i16_to_f32(data: &[i16]) -> Vec<f32> { ... }
// ...
/// Convert interleaved multi-channel samples to mono by averaging channels.
pub(crate) fn to_mono(samples: &[f32], channels: usize) -> Vec<f32> { ... }There was a problem hiding this comment.
Fixed in 88a937fba. Moved the /// Convert interleaved multi-channel samples to mono … line back down to sit immediately above pub(crate) fn to_mono, so to_mono regains its doc comment and i16_to_f32 no longer carries the stray, wrong first line. Good catch — thanks.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #4537 — fix(voice): handle U16 sample format in the fallback capture path
Walkthrough
This PR closes a real asymmetry in record_on_thread: the preferred stream-building path handled F32/I16/U16, but the default-config fallback path handled only F32/I16, so a device whose default config is U16 (some Windows drivers, older hardware) would fail voice capture with unsupported fallback format: U16 even though U16 is fully supported on the preferred path. The fix adds the missing U16 fallback arm and, opportunistically, extracts the duplicated inline i16→f32 / u16→f32 conversions into two pub(crate) helpers that are now called from all four call sites and unit-tested. The change is small, correct, behaviour-preserving on existing formats, and does exactly what the title claims — a clean, well-motivated fix.
Changes
| File | Summary |
|---|---|
src/openhuman/voice/audio_capture.rs |
Adds SampleFormat::U16 arm to the fallback stream match; extracts i16_to_f32 / u16_to_f32 helpers and calls them from the preferred + fallback callbacks. |
src/openhuman/voice/audio_capture_tests.rs |
Adds unit tests pinning the normalisation of both helpers (mid-scale → 0.0, endpoints → ±1.0, order preservation). |
Actionable comments (1)
💡 Refactor / suggestion
1. src/openhuman/voice/audio_capture.rs:533 — orphaned to_mono doc comment
The two new helpers were inserted between to_mono's doc comment (/// Convert interleaved multi-channel samples to mono by averaging channels.) and the to_mono fn. As a result that line now documents i16_to_f32 (giving it a wrong, stray first doc line), and to_mono (~line 550) is left with no doc comment at all. Purely cosmetic — no runtime effect — but worth fixing while the code is fresh.
Suggested change:
// before — stray line above i16_to_f32
/// Convert interleaved multi-channel samples to mono by averaging channels.
/// Convert interleaved signed `i16` PCM samples to normalised `f32` in
/// `[-1.0, 1.0)` (`s / 32768`). ...
pub(crate) fn i16_to_f32(data: &[i16]) -> Vec<f32> { ... }
...
pub(crate) fn to_mono(samples: &[f32], channels: usize) -> Vec<f32> { ... }
// after — helper docs clean, to_mono keeps its own doc
/// Convert interleaved signed `i16` PCM samples to normalised `f32` in
/// `[-1.0, 1.0)` (`s / 32768`). ...
pub(crate) fn i16_to_f32(data: &[i16]) -> Vec<f32> { ... }
...
/// Convert interleaved multi-channel samples to mono by averaging channels.
pub(crate) fn to_mono(samples: &[f32], channels: usize) -> Vec<f32> { ... }Nitpicks (0)
None.
Questions for the author (0)
None.
Verified / looks good
- Correctness of the new
U16arm: mirrors the preferredU16handler exactly — sameu16_to_f32conversion,to_mono,update_peak_rms, silence gate, andsw.lock().extend_from_slice. The trailingother => Err(...)catch-all is preserved, so genuinely unsupported formats still error cleanly. - Normalisation math:
i16_to_f32(s / 32768) andu16_to_f32((s − 32768) / 32768) both land in[-1.0, 1.0); the docstrings match the formulas. No behaviour change on the existing F32/I16 paths. - Tests:
assert_eq!on floats is safe here — every expected value (0.0, ±0.5, ±1.0) and every divisor (2^n) is exactly representable, so no float-tolerance flakiness. Endpoints and interleaved-order cases are covered. The stream-building itself needs a live cpal device and stays uncovered, consistent with the rest of the module. - Repo conventions: helpers live in the domain file, are
pub(crate), and are documented; no root-level.rs, no controller/registry/event-bus concerns touched. Change is well under the size guideline.
Note: agreeing with CodeRabbit's automated pass (no actionable comments) — the logic is sound; the only thing it missed is the misplaced doc comment above. Leaving this as COMMENT only; approval is the maintainer's call.
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
The extracted i16_to_f32/u16_to_f32 helpers were inserted between to_mono's doc comment and its fn, leaving to_mono undocumented and i16_to_f32 with a stray wrong first doc line. Move the line back above to_mono.
What
Voice capture builds the input stream for the preferred device config, and if that fails, retries with the device's default config. The preferred path handles three sample formats —
F32,I16,U16— but the fallback path handles onlyF32andI16:So when the preferred config fails and the device's default config is
U16(common on some Windows drivers and older hardware), voice recording fails outright withunsupported fallback format: U16— even thoughU16is fully supported on the preferred path. Nothing marks the omission as intentional; it's an asymmetry between two paths that are meant to mirror each other.Change
SampleFormat::U16arm to the fallback match, mirroring the preferredU16handler.i16_to_f32/u16_to_f32, and call them from all four sites. This removes the copy-paste and — unlike the inline closures, which need a live audio device — makes the conversion math unit-testable.Correctness
u16_to_f32uses the same mid-scale formula the preferred path already used ((s - 32768) / 32768), andi16_to_f32the sames / 32768. No behaviour change on the existing F32/I16 paths; the only new behaviour is that aU16fallback device now records instead of erroring.Tests
i16_to_f32/u16_to_f32unit tests inaudio_capture_tests.rspin the normalisation (mid-scale → 0.0, full-scale endpoints → ±1.0). The stream-building itself needs a real cpal device and stays uncovered, consistent with the rest of this module.cargo test --lib voice::audio_capturegreen locally.Summary by CodeRabbit
Refactor
Tests