From 41fe76b90c47687fb9058eb7f5debc844666bcfc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 15 Jul 2026 14:58:49 -0400 Subject: [PATCH] =?UTF-8?q?fix(player):=20identity-locked=20UPnP=20cursor?= =?UTF-8?q?=20+=20single=20index=20writer=20=E2=80=94=20#171?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local ExoPlayer cursor and the Sonos renderer were two competing sources of truth for "what's playing" during a cast. The delegate cursor lagged (forward-only, index-based, size-capped sync, skipped during every load/re-cast window), and TWO writers of PlayerUiState.queueIndex fought: PlayerController.onEvents (reading the lagging cursor) stomped the Sonos-derived index the position tick published, so the in-app player flickered to the pre-cast track and the notification metadata went stale. Step 1 — MinstrelForwardingPlayer.syncLocalCursorToRemote (replaces maybeSyncLocalCursor): align the paused delegate cursor to the track the renderer is actually playing, matched by track-id parsed from the Sonos TrackURI (/api/tracks/{id}/stream) against delegate MediaItem.mediaId (== TrackRef.id). Both directions; survives queue-reload index wobble; nearest-occurrence tiebreak for duplicate tracks; falls back to the Sonos Track index; suppressed during load and while a user transport is pending Sonos's ack. The cursor is now the single authoritative "current track" that both the in-app UI (onEvents) and the notification (getCurrentMediaItem) read. Step 2 — PlayerController: the position tick now patches only position/duration/play-pause/buffer; onEvents is the sole writer of queueIndex/currentTrack. Removed desiredQueueIndex, the forward-only trackChanged path, and publishTickIfChanged. One writer, no stomp. Part of milestone #171 (unify local + UPnP behind one cursor). Fixes the flicker + stale-notification symptoms; supersedes #1211/#608/#612. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../player/MinstrelForwardingPlayer.kt | 113 ++++++++++++++---- .../minstrel/player/PlayerController.kt | 87 ++++---------- 2 files changed, 113 insertions(+), 87 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 49f0cdc5..eb8c52bd 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 @@ -17,6 +17,7 @@ import com.fabledsword.minstrel.player.output.ActiveUpnpHolder import com.fabledsword.minstrel.player.output.upnp.SoapFaultException import com.fabledsword.minstrel.player.output.upnp.TransportState import java.io.IOException +import kotlin.math.abs import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job @@ -58,8 +59,10 @@ import timber.log.Timber * native queue via ClearQueue + AddURIToQueue, then points the * transport at x-rincon-queue:#0. Skip/prev/seekTo delegate to * AVTransport Next/Previous/SeekToTrack so Sonos manages gap-free - * advance natively. PollLoop syncs the local cursor by comparing the - * 1-based Track index from GetPositionInfo. + * advance natively. PollLoop keeps the local cursor aligned to the + * track the renderer is actually playing, matched by track identity + * (the id in GetPositionInfo's TrackURI) so it survives queue-reload + * index drift -- see [syncLocalCursorToRemote]. */ class MinstrelForwardingPlayer( private val delegate: Player, @@ -526,18 +529,8 @@ class MinstrelForwardingPlayer( /** * One poll tick: read position + transport state from Sonos, apply to - * [remoteState], and forward-sync the local cursor to Sonos's Track - * index when not in queue load. - * - * Cursor sync is gated on `holder.target == null` (= not loading) - * because during load Sonos reports Track=1 while we're still - * appending, and syncing would race the SetAV+Seek that lands - * after. Outside load, forward sync catches Sonos auto-advances - * (queue end-of-track), Sonos-app driven Next presses, and any - * drift after a brief poll-failure burst that didn't trip the - * drop threshold. Forward-only because a Next override we just - * issued can race with a poll still reporting the prior Track -- - * the next poll catches up safely. + * [remoteState], and sync the local cursor to the track the renderer is + * actually playing (see [syncLocalCursorToRemote]). */ private suspend fun pollOnce(active: ActiveUpnp) { val info = active.avTransport.getPositionInfo() @@ -553,7 +546,7 @@ class MinstrelForwardingPlayer( trackUri = info.trackUri, trackNumber = info.track, ) - maybeSyncLocalCursor(info.track) + syncLocalCursorToRemote(sonosTrack = info.track, trackUri = info.trackUri) val transport = active.avTransport.getTransportInfo() when (transport.state) { TransportState.PLAYING -> { @@ -577,23 +570,95 @@ class MinstrelForwardingPlayer( notifyRemoteStateChanged() } - private fun maybeSyncLocalCursor(sonosTrack: Int) { - if (holder.target.value != null) return - if (sonosTrack <= 0) return - val sonosIdx = sonosTrack - 1 + /** + * Align the paused local delegate cursor to the track the renderer is + * actually playing, so the un-overridden current-item getters + * (getCurrentMediaItem/Index -- what both the in-app player UI via + * PlayerController.onEvents AND the MediaSession notification read) always + * resolve to that track. This is THE single source of truth for "what's + * playing" during a cast. + * + * Identity-first: match the track-id embedded in the renderer's current URI + * against the delegate's MediaItem.mediaId (PlayerController sets mediaId = + * TrackRef.id), so the index-offset wobble across queue reloads / re-casts + * can never park the cursor on a stale (pre-cast) track. Falls back to the + * 1-based Sonos Track index only when the URI can't be resolved to a queue + * item. Unlike the old forward-only sync this moves the cursor in BOTH + * directions -- a renderer Previous / re-cast to a lower track pulls it back + * too. Runs on the application looper (delegate access contract). + * + * Suppressed while: + * - loading ([isLoadingUpnp]) -- the native Sonos queue is still being + * (re)uploaded, so its Track index is meaningless; and + * - a user transport (next/prev/seekTo idx) is pending Sonos's ack -- + * the override already advanced the delegate optimistically, and Sonos + * still reports the OLD track for a beat, so syncing now would undo the + * user's press. The pending deadline is honoured directly so a missed + * clear can't wedge the sync. + */ + private fun syncLocalCursorToRemote(sonosTrack: Int, trackUri: String) { + if (isLoadingUpnp()) return + val pendingDeadline = remoteState.pendingTransportDeadlineMs + if (pendingDeadline > 0L && SystemClock.elapsedRealtime() < pendingDeadline) return handler.post { - val localIdx = delegate.currentMediaItemIndex - if (sonosIdx > localIdx && sonosIdx < delegate.mediaItemCount) { + val target = resolveRemoteCursorIndex(sonosTrack, trackUri) ?: return@post + if (target != delegate.currentMediaItemIndex) { Timber.w( - "UPnP cursor catch-up: local=%d -> sonos=%d", - localIdx, sonosIdx, + "UPnP cursor sync: local=%d -> %d", + delegate.currentMediaItemIndex, target, ) - delegate.seekTo(sonosIdx, 0L) + delegate.seekTo(target, 0L) } } } + /** + * Most-authoritative -> most-degraded (project rule #48): the track-id + * parsed from the renderer's current URI wins; else the 1-based Sonos Track + * index; else null (leave the cursor put rather than surface a wrong track). + * Must run on the application looper -- reads the delegate timeline. + */ + private fun resolveRemoteCursorIndex(sonosTrack: Int, trackUri: String): Int? { + val byId = trackIdFromStreamUri(trackUri)?.let { id -> + nearestIndexWithMediaId(id, preferNear = sonosTrack - 1) + } + return byId ?: (sonosTrack - 1).takeIf { it in 0 until delegate.mediaItemCount } + } + + /** + * Index of the delegate MediaItem whose mediaId == [mediaId], preferring the + * occurrence nearest [preferNear] so a track that appears twice in the queue + * doesn't snap the cursor across the queue. null if the id isn't present. + */ + private fun nearestIndexWithMediaId(mediaId: String, preferNear: Int): Int? { + var best: Int? = null + var bestDist = Int.MAX_VALUE + for (i in 0 until delegate.mediaItemCount) { + if (delegate.getMediaItemAt(i).mediaId == mediaId) { + val dist = abs(i - preferNear) + if (dist < bestDist) { + best = i + bestDist = dist + } + } + } + return best + } + + /** Extract `{id}` from a `.../api/tracks/{id}/stream...` stream URI. */ + private fun trackIdFromStreamUri(uri: String): String? { + val start = uri.indexOf(TRACKS_PATH_MARKER) + if (start < 0) return null + val idStart = start + TRACKS_PATH_MARKER.length + val idEnd = uri.indexOf('/', idStart) + return if (idEnd > idStart) uri.substring(idStart, idEnd) else null + } + private companion object { + // Path segment that precedes the track-id in a stream URI + // (…/api/tracks/{id}/stream…) — used to map the renderer's current URI + // back to a delegate MediaItem by identity. + const val TRACKS_PATH_MARKER = "/api/tracks/" const val POLL_INTERVAL_MS = 1_000L const val NON_PLAYING_CONFIRM = 2 const val SEEK_ACK_WINDOW_MS = 2_000L 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 5e200171..6af12b3c 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 @@ -598,18 +598,19 @@ class PlayerController @Inject constructor( } /** - * One position-polling tick. Owns track-change detection too: when UPnP - * is active and the wrapped ExoPlayer is paused, `delegate.seekTo` from - * `maybeSyncLocalCursor` may not fire `onMediaItemTransition`, leaving - * uiState.queueIndex stuck on the old track even after Sonos has - * advanced. So the tick reads Sonos's reported Track as the source of - * truth, rebuilds the index/title fields itself, and force-syncs the - * wrapped player as defense in depth. + * One position-polling tick. Patches ONLY the smoothly-changing transport + * fields (position / duration / play-pause / buffer) via `.copy()`. The + * queue index + current track are owned solely by [onEvents], which reads + * the authoritative delegate cursor: during a cast, + * [MinstrelForwardingPlayer.syncLocalCursorToRemote] keeps that cursor on + * the track the renderer is actually playing (matched by identity), so there + * is exactly one writer of "what's playing" and a stale tick can never + * revert it to the old track. (Previously the tick was a second index writer + * that fought onEvents — the flicker-to-stale-track bug.) */ private fun tickPositionPoll(controller: MediaController) { val upnpActive = activeUpnpHolder.active.value != null resolvePendingTransport(controller, upnpActive) - val pendingTransport = upnpActive && remoteState.pendingTransportDeadlineMs > 0L val effectiveIsPlaying = if (upnpActive) remoteState.isPlaying else controller.isPlaying val effectivePosition = if (upnpActive) { @@ -617,27 +618,29 @@ class PlayerController @Inject constructor( } else { controller.currentPosition } - val desiredIdx = desiredQueueIndex(controller, upnpActive) + val idx = controller.currentMediaItemIndex val current = uiStateInternal.value val newPos = effectivePosition.coerceAtLeast(0) val newDur = effectiveDuration( upnpActive, remoteState.durationMs, controller.duration, - desiredIdx = desiredIdx, - controllerIdx = controller.currentMediaItemIndex, + desiredIdx = idx, + controllerIdx = idx, ) val newBuf = controller.bufferedPosition.coerceAtLeast(0) - // Track adjustments are forward-only AND suppressed while a user - // transport press is pending Sonos confirmation. Together those keep - // either direction of user input from being undone by a stale poll. - val trackChanged = !pendingTransport && - desiredIdx > current.queueIndex && - desiredIdx in queueRefs.indices - publishTickIfChanged( - current, trackChanged, desiredIdx, - effectiveIsPlaying, newPos, newDur, newBuf, - ) + val somethingChanged = current.isPlaying != effectiveIsPlaying || + current.positionMs != newPos || + current.durationMs != newDur || + current.bufferedPositionMs != newBuf + if (somethingChanged) { + uiStateInternal.value = current.copy( + isPlaying = effectiveIsPlaying, + positionMs = newPos, + durationMs = newDur, + bufferedPositionMs = newBuf, + ) + } } /** @@ -654,48 +657,6 @@ class PlayerController @Inject constructor( } } - @Suppress("LongParameterList") // assembled at one tick call site; refactor would cost clarity - private fun publishTickIfChanged( - current: PlayerUiState, - trackChanged: Boolean, - desiredIdx: Int, - effectiveIsPlaying: Boolean, - newPos: Long, - newDur: Long, - newBuf: Long, - ) { - val somethingChanged = trackChanged || - current.isPlaying != effectiveIsPlaying || - current.positionMs != newPos || - current.durationMs != newDur - if (!somethingChanged) return - val newTrack = if (trackChanged) queueRefs[desiredIdx] else current.currentTrack - val newIdx = if (trackChanged) desiredIdx else current.queueIndex - uiStateInternal.value = current.copy( - currentTrack = newTrack, - queueIndex = newIdx, - isPlaying = effectiveIsPlaying, - positionMs = newPos, - durationMs = newDur, - bufferedPositionMs = newBuf, - ) - // Intentionally do NOT call controller.seekTo here. That would route - // through MinstrelForwardingPlayer's seekTo override and re-issue - // AVTransport.SeekToTrack to Sonos -- which seeks Sonos back to the - // start of the same track it's already playing, restarting the song. - // The wrapped player's index is kept in sync by maybeSyncLocalCursor's - // delegate.seekTo (which bypasses the override). If it lags briefly, - // the next pollOnce catches up; the uiState above already reflects - // Sonos's truth for the user. - } - - private fun desiredQueueIndex(controller: MediaController, upnpActive: Boolean): Int = - if (upnpActive) { - (remoteState.trackNumber - 1).coerceAtLeast(0) - } else { - controller.currentMediaItemIndex - } - // ── Remote position interpolation state ────────────────────────────── // remoteState.positionMs is only refreshed by ForwardingPlayer's 1Hz // SOAP poll (and only when the round-trip completes -- screen-off WiFi