server: report the real volume, mute and position
The volume display read 0% always, because the level never reached a client. Three holes, each sufficient on its own: - `Init` hardcoded `volume: 0.0`, `mute: false` and a zeroed position. - `ChangeVolume` broadcast nothing, so no client learned the level had changed and the display could not recover from that init value. The web slider was equally silent and never tracked J/K. - `PlaybackCommand::VolumeChanged`/`MuteChanged` are handled but sent by nobody — a broadcast path that looks real and does nothing, which is presumably how the above went unnoticed. Left alone here. `ChangeVolume` now broadcasts the level the engine actually took, so clients see the 1.1 clamp rather than what they asked for, plus `Mute(false)`, since `set_volume` unmutes and the indicator would otherwise stick. The hardcoding had a cause: the init response is built while holding the queue's std Mutex guard, which cannot be held across an await. The player reads now happen before the lock is taken, each with a 1 s budget — `Init` is the connect path, and the engine can be 30 s deep in opening a stream, so a client must get a usable snapshot rather than hang. Fixing the init position also fixes click-to-seek against a paused server: no position ticks flow while paused, so the web client sent `target - 0` and the engine added it to the real position. Adds `Player::is_muted`, since only `toggle_mute` existed and that cannot ask without changing the state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
cc95e109fc
commit
034737d14f
|
|
@ -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<bool> {
|
||||
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
|
||||
|
|
|
|||
|
|
@ -93,6 +93,9 @@ pub enum PlayerEngineCommand {
|
|||
SeekBy(i64, Sender<Result<Duration>>),
|
||||
GetVolume(Sender<f32>),
|
||||
ToggleMute(Sender<bool>),
|
||||
/// The current muted state, so a connecting client can be told it
|
||||
/// rather than assuming "not muted".
|
||||
GetMuted(Sender<bool>),
|
||||
GetPaused(Sender<Result<bool>>),
|
||||
/// 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),
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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) => 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, "changing volume");
|
||||
if let Err(err) = self.player.set_volume(volume + delta).await {
|
||||
warn!("set_volume failed: {err:?}");
|
||||
}
|
||||
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) {
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in New Issue