fix sample drop in resampler on rate change boundary

[yujonglee]

No description provided.

[netlify]

✅ Deploy Preview for hyprnote ready!

Name	Link
🔨 Latest commit	79b0a5d
🔍 Latest deploy log	https://app.netlify.com/projects/hyprnote/deploys/69227a9983299f0008059333
😎 Deploy Preview	https://deploy-preview-1802--hyprnote.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[coderabbitai]

📝 Walkthrough

Walkthrough

Added a pending_sample field to ResamplerDynamicNew to buffer samples detected during rate changes. Modified the poll_next loop to defer sample emission until the backend is rebuilt with the new rate or a chunk is yielded, ensuring proper synchronization. Added a test validating boundary handling during rate transitions.

Changes

Cohort / File(s)	Summary
Resampler rate-change buffering crates/audio-utils/src/resampler/dynamic_new.rs	Introduced pending_sample: Option<(f32, u32)> field to temporarily hold samples during rate changes; modified poll_next loop to buffer and defer emission until backend rebuild or chunk yield; adjusted control flow to process pending sample before yielding or draining.
Rate-change boundary test crates/audio-utils/src/resampler/mod.rs	Added async test test_dynamic_new_rate_change_boundary verifying correct sample ordering when transitioning between different source rates (8000 Hz → 16000 Hz).

Sequence Diagram

sequenceDiagram
    participant Source
    participant ResamplerDynamicNew
    participant Backend
    participant Consumer

    loop poll_next
        alt pending_sample exists
            ResamplerDynamicNew->>ResamplerDynamicNew: Check for rate change
            alt rate changed
                ResamplerDynamicNew->>Backend: Rebuild with new_rate
                ResamplerDynamicNew->>Backend: Push pending sample
                ResamplerDynamicNew->>ResamplerDynamicNew: Clear pending_sample
            else rate same
                ResamplerDynamicNew->>Backend: Push pending_sample
                ResamplerDynamicNew->>ResamplerDynamicNew: Clear pending_sample
            end
        else no pending_sample
            ResamplerDynamicNew->>Source: Consume next sample
            alt source produced sample
                ResamplerDynamicNew->>ResamplerDynamicNew: Detect rate change
                alt rate changed
                    ResamplerDynamicNew->>ResamplerDynamicNew: Store as pending_sample
                else rate same
                    ResamplerDynamicNew->>Backend: Push sample immediately
                end
            end
        end
        
        ResamplerDynamicNew->>Backend: try_yield_chunk()
        alt chunk available
            ResamplerDynamicNew->>Consumer: Yield chunk
        else no chunk
            ResamplerDynamicNew->>Backend: Drain remaining
        end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• State synchronization logic: The pending_sample field introduces state that must be carefully tracked across loop iterations; verify correct handling when rate changes occur vs. when they don't.
• Control flow ordering: Ensure samples are pushed to backend only after rate adjustments, and that chunk yielding doesn't lose or duplicate pending samples.
• Test coverage: The boundary test validates one transition path; consider whether additional rate-change scenarios are adequately covered elsewhere.

Possibly related PRs

• 24k sample-rate in macOS to minimize resampling #1741: Directly modifies ResamplerDynamicNew internals including backend/driver logic, rebuild operations, and the poll loop—closely related structural changes.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to assess relatedness to the changeset.	Add a description explaining the bug that was fixed, why samples were being dropped, and how the pending_sample mechanism resolves this issue.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix sample drop in resampler on rate change boundary' directly relates to the main change: addressing sample handling during rate changes in the resampler.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch restore-pending-sample-resampler

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

crates/audio-utils/src/resampler/dynamic_new.rs (2)

122-135: Pending-sample state addition looks consistent with struct invariants

The new pending_sample: Option<(f32, u32)> field and its initialization to None in new(...) integrate cleanly with the existing private state; there’s no API break since fields remain non‑pub, and the tuple carries exactly what is needed to replay the boundary sample under the correct rate.

If you want to make this a bit more self‑documenting, a small type alias (e.g. type PendingSample = (f32, u32);) or a short field comment explaining (sample, source_rate) would help future readers, but that’s purely optional.

Also applies to: 157-166

---

219-280: Rate‑change handling via pending_sample appears correct; relies on backend drain contract

The revised poll_next loop that prioritizes pending_sample and calls drain_for_rate_change() before rebuilding the backend looks sound:

• For both Passthrough and Resampler backends, Ok(false) from drain_for_rate_change() implies there is still output buffered, and the subsequent try_yield_chunk(true) will always yield a chunk before re‑queuing pending_sample, so there’s no risk of spinning without progress.
• Ok(true) only occurs once the backend has no more output, at which point rebuilding with new_rate and then pushing the stored sample ensures the boundary sample is processed exactly once under the new configuration.
• The EOS path still goes through drain_at_eos() + draining + try_yield_chunk(true) and cannot intersect with a live pending_sample, so there’s no way to “strand” the boundary sample during shutdown.

Overall this fixes the original boundary drop without introducing ordering or duplication issues; the control flow is a bit dense but coherent.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6fd97fe and 79b0a5d.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

• crates/audio-utils/src/resampler/dynamic_new.rs (4 hunks)
• crates/audio-utils/src/resampler/mod.rs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

crates/audio-utils/src/resampler/mod.rs (4)

crates/audio-utils/src/resampler/dynamic_new.rs (1)

• new (140-167)

crates/audio-utils/src/resampler/dynamic_old.rs (1)

• new (23-37)

crates/audio-utils/src/resampler/driver.rs (1)

• new (20-35)

crates/audio-utils/src/resampler/static_new.rs (1)

• new (35-47)

⏰ 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). (1)

• GitHub Check: fmt