fix(player): address audio by the server's contract, and own the viewer's pick - #173
Merged
RXWatcher merged 11 commits intoAug 6, 2026
Merged
Conversation
The HUD Audio row described what the transcode delivered rather than what the file holds. "Reasonable Doubt" carries English DTS 5.1; the session transcodes it to stereo AAC, and the row read "UND AAC Stereo" while the detail page and its AUDIO selector both correctly said "English - DTS - 5.1". The row now resolves the source track from the playback plan's stable server audio index and labels it from the catalog, falling back to the mounted Media3 track only when there is no plan identity or no catalog audio metadata. audioSummaryForServerIndex looks the track up by AudioTrack.index rather than by list position. Those are different numbers: the server index is a serialized field and is not constrained to equal its position, so a positional lookup silently resolves the wrong row whenever indices are non-contiguous. Verified on device: a DTS 5.1 source in a transcoded session now reads "English - DTS - 5.1". Deliberately NOT included, because they are a separate change rather than a labelling fix -- an adjudication with Codex established the blast radius is wider than the HUD: - selectedServerAudioTrackIndex still prefers the Media3 ordinal over the plan index and maps it positionally into the catalog. That is the documented UNRESOLVED note at TvPlayerViewModel: it is structurally guaranteed to pick the wrong source track for a one-audio remux of a non-first track. - The HUD picker is still gated and keyed on the Media3 track count, so a transcoded multi-audio title cannot be switched at all. - The detail screen sends a catalog-list ORDINAL through navigation and the player submits it as the server's stable index. - Episode carry-over captures language/codec/channels from the delivered track, so a transcoded DTS source becomes an AAC preference for the next episode. - The remote command contract names its value audio_track_index but TV handles it as a player ordinal. None of that is verifiable here: every audio file in this library has a single track, so the multi-track path cannot be exercised on device. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Probing the running server (official latest, sha256:5f83c105, 2026-08-04)
for a two-audio-track file settles this:
AUDIO {"title":"English DTS 5.1","language":"en","codec":"dts",...}
AUDIO {"title":"Dutch AAC Stereo","language":"nl","codec":"aac",...}
SUB {"index":2,"language":"en",...}
Audio tracks carry NO index on the wire. Subtitles do. So
AudioTrack.index deserialises to its `0` default on every audio row and
identifies nothing, while `effective_audio_track_index` and the V3
start/replan `audio_track_index` are ORDINALS into audio_tracks --
`audio_track_id` is even built as `file:<id>:audio:<ordinal>`.
Every consumer that resolved audio through AudioTrack.index was
therefore reading 0:
- selectAudioOption mapped the pick through getOrNull(ordinal).index, so
every explicit audio choice asked the server for track 0. Picking
Dutch played English.
- The HUD picker keyed rows on that index, so a two-track file rendered
both rows as "English - DTS - 5.1 - Default".
- tvAudioTrackPersistenceUpdate matched on it and found nothing above 0,
so a committed choice was silently never persisted and reopening the
item lost it.
- resolveTvRemoteAudioIntent returned it, so remote set_audio_track
always requested 0.
- Episode carry-over built every EpisodeAudioCandidate index from it,
collapsing all candidates to 0.
All of the above now use the catalog ordinal. The HUD is driven from the
catalog rather than from Media3, so the row is gated on what the FILE
holds instead of what this stream delivered -- a transcode collapses
Media3 to one track, which disabled the row outright and made audio
unswitchable for the whole session.
selectedServerAudioTrackIndex also now prefers the plan over the mounted
Media3 ordinal, validated against the catalog. That resolves the
UNRESOLVED note in place: a transcoded stream carries only the chosen
track and reports ordinal zero, so preferring it made the next replan
ask for track zero and reverted the audio to the first language.
Verified on device against a purpose-built two-track fixture (English
DTS 5.1 / Dutch AAC stereo at non-contiguous stream indices): the picker
lists both correctly, and selecting Dutch commits with the HUD following.
Still on AudioTrack.index and NOT migrated here -- phone player
selection/persistence, the shared audio fingerprint, and the media-info
dialog. AudioTrack.index should ultimately be deleted so the remaining
assumptions become compile errors rather than silent zeroes.
Open: after the switch the plan and HUD report Dutch, but the Info tab
still shows the mounted format as AUDIO/VND.DTS. That is either stale
stats or the server acknowledging the ordinal while still delivering
track 0; the Media3 track snapshot after the replan tells them apart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Groundwork for making TV audio selection actually switch audio. Device evidence (Shield, two-audio-track fixture): selecting "Dutch AAC Stereo" updated the plan and the HUD, but the Media3 track snapshot still read `2:audio[selected=true audio/vnd.dts language=en]` with Dutch `selected=false`, and the audio HW config stayed at 6 channels. The stream direct-plays and carries BOTH tracks; TV audio selection only ever stages a server replan and never calls AudioTrackManager.selectAudioTrack, so the local selection never moves. matchMountedAudioTrack answers "which mounted track IS this catalog row", which is the piece any fix needs and which cannot be done positionally: Media3 ordinals and catalog ordinals are different spaces, and in the fixture the mounted order is the reverse of the catalog's. canonicalAudioCodecFamily exists because the existing episode-handoff normaliser only strips to the last path segment, leaving `mp4a-latm` against a catalog `aac` -- it matches nothing. Both sides now reduce to a family, so catalog `dts`/`aac` meet Media3 `audio/vnd.dts`/ `audio/mp4a-latm`. The matcher is deliberately conservative. A transcoded representation must NOT match its own source -- a DTS 5.1 row delivered as undetermined-language stereo AAC returns null, because the right answer there is a server replan, not playing the wrong track. Unresolved ties return null for the same reason resolveEpisodeAudioIntent does. Lives in android-shared, not the TV app: phone stages the identical replan-only path and is exposed to the same bug whenever its direct-play stream carries multiple audio tracks. No behaviour change yet -- nothing calls this. The wiring is a larger piece: it needs generation-tagged requests, apply-then-confirm through onTracksChanged before committing, pending vs committed state kept separate, re-application after every remount (the Media3 override is bound to a MediaTrackGroup that setMediaItem replaces), HUD selection read from committed state rather than the plan, and the positional assumption in persisted-selection restore fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Audio selection now actually changes the audio.
Verified on device (Shield, two-audio-track fixture, diagnostics build):
want=[lang=nl codec=aac ch=2 title=Dutch AAC Stereo]
mounted=[ord=0 lang=nl codec=audio/mp4a-latm ch=2 label=Dutch AAC Stereo],
[ord=1 lang=en codec=audio/vnd.dts ch=6 label=English DTS 5.1]
-> match=0
Track snapshot: 1:audio[selected=true mp4a.40.2 nl Dutch AAC Stereo]
2:audio[selected=false audio/vnd.dts en English DTS 5.1]
onTracksChanged audioCount=2 selectedOrdinal=0 pending=0
Media3 switched in place, with no remount and no player error.
Before this, TV audio selection only ever staged a server replan. For a
direct-play stream carrying several audio tracks the server acknowledged
the new ordinal and the plan and HUD moved, while the renderer kept
decoding the original -- the HUD claimed Dutch over audible English.
selectAudioOption now asks matchMountedAudioTrack whether the mounted
stream already carries the chosen catalog row. If it does, the track is
selected on the player and the replan skipped: rebuilding the session to
deliver audio the viewer is already receiving is pointless, and the
replan path is heavier and remounts. If it does not -- a genuine
single-track transcode -- it falls through to the replan exactly as
before.
Nothing commits on request. AudioTrackManager.selectAudioTrack returns
Unit and does nothing silently when the group is absent, so "we asked" is
not evidence; the choice is committed only once onTracksChanged shows the
target selected. Requests carry a generation so a stale callback cannot
commit a newer choice, and the row renders a pending selection as "…"
rather than claiming a track the player has not switched to.
The one case with no confirmation event is re-selecting the track already
playing: Media3 emits no onTracksChanged for a no-op override, which
would strand the row on "…" forever, so that commits immediately. Found
by driving it on the device.
On commit the choice outranks the plan for the HUD and for every later
replan request, so a subtitle/quality/output-route replan can no longer
quietly reinstate the server's track, and it is persisted as an audio
fingerprint with the subtitle side left Preserved. A remount installs a
new MediaTrackGroup and the override was bound to the old one, so the
committed choice is re-applied from onTracksChanged rather than assumed
to survive.
Separately observed and NOT fixed here: starting playback with Dutch
already chosen on the detail page still mounts English
(selectedOrdinal=1), so the start path does not apply the detail
selection to the player either. Phone remains on the replan-only path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Choosing audio on the detail page and pressing Play still mounted English: the start request carried the pick and the server honoured it in the plan, but a direct-play stream delivering every track let Media3 choose its own default and nothing applied the choice to the player. Verified on device (Shield, two-track fixture), 76ms apart: 1:audio[selected=false nl Dutch] 2:audio[selected=true en English] 1:audio[selected=true nl Dutch] 2:audio[selected=false en English] Four entry points were each deciding the same thing through their own nullable field -- the launch ordinal, a persisted fingerprint, the HUD, and the remote -- so they raced, and the winner was whichever ran last. They now write one generation-owned TvDesiredAudio and a single resolver reconciles it against every track snapshot. The intent is never cleared on read. Clearing it on the first callback discarded it whenever that callback was empty or partial, which reproduced the original bug exactly. Because it stays live it also serves as the remount re-application path: setMediaItem replaces the MediaTrackGroup the override was bound to, so a confirmed choice has to be applied again rather than assumed to survive. Confirmation resolves the wanted row once against the whole mounted snapshot and compares ordinals within that snapshot. Matching against a one-element list instead asks a different question -- the matcher stops as soon as one candidate remains, so a main mix and its commentary, same language and codec, would confirm each other. The intent is scoped to a fileId. The outgoing version stays interactive while a replacement loads, and audio ordinals are per-file, so a pick made in that window would otherwise be reconciled against the new file and silently change language. onSelectFileVersion now validates before mutating: a no-op or unknown id used to drop the intent and then return without switching anything. Requests carry the mount nonce, so a remount that resolves the wanted track to the same ordinal still reissues instead of matching the in-flight request and leaving the override on a replaced group. An explicit choice claims the durable restore, so a later fingerprint resolution cannot mint a newer generation for an older decision. Launch and persisted restores commit without raising manualAudioSelectionApplied: putting playback back where it was is not a fresh decision and must not become an episode-carry-over preference. Known gaps, deliberately not fixed here: - Mount identity keys on transportMountNonce, which tracks intended primary mounts. A subtitle-refresh remount replaces the MediaItem without changing it, so re-application can be missed there. The right fix is a backend-owned MediaItem generation incremented on every setMediaItem, passed with the track snapshot. - Launch provenance: a fresh detail-page pick and a durable value seeded onto the detail page arrive as the same nullable ordinal, so both are treated as restores. A genuine pick therefore restores correctly but does not carry to the next episode. Distinguishing them needs a flag threaded through the nav route. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Closes the two gaps left by the unified audio intent. Mount identity. Reapplication after a remount was keyed on transportMountNonce, which tracks INTENDED primary mounts -- a subtitle refresh replaces the media item without moving it. Threading a backend-owned setMediaItem counter instead looked like the answer but is not sound either: the counter is read separately from the Tracks object, so a callback queued for the old item can observe the new value and suppress exactly the reissue that mattered, and a backend rebuilt by Compose restarts its count and collides with the old one. There is no reliable mount identity available from outside the backend, so the request no longer claims one. Each issuance carries a monotonic attempt instead, purely so StateFlow cannot conflate it -- an identical value is dropped, and re-applying after a remount looks identical. The resolver now simply reissues on any snapshot where the wanted track is present and unselected. Applying the override is idempotent and onTracksChanged only fires when tracks actually change, so it cannot spin, and no mount bookkeeping is needed at all. The backend surface added for this is removed again rather than left as a misleading handle. Provenance. A fresh detail-page pick and a durable value seeded onto that page arrived as the same ordinal, so both were treated as restores and a real pick never carried to the next episode. TvItemDetailViewModel now records audioPickedThisSession -- set only by onAudioTrackSelected, cleared on version switch, never by seedPersistedTrackSelection -- and it travels through onPlay, the Player route (audioPicked=true, emitted only when true) and into TvDesiredAudio.explicit. Series and season detail select through a separate next-up path, whose handler set no provenance while Play forwarded the unrelated container-level flag, so a fresh next-up pick was misreported. It now has its own nextUpAudioPickedThisSession, and the screen reads whichever matches the ordinal it is actually sending. The route argument is declared in the graph with a default rather than left to inference, so a restored back stack has a defined value. Verified on device: launching with Dutch restored, Media3 selects English at mount and the resolver corrects it 72ms later 1:audio[selected=false nl] 2:audio[selected=true en] 1:audio[selected=true nl] 2:audio[selected=false en] Tests: route emits audioPicked only for a fresh pick, and the ROUTE pattern declares the argument. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Every review round of this area produced a case that neither reasoning nor the pure-matcher tests caught -- a discarded intent, a false confirmation, a cross-file ordinal. The decision logic lived inside a ViewModel with fourteen constructor dependencies, so none of it was reachable from a test. reconcileDesiredAudioAction is that decision as a pure function over (desired, activeFileId, catalog, mounted, selectedOrdinal, planAudioOrdinal), returning None / DropForeignFile / Confirm / Apply(ordinal). The ViewModel keeps the orchestration -- generations, persistence, the request flow -- and now just dispatches on the result. The cases are the bugs, written down: - an empty or partial snapshot decides nothing and does NOT consume the intent, which is what made a launch pick silently fail - the wanted track present but unselected applies its MOUNTED ordinal, with catalog order deliberately reversed against mounted order so a positional answer fails the test - an intent belonging to another file is dropped rather than applied - an absent track is satisfied only when the plan names that catalog row, since a transcode's recoded output cannot identity-match its source - commentary does not confirm the main mix: same language, same codec, and resolving against a one-element list let them confirm each other - a reorder moves the target and invalidates the ordinal that used to be right - a confirmed choice is reapplied once the player is no longer on it Mutation-checked: making an empty snapshot consume the intent fails emptySnapshotDecidesNothing and nothing else. Device re-verified after the extraction: Media3 selects English at mount, the resolver corrects to Dutch 22ms later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
The phone player had the same phantom-index bug TV did. Audio is
addressed by ORDINAL into audio_tracks -- the wire carries no index for
audio tracks, only subtitles get one -- so AudioTrack.index deserialises
to its 0 default on every row.
- selectedServerAudioTrackIndex returned audioTracks.getOrNull(ordinal)
.index, i.e. 0 for every track: every explicit pick asked the server
for track 0, so choosing the second language played the first. It now
returns the range-checked ordinal.
- selectedAudioTrackOrdinal searched by .index with two fallbacks; it is
the identity mapping.
- Persistence resolved committed.audioTrackIndex through
firstOrNull { it.index == ... }, which matched nothing above row zero,
so a committed choice was silently never written and reopening the item
lost it. Extracted as mobileAudioTrackPersistenceUpdate and resolved by
ordinal, mirroring TV.
- Subtitle auto-selection searched .index first and only worked because
of the ordinal fallback behind it; it is a direct ordinal lookup now.
Test fixtures across the phone suite fabricated AudioTrack(index = 2) and
index = 7 -- shapes the server never sends -- and were preserving the
mistake. They now leave index unset and make the ordinal meaningful by
supplying two rows.
Mutation-checked: restoring the .index lookup in persistence fails the
new committed-ordinal test and nothing else.
Restore needed no change: it already resolved the fingerprint to a
catalog ordinal, which is now consistent with the rest of the chain.
Stored fingerprints stay valid -- audioTrackFingerprint includes
track.index, which has always been 0 in practice, so current rows
generate the same string and no migration is required.
Still open on phone: direct-play multi-audio. The server plan now records
the right ordinal, but phone's onTracksChanged handles subtitles only and
never applies a Media3 override, so a file delivering several audio
tracks can show the chosen row while playback stays on Media3's default.
Transcoded and server-materialised audio are fixed by this change; direct
play needs the local-selection path TV now has.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Moves the desired-audio decision out of androidTvApp into android-shared so phone can reuse it rather than growing a second copy: TvAudioReconcile -> AudioReconcile, TvDesiredAudio -> DesiredAudio, TvLocalAudioSelection -> LocalAudioSelection, TvAudioReconcileAction -> AudioReconcileAction. The tests move with it. No behaviour change: TV imports the same functions it called before, and the full suite is unchanged. Phone's local (no-replan) selection is NOT included. It was written and reviewed, and is held back deliberately -- see below. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
…s it Verified on a Pixel 10 Pro XL against the two-track fixture. Selecting "Dutch AAC Stereo" in the tracks sheet: 1:audio[selected=true mp4a.40.2 nl Dutch AAC Stereo] 2:audio[selected=false audio/vnd.dts en English DTS 5.1] One snapshot, no remount -- switched in place. Phone's audio selection only ever staged a server replan, and its onTracksChanged handled subtitles alone, so a direct-play file carrying several audio tracks showed the chosen row while the renderer kept decoding Media3's default. It now runs the same machinery as TV: one generation-owned DesiredAudio that every entry point writes, reconciled against each track snapshot by the shared decision, applied through the backend and committed only once a snapshot shows the target selected. Three things this needed beyond copying TV, each found in review: - A local confirmation has to publish what a replan commit publishes. uiState.selectedAudioIndex, routeIntentState.applyCommittedTracks and the transaction's committed audio all previously moved only when the adapter published a snapshot, so a local switch left the sheet checkmark, route redelivery, recovery and Cast on stale audio. updatePlaybackContext was the wrong tool for the last of those -- it only replaces context, and on a content or session mismatch it resets. MobileSubtitleTransactionAdapter.commitLocallyAppliedAudio moves the reducer's committed audio directly, and queues while a commit is in flight so it cannot race that transaction. - A failed local switch needs a way out. AudioTrackManager returns Unit and does nothing silently when the group has gone, and a no-op produces no callback, so an unbounded local path could leave the request unapplied forever. After MAX_LOCAL_AUDIO_ATTEMPTS snapshots that have not taken, it falls back to the server replan. - Launch, persisted restore and offline playback all have to seed the intent. Restore previously ran only when the persisted ordinal differed from the server's, but equality with the plan is not evidence the RENDERER is on that track -- which is the whole case this exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
…selection # Conflicts: # androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe PR standardizes audio selection on catalog ordinals. It adds mounted-track identity matching and reconciliation, integrates local switching and persistence on mobile and TV, and carries explicit audio-selection provenance through TV navigation and playback UI. ChangesAudio selection and playback
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DetailScreen
participant TvRoute
participant TvPlayerScreen
participant TvPlayerViewModel
participant TvPlayerHud
participant VideoPlaybackBackend
DetailScreen->>TvRoute: pass audioPickedThisSession
TvRoute->>TvPlayerScreen: parse and forward audio provenance
TvPlayerScreen->>TvPlayerViewModel: create launch state
TvPlayerHud->>TvPlayerViewModel: submit catalog audio ordinal
TvPlayerViewModel->>VideoPlaybackBackend: apply mounted audio ordinal
VideoPlaybackBackend-->>TvPlayerViewModel: publish selected track
TvPlayerViewModel-->>TvPlayerHud: update desired ordinal and confirmation
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audio selection was addressed the wrong way on both clients, and the viewer's pick had no single owner.
The server contract
The server sends no wire
indexfor audio tracks — subtitles carry one, audio does not. Audio is addressed by its ordinal in the version'saudio_tracks. Both clients were mapping picks throughAudioTrack.index, a field the server never populates for audio, so every selection collapsed to0. Choosing any track played the first one.One owned intent
The desired track was spread across several nullable fields that raced to decide the same thing — the detail page's launch pick, a persisted fingerprint, the HUD, and the remote each wrote their own. This replaces them with one generation-owned intent that every entry point writes to, reconciled against each track snapshot by a single resolver.
That matters because a pick has to survive things that can each fail independently: staging, validation, adoption, mount, and rollback. A flag raised on intent carried whatever track survived a failure into the next episode as though it had been chosen; it is now set on commit.
Direct-play streams
A direct-play stream carries every audio track, so Media3 still picked its own default even when the start request named the viewer's choice — selecting Dutch on the detail page and pressing Play mounted English. Both clients now switch on the player when the stream already carries the track, rather than assuming the server's plan settled it.
Labelling
Player audio is labelled by source identity rather than delivered format. A transcoded stream reports what the transcoder produced, so a 5.1 source delivered as stereo was labelled stereo — the viewer could no longer find the track they picked.
Shared logic
The reconcile decision and its intent types are extracted and shared between the clients, with the TV path's failure modes covered by tests, so the two implementations cannot drift apart again.
Testing
Full suite green on all four modules. One flake surfaced under parallel load in
PlayerViewModelLoadOwnershipIntegrationTest— a 5s real-timeawaitConditiondeadline, unrelated to this change; it passes consistently on re-run, and the harness is reworked later in the stack.Merged
mainin to resolve one adjacency conflict inTvPlayerViewModelwhere this branch's audio block met a KDoc relocated by #170; both sides kept.🤖 Generated with Claude Code
https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Summary by CodeRabbit
New Features
Bug Fixes