From fd7d6dac4c4349c2af24b8346f57e8e77a74dc62 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 12 Jun 2026 21:20:26 -0400 Subject: [PATCH] feat(android): adopt a running UPnP session + tighten cast recovery/anti-stickiness Three related improvements to UPnP/Sonos session handling, on top of the WiFi-lock + drop-suppression fixes. 1. Adopt a running session instead of clear+reload (the headline). Selecting a renderer always did removeAllTracksFromQueue + full reload -- a jarring restart if the speaker was already playing our queue (e.g. after the phone got disconnected but the autonomous Sonos kept going). selectUpnp now probes the renderer first; if it's mid-playback on the same track id at the same queue index, we ATTACH in place: sync the local cursor to its position, wire ActiveUpnpHolder, start polling -- no clear, no reload, and skip/seek immediately drive its live queue. Falls through to clear+reload when it isn't our queue. New PlayerController.moveCursorTo aligns the local cursor without auto-playing. 2. Discovery expiry + selection revert (anti-stickiness). upsertRoute only ever added, so a powered-off renderer lingered in the picker forever and could pin a stale selection. Stamp lastSeen per route; after the picker's active M-SEARCH scan, prune routes that didn't re-announce. A collector reverts the selection to the phone when the selected route leaves discovery while we're not actively casting -- so a later play never targets a ghost. Pruning is tied to picker-open scans only (no background timer -> no row flicker). 3. Reconcile immediately on network recovery. When NetworkStatus flips back to Healthy while a route is active, nudge an immediate poll instead of waiting up to POLL_INTERVAL_MS -- the held session re-confirms the renderer in one round-trip. Verified (read-only): the tap-play-onto-dead-route fallback still fires when the phone's network is Healthy (the poll-loop drop path is unchanged for that case); the drop-suppression gate only holds during phone-side outages, where a local fallback couldn't play either. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../player/MinstrelForwardingPlayer.kt | 14 ++ .../minstrel/player/PlayerController.kt | 14 ++ .../player/output/OutputPickerController.kt | 130 ++++++++++++++++++ .../output/upnp/UpnpDiscoveryController.kt | 40 +++++- 4 files changed, 197 insertions(+), 1 deletion(-) 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 9fe589b5..49f0cdc5 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 @@ -120,6 +120,20 @@ class MinstrelForwardingPlayer( scope.launch { holder.active.collect { active -> onActiveChanged(active) } } + scope.launch { + // When the phone's own network recovers, reconcile a held cast + // session immediately instead of waiting up to POLL_INTERVAL_MS: + // the renderer kept playing, so one round-trip re-confirms it and + // resyncs the cursor/position. Only a transition *into* Healthy + // while a route is active matters. + var prev = networkStatus.state.value + networkStatus.state.collect { next -> + if (next == ServerHealth.Healthy && prev != ServerHealth.Healthy && isRemote()) { + pollTrigger.trySend(Unit) + } + prev = next + } + } // Process lifecycle is observed on the main thread; ProcessLifecycleOwner's // addObserver requires it. The observer just trySend's to the channel. handler.post { 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 e0f49216..f571f6a9 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 @@ -192,6 +192,20 @@ class PlayerController @Inject constructor( controller.play() } + /** + * Align the local cursor to [index] + [positionMs] WITHOUT auto-playing. + * Used by the UPnP adopt path: when attaching to a renderer that's already + * playing our queue, we sync the local cursor to the renderer's current + * track/position so skip/seek and the UI map onto its live session — the + * renderer keeps the audio, the local player stays paused. Unlike + * [seekToIndex] this issues no play(). + */ + fun moveCursorTo(index: Int, positionMs: Long) { + val controller = mediaController ?: return + if (index !in queueRefs.indices) return + runOnControllerThread(controller) { controller.seekTo(index, positionMs) } + } + /** * Replace the queue with [tracks] starting at [initialIndex]. * [source] tags the queue with its origin (e.g. "for_you") so the 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 8d690954..80d9a6b1 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 @@ -15,6 +15,7 @@ import com.fabledsword.minstrel.player.output.upnp.AVTransportClient import com.fabledsword.minstrel.player.output.upnp.RenderingControlClient import com.fabledsword.minstrel.player.output.upnp.SoapClient import com.fabledsword.minstrel.player.output.upnp.SoapFaultException +import com.fabledsword.minstrel.player.output.upnp.TransportState import com.fabledsword.minstrel.player.output.upnp.UpnpDiscoveryController import com.fabledsword.minstrel.player.output.upnp.bareUdn import dagger.hilt.android.qualifiers.ApplicationContext @@ -29,6 +30,7 @@ import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import okhttp3.OkHttpClient import timber.log.Timber import javax.inject.Inject @@ -172,6 +174,31 @@ class OutputPickerController @Inject constructor( } scope.launch { observeQueueChangesForSonosResync() } scope.launch { observeIdleRevertWhileUpnp() } + scope.launch { observeSelectedRouteDisappearance() } + } + + /** + * Revert the UPnP selection to the phone when the selected renderer leaves + * discovery (powered off / off the LAN) while we are NOT actively casting + * to it. Without this the selection could pin to a ghost route, so a later + * "play" would target a speaker that's gone. An *active* cast is left alone + * -- its own poll loop arbitrates liveness and won't drop on a mere + * discovery gap. Routes leave discovery via [UpnpDiscoveryController]'s + * post-picker-scan prune; this reacts to the resulting routes change. + */ + private suspend fun observeSelectedRouteDisappearance() { + upnpDiscovery.routes.collect { routes -> + val selected = selectedUpnpRouteIdInternal.value + val present = routes.any { it.id == selected } + val casting = activeUpnpHolder.active.value?.routeId == selected + if (selected != null && !present && !casting) { + Timber.w( + "Output: selected UPnP route %s left discovery; reverting to phone", + selected, + ) + selectedUpnpRouteIdInternal.value = null + } + } } /** @@ -541,6 +568,13 @@ class OutputPickerController @Inject constructor( // local audio while we queue up Sonos. playerController.pause() selectedUpnpRouteIdInternal.value = effectiveRoute.id + // If the renderer is already playing this exact queue (a "stray" + // session we got disconnected from), attach to it in place instead of + // wiping + reloading -- no jarring restart, and skip/seek land on its + // live queue. Falls through to the clear+reload path when it isn't ours. + if (tryAdoptRunningSession(transport, rendering, effectiveRoute, uiState.queue)) { + return@withLock + } // 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. @@ -567,6 +601,102 @@ class OutputPickerController @Inject constructor( } } + /** + * The renderer's current position + transport state, validated as an + * adoptable continuation of our local queue: same track id at the same + * 1-based index. Null means "not ours / not playing" -> caller reloads. + */ + private data class AdoptPlan( + val localIndex: Int, + val positionMs: Long, + val durationMs: Long, + val trackUri: String, + val trackNumber: Int, + val playing: Boolean, + ) + + /** + * Attach to a renderer that is *already* playing our queue (a session the + * phone got disconnected from but the speaker kept streaming) instead of + * clearing + reloading it. Probes the renderer's live state, and if it's + * mid-playback on the same track id at the same queue index, syncs the + * local cursor to match and wires [ActiveUpnpHolder] -- the speaker never + * skips a beat, and skip/seek now drive its live queue. Returns false (and + * the caller does the full clear+reload) when the renderer isn't playing + * our queue. The local player is already paused by the caller. + */ + private suspend fun tryAdoptRunningSession( + transport: AVTransportClient, + rendering: RenderingControlClient?, + route: OutputRoute, + localQueue: List, + ): Boolean { + val plan = probeAdoptable(transport, route, localQueue) ?: return false + Timber.w( + "UPnP adopt: attaching to live session on %s at track %d (%dms)", + route.name, plan.trackNumber, plan.positionMs, + ) + // active is still null here, so this aligns the *local* cursor without + // emitting SOAP; the holder.set below then starts the poll loop, which + // reconciles to the renderer's advancing position from here. + playerController.moveCursorTo(plan.localIndex, plan.positionMs) + remoteState.applyPositionInfo( + positionMs = plan.positionMs, + durationMs = plan.durationMs, + trackUri = plan.trackUri, + trackNumber = plan.trackNumber, + ) + remoteState.setPlayIntent(plan.playing) + activeUpnpHolder.set( + ActiveUpnp( + routeId = route.id, + routeName = route.name, + avTransport = transport, + rendering = rendering, + ), + ) + return true + } + + private suspend fun probeAdoptable( + transport: AVTransportClient, + route: OutputRoute, + localQueue: List, + ): AdoptPlan? { + val probe = runCatching { + transport.getTransportInfo() to transport.getPositionInfo() + }.getOrElse { + Timber.w(it, "UPnP adopt: probe failed for %s; will reload", route.name) + return null + } + val (info, pos) = probe + val idx0 = pos.track - 1 + val playingId = trackIdFromStreamUrl(pos.trackUri) + val midPlayback = + info.state == TransportState.PLAYING || info.state == TransportState.PAUSED + val aligned = midPlayback && playingId != null && + idx0 in localQueue.indices && localQueue[idx0].id == playingId + return if (aligned) { + AdoptPlan( + localIndex = idx0, + positionMs = pos.relTimeMs, + durationMs = pos.trackDurationMs, + trackUri = pos.trackUri, + trackNumber = pos.track, + playing = info.state == TransportState.PLAYING, + ) + } else { + null + } + } + + /** Extract our track id from a stream URL `.../api/tracks//stream...`. */ + private fun trackIdFromStreamUrl(url: String): String? { + val segments = url.toHttpUrlOrNull()?.pathSegments ?: return null + val i = segments.indexOf("tracks") + return if (i >= 0) segments.getOrNull(i + 1) else null + } + private suspend fun loadQueueOnSonos( transport: AVTransportClient, route: OutputRoute, diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/output/upnp/UpnpDiscoveryController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/output/upnp/UpnpDiscoveryController.kt index cbf22ddc..8426c875 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/output/upnp/UpnpDiscoveryController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/output/upnp/UpnpDiscoveryController.kt @@ -8,6 +8,7 @@ import com.fabledsword.minstrel.player.output.upnp.sonos.ZoneGroupTopologyClient import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow @@ -17,6 +18,7 @@ import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import okhttp3.OkHttpClient import okhttp3.Request import timber.log.Timber +import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.TimeUnit import javax.inject.Inject import javax.inject.Singleton @@ -58,6 +60,12 @@ class UpnpDiscoveryController @Inject constructor( private val routesInternal = MutableStateFlow>(emptyList()) val routes: StateFlow> = routesInternal.asStateFlow() + // Wall-clock of the last NOTIFY / M-SEARCH response per route id. Drives + // post-active-scan pruning so a powered-off renderer leaves the picker + // (and can't pin a stale selection) instead of lingering for the process + // lifetime -- upsert only ever added before. + private val lastSeenMs = ConcurrentHashMap() + private val sonosTopologyInternal = MutableStateFlow>(emptyList()) val sonosTopology: StateFlow> = sonosTopologyInternal.asStateFlow() @@ -78,7 +86,31 @@ class UpnpDiscoveryController @Inject constructor( * that just opened). */ fun upgradeDiscovery() { - appScope.launch { ssdp.requestActiveScan() } + appScope.launch { + val scanStartMs = System.currentTimeMillis() + ssdp.requestActiveScan() + // Give present devices time to answer the M-SEARCH (+ our follow-up + // description fetch), then drop any route that didn't re-announce. + // Pruning is tied to picker-open scans only -- no background timer, + // so route rows never flicker while the user isn't looking. + delay(ACTIVE_SCAN_PRUNE_DELAY_MS) + pruneRoutesNotSeenSince(scanStartMs) + } + } + + /** + * Drop routes whose last NOTIFY / M-SEARCH response predates [cutoffMs] + * -- i.e. they didn't answer the active scan that just ran, so they're + * gone from the LAN. Called only after [upgradeDiscovery]'s scan window. + */ + private fun pruneRoutesNotSeenSince(cutoffMs: Long) { + val before = routesInternal.value + val survivors = before.filter { (lastSeenMs[it.id] ?: 0L) >= cutoffMs } + if (survivors.size == before.size) return + val removed = before.filterNot { it in survivors }.map { it.id } + removed.forEach { lastSeenMs.remove(it) } + Timber.w("UPnP discovery: pruned %d stale route(s): %s", removed.size, removed) + routesInternal.value = survivors } /** @@ -115,6 +147,7 @@ class UpnpDiscoveryController @Inject constructor( } private fun upsertRoute(route: UpnpRoute) { + lastSeenMs[route.id] = System.currentTimeMillis() val current = routesInternal.value val idx = current.indexOfFirst { it.id == route.id } routesInternal.value = if (idx < 0) { @@ -240,6 +273,11 @@ class UpnpDiscoveryController @Inject constructor( // trips the drop-recovery quickly. Conservative enough not to false-drop // a slow-but-alive LAN; read timeout still inherits the shared client. const val CONTROL_CONNECT_TIMEOUT_SECONDS = 2L + + // Grace window after an M-SEARCH burst before pruning non-responders. + // Covers the SSDP round-trip plus our per-device description fetch; + // generous enough that a present-but-slow renderer isn't false-pruned. + const val ACTIVE_SCAN_PRUNE_DELAY_MS = 4_000L } }