From ece37e9a925ec4dbb322f2da06c1f1632e337193 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 3 Jun 2026 21:42:01 -0400 Subject: [PATCH] fix(android): remove pollLoop natural-advance -- races with selectUpnp and skip Polling alone cannot distinguish Sonos auto-advancing via SetNextAVTransportURI from URI changes we made ourselves via syncCurrentItemToRemote. This produced two races: (1) activation race -- first poll returns stale URI from prior session, second returns new URI, false-positive fires and double-advances the cursor; (2) user-skip race -- skip's syncCurrentItemToRemote changes the URI, next poll sees the change and fires again. Remove the detection block and previousTrackUri capture from pollOnce entirely. pollLoop is now a pure state-tracker (position + transport state) plus the one-shot initial pre-queue gate. GENA event subscriptions to AVTransport LastChange are the correct fix; deferred to its own slice (see parity-map). Co-Authored-By: Claude Sonnet 4.6 --- .../player/MinstrelForwardingPlayer.kt | 56 ++++++------------- 1 file changed, 18 insertions(+), 38 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 34cec946..bb56e9e6 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 @@ -53,9 +53,9 @@ class MinstrelForwardingPlayer( holder.active.collect { active -> onActiveChanged(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. + // overrides 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 @@ -169,11 +169,9 @@ class MinstrelForwardingPlayer( /** * Speculative pre-queue of the next track so Sonos can advance gap-free. - * Failures (token mint OR SetNextAVTransportURI rejection) silently fall - * back to the poll-driven advance path -- the pollLoop detects STOPPED + - * 0:00 RelTime and calls syncCurrentItemToRemote on whatever the new - * currentMediaItem is. The only consequence of failure here is a sub-second - * audible gap at track boundary, not a broken route. + * Failures (token mint OR SetNextAVTransportURI rejection) are logged and + * swallowed. The only consequence is a sub-second audible gap at the track + * boundary, not a broken route. */ private suspend fun preQueueNext(active: ActiveUpnp) { val nextMediaId = handlerEvaluate { nextItemMediaId() } ?: return @@ -223,6 +221,15 @@ class MinstrelForwardingPlayer( } private suspend fun pollLoop(active: ActiveUpnp) { + // pollLoop is a state-tracker for position + transport state. We do NOT + // attempt to mirror Sonos's auto-advance into the local queue cursor -- + // that would require GENA event subscriptions to AVTransport's LastChange + // event (see the parity-map). Without that, polling alone cannot + // distinguish "Sonos finished track N and auto-played track N+1 via + // SetNextAVTransportURI" from "we just changed Sonos's URI ourselves + // via syncCurrentItemToRemote". The local cursor stays in sync via user + // transport actions (pause/seek/skip flow through the override which + // re-runs syncCurrentItemToRemote). var initialPreQueueDone = false while (scope.isActive && holder.active.value?.routeId == active.routeId) { val outcome = runCatching { pollOnce(active) } @@ -255,16 +262,11 @@ 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. + * One poll tick: read position + transport state from Sonos and apply to + * [remoteState]. 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, @@ -278,28 +280,6 @@ class MinstrelForwardingPlayer( 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. */