Repository navigation
Add a real-time speech recognition example with microphone for Rust API. - #3205
Conversation
dosubot
Bot
added
the
size:L
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a real-time microphone ASR example, argument-driven chunked streaming for ZipFormer, a DisplayManager for throttled terminal rendering, language-specific run scripts (English and bilingual), Cargo manifest updates (features, deps, versions), and CI script changes to invoke both run scripts. Changes
Sequence DiagramsequenceDiagram
participant Mic as Microphone
participant CPAL as CPAL (Input Stream)
participant Main as Main Loop
participant Recognizer as OnlineRecognizer
participant Display as DisplayManager
Main->>CPAL: build input stream (tx channel)
CPAL->>Mic: open device & start capture
loop per audio chunk
Mic->>CPAL: capture samples
CPAL->>Main: send Vec<f32> via channel
Main->>Recognizer: feed_chunk(samples)
Recognizer->>Recognizer: accept_waveform / maybe_decode
Recognizer->>Main: return partial/final result
Main->>Display: update_text / finalize_sentence
Display->>Display: render (throttled)
end
Recognizer->>Main: endpoint detected
Main->>Recognizer: reset stream
Main->>Display: finalize & render final sentences
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @csukuangfj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Rust API examples by introducing real-time speech recognition capabilities directly from a microphone. It integrates necessary audio input libraries and command-line argument parsing, alongside a new display utility for a more interactive user experience. The changes streamline the execution of examples and prepare the API for more dynamic use cases. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable real-time speech recognition example using a microphone, showcasing the streaming capabilities of the Rust API. The changes are well-structured, including the new streaming_zipformer_microphone.rs example, a DisplayManager for improved real-time feedback, and refactoring of an existing example to use command-line arguments. My review includes suggestions to enhance the robustness of the new microphone example by fixing a potential busy-loop, removing redundant code, and adopting a more idiomatic approach for feature-gated examples in Rust. Overall, this is a great contribution.
| let samples = rx.recv().unwrap_or_default(); | ||
| buffer.extend_from_slice(&samples); |
There was a problem hiding this comment.
Using unwrap_or_default() here can lead to a busy-loop with high CPU usage if the audio stream is closed and the sender is dropped. In that scenario, recv() will continuously return an Err, which unwrap_or_default() converts to an empty Vec. The loop will then spin without blocking. It's better to handle the Err case explicitly to break the loop and allow the program to exit gracefully.
match rx.recv() {
Ok(samples) => buffer.extend_from_slice(&samples),
Err(_) => {
println!("\nAudio stream closed. Exiting.");
break;
}
}| if last_render.elapsed() > Duration::from_millis(200) { | ||
| display.render(); | ||
| last_render = Instant::now(); | ||
| } |
There was a problem hiding this comment.
The DisplayManager::render() method already contains throttling logic to prevent excessive screen updates. This manual check for last_render.elapsed() is redundant. You can simplify the code by calling display.render() unconditionally on each loop iteration and removing the last_render variable defined on line 99.
display.render();| #[cfg(not(feature = "mic"))] | ||
| panic!( | ||
| "Feature `mic` is not enabled. Please build with `--features mic` to use the microphone example." | ||
| ); |
There was a problem hiding this comment.
The use cpal statement at the top of the file will cause a compilation error if the mic feature is not enabled, making this panic! unreachable. The idiomatic way to handle feature-gated examples is to use required-features in Cargo.toml. If you apply that suggestion, this check becomes redundant and can be removed.
| # Feature for using microphone | ||
| mic = ["cpal"] |
There was a problem hiding this comment.
To correctly handle examples that depend on optional features, the idiomatic approach in Rust is to specify required-features for the example in Cargo.toml. This ensures the example only builds when the necessary feature is enabled, providing a better developer experience and avoiding compilation errors. This also makes the manual panic! in streaming_zipformer_microphone.rs unnecessary.
| # Feature for using microphone | |
| mic = ["cpal"] | |
| # Feature for using microphone | |
| mic = ["cpal"] | |
| [[example]] | |
| name = "streaming_zipformer_microphone" | |
| required-features = ["mic"] |
There was a problem hiding this comment.
Pull request overview
Adds a Rust real-time streaming ASR microphone example and improves the existing streaming Zipformer example by moving model/audio paths to CLI args, plus supporting scripts and version bumps.
Changes:
- Introduce
DisplayManagerin thesherpa-onnxRust crate for incremental/streaming output. - Add
streaming_zipformer_microphoneexample (behind amicfeature) and new run scripts for English + bilingual models. - Update the WAV-based streaming example and README to use
claparguments; bump Rust crate versions to0.1.2.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
sherpa-onnx/rust/sherpa-onnx/src/lib.rs |
Exposes new display module publicly. |
sherpa-onnx/rust/sherpa-onnx/src/display.rs |
Adds DisplayManager for throttled terminal rendering of partial/final text. |
sherpa-onnx/rust/sherpa-onnx/Cargo.toml |
Bumps crate + sys dependency version to 0.1.2. |
sherpa-onnx/rust/sherpa-onnx-sys/Cargo.toml |
Bumps sys crate version to 0.1.2. |
rust-api-examples/examples/streaming_zipformer.rs |
Converts hardcoded paths to CLI args via clap. |
rust-api-examples/examples/streaming_zipformer_microphone.rs |
New microphone streaming example using cpal + DisplayManager. |
rust-api-examples/Cargo.toml |
Adds anyhow, clap, optional cpal, and mic feature; bumps to 0.1.2. |
rust-api-examples/README.md |
Updates command example to include all required CLI args. |
rust-api-examples/run-streaming-zipformer-en.sh |
New script to download/run English model with CLI args. |
rust-api-examples/run-streaming-zipformer-zh-en.sh |
New script to download/run bilingual model with CLI args. |
rust-api-examples/run-streaming-zipformer-microphone-zh-en.sh |
New script to run microphone example with --features mic. |
rust-api-examples/run-streaming-zipformer.sh |
Removes old script that ran the streaming example without args. |
rust-api-examples/.gitignore |
Adds a negation pattern for run-*.sh. |
.github/scripts/test-rust.sh |
CI script now runs the new English + bilingual run scripts. |
rust-api-examples/Cargo.lock |
Updates lockfile for new deps/features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // | ||
| // See ../README.md for how to run it | ||
| // | ||
| // See ./streaming_zipformer.rs for how to recongize a wave file. |
There was a problem hiding this comment.
Typo in comment: "recongize" -> "recognize".
| // See ./streaming_zipformer.rs for how to recongize a wave file. | |
| // See ./streaming_zipformer.rs for how to recognize a wave file. |
| cargo run --example streaming_zipformer -- \ | ||
| --wav sherpa-onnx-streaming-zipformer-en-2023-06-21/test_wavs/1.wav \ | ||
| --encoder sherpa-onnx-streaming-zipformer-en-2023-06-21/encoder-epoch-99-avg-1.int8.onnx \ | ||
| --decoder sherpa-onnx-streaming-zipformer-en-2023-06-21/decoder-epoch-99-avg-1.onnx \ | ||
| --joiner sherpa-onnx-streaming-zipformer-en-2023-06-21/joiner-epoch-99-avg-1.int8.onnx \ | ||
| --tokens sherpa-onnx-streaming-zipformer-en-2023-06-21/tokens.txt \ | ||
| --provider cpu \ | ||
| --debug |
There was a problem hiding this comment.
The README shows how to run the WAV-based streaming example, but the PR adds a microphone real-time example and new run-streaming-zipformer-microphone-*.sh scripts. Consider documenting how to run the microphone example here (including the required --features mic and any platform notes), otherwise the new functionality is hard to discover.
| pub struct DisplayManager { | ||
| sentences: Vec<String>, | ||
| current_text: String, | ||
| last_render: Instant, | ||
| } |
There was a problem hiding this comment.
DisplayManager stores all finalized sentences in an unbounded Vec<String> and re-renders the whole history each time. For long-running microphone sessions this can grow memory and slow rendering. Consider keeping only the last N sentences (configurable), or providing a way to clear/compact history.
| @@ -1 +1,2 @@ | |||
| target | |||
| !run-*.sh | |||
There was a problem hiding this comment.
.gitignore only ignores target, so !run-*.sh currently has no effect and can be removed to avoid confusion (negation patterns only matter if a broader ignore rule would match these files).
| !run-*.sh |
| use anyhow::Result; | ||
| use clap::Parser; | ||
| use cpal::traits::{DeviceTrait, HostTrait, StreamTrait}; | ||
| use sherpa_onnx::{DisplayManager, OnlineRecognizer, OnlineRecognizerConfig}; | ||
| use std::sync::mpsc; | ||
| use std::time::{Duration, Instant}; |
There was a problem hiding this comment.
The file unconditionally imports and uses cpal, but cpal is an optional dependency behind the mic feature. As written, cargo run --example streaming_zipformer_microphone without --features mic will fail to compile before reaching the #[cfg(not(feature = "mic"))] panic! statement. Gate the cpal imports/implementation behind #[cfg(feature = "mic")] and provide an alternate main (or compile_error!) for the non-mic case so the example compiles and prints a clear message.
| fn build_input_stream(device: &cpal::Device, tx: mpsc::Sender<Vec<f32>>) -> Result<cpal::Stream> { | ||
| let config = device.default_input_config()?.config(); | ||
| let err_fn = |err| eprintln!("Audio stream error: {:?}", err); | ||
|
|
||
| let stream = device.build_input_stream( | ||
| &config, | ||
| move |data: &[f32], _: &cpal::InputCallbackInfo| { | ||
| if !data.is_empty() { | ||
| let _ = tx.send(data.to_vec()); | ||
| } | ||
| }, | ||
| err_fn, | ||
| None, | ||
| )?; |
There was a problem hiding this comment.
build_input_stream assumes the input sample type is f32 (|data: &[f32]|) and will fail on devices whose default input format is i16/u16. Also, the callback receives interleaved multi-channel audio; forwarding it directly to accept_waveform will be incorrect for stereo devices. Consider branching on default_input_config()?.sample_format() and converting to f32, and down-mix or select a channel based on config.channels.
| let mut last_render = Instant::now(); | ||
|
|
||
| loop { | ||
| let samples = rx.recv().unwrap_or_default(); |
There was a problem hiding this comment.
rx.recv().unwrap_or_default() hides channel disconnects. If the sender drops, recv() will return immediately with an error and this loop will spin (high CPU) while processing empty buffers. Handle Err(_) by breaking out of the loop (or returning) instead of defaulting to an empty Vec.
| let samples = rx.recv().unwrap_or_default(); | |
| let samples = match rx.recv() { | |
| Ok(samples) => samples, | |
| Err(_) => break, | |
| }; |
| if recognizer.is_endpoint(&stream) { | ||
| if let Some(result) = recognizer.get_result(&stream) { | ||
| if !result.text.is_empty() { | ||
| display.finalize_sentence(); | ||
| } | ||
| } | ||
| recognizer.reset(&stream); | ||
| } |
There was a problem hiding this comment.
On endpoint, you call display.finalize_sentence() without ensuring DisplayManager.current_text reflects the final result.text. If the last partial update differs from the final hypothesis, the finalized sentence may be stale/truncated. Update the display with the final result.text (or pass the text into finalize_sentence) before finalizing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
rust-api-examples/.gitignore (1)
1-2:!run-*.shnegation is redundant here.Root-level
run-*.shfiles aren't matched by thetargetpattern, so the negation has no practical effect. This is harmless, but if the intent is simply to make the scripts explicitly tracked, a brief comment clarifying that would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-api-examples/.gitignore` around lines 1 - 2, The .gitignore contains a redundant negation pattern '!run-*.sh' because the preceding 'target' entry does not match root-level run-*.sh files; remove the '!run-*.sh' line (or replace it with a short explanatory comment) so the file only lists meaningful ignore patterns and/or documents why scripts should be tracked.sherpa-onnx/rust/sherpa-onnx/src/lib.rs (1)
1-6:DisplayManagerin the core library crate widens its public API beyond ASR concerns.Display/rendering logic is a presentation-layer utility that's more naturally scoped to example code than to the core
sherpa-onnxlibrary. Exposing it viapub use display::*;means every downstream consumer of the crate getsDisplayManagerin scope, and future additions todisplay.rsautomatically become part of the public API.Consider either:
- Keeping
DisplayManagerinside the examples crate (rust-api-examples), where it's actually used; or- Re-exporting explicitly (
pub use display::DisplayManager;) rather than via glob, to keep the surface area intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/lib.rs` around lines 1 - 6, The crate currently re-exports the entire display module via `pub use display::*;`, which unintentionally widens the public API (including `DisplayManager`); replace the glob export with an explicit re-export or remove it and move the display code to examples. Concretely, in `lib.rs` stop using `pub use display::*;` and either 1) change it to `pub use display::DisplayManager;` so only the `DisplayManager` symbol is public, or 2) remove the re-export entirely and move `display.rs` (and the `DisplayManager` type) into the `rust-api-examples` crate; ensure any example code imports the moved type from the examples crate after the change.rust-api-examples/examples/streaming_zipformer.rs (1)
44-46:default_value_t = falseon aboolfield is redundant.The default
ArgActionfor aboolfield in clap's derive API isSetTrue, sofalseis already the implicit default when the flag is absent. The annotation can be simplified to just#[arg(long)].🧹 Suggested simplification
- #[arg(long, default_value_t = false)] - debug: bool, + #[arg(long)] + debug: bool,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-api-examples/examples/streaming_zipformer.rs` around lines 44 - 46, The #[arg(long, default_value_t = false)] on the debug bool field is redundant; update the clap annotation for the struct field named debug to remove default_value_t and use #[arg(long)] only so the ArgAction default (SetTrue) is used and the flag behaves as a simple --debug boolean switch.sherpa-onnx/rust/sherpa-onnx/src/display.rs (1)
14-22: Consider implementingDefaultforDisplayManager.Any public type with a canonical "empty" constructor
new()should also implementDefault— it's idiomatic Rust and makes the type usable with generic bounds (T: Default) and struct update syntax.♻️ Suggested addition
impl DisplayManager { /// Create a new DisplayManager pub fn new() -> Self { Self { sentences: Vec::new(), current_text: String::new(), last_render: Instant::now(), } } +} + +impl Default for DisplayManager { + fn default() -> Self { + Self::new() + } +} + +impl DisplayManager {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/display.rs` around lines 14 - 22, Add a Default implementation for DisplayManager so callers can use T: Default and struct update syntax; implement impl Default for DisplayManager with default() delegating to DisplayManager::new() (i.e., return Self::new()) and keep current fields unchanged; update any module exports if necessary to keep the type public.rust-api-examples/examples/streaming_zipformer_microphone.rs (1)
97-133: Redundant external render throttle —last_renderon line 99 is dead weight.
DisplayManager::render()already performs its own 200 ms throttle internally (display.rslines 46–52). The outerlast_rendervariable and the guard at lines 130–133 add a second, independent 200 ms gate, but sincerender()returns early regardless, the outer gate never causes a rendering that the inner one would have skipped. Additionally, the twoInstantclocks drift independently, making the behaviour harder to reason about.♻️ Suggested fix — remove the redundant outer throttle
fn run_recognition_loop(...) { let mut display = DisplayManager::new(); let mut buffer = Vec::<f32>::new(); - let mut last_render = Instant::now(); loop { // ... chunk processing ... - if last_render.elapsed() > Duration::from_millis(200) { - display.render(); - last_render = Instant::now(); - } + display.render(); // DisplayManager throttles internally } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-api-examples/examples/streaming_zipformer_microphone.rs` around lines 97 - 133, Remove the redundant external throttle by deleting the last_render variable and the outer time-check that gates calls to DisplayManager::render(): remove the declaration let mut last_render = Instant::now(); and the if last_render.elapsed() > Duration::from_millis(200) { display.render(); last_render = Instant::now(); } block, and instead call display.render() directly in the loop (leaving DisplayManager::render() to handle its internal 200ms throttle); update any now-unused Instant/Duration imports if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-api-examples/examples/streaming_zipformer_microphone.rs`:
- Line 8: Fix the typo in the comment line "See ./streaming_zipformer.rs for how
to recongize a wave file." by replacing "recongize" with "recognize" so the
comment reads "See ./streaming_zipformer.rs for how to recognize a wave file."
- Around line 101-103: The loop currently calls rx.recv().unwrap_or_default()
which turns a disconnected channel error into an empty Vec and causes a busy
spin; change the rx.recv() handling to an explicit match on Result (e.g., match
rx.recv()) so that on Ok(samples) you extend the buffer
(buffer.extend_from_slice(&samples)) and on Err(e) you break the loop or
return/log the error (surface the RecvError) instead of returning an empty Vec;
update the loop containing rx.recv() and buffer.extend_from_slice(&samples) to
use this match-based handling so the stream stops or reports the error when the
sender is dropped.
- Around line 72-87: The callback currently assumes f32 samples and will panic
on devices with i16/u16 formats; update build_input_stream to inspect
device.default_input_config()?.sample_format() and branch for SampleFormat::F32,
::I16 and ::U16, building an input stream whose callback signature matches the
device sample type and converts each sample to f32 (e.g., via
cpal::Sample::to_f32) before pushing into the tx Sender<Vec<f32>>; keep err_fn
the same and ensure the closure sends a Vec<f32> only after converting the
incoming slice of the native sample type to f32.
---
Nitpick comments:
In `@rust-api-examples/.gitignore`:
- Around line 1-2: The .gitignore contains a redundant negation pattern
'!run-*.sh' because the preceding 'target' entry does not match root-level
run-*.sh files; remove the '!run-*.sh' line (or replace it with a short
explanatory comment) so the file only lists meaningful ignore patterns and/or
documents why scripts should be tracked.
In `@rust-api-examples/examples/streaming_zipformer_microphone.rs`:
- Around line 97-133: Remove the redundant external throttle by deleting the
last_render variable and the outer time-check that gates calls to
DisplayManager::render(): remove the declaration let mut last_render =
Instant::now(); and the if last_render.elapsed() > Duration::from_millis(200) {
display.render(); last_render = Instant::now(); } block, and instead call
display.render() directly in the loop (leaving DisplayManager::render() to
handle its internal 200ms throttle); update any now-unused Instant/Duration
imports if necessary.
In `@rust-api-examples/examples/streaming_zipformer.rs`:
- Around line 44-46: The #[arg(long, default_value_t = false)] on the debug bool
field is redundant; update the clap annotation for the struct field named debug
to remove default_value_t and use #[arg(long)] only so the ArgAction default
(SetTrue) is used and the flag behaves as a simple --debug boolean switch.
In `@sherpa-onnx/rust/sherpa-onnx/src/display.rs`:
- Around line 14-22: Add a Default implementation for DisplayManager so callers
can use T: Default and struct update syntax; implement impl Default for
DisplayManager with default() delegating to DisplayManager::new() (i.e., return
Self::new()) and keep current fields unchanged; update any module exports if
necessary to keep the type public.
In `@sherpa-onnx/rust/sherpa-onnx/src/lib.rs`:
- Around line 1-6: The crate currently re-exports the entire display module via
`pub use display::*;`, which unintentionally widens the public API (including
`DisplayManager`); replace the glob export with an explicit re-export or remove
it and move the display code to examples. Concretely, in `lib.rs` stop using
`pub use display::*;` and either 1) change it to `pub use
display::DisplayManager;` so only the `DisplayManager` symbol is public, or 2)
remove the re-export entirely and move `display.rs` (and the `DisplayManager`
type) into the `rust-api-examples` crate; ensure any example code imports the
moved type from the examples crate after the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust-api-examples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.github/scripts/test-rust.shrust-api-examples/.gitignorerust-api-examples/Cargo.tomlrust-api-examples/README.mdrust-api-examples/examples/streaming_zipformer.rsrust-api-examples/examples/streaming_zipformer_microphone.rsrust-api-examples/run-streaming-zipformer-en.shrust-api-examples/run-streaming-zipformer-microphone-zh-en.shrust-api-examples/run-streaming-zipformer-zh-en.shrust-api-examples/run-streaming-zipformer.shsherpa-onnx/rust/sherpa-onnx-sys/Cargo.tomlsherpa-onnx/rust/sherpa-onnx/Cargo.tomlsherpa-onnx/rust/sherpa-onnx/src/display.rssherpa-onnx/rust/sherpa-onnx/src/lib.rs
💤 Files with no reviewable changes (1)
- rust-api-examples/run-streaming-zipformer.sh
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rust-api-examples/examples/streaming_zipformer_microphone.rs (1)
1-16: Previous review issues have been addressed.The three issues from the prior round — the typo, the hardcoded
f32sample format, and theunwrap_or_default()busy-spin — are all resolved in this revision. The code now properly branches onSampleFormatand handles channel disconnection gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-api-examples/examples/streaming_zipformer_microphone.rs` around lines 1 - 16, All previous review issues are resolved: the typo was fixed, audio path now branches on cpal::SampleFormat instead of hardcoding f32 (look for match on SampleFormat), and the receiver handling no longer uses unwrap_or_default() busy-wait but instead checks for channel disconnection (see the mpsc receiver usage and stream callback in combination with DeviceTrait/StreamTrait). No code changes required—approve and merge this revision.
🧹 Nitpick comments (2)
rust-api-examples/examples/streaming_zipformer_microphone.rs (2)
188-210:default_input_config()is queried twice — once here and once insidebuild_input_stream.Consider querying the
SupportedStreamConfigonce inmainand passing it intobuild_input_streamto avoid the redundant device query and to guarantee both call-sites see the same config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-api-examples/examples/streaming_zipformer_microphone.rs` around lines 188 - 210, The code calls device.default_input_config() in main and again inside build_input_stream, causing a redundant query and potential mismatch; change main to call device.default_input_config() once (as you already do into the variable supported) and modify build_input_stream to accept that SupportedStreamConfig (or its concrete type) as an argument instead of querying the device again, then remove the default_input_config() call from build_input_stream; ensure main passes supported (and that sample_rate uses supported) and update any call-sites/signatures accordingly (e.g., main, build_input_stream, and any tests).
163-181: Redundant secondget_resultcall inside the endpoint check.
get_resultis called at line 166 and again at line 174 with no interveningdecode, so both return the same value. You can reuse the text from the first call:♻️ Suggested simplification
while recognizer.is_ready(&stream) { recognizer.decode(&stream); if let Some(result) = recognizer.get_result(&stream) { let text = result.text; if !text.is_empty() { display.update_text(&text); } - } - if recognizer.is_endpoint(&stream) { - if let Some(result) = recognizer.get_result(&stream) { - if !result.text.is_empty() { + if recognizer.is_endpoint(&stream) { + if !text.is_empty() { display.finalize_sentence(); } + recognizer.reset(&stream); } - recognizer.reset(&stream); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-api-examples/examples/streaming_zipformer_microphone.rs` around lines 163 - 181, The loop calls recognizer.get_result(&stream) twice (once before the endpoint check and again inside it) returning the same result; reuse the earlier result to avoid the redundant call by capturing the result (and its text) from the first get_result(&stream) invocation and then, inside the recognizer.is_endpoint(&stream) branch, reference that saved result/text to decide whether to call display.finalize_sentence() before calling recognizer.reset(&stream); adjust variable scope so the result is accessible to both the update_text and endpoint checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-api-examples/examples/streaming_zipformer_microphone.rs`:
- Around line 95-108: The I16 branch of the input stream closure
(SampleFormat::I16 -> device.build_input_stream, the closure move |data: &[i16],
_: &cpal::InputCallbackInfo|) normalizes by dividing by i16::MAX which maps
i16::MIN to slightly less than -1.0; change the normalization to use 32768.0 (or
call cpal::Sample::to_f32() for each sample) so values are clamped into [-1.0,
1.0] before sending via tx.send(samples).
---
Duplicate comments:
In `@rust-api-examples/examples/streaming_zipformer_microphone.rs`:
- Around line 1-16: All previous review issues are resolved: the typo was fixed,
audio path now branches on cpal::SampleFormat instead of hardcoding f32 (look
for match on SampleFormat), and the receiver handling no longer uses
unwrap_or_default() busy-wait but instead checks for channel disconnection (see
the mpsc receiver usage and stream callback in combination with
DeviceTrait/StreamTrait). No code changes required—approve and merge this
revision.
---
Nitpick comments:
In `@rust-api-examples/examples/streaming_zipformer_microphone.rs`:
- Around line 188-210: The code calls device.default_input_config() in main and
again inside build_input_stream, causing a redundant query and potential
mismatch; change main to call device.default_input_config() once (as you already
do into the variable supported) and modify build_input_stream to accept that
SupportedStreamConfig (or its concrete type) as an argument instead of querying
the device again, then remove the default_input_config() call from
build_input_stream; ensure main passes supported (and that sample_rate uses
supported) and update any call-sites/signatures accordingly (e.g., main,
build_input_stream, and any tests).
- Around line 163-181: The loop calls recognizer.get_result(&stream) twice (once
before the endpoint check and again inside it) returning the same result; reuse
the earlier result to avoid the redundant call by capturing the result (and its
text) from the first get_result(&stream) invocation and then, inside the
recognizer.is_endpoint(&stream) branch, reference that saved result/text to
decide whether to call display.finalize_sentence() before calling
recognizer.reset(&stream); adjust variable scope so the result is accessible to
both the update_text and endpoint checks.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust-api-examples/Cargo.tomlrust-api-examples/README.mdrust-api-examples/examples/streaming_zipformer_microphone.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust-api-examples/README.md
| SampleFormat::I16 => device.build_input_stream( | ||
| &config, | ||
| move |data: &[i16], _: &cpal::InputCallbackInfo| { | ||
| if !data.is_empty() { | ||
| // Normalize i16 -> [-1.0, 1.0] | ||
| let samples: Vec<f32> = | ||
| data.iter().map(|&s| s as f32 / i16::MAX as f32).collect(); | ||
|
|
||
| let _ = tx.send(samples); | ||
| } | ||
| }, | ||
| err_fn, | ||
| None, | ||
| )?, |
There was a problem hiding this comment.
I16 normalization can produce values slightly outside [-1.0, 1.0].
Dividing by i16::MAX (32767) means i16::MIN (−32768) maps to ≈ −1.000031, exceeding the expected [-1.0, 1.0] range. The standard approach (and what cpal::Sample::to_f32() does internally) is to divide by 32768.0:
🔧 Suggested fix
// Normalize i16 -> [-1.0, 1.0]
let samples: Vec<f32> =
- data.iter().map(|&s| s as f32 / i16::MAX as f32).collect();
+ data.iter().map(|&s| s as f32 / 32768.0).collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-api-examples/examples/streaming_zipformer_microphone.rs` around lines 95
- 108, The I16 branch of the input stream closure (SampleFormat::I16 ->
device.build_input_stream, the closure move |data: &[i16], _:
&cpal::InputCallbackInfo|) normalizes by dividing by i16::MAX which maps
i16::MIN to slightly less than -1.0; change the normalization to use 32768.0 (or
call cpal::Sample::to_f32() for each sample) so values are clamped into [-1.0,
1.0] before sending via tx.send(samples).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ./run-streaming-zipformer.sh | ||
| ./run-streaming-zipformer-en.sh | ||
| ./run-streaming-zipformer-zh-en.sh |
There was a problem hiding this comment.
CI currently runs the file-based streaming examples, but the new microphone example is gated behind the mic feature and isn’t compiled anywhere. Adding a compile-only step (e.g., cargo check --example streaming_zipformer_microphone --features mic) here would prevent microphone-example build breakages from slipping in, even if CI can’t run it due to missing audio devices.
| ./run-streaming-zipformer-zh-en.sh | |
| ./run-streaming-zipformer-zh-en.sh | |
| cargo check --example streaming_zipformer_microphone --features mic |
| fn build_input_stream(device: &cpal::Device, tx: mpsc::Sender<Vec<f32>>) -> Result<cpal::Stream> { | ||
| let supported = device.default_input_config()?; | ||
| let config = supported.config(); | ||
| let sample_format = supported.sample_format(); | ||
|
|
||
| let err_fn = |err| eprintln!("Audio stream error: {:?}", err); | ||
|
|
||
| let stream = match sample_format { | ||
| SampleFormat::F32 => device.build_input_stream( | ||
| &config, | ||
| move |data: &[f32], _: &cpal::InputCallbackInfo| { | ||
| if !data.is_empty() { | ||
| // Already expected to be in [-1.0, 1.0] | ||
| let _ = tx.send(data.to_vec()); | ||
| } | ||
| }, | ||
| err_fn, | ||
| None, | ||
| )?, |
There was a problem hiding this comment.
build_input_stream moves tx into the move closure in the first match arm, but the same tx is also referenced in the other SampleFormat arms. This will not compile because tx has been moved. Clone the mpsc::Sender (it’s Clone) per arm (or before the match) so each closure captures its own sender; similarly, avoid reusing the same err_fn value across arms (define it as a fn/function pointer or create a fresh closure per arm).
| if let Some(result) = recognizer.get_result(&stream) { | ||
| let text = result.text; | ||
| if !text.is_empty() { | ||
| display.update_text(&text); | ||
| } | ||
| } | ||
|
|
||
| if recognizer.is_endpoint(&stream) { | ||
| if let Some(result) = recognizer.get_result(&stream) { | ||
| if !result.text.is_empty() { | ||
| display.finalize_sentence(); | ||
| } | ||
| } | ||
| recognizer.reset(&stream); | ||
| } |
There was a problem hiding this comment.
At endpoint, you call display.finalize_sentence() based on result.text, but finalize_sentence() pushes whatever is currently in DisplayManager.current_text. If the last partial text wasn’t updated to the final result (or differs), this can finalize the wrong sentence. Consider updating the display with the endpoint result.text (or changing finalize_sentence to accept the final text) and avoid calling get_result() twice per decode loop by reusing the same decoded result.
| // Clear screen (ANSI escape) | ||
| print!("\x1B[2J\x1B[1;1H"); | ||
| println!("=== Speech Recognition with Next-gen Kaldi ==="); | ||
| println!("-----------------------------------------------"); |
There was a problem hiding this comment.
DisplayManager::render() clears the screen with print!() and then prints output, but it never flushes stdout. In an interactive/real-time display this can result in updates not appearing promptly (depending on buffering). Consider flushing stdout at the end of render() (or after the clear-screen escape) to ensure the display updates immediately.
| Self { | ||
| sentences: Vec::new(), | ||
| current_text: String::new(), | ||
| last_render: Instant::now(), |
There was a problem hiding this comment.
DisplayManager::new() initializes last_render to Instant::now(), so the first call to render() will almost always be throttled and produce no output until ~200ms later. If the intention is to show the initial screen immediately, initialize last_render so that the first render is not throttled (e.g., set it to an earlier instant or track an Option<Instant>).
| last_render: Instant::now(), | |
| // Initialize last_render sufficiently in the past so the first render is not throttled | |
| last_render: Instant::now() | |
| - Duration::from_millis(200), |
dosubot
Bot
added
size:XL
Summary by CodeRabbit
New Features
Documentation
Chores
Tooling