diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt index 1bc746d4..ef7efba0 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt @@ -61,7 +61,20 @@ class MinstrelForwardingPlayer( private fun isRemote(): Boolean = holder.active.value != null + /** + * Returns true when selectUpnp has marked a UPnP route as the intended + * target but loadQueueOnSonos hasn't yet wired ActiveUpnp. During this + * window we drop transport commands silently -- they would hit Sonos's + * stale state from a prior session and trigger restarts. + */ + private fun isLoadingUpnp(): Boolean = + holder.target.value != null && holder.active.value == null + override fun play() { + if (isLoadingUpnp()) { + Timber.w("ForwardingPlayer.play() dropped -- UPnP loading") + return + } val active = holder.active.value Timber.w("ForwardingPlayer.play() active=%s", active?.routeName) if (active == null) { @@ -76,6 +89,10 @@ class MinstrelForwardingPlayer( } override fun pause() { + if (isLoadingUpnp()) { + Timber.w("ForwardingPlayer.pause() dropped -- UPnP loading") + return + } val active = holder.active.value Timber.w("ForwardingPlayer.pause() active=%s", active?.routeName) if (active == null) { @@ -90,6 +107,10 @@ class MinstrelForwardingPlayer( } override fun seekTo(positionMs: Long) { + if (isLoadingUpnp()) { + Timber.w("ForwardingPlayer.seekTo(positionMs) dropped -- UPnP loading") + return + } val active = holder.active.value Timber.w("ForwardingPlayer.seekTo(%dms) active=%s", positionMs, active?.routeName) if (active == null) { @@ -114,6 +135,10 @@ class MinstrelForwardingPlayer( * [positionMs] is non-zero. */ override fun seekTo(mediaItemIndex: Int, positionMs: Long) { + if (isLoadingUpnp()) { + Timber.w("ForwardingPlayer.seekTo(idx, positionMs) dropped -- UPnP loading") + return + } val active = holder.active.value Timber.w( "ForwardingPlayer.seekTo(idx=%d, %dms) active=%s", @@ -135,6 +160,10 @@ class MinstrelForwardingPlayer( } override fun seekToNextMediaItem() { + if (isLoadingUpnp()) { + Timber.w("ForwardingPlayer.seekToNextMediaItem() dropped -- UPnP loading") + return + } val active = holder.active.value Timber.w("ForwardingPlayer.seekToNextMediaItem() active=%s", active?.routeName) if (active == null) { @@ -152,6 +181,10 @@ class MinstrelForwardingPlayer( } override fun seekToPreviousMediaItem() { + if (isLoadingUpnp()) { + Timber.w("ForwardingPlayer.seekToPreviousMediaItem() dropped -- UPnP loading") + return + } val active = holder.active.value Timber.w("ForwardingPlayer.seekToPreviousMediaItem() active=%s", active?.routeName) if (active == null) { diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt index 0b3aaa98..ff44c4bd 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt @@ -322,12 +322,15 @@ class PlayerController @Inject constructor( * and advance past the dead track. Otherwise no-op. */ private fun handleZeroDurationIfNeeded(controller: MediaController, idx: Int) { - // Skip during UPnP playback. ExoPlayer is intentionally paused and never - // buffers tracks, so duration is permanently 0 / TIME_UNSET -- without - // this guard we rapid-advance through the entire local queue (and spam - // /api/playback-errors) every time the user activates a UPnP route. + // Skip during UPnP playback (active) AND during the activation load + // window (target set, active not yet wired). ExoPlayer is intentionally + // paused throughout both windows so its STATE_READY duration is always + // 0 / TIME_UNSET -- without this guard we rapid-advance through the + // entire local queue (and spam /api/playback-errors). val current = queueRefs.getOrNull(idx) - if (current == null || activeUpnpHolder.active.value != null) return + val upnpEngaged = activeUpnpHolder.active.value != null || + activeUpnpHolder.target.value != null + if (current == null || upnpEngaged) return val duration = controller.duration val isZeroDuration = duration <= 0L || duration == androidx.media3.common.C.TIME_UNSET if (!isZeroDuration) return diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/ActiveUpnpHolder.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/ActiveUpnpHolder.kt index c92201c6..601925ac 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/ActiveUpnpHolder.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/output/ActiveUpnpHolder.kt @@ -23,7 +23,21 @@ data class ActiveUpnp( @Singleton class ActiveUpnpHolder @Inject constructor() { + private val internal = MutableStateFlow(null) val active: StateFlow = internal.asStateFlow() + + /** + * Pending route id during selectUpnp's queue-load window. Set when + * loadQueueOnSonos starts; cleared on completion (success or failure). + * ForwardingPlayer overrides treat (target != null && active == null) + * as "UPnP intended but SOAP not yet wired" -- drop transport commands + * silently rather than send them to a half-loaded queue. + */ + private val targetInternal = MutableStateFlow(null) + val target: StateFlow = targetInternal.asStateFlow() + fun set(active: ActiveUpnp?) { internal.value = active } + + fun setTarget(routeId: String?) { targetInternal.value = routeId } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt index dcef6235..971b96af 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/output/OutputPickerController.kt @@ -166,6 +166,7 @@ class OutputPickerController @Inject constructor( val capturedPositionMs = remoteState.positionMs val wasPlayingRemote = remoteState.isPlaying activeUpnpHolder.set(null) + activeUpnpHolder.setTarget(null) selectedUpnpRouteIdInternal.value = null playerController.seekTo(capturedPositionMs) if (wasPlayingRemote) playerController.play() @@ -221,6 +222,7 @@ class OutputPickerController @Inject constructor( runCatching { active?.avTransport?.stop() } .onFailure { Timber.w(it, "UPnP Stop failed during disconnect") } activeUpnpHolder.set(null) + activeUpnpHolder.setTarget(null) selectedUpnpRouteIdInternal.value = null playerController.seekTo(capturedPositionMs) if (wasPlayingRemote) playerController.play() @@ -235,10 +237,14 @@ class OutputPickerController @Inject constructor( * clear the device's queue, load every track from our local queue * via AddURIToQueue, point the transport at the queue URI, seek to * the current index, and play. Wrapped in `runCatching` — SOAP - * failure abandons the selection cleanly. Local pause is handled by - * MinstrelForwardingPlayer.onActiveChanged via holder.set(), so we - * do not call playerController.pause() here (it would race with the - * SOAP override once holder.active is non-null). + * failure abandons the selection cleanly. + * + * Order of operations is deliberate: + * 1. Pause local so the user doesn't keep hearing local audio. + * 2. Set target early so ForwardingPlayer drops transport taps + * while the 17-second queue load is in progress. + * 3. Wire active LAST (after loadQueueOnSonos) so SOAP commands + * are never routed to a half-loaded Sonos queue. */ private suspend fun selectUpnp(route: OutputRoute) = selectUpnpMutex.withLock { val uiState = playerController.uiState.value @@ -262,20 +268,29 @@ class OutputPickerController @Inject constructor( return@withLock } val rendering = renderingClientFor(effectiveRoute.id) + // Pause local before flipping UI state -- user shouldn't keep hearing + // local audio while we queue up Sonos. + playerController.pause() selectedUpnpRouteIdInternal.value = effectiveRoute.id - activeUpnpHolder.set( - ActiveUpnp( - routeId = effectiveRoute.id, - routeName = effectiveRoute.name, - avTransport = transport, - rendering = rendering, - ), - ) + // Mark UPnP loading. ForwardingPlayer overrides drop transport commands + // silently while target is set but active is null -- the user's premature + // taps don't hit Sonos's stale state from a prior session. + activeUpnpHolder.setTarget(effectiveRoute.id) runCatching { loadQueueOnSonos(transport, effectiveRoute, uiState.queue, uiState.queueIndex) + // Wire active LAST -- SOAP path is now safe to use. + activeUpnpHolder.set( + ActiveUpnp( + routeId = effectiveRoute.id, + routeName = effectiveRoute.name, + avTransport = transport, + rendering = rendering, + ), + ) }.onFailure { e -> Timber.w(e, "UPnP select failed for route ${effectiveRoute.id}") activeUpnpHolder.set(null) + activeUpnpHolder.setTarget(null) selectedUpnpRouteIdInternal.value = null } }