24k sample-rate in macOS to minimize resampling#1741
Conversation
|
Warning Rate limit exceeded@yujonglee has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCore resampler components refactored with a Backend abstraction that enables passthrough optimization when input/output rates match, eliminating unnecessary interpolation. Listener actors updated to use a centralized platform-specific SAMPLE_RATE constant. Changes
Sequence Diagram(s)sequenceDiagram
participant poll as ResamplerDynamicNew::poll()
participant backend as Backend
participant passthrough as Passthrough Buffer
participant resampler as Rubato Resampler
alt Source rate == Target rate
poll->>backend: Backend::Passthrough(buffer)
activate backend
backend->>passthrough: push_sample(sample)
backend->>passthrough: try_yield_chunk()
passthrough-->>backend: Some(chunk)
deactivate backend
else Source rate != Target rate
poll->>backend: Backend::Resampler(resampler)
activate backend
backend->>resampler: push_sample(sample)
backend->>resampler: try_yield_chunk()
resampler-->>backend: Some(resampled_chunk)
deactivate backend
end
backend-->>poll: chunk
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
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 |
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/audio-utils/src/resampler/dynamic_new.rs(6 hunks)crates/audio-utils/src/resampler/dynamic_old.rs(4 hunks)crates/audio-utils/src/resampler/mod.rs(3 hunks)plugins/listener/src/actors/controller.rs(0 hunks)plugins/listener/src/actors/listener.rs(1 hunks)plugins/listener/src/actors/mod.rs(1 hunks)plugins/listener/src/actors/recorder.rs(1 hunks)plugins/listener/src/actors/source.rs(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/listener/src/actors/controller.rs
🧰 Additional context used
🧬 Code graph analysis (4)
crates/audio-utils/src/resampler/dynamic_old.rs (2)
crates/audio-utils/src/resampler/mod.rs (3)
ResamplerDynamicOld(142-143)new(39-47)source(25-25)crates/audio-utils/src/resampler/dynamic_new.rs (1)
new(139-165)
crates/audio-utils/src/resampler/mod.rs (1)
crates/audio-utils/src/resampler/dynamic_new.rs (1)
new(139-165)
plugins/listener/src/actors/source.rs (1)
crates/audio-utils/src/lib.rs (1)
chunk_size_for_stt(230-236)
crates/audio-utils/src/resampler/dynamic_new.rs (2)
crates/audio-utils/src/resampler/dynamic_old.rs (1)
new(23-37)crates/audio-utils/src/resampler/driver.rs (3)
new(20-35)push_sample(73-75)process_all_ready_blocks(80-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (8)
crates/audio-utils/src/resampler/dynamic_old.rs (1)
19-19: LGTM: Bypass optimization is correctly implemented.The bypass flag properly short-circuits interpolation when source and target rates match, and the
seededflag ensures correct re-initialization when transitioning from bypass to non-bypass mode after a rate change.Also applies to: 25-25, 35-35, 47-47, 65-67
crates/audio-utils/src/resampler/mod.rs (1)
136-136: Updated test expectations reflect algorithmic changes.The modified expected sample counts align with the bypass optimization introduced in this PR.
Also applies to: 146-146
crates/audio-utils/src/resampler/dynamic_new.rs (2)
21-120: Excellent Backend abstraction design.The
Backendenum cleanly separates passthrough buffering from Rubato-based resampling. Thedrain_for_rate_changeanddrain_at_eosmethods ensure no samples are lost when switching between backends during rate changes.
146-155: Passthrough optimization correctly implemented.The constructor properly selects
Backend::Passthroughwhen rates match, andrebuild_backendcorrectly switches between passthrough and resampling modes when the source rate changes. The poll loop ensures samples are drained before switching backends (lines 227-243).Also applies to: 167-178
plugins/listener/src/actors/mod.rs (1)
13-16: Centralized, platform-specificSAMPLE_RATElooks goodThe cfg-gated
SAMPLE_RATEcleanly centralizes the rate and makes the macOS 24 kHz vs others 16 kHz distinction explicit for all actor modules usingsuper::SAMPLE_RATE. No issues from this definition itself.plugins/listener/src/actors/recorder.rs (1)
60-65: Recorder WAV spec now matches runtime sample rateUsing
super::SAMPLE_RATEfor the WAVsample_ratekeeps the on-disk spec aligned with the audio data produced bySourceActor(24 kHz on macOS, 16 kHz elsewhere). This makes the recorder consistent with the capture and STT pipelines.plugins/listener/src/actors/source.rs (1)
262-278: Consistent use of sharedSAMPLE_RATEin capture pipelineSwitching both mic and speaker to use
chunk_size_for_stt(super::SAMPLE_RATE)and.resampled_chunks(super::SAMPLE_RATE, chunk_size)ties the capture side cleanly to the centralized constant. That keeps chunk sizing and resampling behavior aligned with the OS-specific rate used elsewhere (listener and recorder), while leaving non-macOS behavior effectively unchanged.plugins/listener/src/actors/listener.rs (1)
205-216: Confirm whether owhisper server actually uses thesample_rateparameterThe original review comment's observation is valid. I found that:
- Single mode (line 212) now passes
super::SAMPLE_RATE(24 kHz on macOS)- Dual mode (line 252) still passes hard-coded
16000- Both capture paths via
SourceActorsend 24 kHz on macOS for either modeHowever, the owhisper server appears to hard-code resampling to 16 kHz internally (see
owhisper-server/src/server.rs:428.to_i16_le_chunks(16000, 512)andrealtime.rs:137.resample(16000)), which suggests thesample_rateparameter may be informational only and currently ignored.What you should verify:
- Does owhisper server actually use the
ListenParams.sample_ratefield, or is it always hard-coded to 16 kHz regardless of what's passed?- If it's ignored today, will future versions respect it?
Why it matters:
- If
sample_rateis ignored: the inconsistency is harmless but confusing; consider adding a code comment explaining why- If
sample_rateis (or will be) used: Dual mode on macOS needs to be updated tosuper::SAMPLE_RATEto align with whatSourceActoris actually sending, or explicitly document why it differs
yujonglee
deleted the
24k-sample-rate-in-macos-to-minimize-resamlping
branch
1 participant
No description provided.