Conversation
![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 (1)
crates/audio/src/speaker/windows.rs (1)
102-113: Consider using chunks_exact for more efficient byte conversion.The manual byte array construction can be simplified and made more efficient.
- let mut samples = Vec::new(); - while temp_queue.len() >= 4 { - let bytes = [ - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - ]; - let sample = f32::from_le_bytes(bytes); - samples.push(sample); - } + let bytes: Vec<u8> = temp_queue.drain(..).collect(); + let samples: Vec<f32> = bytes + .chunks_exact(4) + .map(|chunk| f32::from_le_bytes([chunk[0], chunk[1], chunk[2], chunk[3]])) + .collect();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapps/desktop/src-tauri/dlls/onnxruntime.dllis excluded by!**/*.dll
📒 Files selected for processing (15)
.cargo/config.toml(0 hunks)apps/desktop/src-tauri/.cargo/debug-config.toml(1 hunks)crates/aec/src/lib.rs(3 hunks)crates/audio/Cargo.toml(1 hunks)crates/audio/src/speaker/mod.rs(2 hunks)crates/audio/src/speaker/windows.rs(1 hunks)crates/calendar-apple/Cargo.toml(1 hunks)crates/onnx/Cargo.toml(1 hunks)crates/pyannote-local/src/embedding.rs(2 hunks)crates/pyannote-local/src/segmentation.rs(5 hunks)crates/tcc/Cargo.toml(1 hunks)crates/tcc/build.rs(1 hunks)crates/tcc/src/lib.rs(1 hunks)crates/vad/Cargo.toml(1 hunks)crates/vad/src/lib.rs(2 hunks)
💤 Files with no reviewable changes (1)
- .cargo/config.toml
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
crates/tcc/build.rscrates/pyannote-local/src/embedding.rscrates/aec/src/lib.rscrates/vad/src/lib.rscrates/tcc/src/lib.rscrates/audio/src/speaker/mod.rscrates/pyannote-local/src/segmentation.rscrates/audio/src/speaker/windows.rs
🧬 Code Graph Analysis (2)
crates/audio/src/speaker/mod.rs (4)
crates/audio/src/speaker/windows.rs (3)
new(15-19)stream(21-42)sample_rate(59-61)crates/audio/src/stream.rs (2)
new(12-17)sample_rate(28-30)crates/audio/src/speaker/macos.rs (3)
new(57-109)stream(161-190)sample_rate(38-47)crates/audio/src/lib.rs (2)
stream(100-113)sample_rate(157-163)
crates/pyannote-local/src/segmentation.rs (1)
crates/aec/src/lib.rs (4)
outputs(181-184)outputs(189-192)outputs(211-214)outputs(219-222)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (27)
crates/calendar-apple/Cargo.toml (1)
7-10: Inconsistency between AI summary and code structure.The AI summary states that
anyhow,chrono, anditertoolsdependencies were moved to the macOS-specific section, but the code shows they remain in the general[dependencies]section. Since this is an Apple-specific crate, these dependencies should likely be scoped to macOS only.Please clarify the intended dependency scoping. If these dependencies should be macOS-specific, apply this diff:
[dependencies] -anyhow = { workspace = true } -chrono = { workspace = true } hypr-calendar-interface = { path = "../calendar-interface", package = "calendar-interface" } -itertools = { workspace = true } [target.'cfg(target_os = "macos")'.dependencies] +anyhow = { workspace = true } +chrono = { workspace = true } block2 = "0.5.1" +itertools = { workspace = true } objc2 = "0.5.2"Likely an incorrect or invalid review comment.
crates/tcc/build.rs (1)
2-12: LGTM! Clean platform-specific build logic.The conditional compilation properly restricts Swift linking to macOS and provides helpful feedback for other platforms. The implementation follows Rust best practices for platform-specific code.
crates/tcc/Cargo.toml (1)
6-10: LGTM! Proper platform-specific dependency scoping.The
swift-rsdependencies are correctly scoped to macOS only, which aligns with the conditional compilation in the build script and library code. The consistent git revision across build and runtime dependencies is good practice.crates/tcc/src/lib.rs (2)
1-10: LGTM! Well-structured platform-specific implementation.The conditional compilation properly separates macOS Swift integration from the fallback implementation. The fallback returning
trueis reasonable for non-macOS platforms where this permission check may not be applicable.
18-23: LGTM! Proper platform-specific test handling.The test correctly uses
unsafecalls only on macOS where the Swift function is available, while using the safe Rust function on other platforms. The platform-specific test logic is clean and follows Rust best practices.crates/onnx/Cargo.toml (1)
14-18: LGTM! Well-structured conditional dependency setup.The conditional dependencies based on debug assertions make sense - debug builds get the
load-dynamicfeature for easier debugging while release builds stay minimal. This aligns with the API changes in the consuming code.apps/desktop/src-tauri/.cargo/debug-config.toml (1)
1-28: ORT_DYLIB_PATH is validThe debug configuration properly addresses MSVC runtime linking issues with the appropriate
rustflagsand environment variables. TheORT_DYLIB_PATHpoints to./dlls/onnxruntime.dll, which exists atapps/desktop/src-tauri/dlls/onnxruntime.dll. No changes required.crates/aec/src/lib.rs (7)
6-6: LGTM! Added necessary import for updated API.The TensorRef import is required for the new ONNX Runtime API usage.
176-179: LGTM! Properly migrated to new ONNX Runtime API.The explicit conversion to
TensorRefwith proper error handling is the correct approach for the updated ort crate API.
184-184: LGTM! Consistent output extraction method.The change from
try_extract_tensortotry_extract_arrayaligns with the new API patterns.
192-192: LGTM! Consistent output extraction method.Properly using the new
try_extract_arraymethod for state tensor extraction.
206-209: LGTM! Consistent API migration in run_model_2.The same proper TensorRef conversion pattern is applied consistently in the second model method.
214-214: LGTM! Consistent output extraction.Using the updated
try_extract_arraymethod for output tensor extraction.
222-222: LGTM! Consistent state tensor extraction.Properly using the new API for state tensor extraction with
try_extract_array.crates/vad/src/lib.rs (5)
5-8: LGTM! Updated imports for new API.The TensorRef import is necessary for the ONNX Runtime API changes.
52-56: LGTM! Properly migrated to TensorRef API.The explicit conversion to
TensorReffor all input tensors with proper error handling is the correct approach for the updated ort crate.
62-62: LGTM! Consistent output extraction method.Using the new
try_extract_arraymethod for state tensor extraction.
69-69: LGTM! Consistent output extraction method.Properly using
try_extract_arrayfor the c_tensor state extraction.
75-75: LGTM! Consistent output extraction method.Using the updated
try_extract_arraymethod for probability tensor extraction.crates/vad/Cargo.toml (1)
12-16: LGTM! Consistent conditional dependency setup.The conditional dependencies match the pattern used in the onnx crate, properly separating debug and release build features for the ort dependency.
crates/pyannote-local/src/embedding.rs (1)
5-5: LGTM! Consistent ONNX Runtime API updates.The changes correctly update the ONNX Runtime integration to use the new API pattern with
TensorRef::from_array_viewfor inputs andtry_extract_arrayfor outputs, maintaining proper error handling throughout.Also applies to: 30-30, 33-33
crates/pyannote-local/src/segmentation.rs (2)
3-3: LGTM! Consistent ONNX Runtime API updates.The changes correctly update the ONNX Runtime integration to match the new API pattern, consistent with the changes in
embedding.rs.Also applies to: 50-50, 53-53
55-55: Good refactoring to static methods.Converting
process_outputs,find_max_index, andcreate_segmentto static methods is appropriate since they don't access any instance state. This makes the code clearer by explicitly showing these are utility functions.Also applies to: 67-67, 84-84, 95-95, 103-103, 119-119
crates/audio/Cargo.toml (1)
30-30: Approved: wasapi v0.19.0 dependency update verifiedConfirmed that the
wasapi = "0.19.0"release exists on crates.io and provides all required Windows audio‐capture functionality, including:
- System loopback capture (“what you hear”)
- Application‐specific loopback via
AudioClient::new_application_loopback_client(...)- Shared and exclusive modes with event-driven or polled buffering
- Acoustic Echo Cancellation support
- Device enumeration, notifications, and low-level WASAPI interface wrappers
No further changes needed.
crates/audio/src/speaker/mod.rs (2)
32-43: Platform support expansion looks correct.The conditional compilation attributes properly enable Windows support alongside macOS.
161-193: Well-structured test with proper error handling.The test gracefully handles cases where WASAPI might not be available and provides informative output. Good use of early returns to skip the test when audio is unavailable.
crates/audio/src/speaker/windows.rs (1)
144-168: Correct async Stream implementation with proper waker handling.The poll_next implementation correctly handles the waker pattern and uses a double-check to avoid race conditions between checking the queue and registering the waker.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/audio/src/speaker/windows.rs (1)
15-19: Sample rate override parameter is still not implemented.The
sample_rate_overrideparameter is accepted but not used in the audio capture. The sample rate remains hardcoded to 44100 Hz in theWaveFormatinitialization.Also applies to: 74-74
🧹 Nitpick comments (3)
crates/denoise/Cargo.toml (1)
6-9: Add brief documentation & double-check upstream featureThe new
default/load-dynamicfeature gate is syntactically correct. Two small follow-ups will avoid confusion later:
- Add a short comment or
package.metadata.docs.rssection describing what enablingload-dynamicdoes so downstream users don’t have to dive into code to understand the behaviour.- Verify that
hypr-onnxdoes indeed expose aload-dynamicfeature; otherwisecargowill fail at feature resolution.crates/audio/src/speaker/windows.rs (2)
74-74: Consider making audio format configurable.The audio format is hardcoded to mono (1 channel). Many Windows audio devices output stereo, and capturing only one channel might lose important audio information.
Consider making the channel count configurable or defaulting to stereo:
-let desired_format = WaveFormat::new(32, 32, &SampleType::Float, 44100, 1, None); +// Use stereo by default or make it configurable +let desired_format = WaveFormat::new(32, 32, &SampleType::Float, 44100, 2, None);
98-101: Consider reducing the event timeout for better responsiveness.A 3-second timeout might cause poor audio responsiveness and delayed shutdown.
-if h_event.wait_for_event(3000).is_err() { +// Use a shorter timeout for better responsiveness +if h_event.wait_for_event(100).is_err() { - error!("timeout error, stopping capture"); - break; + // Check shutdown flag on timeout + let state = waker_state.lock().unwrap(); + if state.shutdown { + break; + } + continue; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src-tauri/Cargo.toml(1 hunks)crates/aec/Cargo.toml(1 hunks)crates/audio/src/speaker/mod.rs(2 hunks)crates/audio/src/speaker/windows.rs(1 hunks)crates/chunker/Cargo.toml(1 hunks)crates/denoise/Cargo.toml(1 hunks)crates/onnx/Cargo.toml(1 hunks)crates/pyannote-local/Cargo.toml(1 hunks)crates/vad/Cargo.toml(1 hunks)plugins/listener/Cargo.toml(1 hunks)plugins/local-stt/Cargo.toml(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- crates/pyannote-local/Cargo.toml
- crates/chunker/Cargo.toml
- crates/aec/Cargo.toml
- plugins/listener/Cargo.toml
- apps/desktop/src-tauri/Cargo.toml
- plugins/local-stt/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/onnx/Cargo.toml
- crates/vad/Cargo.toml
- crates/audio/src/speaker/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
crates/audio/src/speaker/windows.rs
🧬 Code Graph Analysis (1)
crates/audio/src/speaker/windows.rs (2)
crates/audio/src/speaker/mod.rs (7)
new(33-36)new(39-43)stream(46-49)stream(52-56)sample_rate(89-91)sample_rate(94-96)poll_next(67-80)crates/audio/src/speaker/macos.rs (4)
new(57-109)stream(161-190)sample_rate(38-47)poll_next(196-215)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (2)
crates/audio/src/speaker/windows.rs (2)
153-166: Good implementation of clean shutdown mechanism.The Drop trait properly signals shutdown and joins the capture thread, addressing the concern from previous reviews about thread cleanup.
103-123: Verify direct f32 sample support in WASAPI
- Location: crates/audio/src/speaker/windows.rs (lines 103–123)
- Action: Check if
render_clientoffers a direct f32‐based read API (e.g. aread_samples::<f32>(),read_f32(), or similar). If so, switch to that to avoid manual byte decoding.- Fallback optimization: Replace the
VecDeque+ multiplepop_front()calls with a single byte buffer andchunks_exactconversion:-let mut temp_queue = VecDeque::new(); -if let Err(e) = render_client.read_from_device_to_deque(&mut temp_queue) { - error!("Failed to read audio data: {}", e); - continue; -} - -if temp_queue.is_empty() { - continue; -} - -let mut samples = Vec::new(); -while temp_queue.len() >= 4 { - let bytes = [ - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - ]; - let sample = f32::from_le_bytes(bytes); - samples.push(sample); -} +// If WASAPI supports direct f32 reads, use that here: // let samples = render_client.read_samples::<f32>().map_err(|e| error!(…))?; + +// Otherwise, read raw bytes into a Vec<u8> and convert efficiently +let mut byte_buffer = Vec::new(); +if let Err(e) = render_client.read_from_device_to_vec(&mut byte_buffer) { + error!("Failed to read audio data: {}", e); + continue; +} + +let mut samples = Vec::with_capacity(byte_buffer.len() / 4); +for chunk in byte_buffer.chunks_exact(4) { + let bytes: [u8; 4] = chunk.try_into().unwrap(); + samples.push(f32::from_le_bytes(bytes)); +}Please verify whether the WASAPI client already supports native f32 reads. If not, apply the fallback optimization above.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/audio/src/speaker/windows.rs (1)
82-82: Sample rate is hardcoded despite override parameter.The WASAPI format is hardcoded to 44100 Hz, but the
sample_rate_overrideparameter suggests this should be configurable. This creates inconsistency between the actual capture rate and the reported sample rate.- let desired_format = WaveFormat::new(32, 32, &SampleType::Float, 44100, 1, None); + let sample_rate = self.sample_rate_override.unwrap_or(44100); + let desired_format = WaveFormat::new(32, 32, &SampleType::Float, sample_rate, 1, None);Note: You'll need to pass the sample rate override into the capture loop function.
🧹 Nitpick comments (2)
crates/audio/src/speaker/windows.rs (2)
129-138: Optimize byte-to-float conversion.The current implementation manually pops 4 bytes and converts them to f32. This is inefficient and could be simplified using slice operations.
- let mut samples = Vec::new(); - while temp_queue.len() >= 4 { - let bytes = [ - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - temp_queue.pop_front().unwrap(), - ]; - let sample = f32::from_le_bytes(bytes); - samples.push(sample); - } + let mut samples = Vec::new(); + let bytes: Vec<u8> = temp_queue.drain(..).collect(); + for chunk in bytes.chunks_exact(4) { + if let Ok(bytes_array) = <[u8; 4]>::try_from(chunk) { + let sample = f32::from_le_bytes(bytes_array); + samples.push(sample); + } + }
146-148: Make buffer size configurable.The buffer size is hardcoded to 8192 samples, which may not be suitable for all use cases. Consider making this configurable or documenting the rationale for this specific size.
+ const MAX_BUFFER_SIZE: usize = 8192; - if len > 8192 { - queue.drain(0..(len - 8192)); + if len > MAX_BUFFER_SIZE { + queue.drain(0..(len - MAX_BUFFER_SIZE)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/audio/src/speaker/windows.rs(1 hunks)crates/onnx/Cargo.toml(1 hunks)crates/vad/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/onnx/Cargo.toml
- crates/vad/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
crates/audio/src/speaker/windows.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci
🔇 Additional comments (3)
crates/audio/src/speaker/windows.rs (3)
78-99: LGTM: Excellent error handling pattern.The initialization result pattern with proper error propagation through channels is well-implemented. This addresses the previous concern about error propagation from audio initialization.
174-186: LGTM: Proper resource cleanup.The Drop implementation correctly sets the shutdown flag and joins the capture thread, addressing the previous concern about thread cleanup. The error handling for thread joining is also appropriate.
195-200: LGTM: Shutdown state handling implemented.The poll_next method now properly checks for shutdown state and returns None when shutting down, addressing the previous concern about stream completion signaling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/audio/src/speaker/windows.rs (1)
80-80: Sample rate mismatch between override and WASAPI initialization.The WASAPI format is hardcoded to 44100 Hz, but the
sample_rate()method can return a different value ifsample_rate_overrideis provided. This creates inconsistency between the actual capture rate and the reported rate.-let desired_format = WaveFormat::new(32, 32, &SampleType::Float, 44100, 1, None); +let sample_rate = self.sample_rate_override.unwrap_or(44100); +let desired_format = WaveFormat::new(32, 32, &SampleType::Float, sample_rate, 1, None);However, since the
sample_rate_overrideis not accessible in this static method, you'll need to pass it as a parameter tocapture_audio_loop.
🧹 Nitpick comments (1)
crates/audio/src/speaker/windows.rs (1)
144-146: Document the buffer size limit rationale.The hardcoded 8192 sample limit for queue management lacks explanation. This affects memory usage and latency characteristics.
+// Limit buffer to 8192 samples (~185ms at 44100 Hz) to prevent unbounded memory growth let len = queue.len(); if len > 8192 { queue.drain(0..(len - 8192)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/audio/src/speaker/windows.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
crates/audio/src/speaker/windows.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (4)
crates/audio/src/speaker/windows.rs (4)
127-136: LGTM: Proper byte-to-float conversion.The 4-byte to f32 conversion correctly handles 32-bit float samples with little-endian byte order, matching the WASAPI format specification.
172-184: LGTM: Proper shutdown handling.The Drop implementation correctly sets the shutdown flag and joins the capture thread, addressing the previous review concern about thread cleanup.
193-223: LGTM: Proper async streaming with shutdown support.The poll_next implementation correctly handles shutdown state, queue management, and waker patterns, addressing previous review concerns about shutdown handling.
99-166: LGTM: Comprehensive initialization error handling.The initialization error propagation via channel communication properly addresses the previous review concern about silent failures, ensuring the stream consumer is notified of initialization issues.
2 participants
resolves #66
Todo
apps/desktop/src-tauri/.cargo/debug-config.toml) to solveminkernel\crts\ucrt\src\appcrt\lowio\read.cpp(_osfile(fh) & FOPEN)errorpnpm -F desktop tauri dev --target x86_64-pc-windows-msvc --config ./src-tauri/tauri.conf.stable.json --verbose -- -- --config .cargo/debug-config.tomlfor debug runload-dynamicfeature- try release runtime configuration with /MD flag:.cargo/config.tomlReferences
Summary
💥 app shutdown without errorNo error but recording does not work✅ works!