Skip to content
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

Do not use cpal errors when rodio runs into an issue #703

Merged
merged 2 commits into from
Feb 18, 2025
Merged

Do not use cpal errors when rodio runs into an issue #703

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c8d554
Do not use cpal errors when rodio runs into an issue
dvdsk Feb 17, 2025
17692f5
rename
dvdsk Feb 17, 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
26 changes: 15 additions & 11 deletions src/stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,9 @@ pub enum StreamError {
SupportedStreamConfigsError(cpal::SupportedStreamConfigsError),
/// Could not find any output device
NoDevice,
/// New cpal sample format that rodio does not yet support please open
/// an issue if you run into this.
UnknownSampleFormat,
}

impl fmt::Display for StreamError {
Expand All @@ -299,6 +302,7 @@ impl fmt::Display for StreamError {
Self::DefaultStreamConfigError(e) => e.fmt(f),
Self::SupportedStreamConfigsError(e) => e.fmt(f),
Self::NoDevice => write!(f, "NoDevice"),
Self::UnknownSampleFormat => write!(f, "UnknownSampleFormat"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe UnsupportedSampleFormat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UnexpectedSampleFormat? Since its pretty strange if it happens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It sounds like we expected something else instead... "Unknown" in this situation is only unknown to rodio but not cpal :) I like "unsupported" the best here but "unexpected" would also do.

}
}
}
Expand All @@ -311,6 +315,7 @@ impl error::Error for StreamError {
Self::DefaultStreamConfigError(e) => Some(e),
Self::SupportedStreamConfigsError(e) => Some(e),
Self::NoDevice => None,
Self::UnknownSampleFormat => None,
}
}
}
Expand All @@ -321,30 +326,28 @@ impl OutputStream {
config: &OutputStreamConfig,
) -> Result<OutputStream, StreamError> {
let (controller, source) = mixer(config.channel_count, config.sample_rate);
Self::init_stream(device, config, source)
.map_err(StreamError::BuildStreamError)
.and_then(|stream| {
stream.play().map_err(StreamError::PlayStreamError)?;
Ok(Self {
_stream: stream,
mixer: controller,
})
Self::init_stream(device, config, source).and_then(|stream| {
stream.play().map_err(StreamError::PlayStreamError)?;
Ok(Self {
_stream: stream,
mixer: controller,
})
})
}

fn init_stream(
device: &cpal::Device,
config: &OutputStreamConfig,
mut samples: MixerSource<f32>,
) -> Result<cpal::Stream, cpal::BuildStreamError> {
) -> Result<cpal::Stream, StreamError> {
let error_callback = |err| {
#[cfg(feature = "tracing")]
tracing::error!("error initializing output stream: {err}");
#[cfg(not(feature = "tracing"))]
eprintln!("error initializing output stream: {err}");
};
let sample_format = config.sample_format;
let config = config.into();
let config: cpal::StreamConfig = config.into();
match sample_format {
cpal::SampleFormat::F32 => device.build_output_stream::<f32, _, _>(
&config,
Expand Down Expand Up @@ -452,8 +455,9 @@ impl OutputStream {
error_callback,
None,
),
_ => Err(cpal::BuildStreamError::StreamConfigNotSupported),
_ => return Err(StreamError::UnknownSampleFormat),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can only happen with incompatible CPAL version. I wonder why they decided to mark cpal sample format enum as non_exhaustive. This could probably panic instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah had the same thought. But decided they must have done it for a reason. One of their examples (beep) has some commented out sample formats, so they might add those in the future (24bit for example)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason I do not understand this is that cargo compiles both rodio and cpal from sources every time, so it can check that all match branches are there. Maybe there are some scenarios where pre-compiled Rust libraries are used, but I have not seen such projects so far.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I recommend reading the RFC for non_exhaustive, it goes through all the reasons for using it. https://rust-lang.github.io/rfcs/2008-non-exhaustive.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and cpal is adding 24bit support to this enum it seems. Its non breaking because they have non_exhaustive on the enum. RustAudio/cpal#927

}
.map_err(StreamError::BuildStreamError)
}
}

Expand Down