Do not use cpal errors when rodio runs into an issue #703
Conversation
This prevents a search for BuildStreamError::StreamConfigNotSupported pointing to rodio code. That puts you on the wrong leg making you think the issue is in rodio where it is in Cpal. This was the only case in rodio where we issue a cpal error.
src/stream.rs
Outdated
| @@ -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"), | |||
There was a problem hiding this comment.
Maybe UnsupportedSampleFormat?
There was a problem hiding this comment.
UnexpectedSampleFormat? Since its pretty strange if it happens.
There was a problem hiding this comment.
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.
src/stream.rs
Outdated
| @@ -452,8 +455,9 @@ impl OutputStream { | |||
| error_callback, | |||
| None, | |||
| ), | |||
| _ => Err(cpal::BuildStreamError::StreamConfigNotSupported), | |||
| _ => return Err(StreamError::UnknownSampleFormat), | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
I thought about pushing it to main directly 😅 you should have done that 😛 |
|
should be good, if it is please merge it 👍 |
This prevents a search for BuildStreamError::StreamConfigNotSupported pointing to rodio code. That puts you on the wrong leg making you think the issue is in rodio where it is in Cpal. This was the only case in rodio where we issue a cpal error.