Repository navigation
Conversation
MicrophoneBuilder carries an error callback but has no way to set one, so every microphone prints each stream error to stderr. An application that wants to log them its own way, rate-limit them, or close a stream that keeps failing cannot. Add with_error_callback, as DeviceSinkBuilder has.
| let builder = MicrophoneBuilder::new().with_error_callback(move |_| { | ||
| counted.fetch_add(1, Ordering::Relaxed); | ||
| }); | ||
| let mut callback = builder.error_callback.clone(); |
There was a problem hiding this comment.
I don't think this test is particularly helpful, it's not really testing anything just moving a callback into and out of rodio. I'm okay with scrapping it unless you can figure out a good way to cause a cpal failure and observe the results (everywhere this runs and in CI).
There was a problem hiding this comment.
Removed. I don't have a way to make cpal fail that would work everywhere this runs, CI included, so there's nothing to replace it with. The doc examples still get compiled.
| /// ```no_run | ||
| /// # use rodio::microphone::MicrophoneBuilder; | ||
| /// let builder = MicrophoneBuilder::new() | ||
| /// .with_error_callback(|err| eprintln!("microphone: {err}")); |
There was a problem hiding this comment.
Right now this example just implements the default error handling. Can you think of something closer to what a user would want? Maybe store the last error in a global or shove it into an mpsc? I haven't thought about this but it seems you have since you've opened this PR :3
There was a problem hiding this comment.
Changed it to an mpsc. The callback forwards DeviceNotAvailable and StreamInvalidated to the application, which drops the stream and reopens it. The channel is a sync_channel(1) with try_send, because a failing stream can report the same error thousands of times a second (that's how we ran into this) and the callback shouldn't block or pile them up. The filter is the same one as examples/error_callback.rs.
|
Thanks for the PR a nice addition that fits the builder well, think we planned to do this at some point but it slipped through.
Would be most welcome! |
The example only repeated the default, printing to stderr. Send the errors that need action through a bounded channel instead, which is what an application reopening the stream would do. Remove the test: it moved a callback into the builder and out again, with no cpal failure behind it.
SpeakersBuilder has the same gap as MicrophoneBuilder: it carries an error callback but has no way to set one. Add with_error_callback in the same form.
|
Added |
crmne
changed the title
MicrophoneBuilderandSpeakersBuildercarry an error callback type but have no way to set one, so every stream uses the default, which logs each stream error. An application that wants to log errors its own way, rate-limit them, or close a stream that keeps failing has no way in.This adds
with_error_callbackto both, matchingDeviceSinkBuilder::with_error_callback. It keeps the device and config already set and changes only the callback type. The doc examples show sending the errors that need action through a bounded channel. There is a CHANGELOG entry under Unreleased.We hit this in ZapFast (crmne/zapfast#507): a failing ALSA stream reported the same error tens of thousands of times a second, and the output side could be rate-limited while the microphone side could not.