fix(alsa): hanging double-opens with BufferSize::Fixed om some drivers#1214
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ALSA backend to avoid an extra open/close cycle when validating BufferSize::Fixed (which can leave some drivers in a bad state), and adds a defensive PCM-state check when poll() times out to prevent hangs when error events aren’t delivered.
Changes:
- Move
BufferSize::Fixedrange validation toset_hw_params_from_format()so it uses the same PCM handle as the streaming open (avoids a second open/close). - On
poll()timeout, query PCM state and surfaceDisconnected/XRun/Suspendedconditions even ifPOLLERR/POLLHUPwasn’t delivered.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
As I wrote in the issue there are no apparent changes for me, but it also does not negatively effect the current behavior on the rasperry pi zero w, so I would say its better than before. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/host/alsa/mod.rs:595
default_configtreatsErrorKind::UnsupportedConfigfromsupported_configs(stream_t)as “device does not support {dir}”, butUnsupportedConfigcan also mean other configuration/parameter failures. SinceDevicealready tracks direction viaself.direction/supports_input()/supports_output(), consider checking that first and returningErrorKind::UnsupportedOperationwithout relying on errno/kind mapping fromsnd_pcm_open(). This would also make behavior consistent forsupported_*_configs()callers on the wrong direction.
let mut formats: Vec<_> = {
match self.supported_configs(stream_t) {
// EINVAL when querying direction the device does not support (input-only or output-only)
Err(err) if err.kind() == ErrorKind::UnsupportedConfig => {
let dir = match stream_t {
alsa::Direction::Capture => "input",
alsa::Direction::Playback => "output",
};
return Err(Error::with_message(
ErrorKind::UnsupportedOperation,
format!("Device does not support {dir}"),
));
3c0b18b to
d6fe895
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The range validation for
BufferSize::Fixedwas callingdefault_output_config()before opening the stream, which opened and closed the PCM device once just to get the buffer size range. This may leave the driver in a bad state for the subsequent streaming open.Second, to defend against hanging processes in such an event, the worker now checks PCM state when
poll()times out.Fixes #1157