From c556388a6bf4fa819ef842b4ac205ffe970c3d84 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 3 Jun 2026 19:47:19 -0400 Subject: [PATCH] fix(android): UPnP activation order -- pause local before holder.set; sync+preQueue sequential; diagnostic Timber.w - OutputPickerController.selectUpnp: pause ExoPlayer BEFORE setting activeUpnpHolder so ForwardingPlayer.pause() routes to ExoPlayer, not SOAP; remove now-redundant playerController.pause() from inside runCatching; bump activation Timber.i -> Timber.w for release logcat - MinstrelForwardingPlayer: remove Player.Listener onMediaItemTransition that raced with seekToNext/Prev override's syncCurrentItemToRemote; seekToNext/Prev now launch sync -> preQueueNext sequentially in one coroutine; remove early preQueueNext from onActiveChanged (raced with selectUpnp SOAP); move initial pre-queue to pollLoop, fires once trackUri lands confirming Sonos accepted SetAV+Play - Extract pollOnce from pollLoop to stay within detekt LongMethod=60; natural-advance branch now calls preQueueNext explicitly (no listener) Co-Authored-By: Claude Sonnet 4.6 --- .../player/MinstrelForwardingPlayer.kt | 137 ++++++++++-------- .../player/output/OutputPickerController.kt | 15 +- 2 files changed, 89 insertions(+), 63 deletions(-) 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 8aaedd0e..6099fb51 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 @@ -52,15 +52,10 @@ class MinstrelForwardingPlayer( scope.launch { holder.active.collect { active -> onActiveChanged(active) } } - delegate.addListener(object : Player.Listener { - override fun onMediaItemTransition( - mediaItem: androidx.media3.common.MediaItem?, - reason: Int, - ) { - val active = holder.active.value ?: return - scope.launch { preQueueNext(active) } - } - }) + // No Player.Listener -- the seekToNextMediaItem/seekToPreviousMediaItem + // overrides and pollLoop natural-advance branch each call preQueueNext + // explicitly. A listener-based path raced with the override's launched + // syncCurrentItemToRemote, producing non-deterministic SOAP order. } private fun isRemote(): Boolean = holder.active.value != null @@ -117,9 +112,13 @@ class MinstrelForwardingPlayer( // Advance the local cursor first so syncCurrentItemToRemote can read // the new currentMediaItem on the application looper. ExoPlayer stays // paused while UPnP is active (playWhenReady=false invariant), so this - // does not produce local audio. + // does not produce local audio. After the sync lands, preQueueNext + // primes the next-next track on Sonos -- sequential, not raced. super.seekToNextMediaItem() - scope.launch { syncCurrentItemToRemote(active) } + scope.launch { + syncCurrentItemToRemote(active) + preQueueNext(active) + } } override fun seekToPreviousMediaItem() { @@ -128,12 +127,12 @@ class MinstrelForwardingPlayer( super.seekToPreviousMediaItem() return } - // Advance the local cursor first so syncCurrentItemToRemote can read - // the new currentMediaItem on the application looper. ExoPlayer stays - // paused while UPnP is active (playWhenReady=false invariant), so this - // does not produce local audio. + // Same as seekToNextMediaItem: sync then pre-queue in one coroutine. super.seekToPreviousMediaItem() - scope.launch { syncCurrentItemToRemote(active) } + scope.launch { + syncCurrentItemToRemote(active) + preQueueNext(active) + } } override fun getCurrentPosition(): Long = @@ -206,59 +205,33 @@ class MinstrelForwardingPlayer( private fun onActiveChanged(active: ActiveUpnp?) { pollJob?.cancel() if (active != null) { + Timber.w("UPnP active: %s -- pollLoop starting", active.routeName) pollJob = scope.launch { pollLoop(active) } - // Pre-queue the next track immediately so Sonos can advance - // gap-free into the second track without waiting for the first - // onMediaItemTransition event (which only fires on user skips). - scope.launch { preQueueNext(active) } + // Do NOT pre-queue here: selectUpnp's SetAV+Play SOAP may not have + // landed yet. Initial pre-queue fires from pollLoop once the first + // successful poll confirms Sonos accepted the track (trackUri lands). } else { remoteState.reset() } } private suspend fun pollLoop(active: ActiveUpnp) { + var initialPreQueueDone = false while (scope.isActive && holder.active.value?.routeId == active.routeId) { - val outcome = runCatching { - // Capture before applyPositionInfo so we can detect a URI - // change that Sonos made via the pre-queued SetNextAVTransportURI - // (natural auto-advance without any user skip action). - val previousTrackUri = remoteState.currentTrackUri - val info = active.avTransport.getPositionInfo() - remoteState.applyPositionInfo( - positionMs = info.relTimeMs, - durationMs = info.trackDurationMs, - trackUri = info.trackUri, - ) - val transport = active.avTransport.getTransportInfo() - when (transport.state) { - TransportState.PLAYING -> remoteState.applyTransportPlaying() - TransportState.PAUSED -> remoteState.applyTransportPaused() - TransportState.STOPPED -> remoteState.applyTransportStopped() - TransportState.TRANSITIONING, TransportState.UNKNOWN -> Unit - } - // Detect remote-side natural advance: Sonos auto-advanced - // through SetNextAVTransportURI so the URI changed without - // any local user action. Advance the local queue cursor so - // the onMediaItemTransition listener fires preQueueNext to - // set up the next-next URI on Sonos, keeping the chain going. - // Both guards must be non-blank: previousTrackUri="" on the - // very first poll (before any state arrives) and we must not - // treat that initial empty-to-URI transition as an advance. - if (info.trackUri.isNotBlank() && - previousTrackUri.isNotBlank() && - info.trackUri != previousTrackUri - ) { - // Run on the application looper. ExoPlayer is paused - // while UPnP is active (playWhenReady=false invariant) so - // the cursor advances without producing local audio. The - // delegate's onMediaItemTransition listener (wired in init) - // then calls preQueueNext to load the next-next track. - handler.post { delegate.seekToNextMediaItem() } - } - } + val outcome = runCatching { pollOnce(active) } if (outcome.isSuccess) { remoteState.recordPollSuccess() + // Initial pre-queue fires once Sonos confirms it accepted the + // SetAV+Play from selectUpnp (trackUri lands in the first + // successful poll). Firing from onActiveChanged would race + // with selectUpnp's SOAP calls. + if (!initialPreQueueDone && remoteState.currentTrackUri.isNotBlank()) { + Timber.w("UPnP initial pre-queue for %s", active.routeName) + preQueueNext(active) + initialPreQueueDone = true + } } else if (remoteState.recordPollFailure()) { + Timber.w("UPnP drop threshold tripped for %s", active.routeName) handler.post { onDrop(active.routeName) } return } @@ -266,6 +239,54 @@ class MinstrelForwardingPlayer( } } + /** + * One poll tick: read position + transport state from Sonos, apply to + * [remoteState], and detect natural-advance (URI change driven by a + * previously queued SetNextAVTransportURI). Extracted from [pollLoop] + * to keep that method within detekt's LongMethod=60 limit. + */ + private suspend fun pollOnce(active: ActiveUpnp) { + // Capture before applyPositionInfo so we can detect a URI + // change that Sonos made via the pre-queued SetNextAVTransportURI + // (natural auto-advance without any user skip action). + val previousTrackUri = remoteState.currentTrackUri + val info = active.avTransport.getPositionInfo() + remoteState.applyPositionInfo( + positionMs = info.relTimeMs, + durationMs = info.trackDurationMs, + trackUri = info.trackUri, + ) + val transport = active.avTransport.getTransportInfo() + when (transport.state) { + TransportState.PLAYING -> remoteState.applyTransportPlaying() + TransportState.PAUSED -> remoteState.applyTransportPaused() + TransportState.STOPPED -> remoteState.applyTransportStopped() + TransportState.TRANSITIONING, TransportState.UNKNOWN -> Unit + } + // Detect remote-side natural advance: Sonos auto-advanced + // through SetNextAVTransportURI so the URI changed without + // any local user action. Advance the local queue cursor and + // call preQueueNext explicitly to set up the next-next URI on + // Sonos. Both guards must be non-blank: previousTrackUri="" on + // the very first poll and we must not treat that initial + // empty-to-URI transition as an advance. + if (info.trackUri.isNotBlank() && + previousTrackUri.isNotBlank() && + info.trackUri != previousTrackUri + ) { + Timber.w( + "UPnP natural-advance detected: %s -> %s", + previousTrackUri, + info.trackUri, + ) + // Run on the application looper. ExoPlayer is paused + // while UPnP is active (playWhenReady=false invariant) so + // the cursor advances without producing local audio. + handler.post { delegate.seekToNextMediaItem() } + preQueueNext(active) + } + } + /** Run [block] on the application looper and bring its result back. */ private suspend fun handlerEvaluate(block: () -> T): T? { val deferred = CompletableDeferred() 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 da21cd22..096a3ef5 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 @@ -258,6 +258,12 @@ class OutputPickerController @Inject constructor( return@withLock } val rendering = renderingClientFor(effectiveRoute.id) + // Pause local FIRST while holder.active is still null so the pause() + // call routes to ExoPlayer, not SOAP. After holder.set() below, + // ForwardingPlayer.pause() would translate this into AVTransport.Pause() + // -- the opposite of what we want, since we are about to tell Sonos + // to start playing. + playerController.pause() selectedUpnpRouteIdInternal.value = effectiveRoute.id activeUpnpHolder.set( ActiveUpnp( @@ -268,18 +274,17 @@ class OutputPickerController @Inject constructor( ), ) runCatching { - Timber.i("UPnP select: mint token for track=$trackId, route=${effectiveRoute.name}") + Timber.w("UPnP select: mint token for track=$trackId, route=${effectiveRoute.name}") val token = streamTokens.mint(trackId) - Timber.i("UPnP select: SetAVTransportURI to ${token.url} mime=${token.mime}") + Timber.w("UPnP select: SetAVTransportURI to ${token.url} mime=${token.mime}") transport.setAVTransportURIWithMetadata( uri = token.url, mime = token.mime, title = token.title, ) - Timber.i("UPnP select: Play") + Timber.w("UPnP select: Play") transport.play() - playerController.pause() - Timber.i("UPnP select: done") + Timber.w("UPnP select: done") }.onFailure { e -> Timber.w(e, "UPnP select failed for route ${effectiveRoute.id}") activeUpnpHolder.set(null)