fix(player): return volume control to the local speaker when a remote session stops
Stopping a remote session left Android stuck on the remote volume slider with no way back to the device speaker. Two causes: 1. `player_stop`'s remote branch sent "Stop" to the session and returned without touching the playback mode, so the manager stayed in Remote. It now drops to Idle, mirroring what the local branch already does. 2. Volume routing was torn down at a single call site (`transfer_to_local_inner`), so every *other* exit from remote mode leaked the Android VolumeProviderCompat. Routing is now derived from the transition inside `set_mode`: entering remote attaches control, any exit from remote hands it back to the local media stream. This also covers the frontend `disconnect()` path (Remote -> Idle) and the local-playback-start paths (Remote -> Local). Adds a `RemoteVolumeControl` trait so the routing rule is unit-testable off-device — the real implementation is Android JNI. Tests cover remote->idle, remote->local, remote->remote (re-arms, never releases), and that local/idle transitions leave routing untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1003,6 +1003,16 @@ pub async fn player_stop(
|
||||
.clone()
|
||||
};
|
||||
client.send_session_command(session_id, "Stop").await?;
|
||||
|
||||
// Stopping the remote session ends the cast, so the manager returns to
|
||||
// Idle — same as a local stop. This is also what hands OS volume control
|
||||
// back to this device: set_mode releases the Android remote volume
|
||||
// provider on any exit from remote mode. Without it the mode stayed
|
||||
// Remote and the system volume slider remained stuck on the remote
|
||||
// session with no way back to the local speaker.
|
||||
playback_mode
|
||||
.0
|
||||
.set_mode(crate::playback_mode::PlaybackMode::Idle);
|
||||
} else {
|
||||
// Local playback
|
||||
let controller = player.0.lock().await;
|
||||
|
||||
@@ -27,6 +27,10 @@ const TICKS_PER_SECOND: f64 = 10_000_000.0;
|
||||
/// send a resume position, so a fresh track casts from 0 rather than ~0.
|
||||
const RESUME_THRESHOLD_SECONDS: f64 = 0.5;
|
||||
|
||||
/// Volume level (0-100) the remote volume slider starts at. The real level is
|
||||
/// corrected by the session poller once the remote session reports its volume.
|
||||
const DEFAULT_REMOTE_VOLUME: i32 = 50;
|
||||
|
||||
/// Convert a live playback position (seconds) into the `StartPositionTicks` to
|
||||
/// hand to a remote session, or `None` if we're effectively at the start.
|
||||
///
|
||||
@@ -42,6 +46,50 @@ fn start_position_ticks_from_seconds(position_seconds: f64) -> Option<i64> {
|
||||
}
|
||||
}
|
||||
|
||||
/// Platform hook for attaching/detaching the OS remote-volume control.
|
||||
///
|
||||
/// On Android, entering remote mode hands the `MediaSession` a
|
||||
/// `VolumeProviderCompat` so hardware volume buttons and the system slider drive
|
||||
/// the *remote* session; leaving remote mode must hand it back to the local
|
||||
/// media stream. Behind a trait so the routing rule (see
|
||||
/// [`PlaybackModeManager::set_mode`]) is unit-testable off-device — the real
|
||||
/// implementation is JNI and only exists on Android.
|
||||
pub trait RemoteVolumeControl: Send + Sync {
|
||||
/// Attach remote-volume control (and, on Android, start the playback service).
|
||||
fn enable(&self, initial_volume: i32);
|
||||
/// Return volume control to the local device speaker.
|
||||
fn disable(&self);
|
||||
}
|
||||
|
||||
/// Production hook: forwards to the Android JNI bridge; no-op elsewhere.
|
||||
struct PlatformRemoteVolumeControl;
|
||||
|
||||
impl RemoteVolumeControl for PlatformRemoteVolumeControl {
|
||||
#[allow(unused_variables)]
|
||||
fn enable(&self, initial_volume: i32) {
|
||||
#[cfg(target_os = "android")]
|
||||
{
|
||||
if let Err(e) = crate::player::enable_remote_volume(initial_volume) {
|
||||
log::warn!(
|
||||
"[PlaybackMode] Failed to enable remote volume/service: {}",
|
||||
e
|
||||
);
|
||||
// Non-fatal - continue; the next poll tick will retry metadata.
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn disable(&self) {
|
||||
#[cfg(target_os = "android")]
|
||||
{
|
||||
if let Err(e) = crate::player::disable_remote_volume() {
|
||||
log::warn!("[PlaybackMode] Failed to disable remote volume: {}", e);
|
||||
// Non-fatal - the mode change itself has already happened.
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Manages playback mode transfers between local and remote sessions
|
||||
pub struct PlaybackModeManager {
|
||||
jellyfin_client: Arc<Mutex<Option<JellyfinClient>>>,
|
||||
@@ -51,6 +99,8 @@ pub struct PlaybackModeManager {
|
||||
/// Optional emitter used to notify the frontend when the mode changes, so its
|
||||
/// mirror store stays in sync with this authoritative one. `None` in tests.
|
||||
event_emitter: Arc<Mutex<Option<Arc<dyn PlayerEventEmitter>>>>,
|
||||
/// Platform hook for OS-level remote volume routing (swapped in tests).
|
||||
remote_volume: Arc<dyn RemoteVolumeControl>,
|
||||
}
|
||||
|
||||
impl PlaybackModeManager {
|
||||
@@ -65,6 +115,24 @@ impl PlaybackModeManager {
|
||||
current_mode: Arc::new(RwLock::new(PlaybackMode::Idle)),
|
||||
is_transferring: Arc::new(AtomicBool::new(false)),
|
||||
event_emitter: Arc::new(Mutex::new(None)),
|
||||
remote_volume: Arc::new(PlatformRemoteVolumeControl),
|
||||
}
|
||||
}
|
||||
|
||||
/// Construct with a custom remote-volume hook (tests).
|
||||
#[cfg(test)]
|
||||
fn with_remote_volume(
|
||||
jellyfin_client: Arc<Mutex<Option<JellyfinClient>>>,
|
||||
player_controller: Arc<TokioMutex<PlayerController>>,
|
||||
remote_volume: Arc<dyn RemoteVolumeControl>,
|
||||
) -> Self {
|
||||
Self {
|
||||
jellyfin_client,
|
||||
player_controller,
|
||||
current_mode: Arc::new(RwLock::new(PlaybackMode::Idle)),
|
||||
is_transferring: Arc::new(AtomicBool::new(false)),
|
||||
event_emitter: Arc::new(Mutex::new(None)),
|
||||
remote_volume,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -86,19 +154,39 @@ impl PlaybackModeManager {
|
||||
/// the frontend's mirror store reconciles to this authoritative value. The
|
||||
/// write lock is released before emitting to avoid holding it across the
|
||||
/// emitter call.
|
||||
///
|
||||
/// Also owns **OS volume routing**, which is derived from the transition
|
||||
/// rather than from each call site: entering remote mode attaches the remote
|
||||
/// volume control, and *any* exit from remote mode hands it back to the local
|
||||
/// speaker. Doing this per-call-site is what caused the bug where stopping a
|
||||
/// remote session (`player_stop` → Idle) left Android stuck on the remote
|
||||
/// volume slider — only the transfer-to-local path tore it down.
|
||||
///
|
||||
/// TRACES: UR-010 | DR-059, IR-021
|
||||
pub fn set_mode(&self, mode: PlaybackMode) {
|
||||
log::info!("[PlaybackMode] Setting mode to: {:?}", mode);
|
||||
let changed = {
|
||||
let (changed, was_remote) = {
|
||||
let mut current = self.current_mode.write_safe();
|
||||
let changed = *current != mode;
|
||||
let was_remote = matches!(*current, PlaybackMode::Remote { .. });
|
||||
*current = mode.clone();
|
||||
changed
|
||||
(changed, was_remote)
|
||||
};
|
||||
|
||||
if !changed {
|
||||
return;
|
||||
}
|
||||
|
||||
// Volume routing follows the transition. Note remote->remote (switching
|
||||
// target session) re-arms rather than releasing control.
|
||||
let is_remote = matches!(mode, PlaybackMode::Remote { .. });
|
||||
if is_remote {
|
||||
self.remote_volume.enable(DEFAULT_REMOTE_VOLUME);
|
||||
} else if was_remote {
|
||||
log::info!("[PlaybackMode] Leaving remote mode - restoring local volume control");
|
||||
self.remote_volume.disable();
|
||||
}
|
||||
|
||||
let (mode_str, session_id) = match &mode {
|
||||
PlaybackMode::Local => ("local".to_string(), None),
|
||||
PlaybackMode::Idle => ("idle".to_string(), None),
|
||||
@@ -122,18 +210,13 @@ impl PlaybackModeManager {
|
||||
/// Both symptoms share this one cause, so this must not be skipped on any
|
||||
/// remote-entry path (notably the empty-queue early return in
|
||||
/// `transfer_to_remote_inner`). No-op / non-Android builds do nothing.
|
||||
#[allow(unused_variables)]
|
||||
///
|
||||
/// [`set_mode`](Self::set_mode) already arms this on entry into remote mode;
|
||||
/// calling it again is harmless (the service start is idempotent) and keeps
|
||||
/// the guarantee when the mode was already remote, which `set_mode` skips as
|
||||
/// a no-op transition.
|
||||
fn enable_remote_control(&self) {
|
||||
#[cfg(target_os = "android")]
|
||||
{
|
||||
if let Err(e) = crate::player::enable_remote_volume(50) {
|
||||
log::warn!(
|
||||
"[PlaybackMode] Failed to enable remote volume/service: {}",
|
||||
e
|
||||
);
|
||||
// Non-fatal - continue; the next poll tick will retry metadata.
|
||||
}
|
||||
}
|
||||
self.remote_volume.enable(DEFAULT_REMOTE_VOLUME);
|
||||
}
|
||||
|
||||
/// Check if currently transferring
|
||||
@@ -766,18 +849,10 @@ impl PlaybackModeManager {
|
||||
// This will be improved in Phase 3 when repository is migrated to Rust.
|
||||
log::debug!("[PlaybackMode] Cannot load media item in Rust yet - frontend handled it");
|
||||
|
||||
// Update mode to local
|
||||
// Update mode to local. This also returns volume control to the local
|
||||
// device speaker — set_mode owns that for every exit from remote mode.
|
||||
self.set_mode(PlaybackMode::Local);
|
||||
|
||||
// Disable remote volume control on Android (return to system volume)
|
||||
#[cfg(target_os = "android")]
|
||||
{
|
||||
if let Err(e) = crate::player::disable_remote_volume() {
|
||||
log::warn!("[PlaybackMode] Failed to disable remote volume: {}", e);
|
||||
// Non-fatal - continue with transfer
|
||||
}
|
||||
}
|
||||
|
||||
log::info!("[PlaybackMode] Successfully transferred to local");
|
||||
Ok(())
|
||||
}
|
||||
@@ -893,6 +968,118 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// Records enable/disable calls so tests can assert volume routing.
|
||||
struct RecordingVolumeControl {
|
||||
calls: Mutex<Vec<&'static str>>,
|
||||
}
|
||||
|
||||
impl RemoteVolumeControl for RecordingVolumeControl {
|
||||
fn enable(&self, _initial_volume: i32) {
|
||||
self.calls.lock().unwrap().push("enable");
|
||||
}
|
||||
fn disable(&self) {
|
||||
self.calls.lock().unwrap().push("disable");
|
||||
}
|
||||
}
|
||||
|
||||
fn manager_with_volume_control() -> (PlaybackModeManager, Arc<RecordingVolumeControl>) {
|
||||
let volume = Arc::new(RecordingVolumeControl {
|
||||
calls: Mutex::new(Vec::new()),
|
||||
});
|
||||
let manager = PlaybackModeManager::with_remote_volume(
|
||||
Arc::new(Mutex::new(None)),
|
||||
Arc::new(TokioMutex::new(crate::player::PlayerController::default())),
|
||||
volume.clone(),
|
||||
);
|
||||
(manager, volume)
|
||||
}
|
||||
|
||||
/// Leaving remote mode must hand volume control back to the local device.
|
||||
///
|
||||
/// Stopping a remote session (`player_stop`) drives the manager
|
||||
/// Remote -> Idle without going through `transfer_to_local`. Before this was
|
||||
/// centralised in `set_mode`, only the transfer path tore the Android
|
||||
/// `VolumeProviderCompat` down, so a plain stop left the system stuck on the
|
||||
/// remote volume slider with no way back to the phone speaker.
|
||||
///
|
||||
/// @req-test: UR-010 - Control playback of Jellyfin remote sessions
|
||||
#[test]
|
||||
fn test_leaving_remote_mode_restores_local_volume() {
|
||||
let (manager, volume) = manager_with_volume_control();
|
||||
|
||||
manager.set_mode(PlaybackMode::Remote {
|
||||
session_id: "sess-1".to_string(),
|
||||
});
|
||||
// The stop path: remote -> idle, no transfer involved.
|
||||
manager.set_mode(PlaybackMode::Idle);
|
||||
|
||||
assert_eq!(
|
||||
*volume.calls.lock().unwrap(),
|
||||
vec!["enable", "disable"],
|
||||
"remote->idle must return volume control to the local speaker"
|
||||
);
|
||||
}
|
||||
|
||||
/// The same must hold for remote -> local (transfer back to this device).
|
||||
///
|
||||
/// @req-test: UR-010 - Control playback of Jellyfin remote sessions
|
||||
#[test]
|
||||
fn test_remote_to_local_restores_local_volume() {
|
||||
let (manager, volume) = manager_with_volume_control();
|
||||
|
||||
manager.set_mode(PlaybackMode::Remote {
|
||||
session_id: "sess-1".to_string(),
|
||||
});
|
||||
manager.set_mode(PlaybackMode::Local);
|
||||
|
||||
assert_eq!(
|
||||
*volume.calls.lock().unwrap(),
|
||||
vec!["enable", "disable"],
|
||||
"remote->local must return volume control to the local speaker"
|
||||
);
|
||||
}
|
||||
|
||||
/// Volume routing must not be touched by transitions that never involve
|
||||
/// remote mode — an idle->local start would otherwise issue a pointless
|
||||
/// `setPlaybackToLocal` on every playback start.
|
||||
///
|
||||
/// @req-test: UR-010 - Control playback of Jellyfin remote sessions
|
||||
#[test]
|
||||
fn test_non_remote_transitions_leave_volume_routing_alone() {
|
||||
let (manager, volume) = manager_with_volume_control();
|
||||
|
||||
manager.set_mode(PlaybackMode::Local);
|
||||
manager.set_mode(PlaybackMode::Idle);
|
||||
manager.set_mode(PlaybackMode::Local);
|
||||
|
||||
assert!(
|
||||
volume.calls.lock().unwrap().is_empty(),
|
||||
"local/idle transitions must not touch remote volume routing"
|
||||
);
|
||||
}
|
||||
|
||||
/// Switching directly between two remote sessions stays remote: control must
|
||||
/// remain attached (re-armed for the new session), never handed back local.
|
||||
///
|
||||
/// @req-test: UR-010 - Control playback of Jellyfin remote sessions
|
||||
#[test]
|
||||
fn test_remote_to_remote_keeps_remote_volume() {
|
||||
let (manager, volume) = manager_with_volume_control();
|
||||
|
||||
manager.set_mode(PlaybackMode::Remote {
|
||||
session_id: "sess-1".to_string(),
|
||||
});
|
||||
manager.set_mode(PlaybackMode::Remote {
|
||||
session_id: "sess-2".to_string(),
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
*volume.calls.lock().unwrap(),
|
||||
vec!["enable", "enable"],
|
||||
"remote->remote re-arms control without releasing it to local"
|
||||
);
|
||||
}
|
||||
|
||||
/// Setting the same mode twice must not re-emit — the frontend reconciler
|
||||
/// (and the event channel) shouldn't be spammed on no-op transitions.
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user