-
Notifications
You must be signed in to change notification settings - Fork 465
ALSA PCM.try_recover: error handling #909
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.
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
Conversation
Ralith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to manage the sound glitches somehow. But that's probably on me for not debouncing
It's probably rodio's fault.
src/host/alsa/mod.rs
Outdated
| if let Err(err) = stream.channel.try_recover(err, true) { | ||
| let description = format!( | ||
| "ALSA EPIPE error. Tried to recover, but failed! Error was: {:?}", | ||
| err, | ||
| ); | ||
| error_callback(BackendSpecificError { description }.into()); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not real-time-safe, and doesn't seem to be related to the core intent of this PR. Maybe just drop it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review ❤️ and change applied! I force-pushed in my branch to keep the PR clean — just 1 commit.
I'm curious (and ignorant), though: what do you mean by not rela-time safe, exactly? Is calling the error_callback risking consuming a significant amount of CPU, such that it'd impact playback? More so than the other 2 branches that already do call error_callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling malloc as you do by invoking format risks blocking the thread for an unpredictable duration, since it might e.g. acquire a lock or invoke mmap internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Makes sense. Thanks. Not relevant to this PR, but I guess the best way around that (even if not particularly ergonomic) would be error_callback(err_code: usize), if we did support that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regular value types, like intelligible enum variants, are fine. Perhaps cpal shouldn't have that description: String field at all, though I expect it's currently mostly used in places where recovery is hopeless anyway.
714ffd7 to
23f2c73
Compare
23f2c73 to
72e930a
Compare
| // TODO: | ||
| // Should we notify the user about successfully recovered errors? | ||
| // Should we notify the user about failures in try_recover, rather than ignoring them? | ||
| // (Both potentially not real-time-safe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the error callback is safe for our purposes. It's the user's responsibility to do real-time-safe things in their callbacks, or accept the risk of failing to.
|
@est31 I hope this isn't rude of me — so sorry of it is! But can you review/merge this, please? 🙏 It's a super small change that adds a lot of value, at least in my use case, and possibly to all ratatui apps that use cpal. |
# Conflicts: # CHANGELOG.md
|
@Ralith sorry to bother you but I'd love to have this one merged, if possible. Can you help me with that? I'm not sure who could review/merge this one, or whether there are some other changes I should address in this PR before it's mergeable. |
|
It's up to @est31, I'm afraid. |
|
Got it. Thank you 🙏 |
|
hi, can you resolve the conflicts? |
|
Sorry for the long wait, I don't have as much time as I used to. |
Done!
No worries. Thank you for reviewing this, and for all the work you do for RustAudio, the let_chains standard and open source in general 🙏 |
When passing
silent=falseto PCM.try_recover, it'll write tostderra detail of the error.In this case, it's always
underrun occurredoroverrun occurred. It logs this even when successfully recovering. This is the relevant code in ALSA's GitHub mirror, as of now:By setting it to silent, we lose this detail (whether it was an under- or over-run). But letting it write directly to stderr is messing up my ratatui app :( and I don't think there's a standard, cross-platform, safe way to prevent anything from writing to std(out|err) in-process.
Failures in
try_handlewill now callerror_callback, but these noisyunderrun occurredalways recovered successfully, in my case.I guess we could also offer some sort of
#[cfg(feature = "alsa-silent-try_recover")]or adding a run-time arg to the function chain all the way up tobuild_output_streamand then Rodio'stry_from_device, or add it as an option instruct StreamConfig. Personally, I'd prefer avoiding the added code complexity of the run-time option, but I could see some value in the build-time option, if properly documented.Also, calling the error callback will wind up doing a
eprintlnthat wasn't happening previously (in rodio, at least), so we're trading onestderroutput for another. That's something to manage in Rodio, but still worth mentioning.For context, I can consistently trigger these by calling Rodio's
sink.try_seektoo quickly (as fast as crossterm'sevent::pollwill trigger it, while I hold down a key I'm mapping toseekin my app).Silencing this error gets rid of the visual noise/glitches, but I still need to manage the sound glitches somehow. But that's probably on me for not debouncing calls to
seekand/or usingOutputStream::try_default()and not setting anything manually inSupportedStreamConfig(buffer_size, I guess).