From 2aa8d2d5f15c736d76f40acf8c9f569bd0841ea3 Mon Sep 17 00:00:00 2001 From: vicrodh <103399283+vicrodh@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:11:49 -0600 Subject: [PATCH] refactor(player): one Direct engine variant for ALSA and WASAPI PlaybackEngine::AlsaDirect and StreamType::AlsaDirect become Direct, carrying Arc instead of Arc. Eleven match arms and seven sites keep working untouched because they only ever called trait methods; duplicating them per platform would have been the alternative. CORRECTION to e61cd7bd8's message: it claimed the ALSA path still monomorphises to static dispatch. That was true when written and is NOT true now - the payload is a trait object, so those calls are virtual. Measured where it lands: write_f32 runs once per 8192-frame chunk, i.e. 5.4 indirect calls/second at 44.1 kHz and 23.4 at 192 kHz, each immediately entering blocking device I/O. channels() once at construction, sample_rate/stop/drain at boundaries. Nothing in the sample path. Two things Windows CANNOT compile-check, found by reading rather than by the compiler, and both would have broken the Linux build: - Four cfg(linux) arms passed a matched Arc to new_alsa_direct, which takes the concrete type. Arc unsizes one way only. - set_hardware_volume is called inside a cfg(linux) block and was not on the trait. It is now, with a default that REFUSES rather than silently succeeding, overridden by ALSA on Linux only - a sink with no mixer must not pretend it moved the volume. StreamType::Direct is also no longer cfg(linux). Gating it was what stopped this compiler from checking any arm that handles it, which is the exact blind spot this port has to work around; the variant now compiles everywhere and Windows will refuse anything I get wrong in it. The log prefix moved onto the sink (DirectSink::log_label) rather than being passed in: four call sites hard-coding "ALSA Direct Engine" is four chances to mislabel a WASAPI stream. play/pause/stop/set_volume use it too - previously only the writer thread did, so a WASAPI stream would have logged ALSA lifecycle lines. Sol reviewed as the Linux compiler before this commit and found no Linux type error; the stale references and hard-coded labels above are its findings. cc --- crates/qbz-audio/src/alsa_direct.rs | 10 +++ crates/qbz-audio/src/backend.rs | 16 ++++ crates/qbz-audio/src/wasapi_direct.rs | 12 ++- crates/qbz-player/src/player/mod.rs | 38 +++++----- .../qbz-player/src/player/playback_engine.rs | 74 ++++++++++++------- 5 files changed, 101 insertions(+), 49 deletions(-) diff --git a/crates/qbz-audio/src/alsa_direct.rs b/crates/qbz-audio/src/alsa_direct.rs index c6c664ac2..7a4e9551d 100644 --- a/crates/qbz-audio/src/alsa_direct.rs +++ b/crates/qbz-audio/src/alsa_direct.rs @@ -1059,4 +1059,14 @@ impl crate::backend::DirectSink for AlsaDirectStream { fn channels(&self) -> u16 { AlsaDirectStream::channels(self) } + fn log_label(&self) -> &'static str { + "ALSA Direct Engine" + } + /// Linux only, because the inherent method is: the non-Linux stub of this + /// type has no mixer at all, and there the trait default refusal is the + /// honest answer. + #[cfg(target_os = "linux")] + fn set_hardware_volume(&self, volume: f32) -> Result<(), String> { + AlsaDirectStream::set_hardware_volume(self, volume) + } } diff --git a/crates/qbz-audio/src/backend.rs b/crates/qbz-audio/src/backend.rs index 2b3eca358..64150f9dd 100644 --- a/crates/qbz-audio/src/backend.rs +++ b/crates/qbz-audio/src/backend.rs @@ -1003,4 +1003,20 @@ pub trait DirectSink: Send + Sync + 'static { fn stop(&self) -> Result<(), String>; fn sample_rate(&self) -> u32; fn channels(&self) -> u16; + + /// Log prefix for this sink. On the trait rather than a parameter so a + /// call site cannot label an ALSA stream "WASAPI" or the reverse - which + /// is exactly what a hard-coded label at four call sites invites. + fn log_label(&self) -> &'static str; + + /// Device-side volume, where the sink has one. + /// + /// The default REFUSES rather than silently succeeding: a sink without a + /// hardware mixer must not pretend it moved the volume. Every call site is + /// already gated on the engine's `hardware_volume` flag, which is false for + /// any such sink - permanently so for WASAPI exclusive, where the phase B + /// contract rules out ISimpleAudioVolume. + fn set_hardware_volume(&self, _volume: f32) -> Result<(), String> { + Err("this sink has no hardware volume".to_string()) + } } diff --git a/crates/qbz-audio/src/wasapi_direct.rs b/crates/qbz-audio/src/wasapi_direct.rs index 62cf033b7..37a5de2e2 100644 --- a/crates/qbz-audio/src/wasapi_direct.rs +++ b/crates/qbz-audio/src/wasapi_direct.rs @@ -759,11 +759,12 @@ mod imp { // Partial period. Zero-padding keeps the samples we DO have // instead of dropping them, which a silent buffer would. buf.resize(need, 0); - // Same reasoning: the last period of a drain is partial by - // definition. - if draining.is_none() && !starved { + // Counted EVERY time, unlike the fully-empty case: a partial + // period is a discrete glitch that just happened, whereas an + // empty queue is one starvation that lasts. Same reasoning as + // above for why a drain's last partial period is not a fault. + if draining.is_none() { underruns.fetch_add(1, Ordering::Relaxed); - starved = true; } } if render.write_to_device(n, &buf, None).is_err() { @@ -1066,4 +1067,7 @@ impl crate::backend::DirectSink for WasapiDirectStream { fn channels(&self) -> u16 { WasapiDirectStream::channels(self) } + fn log_label(&self) -> &'static str { + "WASAPI Direct Engine" + } } diff --git a/crates/qbz-player/src/player/mod.rs b/crates/qbz-player/src/player/mod.rs index a06866c46..4eb5d0ed0 100644 --- a/crates/qbz-player/src/player/mod.rs +++ b/crates/qbz-player/src/player/mod.rs @@ -544,8 +544,14 @@ enum StreamType { #[cfg(target_os = "macos")] exclusive_guard: Option, }, - #[cfg(target_os = "linux")] - AlsaDirect(Arc), + /// Any bit-perfect DIRECT sink: ALSA hw: on Linux, WASAPI exclusive on + /// Windows. One variant rather than one per platform, because everything + /// downstream treats them identically - see `qbz_audio::backend::DirectSink`. + /// + /// NOT cfg-gated: gating it to Linux is what made the Windows compiler + /// unable to check any of the arms that handle it, which is the whole + /// blind spot this port has to work around. + Direct(Arc), /// Native JACK output (#263 Tier 3). QBZ as a JACK client with stable ports; /// NOT bit-perfect (resampled to the graph rate). #[cfg(target_os = "linux")] @@ -898,7 +904,7 @@ fn try_init_stream_with_backend( return Some(result.map(|(stream, mode)| { log::info!("ALSA Direct stream created with mode: {:?}", mode); state.set_bit_perfect_mode(Some(mode)); - StreamType::AlsaDirect(Arc::new(stream)) + StreamType::Direct(Arc::new(stream)) })); } } @@ -2220,8 +2226,7 @@ impl Player { } } } - #[cfg(target_os = "linux")] - StreamType::AlsaDirect(alsa_stream) => { + StreamType::Direct(alsa_stream) => { *consecutive_sink_failures = 0; thread_state.set_stream_error(false); let hardware_volume = thread_settings @@ -2229,7 +2234,7 @@ impl Player { .ok() .map(|s| s.alsa_hardware_volume) .unwrap_or(false); - PlaybackEngine::new_alsa_direct( + PlaybackEngine::new_direct( alsa_stream.clone(), hardware_volume, ) @@ -2666,14 +2671,13 @@ impl Player { } } } - #[cfg(target_os = "linux")] - StreamType::AlsaDirect(alsa_stream) => { + StreamType::Direct(alsa_stream) => { let hardware_volume = thread_settings .lock() .ok() .map(|s| s.alsa_hardware_volume) .unwrap_or(false); - PlaybackEngine::new_alsa_direct( + PlaybackEngine::new_direct( alsa_stream.clone(), hardware_volume, ) @@ -3012,7 +3016,7 @@ impl Player { carrier, device ); - *stream_opt = Some(StreamType::AlsaDirect(stream.clone())); + *stream_opt = Some(StreamType::Direct(stream.clone())); thread_state.set_current_device(Some(device)); *current_track_sample_rate = Some(carrier); *current_track_channels = Some(2); @@ -3134,7 +3138,7 @@ impl Player { if little_endian { "LE" } else { "BE" }, device ); - *stream_opt = Some(StreamType::AlsaDirect(stream.clone())); + *stream_opt = Some(StreamType::Direct(stream.clone())); thread_state.set_current_device(Some(device)); *current_track_sample_rate = Some(rate); *current_track_channels = Some(2); @@ -3364,17 +3368,16 @@ impl Player { } } } - #[cfg(target_os = "linux")] - StreamType::AlsaDirect(alsa_stream) => { + StreamType::Direct(alsa_stream) => { let hardware_volume = thread_settings .lock() .ok() .map(|s| s.alsa_hardware_volume) .unwrap_or(false); - PlaybackEngine::new_alsa_direct( + PlaybackEngine::new_direct( alsa_stream.clone(), hardware_volume, - ) + ) } #[cfg(target_os = "linux")] StreamType::Jack(jack_stream) => { @@ -3614,14 +3617,13 @@ impl Player { } } } - #[cfg(target_os = "linux")] - StreamType::AlsaDirect(alsa_stream) => { + StreamType::Direct(alsa_stream) => { let hardware_volume = thread_settings .lock() .ok() .map(|s| s.alsa_hardware_volume) .unwrap_or(false); - PlaybackEngine::new_alsa_direct( + PlaybackEngine::new_direct( alsa_stream.clone(), hardware_volume, ) diff --git a/crates/qbz-player/src/player/playback_engine.rs b/crates/qbz-player/src/player/playback_engine.rs index 70bc04ee5..d0b29625f 100644 --- a/crates/qbz-player/src/player/playback_engine.rs +++ b/crates/qbz-player/src/player/playback_engine.rs @@ -8,6 +8,7 @@ //! to enable gapless playback. When one source ends, the next is picked up //! seamlessly without interrupting the PCM stream. +#[cfg(target_os = "linux")] use qbz_audio::AlsaDirectStream; #[cfg(target_os = "linux")] use qbz_audio::JackStream; @@ -74,9 +75,14 @@ impl SourceQueue { pub enum PlaybackEngine { /// Rodio-based (PipeWire, Pulse, ALSA via CPAL) Rodio { sink: RodioPlayer }, - /// Direct ALSA (hw: devices, bit-perfect) with gapless source queue - AlsaDirect { - stream: Arc, + /// A bit-perfect DIRECT sink with a gapless source queue: ALSA hw: on + /// Linux, WASAPI exclusive on Windows. The two are interchangeable here - + /// the writer thread only ever calls `DirectSink`. + Direct { + stream: Arc, + /// Log prefix for THIS sink. Without it `play`/`pause`/`stop`/ + /// `set_volume` printed "ALSA Direct Engine" for a WASAPI stream. + label: &'static str, is_playing: Arc, should_stop: Arc, position_frames: Arc, @@ -129,7 +135,13 @@ impl PlaybackEngine { /// Create ALSA Direct engine with gapless source queue. /// Spawns a single writer thread that lives for the engine's lifetime. - pub fn new_alsa_direct(stream: Arc, hardware_volume: bool) -> Self { + /// Takes ANY direct sink. The log prefix comes from the SINK, so no call + /// site can mislabel it. + pub fn new_direct( + stream: Arc, + hardware_volume: bool, + ) -> Self { + let label = stream.log_label(); let is_playing = Arc::new(AtomicBool::new(false)); let should_stop = Arc::new(AtomicBool::new(false)); let position_frames = Arc::new(AtomicU64::new(0)); @@ -158,13 +170,14 @@ impl PlaybackEngine { queue_c, transition_c, channels, - "ALSA Direct Engine", + label, ); }) }; - Self::AlsaDirect { + Self::Direct { stream, + label, is_playing, should_stop, position_frames, @@ -303,7 +316,8 @@ impl PlaybackEngine { sink.append(source); Ok(()) } - Self::AlsaDirect { + Self::Direct { + label, is_playing, should_stop, position_frames, @@ -323,9 +337,9 @@ impl PlaybackEngine { should_stop.store(false, Ordering::SeqCst); source_transition.store(false, Ordering::SeqCst); is_playing.store(true, Ordering::SeqCst); - log::info!("[ALSA Direct Engine] First source queued, playback starting"); + log::info!("[{label}] First source queued, playback starting"); } else { - log::info!("[ALSA Direct Engine] Source queued for gapless transition"); + log::info!("[{label}] Source queued for gapless transition"); } Ok(()) @@ -372,8 +386,10 @@ impl PlaybackEngine { pub fn play(&self) { match self { Self::Rodio { sink } => sink.play(), - Self::AlsaDirect { is_playing, .. } => { - log::info!("[ALSA Direct Engine] Resume requested"); + Self::Direct { + label, is_playing, .. + } => { + log::info!("[{label}] Resume requested"); is_playing.store(true, Ordering::SeqCst); } #[cfg(target_os = "linux")] @@ -393,8 +409,10 @@ impl PlaybackEngine { pub fn pause(&self) { match self { Self::Rodio { sink } => sink.pause(), - Self::AlsaDirect { is_playing, .. } => { - log::info!("[ALSA Direct Engine] Pause requested"); + Self::Direct { + label, is_playing, .. + } => { + log::info!("[{label}] Pause requested"); is_playing.store(false, Ordering::SeqCst); } #[cfg(target_os = "linux")] @@ -425,8 +443,9 @@ impl PlaybackEngine { Self::Rodio { sink } => { sink.stop(); } - Self::AlsaDirect { + Self::Direct { stream, + label, is_playing, should_stop, playback_thread, @@ -435,7 +454,7 @@ impl PlaybackEngine { if should_stop.load(Ordering::SeqCst) { return; // Already stopped } - log::info!("[ALSA Direct Engine] Stop requested"); + log::info!("[{label}] Stop requested"); should_stop.store(true, Ordering::SeqCst); is_playing.store(false, Ordering::SeqCst); @@ -444,7 +463,7 @@ impl PlaybackEngine { } if let Err(e) = stream.stop() { - log::warn!("[ALSA Direct Engine] Stop failed: {}", e); + log::warn!("[{label}] Stop failed: {}", e); } } #[cfg(target_os = "linux")] @@ -493,8 +512,9 @@ impl PlaybackEngine { pub fn set_volume(&self, volume: f32) { match self { Self::Rodio { sink } => sink.set_volume(volume), - Self::AlsaDirect { + Self::Direct { stream, + label, hardware_volume, .. } => { @@ -502,12 +522,12 @@ impl PlaybackEngine { #[cfg(target_os = "linux")] { if let Err(e) = stream.set_hardware_volume(volume) { - log::warn!("[ALSA Direct Engine] Hardware volume failed: {}", e); + log::warn!("[{label}] Hardware volume failed: {}", e); } } } else { log::debug!( - "[ALSA Direct Engine] Hardware volume control disabled (use DAC/amplifier)" + "[{label}] Hardware volume control disabled (use DAC/amplifier)" ); } } @@ -530,7 +550,7 @@ impl PlaybackEngine { pub fn empty(&self) -> bool { match self { Self::Rodio { sink } => sink.empty(), - Self::AlsaDirect { + Self::Direct { is_playing, source_queue, .. @@ -555,7 +575,7 @@ impl PlaybackEngine { pub fn take_source_transition(&self) -> bool { match self { Self::Rodio { .. } => false, - Self::AlsaDirect { + Self::Direct { source_transition, .. } => source_transition .compare_exchange(true, false, Ordering::SeqCst, Ordering::SeqCst) @@ -580,7 +600,7 @@ impl PlaybackEngine { pub fn position_secs(&self) -> Option { match self { Self::Rodio { .. } => None, - Self::AlsaDirect { + Self::Direct { position_frames, stream, .. @@ -615,7 +635,7 @@ impl PlaybackEngine { pub fn duration_secs(&self) -> Option { match self { Self::Rodio { .. } => None, - Self::AlsaDirect { + Self::Direct { duration_frames, stream, .. @@ -640,14 +660,14 @@ impl PlaybackEngine { /// Check if using ALSA Direct engine #[allow(dead_code)] - pub fn is_alsa_direct(&self) -> bool { - matches!(self, Self::AlsaDirect { .. }) + pub fn is_direct(&self) -> bool { + matches!(self, Self::Direct { .. }) } } -/// Single long-lived writer thread for ALSA Direct. +/// Single long-lived writer thread for any bit-perfect direct sink. /// -/// Continuously reads samples from the current source and writes to ALSA. +/// Continuously reads samples from the current source and writes to the sink. /// When a source ends, seamlessly picks up the next one from the queue /// (gapless transition). If no next source is available, drains the ALSA /// buffer and waits for the next source or a stop signal.