ALSA PCM.try_recover: error handling

[lautarodragan]

When passing silent=false to PCM.try_recover, it'll write to stderr a detail of the error.

In this case, it's always underrun occurred or overrun occurred. It logs this even when successfully recovering. This is the relevant code in ALSA's GitHub mirror, as of now:

        if (err == -EPIPE) {
                const char *s;
                if (snd_pcm_stream(pcm) == SND_PCM_STREAM_PLAYBACK)
                        s = "underrun";
                else
                        s = "overrun";
                if (!silent)
                        SNDERR("%s occurred", s);
                err = snd_pcm_prepare(pcm);
                if (err < 0) {
                        SNDERR("cannot recovery from %s, prepare failed: %s", s, snd_strerror(err));
                        return err;
                }
                return 0;
        }

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_handle will now call error_callback, but these noisy underrun occurred always 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 to build_output_stream and then Rodio's try_from_device, or add it as an option in struct 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 eprintln that wasn't happening previously (in rodio, at least), so we're trading one stderr output 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_seek too quickly (as fast as crossterm's event::poll will trigger it, while I hold down a key I'm mapping to seek in 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 seek and/or using OutputStream::try_default() and not setting anything manually in SupportedStreamConfig (buffer_size, I guess).

[Ralith]

I still need to manage the sound glitches somehow. But that's probably on me for not debouncing

It's probably rodio's fault.

[Ralith]

This is not real-time-safe, and doesn't seem to be related to the core intent of this PR. Maybe just drop it?

[lautarodragan]

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?

[Ralith]

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.

[lautarodragan]

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?

[Ralith]

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.

[Ralith]

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.

[lautarodragan]

@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.

[lautarodragan]

@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.

[Ralith]

It's up to @est31, I'm afraid.

[lautarodragan]

Got it. Thank you 🙏