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.

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

Add with_error_callback to OutputStreamBuilder #708

Merged
merged 7 commits into from
Mar 5, 2025

Conversation

will3942
Copy link
Contributor

Allows a user to handle a cpal error, allowing them to optionally recreate the stream if desired or take another action (such as relaunching the app).

I wasn't able to do this without using Box - happy to take any suggestions if you can think of a better implementation as I only have very nascent Rust knowledge.

Closes #465

@dvdsk
Copy link
Collaborator

dvdsk commented Feb 28, 2025

I wasn't able to do this without using Box - happy to take any suggestions if you can think of a better implementation as I only have very nascent Rust knowledge.

make the builder generic over the error callback (you will need to use OutputStreamBuilder<ErrorCallback> instead of Self<ErrorCallback in with_error_callback). There should be no need to make OutputStream generic.

@will3942
Copy link
Contributor Author

I wasn't able to do this without using Box - happy to take any suggestions if you can think of a better implementation as I only have very nascent Rust knowledge.

make the builder generic over the error callback (you will need to use OutputStreamBuilder<ErrorCallback> instead of Self<ErrorCallback in with_error_callback). There should be no need to make OutputStream generic.

I tried that for a while but was unable to get it working with a fallback to the default error callback but will try again next week.

@PetrGlad
Copy link
Collaborator

PetrGlad commented Mar 2, 2025

I think a dyn Fn makes sense here, the callback is not performance-critical.

The problem with opening default stream is that it may try to open stream multiple times. At the same time the call back gets owned at each attempt. See also my comment in open_stream_or_fallback.

@dvdsk
Copy link
Collaborator

dvdsk commented Mar 3, 2025

I think a dyn Fn makes sense here, the callback is not performance-critical.

I worry about the underflow error (when the source can not deliver samples fast enough to the OS). The a slow error handler could make the problem worse, a single underflow error could cause many more. I do not know if the delay caused by dyn is severe enough to cause that.

@will3942
Copy link
Contributor Author

will3942 commented Mar 4, 2025

I think a dyn Fn makes sense here, the callback is not performance-critical.

I worry about the underflow error (when the source can not deliver samples fast enough to the OS). The a slow error handler could make the problem worse, a single underflow error could cause many more. I do not know if the delay caused by dyn is severe enough to cause that.

@dvdsk I have now implemented this using generics instead without dyn dispatch. Let me know your feedback.

Copy link
Collaborator

@dvdsk dvdsk left a comment

Choose a reason for hiding this comment

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

Looks great, if you want to you could make the example a little more in-depth (see comment) but this is good to merge.

@will3942
Copy link
Contributor Author

will3942 commented Mar 5, 2025

Looks great, if you want to you could make the example a little more in-depth (see comment) but this is good to merge.

All done - feel free to merge once you're happy!

@dvdsk
Copy link
Collaborator

dvdsk commented Mar 5, 2025

O.o. CI is broken it seems....

well I'll see if I can fix that in a bit. PR looks great, ill try to get CI sorted and then merge this.

@will3942
Copy link
Contributor Author

will3942 commented Mar 5, 2025

O.o. CI is broken it seems....

well I'll see if I can fix that in a bit. PR looks great, ill try to get CI sorted and then merge this.

CI is all fixed - I broke it by forgetting a cargo fmt. Thanks for your help on this PR!

@dvdsk dvdsk merged commit 4cc1a57 into RustAudio:master Mar 5, 2025
17 of 18 checks passed
@PetrGlad
Copy link
Collaborator

PetrGlad commented Mar 9, 2025

I wonder if anyone can simulate/test underflow error. I could not. On my system no matter how slow the source is pipewire/ALSA does not complain (at least not through CPAL).

@PetrGlad
Copy link
Collaborator

PetrGlad commented Mar 9, 2025

On my system (Linux+ALSA+Pipewire). When I stop audio server I get BackendSpecificError instead.
Generally, I do not believe there are any recoverable errors that we get in the callback.

@will3942
Copy link
Contributor Author

will3942 commented Mar 9, 2025

On my system (Linux+ALSA+Pipewire). When I stop audio server I get BackendSpecificError instead.

Generally, I do not believe there are any recoverable errors that we get in the callback.

On CoreAudio/Mac we get the correct error and can destroy & recreate the output stream with a different device.

@PetrGlad
Copy link
Collaborator

PetrGlad commented Mar 9, 2025

@will3942 Are there any recoverable errors you get from this callback?

@will3942
Copy link
Contributor Author

will3942 commented Mar 9, 2025

@will3942 Are there any recoverable errors you get from this callback?

How do you define recoverable? Recover without having to tear down and destroy the resources?

@roderickvd
Copy link
Collaborator

I wonder if anyone can simulate/test underflow error. I could not. On my system no matter how slow the source is pipewire/ALSA does not complain (at least not through CPAL).

One way to do it is to use an RPi with SD card and do a lot of I/O on it, writes in particular.

@PetrGlad
Copy link
Collaborator

@will3942 Yes, by "recoverable" here I mean being able to proceed without re-initializing the stream.

@PetrGlad
Copy link
Collaborator

@roderickvd The output stream can not know where delay comes from. So I believe that just artificially stalling the sample callback should have the same effect.

@will3942
Copy link
Contributor Author

@will3942 Yes, by "recoverable" here I mean being able to proceed without re-initializing the stream.

Not that I'm aware of, no.

In my application code I will be using this to remove and recreate the OutputStream, checking if the device is still available. It would be nice if Rodio could optionally provide this functionality out of the box but it feels like there's quite a few toggles/options/timeouts you'd want to provide the user in determining how that logic is provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow user to get to cpal errors such as device unplugged.
4 participants