diff --git a/audio-player/src/player.rs b/audio-player/src/player.rs index 51ab391..6fff0f0 100644 --- a/audio-player/src/player.rs +++ b/audio-player/src/player.rs @@ -158,6 +158,16 @@ impl Player { Ok(rx.recv_async().await?) } + /// Whether output is muted, for telling a connecting client the truth + /// instead of assuming it is not. + pub async fn is_muted(&self) -> Result { + let (tx, rx) = flume::bounded(1); + self.tx_engine + .send_async(PlayerEngineCommand::GetMuted(tx)) + .await?; + Ok(rx.recv_async().await?) + } + pub async fn pause(&self) -> Result<()> { let (tx, rx) = flume::bounded(1); self.tx_engine diff --git a/audio-player/src/player_engine.rs b/audio-player/src/player_engine.rs index 0325f15..bd77757 100644 --- a/audio-player/src/player_engine.rs +++ b/audio-player/src/player_engine.rs @@ -93,6 +93,9 @@ pub enum PlayerEngineCommand { SeekBy(i64, Sender>), GetVolume(Sender), ToggleMute(Sender), + /// The current muted state, so a connecting client can be told it + /// rather than assuming "not muted". + GetMuted(Sender), GetPaused(Sender>), /// End of stream for the source started by the given generation. /// Stale generations are ignored so an old track finishing can never @@ -293,6 +296,7 @@ impl PlayerEngine { } PlayerEngineCommand::GetVolume(tx) => send_reply(tx, self.volume()), PlayerEngineCommand::ToggleMute(tx) => send_reply(tx, self.toggle_mute()), + PlayerEngineCommand::GetMuted(tx) => send_reply(tx, self.muted), PlayerEngineCommand::GetPaused(tx) => send_reply(tx, self.is_paused()), PlayerEngineCommand::Eos(generation) => self.handle_eos(generation), } diff --git a/crabidy-server/src/playback.rs b/crabidy-server/src/playback.rs index 9d5390e..ebfb231 100644 --- a/crabidy-server/src/playback.rs +++ b/crabidy-server/src/playback.rs @@ -19,6 +19,8 @@ use std::sync::atomic::{AtomicU64, Ordering}; #[cfg(feature = "fs")] use std::sync::Arc; use std::sync::Mutex; +use std::time::Duration; +use tokio::time::timeout; #[cfg(feature = "fs")] use tracing::info; use tracing::{debug, debug_span, error, instrument, trace, warn, Instrument}; @@ -132,6 +134,10 @@ impl Playback { trace!("handling playback command"); match command { PlaybackCommand::Init { result_tx } => { + // Asked before the queue lock is taken: that lock is a std + // `Mutex`, so it must not be held across an await, which is + // why these three were hardcoded below in the first place. + let (volume, muted, position) = self.player_snapshot().await; let response = { let Ok(queue) = self.queue.lock() else { error!("queue lock poisoned"); @@ -141,10 +147,6 @@ impl Playback { queue_position: queue.current_position() as u32, track: queue.current_track(), }; - let position = TrackPosition { - duration: 0, - position: 0, - }; let play_state = { let Ok(play_state) = self.state.lock() else { error!("play state lock poisoned"); @@ -158,8 +160,8 @@ impl Playback { queue: Some(self.queue_snapshot(&queue)), queue_track: Some(queue_track), play_state: play_state as i32, - volume: 0.0, - mute: false, + volume, + mute: muted, position: Some(position), mods: Some(QueueModifiers { repeat: queue.repeat, @@ -380,12 +382,22 @@ impl Playback { PlaybackCommand::ChangeVolume { delta } => { match self.player.volume().await { - Ok(volume) => { - debug!(volume, delta, "changing volume"); - if let Err(err) = self.player.set_volume(volume + delta).await { - warn!("set_volume failed: {err:?}"); + Ok(volume) => match self.player.set_volume(volume + delta).await { + // Nothing was broadcast here, so no client ever + // learned the level had changed. The level sent is + // the one the engine actually took, so a client + // also sees the clamp at 1.1 rather than the value + // it asked for. `set_volume` unmutes as well + // ("reaching for the volume is an intent to hear + // something"), so the mute indicator needs the + // same news or it stays stuck on muted. + Ok(volume) => { + debug!(volume, delta, "changed volume"); + self.broadcast(StreamUpdate::Volume(volume)); + self.broadcast(StreamUpdate::Mute(false)); } - } + Err(err) => warn!("set_volume failed: {err:?}"), + }, Err(err) => warn!("could not read volume: {err:?}"), }; } @@ -494,6 +506,84 @@ impl Playback { } } + /// The player-owned half of the init snapshot: output level, mute + /// state, and the position within the current track. All three were + /// hardcoded to neutral values, so every client showed 0% volume, an + /// unmuted server and 0:00 until something happened to change them — + /// and while *paused* nothing does, since the position tick skips a + /// paused sink. + /// + /// Every read is bounded. The engine thread is single-threaded and can + /// be busy for up to 30 s opening a network stream, and this is the + /// **connect** path: a client must get a usable snapshot rather than + /// hang waiting for one. On a timeout each field falls back to what the + /// engine starts at, and the next update on the stream corrects it. A + /// late reply lands in a dropped receiver, which the engine only logs. + async fn player_snapshot(&self) -> (f32, bool, TrackPosition) { + /// Deliberately short. The engine has no middle gear here: these + /// are plain field reads, so an idle engine answers in + /// microseconds, while a busy one is mid-`Play` and will not answer + /// for up to 30 s no matter how long we wait. So the budget only + /// decides how long the playback loop — which handles commands one + /// at a time — stalls before giving up on a busy engine. Volume is + /// read first, being the field a user actually looks at. + const READ_TIMEOUT: Duration = Duration::from_secs(1); + + let volume = match timeout(READ_TIMEOUT, self.player.volume()).await { + Ok(Ok(volume)) => volume, + Ok(Err(err)) => { + warn!("could not read volume for init: {err:?}"); + 1.0 + } + Err(_) => { + warn!("timed out reading volume for init"); + 1.0 + } + }; + let muted = match timeout(READ_TIMEOUT, self.player.is_muted()).await { + Ok(Ok(muted)) => muted, + Ok(Err(err)) => { + warn!("could not read mute state for init: {err:?}"); + false + } + Err(_) => { + warn!("timed out reading mute state for init"); + false + } + }; + // Both error when nothing is loaded, which is the ordinary idle + // case rather than a fault — hence zeros without a warning. + let position = match timeout(READ_TIMEOUT, self.player_position()).await { + Ok(position) => position, + Err(_) => { + warn!("timed out reading position for init"); + TrackPosition { + duration: 0, + position: 0, + } + } + }; + (volume, muted, position) + } + + /// The current position, as milliseconds for the wire. Zeros when + /// nothing is playing. + async fn player_position(&self) -> TrackPosition { + let position = self + .player + .elapsed() + .await + .map(|elapsed| elapsed.as_millis().min(u32::MAX.into()) as u32) + .unwrap_or(0); + let duration = self + .player + .duration() + .await + .map(|duration| duration.as_millis().min(u32::MAX.into()) as u32) + .unwrap_or(0); + TrackPosition { duration, position } + } + /// Sends an update to all connected clients. Having no subscribers is /// normal and not an error. fn broadcast(&self, update: StreamUpdate) { diff --git a/plan/summary.md b/plan/summary.md index db6c364..778d6d9 100644 --- a/plan/summary.md +++ b/plan/summary.md @@ -1542,3 +1542,48 @@ rather than `NaN%`, and negative zero reads as `0%`. Noted, not fixed — the web volume slider is `max="1.5"` while the engine clamps to `1.1`, so the top third of its travel silently snaps back. A one-character fix in `cbd-web`, but it is the web client's bug, not this change's. + +## TUI volume display — the server was the bug (2026-07-27) + +The display shipped and read `Volume: 0%` always. The TUI was fine; the level +never existed to be shown. Three separate holes in `crabidy-server`, each of +which alone was enough to break it: + +1. **`Init` hardcoded `volume: 0.0`** (and `mute: false`, and a zeroed + `TrackPosition`). This is the whole reason it read 0% rather than a stale + number. +2. **`PlaybackCommand::ChangeVolume` broadcast nothing.** It read the volume, + set the new one, and told no one — so no client ever learned the level had + changed, and the display could never recover from the bad init value. The + web slider had the same silence, and never tracked `J`/`K` either. +3. **`PlaybackCommand::VolumeChanged` / `MuteChanged` are dead variants** — + handled in `playback.rs`, sent by nobody. They look like the volume + broadcast path while doing nothing, which is presumably how (2) went + unnoticed. Left in place (removing them is unrelated cleanup) but worth + knowing about. + +`ChangeVolume` now broadcasts the level the engine *took*, so clients see the +clamp at 1.1 rather than what they asked for, plus `Mute(false)`, because +`set_volume` unmutes and the indicator would otherwise stick on muted. + +The hardcoding had a cause worth recording: the init response is built while +holding the queue's `std::sync::Mutex` guard, so the block cannot await the +player. The player reads now happen *before* the lock is taken. + +**Bounded, because `Init` is the connect path.** The engine thread is +single-threaded and can be 30 s deep in opening a network stream, so each read +gets a 1 s budget and falls back to what the engine starts at. The budget is +short on purpose: the engine either answers in microseconds (idle) or not for +tens of seconds (mid-`Play`), so the number only decides how long the playback +loop stalls before giving up. A late reply lands in a dropped receiver, which +the engine only logs. + +Fixing the init **position** also fixes click-to-seek against a *paused* +server: no position ticks flow while paused, so the web client's position sat +at the hardcoded 0, and a gauge click sent `target - 0` — which the engine then +added to the real position, seeking to roughly twice the intended point. + +Not covered by a test: `Player::new` is only reachable through `Playback::new`, +and the engine thread opens an audio device, so there is no device-free way to +construct one here. `is_muted()` (a new engine getter — only `toggle_mute` +existed, which cannot be used to *ask*) is likewise verified by reading.