fix(android): UPnP activation order -- pause local before holder.set; sync+preQueue sequential; diagnostic Timber.w
android / Build + lint + test (push) Failing after 1m28s

- 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 <noreply@anthropic.com>
This commit is contained in:
2026-06-03 19:47:19 -04:00
parent edffdec2b2
commit c556388a6b
2 changed files with 89 additions and 63 deletions
@@ -52,15 +52,10 @@ class MinstrelForwardingPlayer(
scope.launch { scope.launch {
holder.active.collect { active -> onActiveChanged(active) } holder.active.collect { active -> onActiveChanged(active) }
} }
delegate.addListener(object : Player.Listener { // No Player.Listener -- the seekToNextMediaItem/seekToPreviousMediaItem
override fun onMediaItemTransition( // overrides and pollLoop natural-advance branch each call preQueueNext
mediaItem: androidx.media3.common.MediaItem?, // explicitly. A listener-based path raced with the override's launched
reason: Int, // syncCurrentItemToRemote, producing non-deterministic SOAP order.
) {
val active = holder.active.value ?: return
scope.launch { preQueueNext(active) }
}
})
} }
private fun isRemote(): Boolean = holder.active.value != null private fun isRemote(): Boolean = holder.active.value != null
@@ -117,9 +112,13 @@ class MinstrelForwardingPlayer(
// Advance the local cursor first so syncCurrentItemToRemote can read // Advance the local cursor first so syncCurrentItemToRemote can read
// the new currentMediaItem on the application looper. ExoPlayer stays // the new currentMediaItem on the application looper. ExoPlayer stays
// paused while UPnP is active (playWhenReady=false invariant), so this // 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() super.seekToNextMediaItem()
scope.launch { syncCurrentItemToRemote(active) } scope.launch {
syncCurrentItemToRemote(active)
preQueueNext(active)
}
} }
override fun seekToPreviousMediaItem() { override fun seekToPreviousMediaItem() {
@@ -128,12 +127,12 @@ class MinstrelForwardingPlayer(
super.seekToPreviousMediaItem() super.seekToPreviousMediaItem()
return return
} }
// Advance the local cursor first so syncCurrentItemToRemote can read // Same as seekToNextMediaItem: sync then pre-queue in one coroutine.
// the new currentMediaItem on the application looper. ExoPlayer stays
// paused while UPnP is active (playWhenReady=false invariant), so this
// does not produce local audio.
super.seekToPreviousMediaItem() super.seekToPreviousMediaItem()
scope.launch { syncCurrentItemToRemote(active) } scope.launch {
syncCurrentItemToRemote(active)
preQueueNext(active)
}
} }
override fun getCurrentPosition(): Long = override fun getCurrentPosition(): Long =
@@ -206,19 +205,47 @@ class MinstrelForwardingPlayer(
private fun onActiveChanged(active: ActiveUpnp?) { private fun onActiveChanged(active: ActiveUpnp?) {
pollJob?.cancel() pollJob?.cancel()
if (active != null) { if (active != null) {
Timber.w("UPnP active: %s -- pollLoop starting", active.routeName)
pollJob = scope.launch { pollLoop(active) } pollJob = scope.launch { pollLoop(active) }
// Pre-queue the next track immediately so Sonos can advance // Do NOT pre-queue here: selectUpnp's SetAV+Play SOAP may not have
// gap-free into the second track without waiting for the first // landed yet. Initial pre-queue fires from pollLoop once the first
// onMediaItemTransition event (which only fires on user skips). // successful poll confirms Sonos accepted the track (trackUri lands).
scope.launch { preQueueNext(active) }
} else { } else {
remoteState.reset() remoteState.reset()
} }
} }
private suspend fun pollLoop(active: ActiveUpnp) { private suspend fun pollLoop(active: ActiveUpnp) {
var initialPreQueueDone = false
while (scope.isActive && holder.active.value?.routeId == active.routeId) { while (scope.isActive && holder.active.value?.routeId == active.routeId) {
val outcome = runCatching { 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
}
delay(POLL_INTERVAL_MS)
}
}
/**
* 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 // Capture before applyPositionInfo so we can detect a URI
// change that Sonos made via the pre-queued SetNextAVTransportURI // change that Sonos made via the pre-queued SetNextAVTransportURI
// (natural auto-advance without any user skip action). // (natural auto-advance without any user skip action).
@@ -238,31 +265,25 @@ class MinstrelForwardingPlayer(
} }
// Detect remote-side natural advance: Sonos auto-advanced // Detect remote-side natural advance: Sonos auto-advanced
// through SetNextAVTransportURI so the URI changed without // through SetNextAVTransportURI so the URI changed without
// any local user action. Advance the local queue cursor so // any local user action. Advance the local queue cursor and
// the onMediaItemTransition listener fires preQueueNext to // call preQueueNext explicitly to set up the next-next URI on
// set up the next-next URI on Sonos, keeping the chain going. // Sonos. Both guards must be non-blank: previousTrackUri="" on
// Both guards must be non-blank: previousTrackUri="" on the // the very first poll and we must not treat that initial
// very first poll (before any state arrives) and we must not // empty-to-URI transition as an advance.
// treat that initial empty-to-URI transition as an advance.
if (info.trackUri.isNotBlank() && if (info.trackUri.isNotBlank() &&
previousTrackUri.isNotBlank() && previousTrackUri.isNotBlank() &&
info.trackUri != previousTrackUri info.trackUri != previousTrackUri
) { ) {
Timber.w(
"UPnP natural-advance detected: %s -> %s",
previousTrackUri,
info.trackUri,
)
// Run on the application looper. ExoPlayer is paused // Run on the application looper. ExoPlayer is paused
// while UPnP is active (playWhenReady=false invariant) so // while UPnP is active (playWhenReady=false invariant) so
// the cursor advances without producing local audio. The // the cursor advances without producing local audio.
// delegate's onMediaItemTransition listener (wired in init)
// then calls preQueueNext to load the next-next track.
handler.post { delegate.seekToNextMediaItem() } handler.post { delegate.seekToNextMediaItem() }
} preQueueNext(active)
}
if (outcome.isSuccess) {
remoteState.recordPollSuccess()
} else if (remoteState.recordPollFailure()) {
handler.post { onDrop(active.routeName) }
return
}
delay(POLL_INTERVAL_MS)
} }
} }
@@ -258,6 +258,12 @@ class OutputPickerController @Inject constructor(
return@withLock return@withLock
} }
val rendering = renderingClientFor(effectiveRoute.id) 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 selectedUpnpRouteIdInternal.value = effectiveRoute.id
activeUpnpHolder.set( activeUpnpHolder.set(
ActiveUpnp( ActiveUpnp(
@@ -268,18 +274,17 @@ class OutputPickerController @Inject constructor(
), ),
) )
runCatching { 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) 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( transport.setAVTransportURIWithMetadata(
uri = token.url, uri = token.url,
mime = token.mime, mime = token.mime,
title = token.title, title = token.title,
) )
Timber.i("UPnP select: Play") Timber.w("UPnP select: Play")
transport.play() transport.play()
playerController.pause() Timber.w("UPnP select: done")
Timber.i("UPnP select: done")
}.onFailure { e -> }.onFailure { e ->
Timber.w(e, "UPnP select failed for route ${effectiveRoute.id}") Timber.w(e, "UPnP select failed for route ${effectiveRoute.id}")
activeUpnpHolder.set(null) activeUpnpHolder.set(null)