fix(alsa): hanging double-opens with BufferSize::Fixed om some drivers#1214
Open
roderickvd wants to merge 3 commits into
Open
fix(alsa): hanging double-opens with BufferSize::Fixed om some drivers#1214roderickvd wants to merge 3 commits into
roderickvd wants to merge 3 commits into
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.
roderickvd
changed the title
|
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. |
Comment on lines
1554
to
+1563
| if let BufferSize::Fixed(buffer_frames) = config.buffer_size { | ||
| hw_params.set_buffer_size_near(DEFAULT_PERIODS * buffer_frames as alsa::pcm::Frames)?; | ||
| hw_params | ||
| .set_period_size_near(buffer_frames as alsa::pcm::Frames, alsa::ValueOr::Nearest)?; | ||
| let buffer_frames = buffer_frames as alsa::pcm::Frames; | ||
|
|
||
| // Validate the requested size against the device's supported ranges using the same PCM | ||
| // handle we'll use for streaming. This avoids a second PCM open (which can disturb | ||
| // hardware clock state on some drivers) while still catching wildly out-of-range | ||
| // requests before set_period_size_near silently rounds them. | ||
| if let Some((min_period, max_period)) = hw_params_period_size_min_max(&hw_params) { | ||
| if !(min_period..=max_period).contains(&buffer_frames) { | ||
| return Err(Error::with_message( |
| - **ALSA**: Fix stream corruption on certain drivers with spurious wakeups. | ||
| - **ALSA**: Fix callbacks firing before `build_*_stream` returns the `Stream` handle. | ||
| - **ALSA**: Fix `BufferSize::Fixed` validation opening the PCM device a second time. | ||
| - **ALSA**: Fix hang on when device raced to an error state without delivering POLLERR. |
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