Skip to content

Conversation

@richiemcilroy
Copy link
Member

@richiemcilroy richiemcilroy commented Oct 21, 2025

  • ensure the Opus encoder chooses the closest supported sample rate instead of defaulting to the minimum

Summary by CodeRabbit

  • Tests

    • Added unit tests for audio rate selection covering exact matches, high/low input clamping scenarios.
    • Adjusted resampling tests to use explicit source/target audio configurations for clearer, more direct validation.
  • Refactor

    • Simplified Opus encoder rate selection logic to improve maintainability and enable targeted testing.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

Walkthrough

Refactored Opus encoder's inline rate-selection into a private helper select_output_rate(input_rate, supported_rates) with unit tests; updated BufferedResampler tests to construct the resampler directly from AudioInfo::new_raw sources instead of wrapping an ffmpeg resampler.

Changes

Cohort / File(s) Summary
Opus rate selection refactor
crates/enc-ffmpeg/src/audio/opus.rs
Extracted inline rate-picking logic into select_output_rate(input_rate, supported_rates) which returns first supported rate >= input or the highest supported rate if none. Added unit tests covering exact match, input above all supported rates, and input below all supported rates.
BufferedResampler test setup
crates/enc-ffmpeg/src/audio/buffered_resampler.rs
Tests now instantiate BufferedResampler::new directly with AudioInfo::new_raw source/target descriptions instead of building and wrapping an ffmpeg resampler; test behavior unchanged otherwise.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Opus encoder setup
    participant Helper as select_output_rate()
    participant Rates as supported_rates

    Caller->>Helper: provide input_rate, supported_rates
    Helper->>Rates: search for first rate >= input_rate
    alt found
        Rates-->>Helper: matching_rate
        Helper-->>Caller: return matching_rate
    else not found
        Rates-->>Helper: no match (all < input_rate)
        Helper->>Rates: select highest_supported_rate
        Rates-->>Helper: highest_rate
        Helper-->>Caller: return highest_rate
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Hopped through rates with nimble paws,

Found the match without the claws.
Tests snugged in, neat and spry,
Resampler tests march straight by.
🎶 A little refactor — hop and sigh.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix: clamp opus rate selection for high input" is clearly related to the main change in the changeset. The primary modification refactors rate selection logic in the Opus encoder by introducing a helper function that intelligently clamps the input rate to the closest supported rate. The title accurately identifies the core component being modified (opus rate selection), the mechanism (clamping), and describes a key scenario (handling high input). While the changeset also handles low input scenarios (clamping to the lowest supported rate) per the test summaries, the title still captures the essence of the fix, which per the PR objective is to "ensure the Opus encoder chooses the closest supported sample rate instead of defaulting to the minimum."
✨ 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 codex/analyze-audio-recording-performance-issue

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa35769 and e8003f5.

📒 Files selected for processing (1)
  • crates/enc-ffmpeg/src/audio/buffered_resampler.rs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust code using rustfmt and ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.

Files:

  • crates/enc-ffmpeg/src/audio/buffered_resampler.rs
crates/*/src/**/*

📄 CodeRabbit inference engine (AGENTS.md)

Rust crates should place tests within the src/ and/or a sibling tests/ directory for each crate inside crates/*.

Files:

  • crates/enc-ffmpeg/src/audio/buffered_resampler.rs
🧬 Code graph analysis (1)
crates/enc-ffmpeg/src/audio/buffered_resampler.rs (1)
crates/media-info/src/lib.rs (1)
  • new_raw (47-55)
⏰ 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). (3)
  • GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
  • GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
  • GitHub Check: Analyze (rust)
🔇 Additional comments (1)
crates/enc-ffmpeg/src/audio/buffered_resampler.rs (1)

281-291: LGTM! Test helper refactored cleanly.

The updated create_resampler function now constructs BufferedResampler directly using AudioInfo::new_raw, which aligns with the refactored API. The parameters are correctly structured, and the approach simplifies test setup while preserving the same test behavior.


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.

❤️ Share

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

@richiemcilroy richiemcilroy marked this pull request as ready for review October 21, 2025 20:14
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
crates/enc-ffmpeg/src/audio/opus.rs (1)

105-111: Clean and efficient implementation!

The logic correctly handles all cases:

  • Returns the first supported rate ≥ input rate
  • Falls back to the maximum supported rate when input exceeds all options
  • Returns None for empty slices (properly handled by the caller)

Optional: Consider adding a doc comment noting that supported_rates should be sorted for optimal results, though this is already ensured by the caller on line 60.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aeaf9cb and fa35769.

📒 Files selected for processing (1)
  • crates/enc-ffmpeg/src/audio/opus.rs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust code using rustfmt and ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.

Files:

  • crates/enc-ffmpeg/src/audio/opus.rs
crates/*/src/**/*

📄 CodeRabbit inference engine (AGENTS.md)

Rust crates should place tests within the src/ and/or a sibling tests/ directory for each crate inside crates/*.

Files:

  • crates/enc-ffmpeg/src/audio/opus.rs
🧬 Code graph analysis (1)
crates/enc-ffmpeg/src/audio/opus.rs (1)
crates/media-info/src/lib.rs (1)
  • rate (125-127)
⏰ 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). (3)
  • GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
  • GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
  • GitHub Check: Analyze (rust)
🔇 Additional comments (2)
crates/enc-ffmpeg/src/audio/opus.rs (2)

62-63: Excellent refactoring that fixes the high input rate bug!

The extraction of rate selection logic into select_output_rate() improves testability and correctly handles the edge case where input rate exceeds all supported rates. The function now properly clamps to the maximum supported rate instead of falling back to the minimum, ensuring better audio quality for high input rates.


113-134: Comprehensive test coverage!

The three test cases effectively validate the function's behavior:

  1. Exact matching rate selection
  2. Clamping to the highest supported rate for high inputs (validates the bug fix)
  3. Selecting the nearest higher rate for low inputs

The tests prevent regression of the original bug where high input rates would incorrectly fall back to the minimum instead of the maximum supported rate.

@Brendonovich Brendonovich merged commit e9d1dc1 into main Oct 22, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants