Skip to content

fix: set sensible cvolume default #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
OpatrilPeter wants to merge 3 commits into from
Closed

fix: set sensible cvolume default #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
136fa6a
fix: set sensible cvolume default
OpatrilPeter Nov 8, 2025
7aaec49
refactor: unify types for channel count
OpatrilPeter Nov 13, 2025
162d68e
tests: add tests for playback/record stream channel invariants
OpatrilPeter Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion src/protocol/command/playback_stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ pub struct PlaybackStreamParams {
pub sample_spec: SampleSpec,

/// Channel map for the stream.
///
/// Number of channels should match `sample_spec.channels`.
pub channel_map: ChannelMap,

/// Index of the sink to connect to.
Expand All @@ -28,6 +30,8 @@ pub struct PlaybackStreamParams {
pub sync_id: u32,

/// Volume of the stream.
///
/// Number of channels should match `sample_spec.channels`.
pub cvolume: Option<ChannelVolume>,

/// Additional properties for the stream.
Expand Down Expand Up @@ -144,7 +148,10 @@ impl TagStructWrite for PlaybackStreamParams {
ts.write_u32(self.buffer_attr.pre_buffering)?;
ts.write_u32(self.buffer_attr.minimum_request_length)?;
ts.write_u32(self.sync_id)?;
ts.write(self.cvolume.unwrap_or_default())?;
ts.write(
self.cvolume
.unwrap_or_else(|| ChannelVolume::muted(self.sample_spec.channels.into())),
)?;
ts.write_bool(self.flags.no_remap_channels)?;
ts.write_bool(self.flags.no_remix_channels)?;
ts.write_bool(self.flags.fix_format)?;
Expand Down Expand Up @@ -367,4 +374,61 @@ mod integration_tests {

Ok(())
}

/// Tests that PlaybackStreamParams maintains consistent
/// channel counts across its implicitly set fields.
#[test]
fn create_playback_stream_channel_count_invariants() -> anyhow::Result<()> {
let (mut sock, protocol_version) = connect_and_init()?;

// Arbitrarily chosen number of channels that should be kept in sync
// across fields (chosen to test beyond the usual 1 or 2).
const CHANNEL_COUNT: u8 = 3;

// Explicitly set case (for reference).
{
write_command_message(
sock.get_mut(),
0,
&Command::CreatePlaybackStream(PlaybackStreamParams {
sample_spec: SampleSpec {
format: SampleFormat::S16Le,
channels: CHANNEL_COUNT,
..Default::default()
},
sync_id: 0,
channel_map: ChannelMap::with_n_channels(CHANNEL_COUNT),
cvolume: Some(ChannelVolume::norm(CHANNEL_COUNT)),
..Default::default()
}),
protocol_version,
)?;

let _ = read_reply_message::<CreatePlaybackStreamReply>(&mut sock, protocol_version)?;
}

// Implicitly set case (on fields that allow it).
{
write_command_message(
sock.get_mut(),
1,
&Command::CreatePlaybackStream(PlaybackStreamParams {
sample_spec: SampleSpec {
format: SampleFormat::S16Le,
channels: CHANNEL_COUNT,
..Default::default()
},
sync_id: 1,
channel_map: ChannelMap::with_n_channels(CHANNEL_COUNT),
cvolume: None,
..Default::default()
}),
protocol_version,
)?;

let _ = read_reply_message::<CreatePlaybackStreamReply>(&mut sock, protocol_version)?;
}

Ok(())
}
}
104 changes: 103 additions & 1 deletion src/protocol/command/record_stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ pub struct RecordStreamParams {
pub sample_spec: SampleSpec,

/// Channel map for the stream.
///
/// Number of channels should match `sample_spec.channels`.
pub channel_map: ChannelMap,

/// Index of the source to connect to.
Expand All @@ -32,6 +34,8 @@ pub struct RecordStreamParams {
pub direct_on_input_index: Option<u32>,

/// Volume of the stream.
///
/// Number of channels should match `sample_spec.channels`.
pub cvolume: Option<ChannelVolume>,

/// Additional properties for the stream.
Expand Down Expand Up @@ -163,7 +167,10 @@ impl TagStructWrite for RecordStreamParams {
ts.write(format)?;
}

ts.write(self.cvolume.unwrap_or_default())?;
ts.write(
self.cvolume
.unwrap_or_else(|| ChannelVolume::muted(self.sample_spec.channels.into())),
)?;
ts.write_bool(self.flags.start_muted.unwrap_or_default())?;
ts.write_bool(self.cvolume.is_some())?;
ts.write_bool(self.flags.start_muted.is_some())?;
Expand Down Expand Up @@ -301,3 +308,98 @@ mod tests {
test_serde(&reply)
}
}

#[cfg(test)]
#[cfg(feature = "_integration-tests")]
mod integration_tests {
use super::*;
use crate::integration_test_util::*;
use crate::protocol::*;

#[test]
fn create_playback_stream() -> anyhow::Result<()> {
let (mut sock, protocol_version) = connect_and_init()?;

write_command_message(
sock.get_mut(),
0,
&Command::CreatePlaybackStream(PlaybackStreamParams {
sample_spec: SampleSpec {
format: SampleFormat::S16Le,
sample_rate: 44100,
channels: 2,
},
channel_map: ChannelMap::stereo(),
cvolume: Some(ChannelVolume::norm(2)),
flags: StreamFlags {
start_corked: true,
start_muted: Some(true),
..Default::default()
},
sink_index: None,
sink_name: Some(CString::new("@DEFAULT_SINK@")?),
..Default::default()
}),
protocol_version,
)?;

let _ = read_reply_message::<CreatePlaybackStreamReply>(&mut sock, protocol_version)?;

Ok(())
}

/// Tests that RecordStreamParams maintains consistent
/// channel counts across its implicitly set fields.
#[test]
fn create_record_stream_channel_count_invariants() -> anyhow::Result<()> {
let (mut sock, protocol_version) = connect_and_init()?;

// Arbitrarily chosen number of channels that should be kept in sync
// across fields (chosen to test beyond the usual 1 or 2).
const CHANNEL_COUNT: u8 = 3;

// Explicitly set case (for reference).
{
write_command_message(
sock.get_mut(),
0,
&Command::CreateRecordStream(RecordStreamParams {
sample_spec: SampleSpec {
format: SampleFormat::S16Le,
channels: CHANNEL_COUNT,
..Default::default()
},
channel_map: ChannelMap::with_n_channels(CHANNEL_COUNT),
cvolume: Some(ChannelVolume::norm(CHANNEL_COUNT)),
..Default::default()
}),
protocol_version,
)?;

let _ = read_reply_message::<CreateRecordStreamReply>(&mut sock, protocol_version)?;
}

// Implicitly set case (on fields that allow it).
{
write_command_message(
sock.get_mut(),
1,
&Command::CreateRecordStream(RecordStreamParams {
sample_spec: SampleSpec {
format: SampleFormat::S16Le,
channels: CHANNEL_COUNT,
..Default::default()
},
channel_map: ChannelMap::with_n_channels(CHANNEL_COUNT),
cvolume: None,
..Default::default()
}),
protocol_version,
)?;

let _ = read_reply_message::<CreateRecordStreamReply>(&mut sock, protocol_version)?;
}

Ok(())
}
}
15 changes: 15 additions & 0 deletions src/protocol/serde/channel_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,21 @@ impl ChannelMap {
map
}

/// Creates a channel map with N (arbitrarily chosen) channels.
///
/// Intended for testing cases where only the number of channels matters.
#[cfg(test)]
#[cfg(feature = "_integration-tests")]
pub(crate) fn with_n_channels(channels: u8) -> Self {
Copy link
Owner

@colinmarc colinmarc Nov 14, 2025

Choose a reason for hiding this comment

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

Let's just inline this in the tests (or add a function inside the #[cfg(test)] block)

Copy link
Author

@OpatrilPeter OpatrilPeter Nov 14, 2025

Choose a reason for hiding this comment

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

Since it's already used in two modules and is in theory useful in any kind of test where you'd want arbitrary ChannelMap in a command (as the channel count must presumably align in any usecase), I think it's better left with the class for now. I added the conditionality, there should be no unused warnings now.

let mut map = ChannelMap::empty();
for pos in
(1..=channels).map(|idx| ChannelPosition::from_u8(idx).expect("known channel index"))
{
map.push(pos);
}
map
}

/// Tries to append another `ChannelPosition` to the end of this map.
///
/// Panics if the map already has MAX_CHANNELS channels.
Expand Down
8 changes: 4 additions & 4 deletions src/protocol/serde/volume.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,17 +136,17 @@ impl ChannelVolume {
}

/// Create a `ChannelVolume` with N channels, all muted.
pub fn muted(channels: usize) -> ChannelVolume {
pub fn muted(channels: u8) -> ChannelVolume {
Self {
channels: channels as u8,
channels,
volumes: [Volume::MUTED; MAX_CHANNELS as usize],
}
}

/// Create a `ChannelVolume` with N channels, all at full volume.
pub fn norm(channels: usize) -> ChannelVolume {
pub fn norm(channels: u8) -> ChannelVolume {
Self {
channels: channels as u8,
channels,
volumes: [Volume::NORM; MAX_CHANNELS as usize],
}
}
Expand Down