-
Notifications
You must be signed in to change notification settings - Fork 465
ASIO: Handle kAsioResetRequest to fix unresponsive driver changes. #1058
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
roderickvd
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.
Thanks for the PR. Some thoughts in the review.
| }; | ||
| // Release lock and call them. | ||
| for cb in callbacks { | ||
| cb(AsioMessageSelectors::kAsioResetRequest); |
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.
The ASIO SDK documentation states: "You cannot reset the driver right now, as this code is called from the driver." - we might want to document that somewhere.
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.
Added in docs for the new StreamError variant.
src/host/asio/stream.rs
Outdated
| if let Ok(mut cb) = error_callback_shared.lock() { | ||
| cb(StreamError::BackendSpecific { | ||
| err: BackendSpecificError { | ||
| description: "ASIO reset request.".to_string(), |
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 think it's OK to add a backend-specific StreamError variant. Next version is going to be v0.17 anyway, so breaking changes are acceptable.
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 did that, unsure if there should be a cfg[feature = "asio"] on that variant specifically, might be more trouble than it's worth, since you can just handle the _ => {} case. I did mark it non-exhaustive, since it is likely there will be more variants added.
|
Having given it some more thought, I think we should call it /// The stream configuration is no longer valid and must be rebuilt.
/// This occurs when the backend requires a full reset:
/// - ASIO: Driver reset request (sample rate/format change via control panel)
/// - JACK: Sample rate changed by server
/// - Other backends: Format/configuration invalidated
StreamInvalidated, |
Fixes #1007.
This PR implements handling for the kAsioResetRequest message from the ASIO driver. This message is sent by drivers (e.g., ASIO4ALL, or when changing sample rates via a control panel) to signal that the stream must be destroyed and recreated. Currently, cpal ignores this, causing the UI/Driver to become unresponsive.
I only handle
kAsioResetRequest. We could handle more, but this is the only one strictly necessary to fix the issue. It's the user's responsibility to handle it and rebuild the whole stream.It sends a
BackendSpecificErrorwith a string description. It's a bit awkward for the user to match against a string, but I didn't want to touch the cross-platformStreamErrorenum and break the API for everyone else.