expose audio mixing as a configurable field#850
Conversation
|
|
📝 WalkthroughWalkthroughIntroduced a new public Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)livekit-api/src/services/egress.rs (1)
⏰ 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). (14)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting Comment |
![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
🤖 Fix all issues with AI agents
In `@livekit-api/src/services/egress.rs`:
- Line 115: The failing CI is due to formatting on the line setting
audio_mixing; re-run rustfmt (cargo fmt) and split or reflow the expression so
it fits style rules—for example wrap the call and cast across lines or use a
local let binding like let audio_mixing =
options.audio_mixing.unwrap_or(proto::AudioMixing::DefaultMixing) as i32; then
use audio_mixing in the struct; update the file containing the audio_mixing
assignment (references: audio_mixing, options.audio_mixing,
proto::AudioMixing::DefaultMixing) and commit the formatted change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
livekit-api/src/services/egress.rs
🧰 Additional context used
🪛 GitHub Actions: Rust Formatting
livekit-api/src/services/egress.rs
[error] 112-114: cargo fmt -- --check failed: formatting inconsistency detected. Expected style change in egress.rs around the audio_mixing line.
⏰ 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). (14)
- GitHub Check: Test (x86_64-unknown-linux-gnu)
- GitHub Check: Test (x86_64-pc-windows-msvc)
- GitHub Check: Test (x86_64-apple-darwin)
- GitHub Check: Build (aarch64-apple-ios-sim)
- GitHub Check: Build (aarch64-linux-android)
- GitHub Check: Build (x86_64-apple-darwin)
- GitHub Check: Build (aarch64-apple-darwin)
- GitHub Check: Build (armv7-linux-androideabi)
- GitHub Check: Build (x86_64-unknown-linux-gnu)
- GitHub Check: Build (aarch64-unknown-linux-gnu)
- GitHub Check: Build (x86_64-linux-android)
- GitHub Check: Build (x86_64-pc-windows-msvc)
- GitHub Check: Build (aarch64-apple-ios)
- GitHub Check: Build (aarch64-pc-windows-msvc)
🔇 Additional comments (1)
livekit-api/src/services/egress.rs (1)
27-28: LGTM!The new
audio_mixingfield is well-documented and appropriately typed asOption<proto::AudioMixing>. The default ofNonecombined with theunwrap_orusage ensures backward compatibility.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Hi @ZywaSahaj, thank you for your PR. Please integrate with main which fixes CI builds. |
|
Hi @ladvoc it's done 😄 |
There was a problem hiding this comment.
Thanks, @ZywaSahaj! To stay consistent with the reset of this module, I would recommend creating a public enum type to represent AudioMixing to keep the protocol level enum as an implementation detail. This also allows implementing default so we can drop the option.
Define public wrapper:
#[derive(Clone, Copy, Debug, Default)]
pub enum AudioMixing {
/// All users are mixed together.
#[default]
DefaultMixing,
/// Agent audio in the left channel, all other audio in the right channel.
DualChannelAgent,
/// Each new audio track alternates between left and right channels.
DualChannelAlternate,
}
impl Into<proto::AudioMixing> for AudioMixing {
fn into(self) -> proto::AudioMixing {
match self {
AudioMixing::DefaultMixing => proto::AudioMixing::DefaultMixing,
AudioMixing::DualChannelAgent => proto::AudioMixing::DualChannelAgent,
AudioMixing::DualChannelAlternate => proto::AudioMixing::DualChannelAlternate,
}
}
}Use into implementation to convert to protocol equivalent
// ...
audio_only: options.audio_only,
audio_mixing: Into::<proto::AudioMixing>::into(options.audio_mixing) as i32,
video_only: options.video_only,
// ...Use in room options without option:
#[derive(Default, Clone, Debug)]
pub struct RoomCompositeOptions {
pub layout: String,
pub encoding: encoding::EncodingOptions,
pub audio_only: bool,
pub video_only: bool,
pub custom_base_url: String,
/// Only applies when audio_only is true (default: DefaultMixing)
pub audio_mixing: AudioMixing,
}
|
Yup, makes sense. fairly new to rust, thanks for the detailed code 😄 |
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.