fix: flush output buffers before pause#1174
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the ALSA host implementation to ensure output buffers are flushed with silence before pausing, preventing stale audio from playing after resuming (fixes #807). It also refactors ALSA stream parameter handling to rely on negotiated period/buffer values and adjusts thread-priority promotion logic.
Changes:
- Add an ALSA output-stream pause state machine to flush one-or-more silent periods before pausing the PCM handle.
- Refactor stream creation to use negotiated
get_params()results (period/buffer sizes), and renamechanneltohandle. - Update changelog entry for the ALSA pause behavior change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/host/alsa/mod.rs |
Implements flushing-before-pause, refactors period/buffer handling, and updates resume/priority logic. |
CHANGELOG.md |
Documents the ALSA pause flush behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // if the buffer size isn't known, let audio_thread_priority choose a sensible default value | ||
| let (buffer_size, _) = handle.get_params().unwrap_or((0, 0)); | ||
| let sample_rate = match handle | ||
| .hw_params_current() | ||
| .and_then(|params| params.get_rate()) | ||
| { | ||
| Ok(rate) => rate, | ||
| Err(err) => { | ||
| eprintln!("Failed to get current hardware parameters for audio thread priority: {err}"); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| if let Err(err) = promote_current_thread_to_real_time(buffer_size, sample_rate) { | ||
| if let Err(err) = promote_current_thread_to_real_time(buffer_size as u32, sample_rate) { | ||
| eprintln!("Failed to promote audio thread to real-time priority: {err}"); |
There was a problem hiding this comment.
boost_current_thread_priority declares hw_params but then calls promote_current_thread_to_real_time(buffer_size, sample_rate) where sample_rate is not defined. This won’t compile and also drops the negotiated sample rate value. Use the rate returned from get_rate() (and convert to SampleRate/u32 as required) and ensure buffer_size is converted to the type expected by promote_current_thread_to_real_time.
| if let Err(err) = result { | ||
| match err.kind() { | ||
| ErrorKind::Xrun => { | ||
| error_callback(err); | ||
| if let Err(err) = stream.channel.prepare() { | ||
| if let Err(err) = stream.handle.prepare() { | ||
| error_callback(err.into()); | ||
| } | ||
| // No need to call start() for output streams after prepare(); |
There was a problem hiding this comment.
try_resume() now reports lack of resume support as ErrorKind::UnsupportedOperation, but output_stream_worker only performs recovery on ErrorKind::Xrun. This means an ESTRPIPE on output may never fall back to prepare(), leaving the stream stuck. Handle UnsupportedOperation the same way as Xrun here (prepare/recover), or translate it back to Xrun in the resume path for output streams.
| } else { | ||
| stream | ||
| .pause_state | ||
| .compare_exchange( | ||
| pause_state.into(), | ||
| PauseState::Flushing { | ||
| remaining: remaining - 1, | ||
| } | ||
| .into(), | ||
| Ordering::Relaxed, | ||
| Ordering::Relaxed, | ||
| ) | ||
| .expect("PauseState::Flushing should only be modified by the worker thread"); |
There was a problem hiding this comment.
Race condition: while flushing (remaining > 0), another thread can call play() and swap pause_state to NotPaused. The worker then hits .compare_exchange(...).expect(...) and will panic, crashing the audio thread. Don’t expect here—handle a failed CAS by reloading the state (or by using a loop / fetch_update) and treating it as a cancelled pause/flush.
| } else { | |
| stream | |
| .pause_state | |
| .compare_exchange( | |
| pause_state.into(), | |
| PauseState::Flushing { | |
| remaining: remaining - 1, | |
| } | |
| .into(), | |
| Ordering::Relaxed, | |
| Ordering::Relaxed, | |
| ) | |
| .expect("PauseState::Flushing should only be modified by the worker thread"); | |
| } else if stream | |
| .pause_state | |
| .compare_exchange( | |
| pause_state.into(), | |
| PauseState::Flushing { | |
| remaining: remaining - 1, | |
| } | |
| .into(), | |
| Ordering::Relaxed, | |
| Ordering::Relaxed, | |
| ) | |
| .is_err() | |
| { | |
| // The state was changed concurrently (for example by `play()` restoring | |
| // `NotPaused`), so treat this as a cancelled pause/flush instead of panicking. |
| let hw_params = self.inner.handle.hw_params_current()?; | ||
| if !hw_params.can_pause() { | ||
| return Err(Error::with_message( | ||
| ErrorKind::UnsupportedOperation, | ||
| "hardware does not support pausing this stream", | ||
| )); | ||
| } | ||
| // Continue when already paused or flushing. | ||
| let _ = self.inner.pause_state.compare_exchange( | ||
| PauseState::NotPaused.into(), | ||
| PauseState::Flushing { | ||
| remaining: hw_params.get_periods().unwrap_or(DEFAULT_PERIODS as u32), | ||
| } | ||
| .into(), | ||
| Ordering::Relaxed, | ||
| Ordering::Relaxed, | ||
| ); |
There was a problem hiding this comment.
StreamTrait::pause() now only transitions the pause_state used by the output worker. For capture streams, process_input never consults pause_state, so pause() no longer pauses capture at all (regression vs the previous channel.pause(true)). Detect capture vs playback (e.g., via handle.info()?.get_stream() or by storing direction in StreamInner) and pause capture streams directly.
| let _ = self.inner.pause_state.compare_exchange( | ||
| PauseState::NotPaused.into(), | ||
| PauseState::Flushing { | ||
| remaining: hw_params.get_periods().unwrap_or(DEFAULT_PERIODS as u32), |
There was a problem hiding this comment.
The flushing counter appears off by one: pause() initializes remaining to the period count, but process_output only pauses hardware when it observes remaining == 0 before decrementing, resulting in one extra full silent period written before pausing. If the intent is to flush exactly N periods, initialize remaining to N-1 (saturating) or change the decrement/check logic accordingly.
| remaining: hw_params.get_periods().unwrap_or(DEFAULT_PERIODS as u32), | |
| remaining: hw_params | |
| .get_periods() | |
| .unwrap_or(DEFAULT_PERIODS as u32) | |
| .saturating_sub(1), |
|
Fun experiment but wrong semantics. See #284. |
This is an interesting exercise to flush the output buffers before pausing, so that no stale audio is played when unpausing, that's robust across period counts and free from race conditions when users rapidly fire pause-play-pause-play.
Some opportunistic fixes and refactoring while there:
I'll see to ASIO next.
Fixes #807