feat(playback): adopt platform-neutral protocol v3 - #200
Conversation
The server now owns the playback protocol as a platform-neutral contract, and this client's job shrinks to speaking it. Most of this change is deletion: the pieces below existed because the wire format was shaped around Media3, and a neutral contract makes them redundant rather than merely unused. - Attempt keys are server-minted. The Kotlin FNV-1a implementation and the fixtures that pinned its output are gone; `plan_attempt_key` arrives on the plan, is stored opaquely, and is echoed on the next replan. `attempted_plan_keys` carries what the server gave us, never anything computed here. - Engines become deliveries. `PlaybackEngineKind` and the `media3_*` capability envelope are replaced by the three neutral delivery classes — `original_http`, `progressive`, `hls` — each self-describing its containers, codecs, subtitle support, and transformations. `PlaybackExecutionPlan` survives only as a player-facing projection built from the plan, not as a wire type. - Android-shaped facts move to `platform_details`. The `Build` dump is a free-form bag the server reads for quirk matching and support diagnostics rather than a set of platform-specific fields on a shared type. - Capability evidence is stated, not implied. This client probes `MediaCodecList` for concrete profile/level/bit-depth tuples, so it advertises `exact` on both video and audio — the only tier the server validates strictly against, and the only one that earns audio passthrough. Cast advertises `declared`. - Output identity travels nested under `client_playback_context.output` as an opaque `output_context_id`; Android's audio route generation counter is exactly the equality-comparable token the server wants. - Track and quality changes are intents, not failures. They now send `track_change` and `quality_change` instead of routing through replan-with-failure or a legacy endpoint, so the server can tell a user choice from a playback problem. `PlaybackProtocolV3ConformanceTest` is the drift gate: it reads the server's golden fixtures, vendored byte-identically under `playback/v3/`, and proves this client both decodes every field the server sends and encodes requests in the shape the server expects. Fields the client deliberately does not model are an explicit allow-list, so a field going unread fails the build naming its JSON path. Attempt keys are asserted only by echo — there is no hash here to check them with, which is the point. Two tests went with their subjects: the transcode-fallback suite (the endpoint is deleted server-side) and the styled-subtitle burn-in suite (burn-in is now a server plan decision). `PlaybackSessionLifecycle` loses a `SessionState.Loading` nobody observed and a `ProfileRepository` it never called. Verified: 3,231 unit tests across shared, android-shared, androidApp, and androidTvApp — 0 failures, 0 errors. Part of the coordinated playback v3 release train; there is no compatibility window, and a client that does not declare `protocol_version: 3` now gets 426.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request migrates Android playback to protocol V3. It replaces engine-based plans with delivery capabilities, server-minted plan keys, opaque output contexts, authoritative subtitle inventories, caller-owned recovery, and updated mobile, TV, audiobook, and Cast flows. ChangesPlayback V3 contract
Session and player execution
Application integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerViewModel
participant PlaybackSessionManager
participant PlaybackApi
participant PlaybackServer
participant Media3Player
PlayerViewModel->>PlaybackSessionManager: start or replan with capabilities
PlaybackSessionManager->>PlaybackApi: send V3 request
PlaybackApi->>PlaybackServer: request playback decision
PlaybackServer-->>PlaybackApi: return plan and planAttemptKey
PlaybackApi-->>PlaybackSessionManager: return ready or terminal result
PlaybackSessionManager-->>PlayerViewModel: adopt plan or publish recovery
PlayerViewModel->>Media3Player: start stream at plan position
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code review — 10 findingsHigh-effort review: 8 independent finder angles (line-by-line, removed-behavior audit, cross-file trace, reuse, simplification, efficiency, altitude, conventions) produced 29 candidates; after dedup, each of the 18 surviving candidates was independently verified against the code (and, where relevant, the companion silo-server repo). 13 confirmed, 2 plausible, 3 refuted. Ranked most-severe first. 1. Blank
|
|
Resolved the Quick104 playback-v3 review at Android head 8566d96.
Vendored all nine server fixtures byte-for-byte and pinned SOURCE to server 30ddc0d09255816e44cd9874ff79a785d8a6fd8e. Typed conformance now covers the neutral feature token, progress_persistence, and draft-v3 HTTP 426 upgrade vector. Validation: focused protocol/conformance/session/Cast/audiobook tests passed; full ./gradlew test passed; shared/android-shared/phone/TV debug lint plus phone/TV vital release lint passed; fixture parity and git diff --check passed. The first full test pass hit an unrelated coroutine Main-dispatcher ordering race in two unchanged ServerSetupPersistenceTest release cases; the exact class passed in isolation and the complete full suite then passed. PR remains draft. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt (1)
795-809: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConvert
StartParams.subtitleTrackIndexbefore renewal
StartParams.subtitleTrackIndexstores a server track index, butloadContentexpects a catalog ordinal. A renewal can select the wrong subtitle or fall back to persisted/automatic selection. Convert the index before callingloadContent, while preserving-1as Off.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt` around lines 795 - 809, Update the renewal handling in sessionLifecycle.missingSessionEvents before calling loadContent: convert renewal.startParams.subtitleTrackIndex from the server track index to the catalog ordinal expected by loadContent, preserving -1 as the Off value. Pass the converted value through initialSubtitleTrackIndex while leaving the other renewal parameters unchanged.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.kt (1)
185-198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe audio codec list has a fabricated fallback path while the evidence tier stays
exact.
detectPlatformSoftwareAudioCodecs()returns a hard-codedlistOf("aac", "mp3")when theMediaCodecListconstruction fails (Line 430). That value is assumed, not probed. The comment on Lines 185-190 states the tier must drop when a path fabricates part of the list, and only exact evidence earns audio passthrough. Consider tracking whether the probe succeeded and reportingaudioEvidenceaccordingly.♻️ Sketch of the fix
- val softwareAudio = advertisedAudioDecodeCodecs( - platformCodecs = detectPlatformSoftwareAudioCodecs(), + val platformCodecs = detectPlatformSoftwareAudioCodecs() + val softwareAudio = advertisedAudioDecodeCodecs( + platformCodecs = platformCodecs, ffmpegAvailable = ffmpegAvailable, isTv = TvModeDetector.isTv(context), )Then derive
audioEvidencefrom a probe-success flag set insidedetectPlatformSoftwareAudioCodecs()instead of the constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.kt` around lines 185 - 198, Track whether detectPlatformSoftwareAudioCodecs() successfully constructs and probes MediaCodecList, including exposing that success flag to its caller. In the capability construction around codecProbe and softwareAudio, derive audioEvidence from this flag, using exact evidence only for successful probes and the appropriate lower tier for the hard-coded fallback; leave videoEvidence and codec lists unchanged.shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt (1)
545-562: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis re-anchor fixture does not match what production sends.
PlaybackSessionManager.reanchorActiveVideoSessionbuilds its re-anchor request without afailureblock. This test suppliesfailure = PlaybackFailureV3(SEEK_REANCHOR_V3_OPERATION). The serialization the test covers is therefore not the serialization production emits. I raise the root concern on the manager.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt` around lines 545 - 562, Update the reanchor fixture in PlaybackProtocolV3Test so PlaybackReplanRequestV3 matches the request built by PlaybackSessionManager.reanchorActiveVideoSession: remove the failure field from this test request. Keep the remaining re-anchor serialization fields unchanged.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.kt (1)
894-918: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSkip a blank committed key so it does not enter
attemptedPlanKeys.This block records the server cursor before
validateForMedia3()runs.PlaybackPlanV3.planAttemptKeydefaults to"", and validation is what rejects a blank key. So a malformed response writescommittedKey = ""intoServerPlanCursor.attemptedPlanKeys, and the next replan sends an empty string insideattempted_plan_keys.The loop detector is unaffected, because
nextKeyalways comes from a validated plan. The effect is a malformed field on the wire that the server folds into its exclusion set.🐛 Proposed fix
- result.data.playbackPlan?.let { committedPlan -> + result.data.playbackPlan + ?.takeIf { it.planAttemptKey.isNotBlank() } + ?.let { committedPlan -> val committedKey = committedPlan.planAttemptKey🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.kt` around lines 894 - 918, When updating the cursor in the committed-plan block, only add committedKey to attemptedPlanKeys when it is non-blank; otherwise preserve attemptedKeys unchanged. Keep the existing cursor update and compareAndSet behavior intact, while ensuring blank PlaybackPlanV3.planAttemptKey values never reach the serialized exclusion set.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt (1)
946-955: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
clientPlaybackContextduring renewal. Production adoption paths can storeStartParamswith a null context, and both renewal paths rebuild the context instead of reusing the negotiated one. Make the context non-null and pass it through recovery to preserve the exact route and capabilities promised by the KDoc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt` around lines 946 - 955, Update StartParams.clientPlaybackContext to be non-null, requiring callers to provide the negotiated context. In both renewal/recovery paths that reconstruct StartParams, reuse and propagate the original clientPlaybackContext rather than rebuilding it or allowing null, preserving the exact route and capabilities documented for playback.
🧹 Nitpick comments (9)
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt (1)
1475-1499: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse one Dolby Vision policy source for subtitle transactions.
subtitlePlaybackContextuses state-flow values, but other playback paths useplayerSettingsStore.dolbyVisionPolicySnapshot(). Their eager defaults differ:dvProfile7Hdr10Fallbackstarts asfalse, while the store flow defaults totrue. Share one resolved policy snapshot so capability data remains consistent during startup and replanning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt` around lines 1475 - 1499, Update subtitlePlaybackContext to obtain a single resolved Dolby Vision policy snapshot from playerSettingsStore.dolbyVisionPolicySnapshot(), rather than constructing DolbyVisionPolicy.Snapshot from the separate state-flow values. Pass that shared snapshot to both capabilityDetector.detect and capabilityDetector.detectPlaybackContext, preserving consistent startup and replanning behavior.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt (1)
72-83: 🚀 Performance & Scalability | 🔵 TrivialConsider indexing the SUP stream.
IndexSeekMapuses(positions, timesUs, durationUs), andC.TIME_UNSETis supported byMergingMediaSource. With one entry, every seek resolves to byte zero. Each nonzero seek or reprepare therefore rescans the full SUP stream. Add seek points if this cost is unacceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt` around lines 72 - 83, Update the seek map used by PgsSupExtractor to index the SUP stream with additional byte-position/time entries, rather than relying solely on the single zero-offset entry. Populate the IndexSeekMap from suitable parsed PGS timestamps and stream positions while preserving C.TIME_UNSET duration support and correct seek behavior.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.kt (1)
94-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the URL guard.
orEmpty().takeIf { ... }.orEmpty()performs two null coalescings for one condition. A single conditional expression is clearer and keeps the same result.♻️ Proposed refactor
- url = item.url.orEmpty().takeIf { - subtitle.mode != PlaybackSubtitleModeV3.BURN_IN && - item.delivery == SUBTITLE_DELIVERY_SIDECAR - }.orEmpty(), + url = if ( + subtitle.mode != PlaybackSubtitleModeV3.BURN_IN && + item.delivery == SUBTITLE_DELIVERY_SIDECAR + ) item.url.orEmpty() else "",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.kt` around lines 94 - 97, Update the url assignment in the PlaybackV3Session subtitle mapping to replace the chained item.url.orEmpty().takeIf(...).orEmpty() with a single conditional expression using the existing subtitle.mode and item.delivery conditions, preserving an empty string when the guard fails.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloLoadControl.kt (1)
76-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLower the log level for this per-selection diagnostic.
calculateTargetBufferBytesruns on every track selection change.Log.ikeeps buffer internals in release logcat.Log.dmatches the diagnostic intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloLoadControl.kt` around lines 76 - 80, In calculateTargetBufferBytes, lower the per-selection diagnostic from Log.i to Log.d while preserving the existing TAG and message contents.shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt (2)
398-404: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the neutral server feature set in this decode fixture.
The embedded JSON advertises only
PLAYBACK_PLAN_V3_FEATURE. The test does not callvalidateForMedia3(), so it passes. But the fixture now depicts a response thatvalidateForMedia3classifies asIncompatible. IncludeNEUTRAL_PLAYBACK_V3_CONTRACT_FEATUREso the fixture stays a valid example.♻️ Proposed fixture update
- """{"protocol_version":3,"server_features":["$PLAYBACK_PLAN_V3_FEATURE"],"outcome":"playable",""" + + """{"protocol_version":3,"server_features":["$PLAYBACK_PLAN_V3_FEATURE",""" + + """"$NEUTRAL_PLAYBACK_V3_CONTRACT_FEATURE"],"outcome":"playable",""" +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt` around lines 398 - 404, Update the embedded JSON fixture in the playback-plan decode test to advertise both PLAYBACK_PLAN_V3_FEATURE and NEUTRAL_PLAYBACK_V3_CONTRACT_FEATURE. Keep the existing decoding and planAttemptKey assertion unchanged so the fixture remains a valid Media3-compatible response.
225-250: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a selected subtitle with a null
index.The
?: 0at Line 238 showsPlaybackTrackIdentityV3.indexis nullable. No test covers a plan where the server selects a subtitle bytrackIdonly. With the currenthasValidSubtitleInventory, that plan returns a non-retryableTerminal. I raise the production-side concern onPlaybackProtocolV3.ktLines 509-513.💚 Proposed test
`@Test` fun subtitleSelectedByTrackIdAloneStaysPlayable() { val result = PlaybackDecisionResponseV3( protocolVersion = PLAYBACK_PROTOCOL_V3, serverFeatures = neutralServerFeatures, outcome = PlaybackDecisionOutcome.PLAYABLE, playbackPlan = plan.copy( selectedTracks = SelectedPlaybackTracksV3( subtitle = PlaybackTrackIdentityV3("file:42:subtitle:0", null), ), subtitle = PlaybackSubtitleDecisionV3( inventory = listOf( PlaybackSubtitleInventoryItemV3( trackId = "file:42:subtitle:0", combinedIndex = 0, source = "external", delivery = SUBTITLE_DELIVERY_SIDECAR, url = "/stream/session-1/subtitles/0.vtt", ), ), ), ), ).validateForMedia3() assertIs<PlaybackV3Validation.Playable>(result) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt` around lines 225 - 250, Add a test alongside unknownSelectedSubtitleDeliveryRequestsASelectionPreservingReplan covering a selected subtitle identified by trackId with a null index; provide matching subtitle inventory and valid sidecar delivery data, then assert validateForMedia3() returns PlaybackV3Validation.Playable rather than Terminal.android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt (1)
552-565: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe KDoc overstates the ordering guarantee.
The KDoc states the recheck happens "only after the lifecycle reporter is fully stopped".
stop()callsreporterJob?.cancel()at Line 496 and does not join it.Job.cancel()is asynchronous, so the reporter coroutine can still be suspended insidesessionManager.reportProgresswhenstop()returns.The behavior is still correct, because
stop()setslastAdoptedSessionId = nullunder the mutex andownsProgressReplythen rejects the late reply. Correct the KDoc to name that mechanism. A reader who trusts the current wording may remove the ownership guard.♻️ Proposed KDoc correction
/** - * Retires a terminal playback attempt and rechecks screen ownership only - * after the lifecycle reporter is fully stopped. Phone and TV must share - * this ordering: publishing the terminal first lets the next progress tick - * observe the retired server session as a 404 and start a fresh attempt. + * Retires a terminal playback attempt, then rechecks screen ownership. + * + * [stop] cancels the reporter without joining it, so a reporter call may + * still be in flight here. [stop] clears `lastAdoptedSessionId` under the + * mutex first, so `ownsProgressReply` discards any late reply. Phone and TV + * must share this ordering: publishing the terminal first lets the next + * progress tick observe the retired server session as a 404 and start a + * fresh attempt. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt` around lines 552 - 565, Update the KDoc for stopTerminalSessionIfCurrent to remove the claim that the lifecycle reporter is fully stopped before rechecking ownership. Document that stop() cancels the reporter and clears lastAdoptedSessionId under the mutex, while ownsProgressReply rejects any late progress reply, preserving the terminal-first ordering without implying cancellation is joined.shared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.kt (1)
433-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument which operations may omit
failure, or validate it.The KDoc states
failureis absent only forINTENT_V3_OPERATIONS.SEEK_REANCHOR_V3_OPERATIONis not in that set, yetPlaybackSessionManager.reanchorActiveVideoSessionbuilds a request withoperation = SEEK_REANCHOR_V3_OPERATIONand nofailure. Either widen this KDoc to cover seek re-anchor, or make the re-anchor path send a failure block. I raise the behavioral half of this on the manager.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.kt` around lines 433 - 437, Update the KDoc for PlaybackProtocolV3.failure to explicitly include SEEK_REANCHOR_V3_OPERATION among operations that may omit failure, matching the request built by PlaybackSessionManager.reanchorActiveVideoSession; leave failure nullable and preserve the existing intent-operation documentation.shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt (1)
77-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse opaque values in output-context tests.
"7"and"9"also fit the removed numeric route-generation representation. Use nonnumeric output context IDs. Assert the nested start-request value.
shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt#L77-L90: use a value such as"tv:hdmi:primary"and assertclient_playback_context.output.output_context_id.shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt#L132-L145: use and assert a nonnumeric route-event output context ID.As per coding guidelines, add focused tests for shared logic only when behavior is critical or high risk.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt` around lines 77 - 90, Update shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt:77-90 to use a nonnumeric output context ID such as "tv:hdmi:primary" and assert it at client_playback_context.output.output_context_id in the nested start request; also update shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt:132-145 to use and assert a nonnumeric route-event output context ID, preserving the existing request assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparer.kt`:
- Around line 110-123: Update the session cleanup around buildCastMediaSpec in
CastPlaybackPreparer so castSession.stopSession(sessionId) runs for every
failure, including non-cancellation exceptions, while preserving propagation of
the original exception and cancellation behavior. Use a catch covering Throwable
or an equivalent success-flag finally block, and retain NonCancellable cleanup.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.kt`:
- Around line 387-401: Update androidPlatformDetails to enforce the documented
128-character value limit by declaring MAX_PLATFORM_DETAIL_CHARS = 128 and using
the existing putBounded helper for Build.ID, Build.DISPLAY, and the joined
SUPPORTED_ABIS value. Preserve the current keys and conditional inclusion
behavior while truncating oversized values before insertion.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt`:
- Around line 730-753: Update handleSessionMissing and the recoveryJob
declaration to make recovery-job ownership thread-safe: ensure recoveryJob is
cross-thread visible, and when the launched recovery coroutine completes, clear
the field only if it still references that same job. Keep recoveryJob assignment
and cleanup synchronized consistently with cancelRecoveryJobs,
adoptActiveSessionIfCurrent, and stop so an older job cannot null or hide a
newer one.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.kt`:
- Around line 2342-2368: Update recordLocalMutation so the
passthroughSuppression.beginAttempt token changes only for audio-route
mutations, specifically the PCM mutation, and remains unchanged for
transport_reopen. Preserve mutation recording and duplicate checks, while
ensuring transport reopening cannot clear blocked layouts or reset the
single-retry state.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloLoadControl.kt`:
- Around line 48-64: Update the Dolby Vision detection in the sizingTracks
mapping used by SiloLoadControl to reuse the existing codec-string detection
logic alongside the sampleMimeType check, recognizing HEVC formats with dvhe.*,
dvh1.*, dva1.*, or dvav.* codecs. Add regression coverage for these codec-based
Dolby Vision formats while preserving the existing MIME-based detection.
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerStagedReplanTest.kt`:
- Line 149: Rename the affected Kotlin test functions to camelCase, preserving
their test behavior: update the three tests in
android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerStagedReplanTest.kt
at lines 149-149, 821-821, and 1319-1319, plus the three tests in
android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/SiloLoadControlTest.kt
at lines 10-10, 22-22, and 34-34.
In `@docs/superpowers/plans/2026-08-02-instant-external-srt-switching-android.md`:
- Line 8: Remove the blank line within the block quote in the document’s
introductory quoted section, keeping consecutive quoted lines contiguous or
closing the quote before the blank line so markdownlint MD028 passes.
In
`@shared/src/androidUnitTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3ConformanceTest.kt`:
- Around line 499-512: Vendor the generated playback/v3/conformance_matrix.json
fixture required by conformanceMatrix(), and add its path to
shared/src/commonTest/resources/playback/v3/SOURCE so the resource is included
in the fixture corpus. Preserve the existing
conformanceMatrixDecodesAndRoundTripsEveryGeneratedScenario expectations.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.kt`:
- Around line 509-513: Update the selected-subtitle lookup in
PlaybackProtocolV3.kt lines 509-513 to match the stable trackId and only compare
combinedIndex when selected.index is non-null; apply the same nullable-index
relaxation to the selected-membership check in hasValidSubtitleInventory. Add a
PlaybackProtocolV3Test.kt lines 225-250 case selecting a subtitle by trackId
with a null index and assert validation returns Playable.
---
Outside diff comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.kt`:
- Around line 185-198: Track whether detectPlatformSoftwareAudioCodecs()
successfully constructs and probes MediaCodecList, including exposing that
success flag to its caller. In the capability construction around codecProbe and
softwareAudio, derive audioEvidence from this flag, using exact evidence only
for successful probes and the appropriate lower tier for the hard-coded
fallback; leave videoEvidence and codec lists unchanged.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt`:
- Around line 946-955: Update StartParams.clientPlaybackContext to be non-null,
requiring callers to provide the negotiated context. In both renewal/recovery
paths that reconstruct StartParams, reuse and propagate the original
clientPlaybackContext rather than rebuilding it or allowing null, preserving the
exact route and capabilities documented for playback.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.kt`:
- Around line 894-918: When updating the cursor in the committed-plan block,
only add committedKey to attemptedPlanKeys when it is non-blank; otherwise
preserve attemptedKeys unchanged. Keep the existing cursor update and
compareAndSet behavior intact, while ensuring blank
PlaybackPlanV3.planAttemptKey values never reach the serialized exclusion set.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt`:
- Around line 795-809: Update the renewal handling in
sessionLifecycle.missingSessionEvents before calling loadContent: convert
renewal.startParams.subtitleTrackIndex from the server track index to the
catalog ordinal expected by loadContent, preserving -1 as the Off value. Pass
the converted value through initialSubtitleTrackIndex while leaving the other
renewal parameters unchanged.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt`:
- Around line 545-562: Update the reanchor fixture in PlaybackProtocolV3Test so
PlaybackReplanRequestV3 matches the request built by
PlaybackSessionManager.reanchorActiveVideoSession: remove the failure field from
this test request. Keep the remaining re-anchor serialization fields unchanged.
---
Nitpick comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt`:
- Around line 552-565: Update the KDoc for stopTerminalSessionIfCurrent to
remove the claim that the lifecycle reporter is fully stopped before rechecking
ownership. Document that stop() cancels the reporter and clears
lastAdoptedSessionId under the mutex, while ownsProgressReply rejects any late
progress reply, preserving the terminal-first ordering without implying
cancellation is joined.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.kt`:
- Around line 94-97: Update the url assignment in the PlaybackV3Session subtitle
mapping to replace the chained item.url.orEmpty().takeIf(...).orEmpty() with a
single conditional expression using the existing subtitle.mode and item.delivery
conditions, preserving an empty string when the guard fails.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloLoadControl.kt`:
- Around line 76-80: In calculateTargetBufferBytes, lower the per-selection
diagnostic from Log.i to Log.d while preserving the existing TAG and message
contents.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.kt`:
- Around line 72-83: Update the seek map used by PgsSupExtractor to index the
SUP stream with additional byte-position/time entries, rather than relying
solely on the single zero-offset entry. Populate the IndexSeekMap from suitable
parsed PGS timestamps and stream positions while preserving C.TIME_UNSET
duration support and correct seek behavior.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt`:
- Around line 1475-1499: Update subtitlePlaybackContext to obtain a single
resolved Dolby Vision policy snapshot from
playerSettingsStore.dolbyVisionPolicySnapshot(), rather than constructing
DolbyVisionPolicy.Snapshot from the separate state-flow values. Pass that shared
snapshot to both capabilityDetector.detect and
capabilityDetector.detectPlaybackContext, preserving consistent startup and
replanning behavior.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.kt`:
- Around line 433-437: Update the KDoc for PlaybackProtocolV3.failure to
explicitly include SEEK_REANCHOR_V3_OPERATION among operations that may omit
failure, matching the request built by
PlaybackSessionManager.reanchorActiveVideoSession; leave failure nullable and
preserve the existing intent-operation documentation.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt`:
- Around line 398-404: Update the embedded JSON fixture in the playback-plan
decode test to advertise both PLAYBACK_PLAN_V3_FEATURE and
NEUTRAL_PLAYBACK_V3_CONTRACT_FEATURE. Keep the existing decoding and
planAttemptKey assertion unchanged so the fixture remains a valid
Media3-compatible response.
- Around line 225-250: Add a test alongside
unknownSelectedSubtitleDeliveryRequestsASelectionPreservingReplan covering a
selected subtitle identified by trackId with a null index; provide matching
subtitle inventory and valid sidecar delivery data, then assert
validateForMedia3() returns PlaybackV3Validation.Playable rather than Terminal.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt`:
- Around line 77-90: Update
shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt:77-90
to use a nonnumeric output context ID such as "tv:hdmi:primary" and assert it at
client_playback_context.output.output_context_id in the nested start request;
also update
shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt:132-145
to use and assert a nonnumeric route-event output context ID, preserving the
existing request assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a2209e9-2567-4ac3-85cc-94723b651df9
📒 Files selected for processing (71)
.agents/skills/test-shield-playback/references/playback-evidence.md.agents/skills/test-shield-playback/scripts/shield-testandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/di/PlayerInfraModule.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/di/PlayerModule.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudiobookPlayerViewModel.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloLoadControl.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SubtitleMountResolver.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/audio/PassthroughSuppressionRegistry.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparer.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PlaybackContainerPolicy.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/AudiobookPlayerTeardownSourceTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackColorRangeFallbackTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackPublicationSettlementIntegrationTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleLoggingTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerSeekReanchorTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerStagedReplanTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerTranscodeFallbackTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackV3SessionTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/SiloLoadControlTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/StyledSubtitleBurnInTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparerTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractorTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleAutoSelection.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileVideoPlaybackStarter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/di/AndroidTvModule.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAudiobookRouting.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleIdentity.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleTransactionAdapter.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsPromptScreen.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvAudiobookRoutingTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/SubtitleTransactionIntegrationTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleSettlementOwnershipTest.ktdocs/playback/01-media3-only-player-architecture.mddocs/playback/02-migration-compatibility-validation.mddocs/playback/04-implementation-status-and-dv-handoff.mddocs/playback/README.mddocs/superpowers/plans/2026-08-02-instant-external-srt-switching-android.mdshared/src/androidUnitTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3ConformanceTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/domain/ManagePlaybackUseCase.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/api/PlaybackApi.ktshared/src/commonMain/kotlin/org/siloserver/silo/repository/PlaybackRepository.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackModelsV2SerializationTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackSessionModelsTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.ktshared/src/commonTest/resources/playback/v3/SOURCEshared/src/commonTest/resources/playback/v3/attempt_keys.jsonshared/src/commonTest/resources/playback/v3/capability_response.jsonshared/src/commonTest/resources/playback/v3/conformance_matrix.jsonshared/src/commonTest/resources/playback/v3/decision_response.jsonshared/src/commonTest/resources/playback/v3/error_response.jsonshared/src/commonTest/resources/playback/v3/replan_request.jsonshared/src/commonTest/resources/playback/v3/route_event.jsonshared/src/commonTest/resources/playback/v3/start_request.jsonshared/src/commonTest/resources/playback/v3/subtitle_inventory.json
💤 Files with no reviewable changes (6)
- android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerTranscodeFallbackTest.kt
- shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackModelsV2SerializationTest.kt
- android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackColorRangeFallbackTest.kt
- android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/StyledSubtitleBurnInTest.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/repository/PlaybackRepository.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/network/api/PlaybackApi.kt
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dbf13b65f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt (1)
797-810: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep renewal subtitle indexes separate from route subtitle indexes.
Lines 801-807 pass
StartParams.subtitleTrackIndexintoinitialSubtitleTrackIndex.applyCoordinatorStateToUiinterpretsinitialSubtitleTrackIndexas a catalog subtitle ordinal at lines 1148-1164. A V3 renewal index uses the combined external-then-embedded index space. An embedded subtitle can therefore resolve to a different track or no track after session renewal.Carry renewal
StartParamsseparately, or bypass catalog-ordinal resolution for renewal loads and use the server-selected subtitle identity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt` around lines 797 - 810, Update the missing-session renewal flow in sessionLifecycle.missingSessionEvents so renewal subtitle selection does not pass StartParams.subtitleTrackIndex as initialSubtitleTrackIndex. Preserve the V3 combined external-then-embedded index semantics by carrying the renewal StartParams separately or using its server-selected subtitle identity, bypassing applyCoordinatorStateToUi’s catalog-ordinal resolution while keeping other renewal parameters unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/ReplayableSubtitleDataSource.kt`:
- Around line 70-110: Refactor ReplayableSubtitleDataSource.open so
synchronized(cache) only covers the cache lookup and final cache publication.
Perform upstream.open, readAllFromUpstream, and related close/state handling
outside the cache monitor, then reacquire the lock to publish the newly created
ReplayableSubtitleEntry and open the replay, preserving cache-hit and
non-whole-resource behavior.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PlaybackStartupStallDetector.kt`:
- Around line 142-176: Update the client-transform deadline logic in
PlaybackStartupStallDetector so clientTransformProgressAtMs is refreshed only by
decoder output progress, not currentPositionMs advances that can be driven by
audio. Preserve seek/timeline reset handling as needed, but remove the
startedProgressMs position-based refresh for this local-transform clock, and add
a regression test covering repeated position advances after firstFrameRendered
that still produces DV7_TRANSFORM_STALL_CLASSIFICATION.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/VideoPlayerMediaSpec.kt`:
- Around line 98-102: Update subtitleCodecFromUrl to isolate the final path
segment after removing the query and fragment, then extract the extension only
from that segment. Preserve the existing blank-extension-to-null behavior so
canonicalSubtitleCodecFamily receives no codec when the path has no extension.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.kt`:
- Around line 385-397: Update the Media3 listener DisposableEffect around its
existing keys to include videoBackend, ensuring the effect is disposed and
recreated whenever the selected backend changes. Keep the existing listener
setup, cleanup, and onTracksChanged behavior unchanged.
---
Outside diff comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt`:
- Around line 797-810: Update the missing-session renewal flow in
sessionLifecycle.missingSessionEvents so renewal subtitle selection does not
pass StartParams.subtitleTrackIndex as initialSubtitleTrackIndex. Preserve the
V3 combined external-then-embedded index semantics by carrying the renewal
StartParams separately or using its server-selected subtitle identity, bypassing
applyCoordinatorStateToUi’s catalog-ordinal resolution while keeping other
renewal parameters unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ddba68e-5fc8-44a2-90c9-97410808fd53
📒 Files selected for processing (36)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/Playability.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/ReplayableSubtitleDataSource.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloLoadControl.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloPlayerFactory.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/VideoPlayerMediaSpec.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/backend/Media3VideoPlaybackBackend.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/seek/PlaybackTimelineSeekPolicy.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/StreamingWebvttExtractor.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PlaybackStartupStallDetector.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/VideoTrackSelectionCoordinator.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetectorDolbyVisionTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/ReplayableSubtitleDataSourceTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/SiloLoadControlTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/VideoPlayerSubtitleMountTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/backend/Media3VideoPlaybackBackendLifecycleTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/seek/PlaybackTimelineSeekPolicyTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/subtitle/StreamingWebvttExtractorTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/PlaybackStartupStallDetectorTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/VideoTrackSelectionCoordinatorTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileVideoPlaybackStarter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapterTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerBackendLifecycleSourceTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleTransactionAdapter.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerBackendLifecycleSourceTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleTransactionAdapterTest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleTransactionAdapter.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt
# Conflicts: # androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerScreen.kt # androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsPromptScreen.kt
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review follow-up is complete at
The configured Shield was unavailable for the final physical playback rerun; prior PGS and DV7-to-DV8.1/TrueHD device coverage remains documented in the PR description. |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudioCapabilityManager.kt (1)
73-114: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize the snapshot update; the compound write is not atomic.
publishCapabilitiesandbumpOutputRouteGenerationeach perform a multi-step update: read_capabilities, incrementgenerationCounter, writeplaybackRouteSnapshot, then write_capabilitiesand_outputRouteGeneration. The steps are not guarded as one unit.Three distinct callbacks reach these methods on different threads:
AudioCapabilitiesReceiver.Listener(line 133).- The
SpatializerBridgecallback (line 154), delivered on a platform executor.DisplayManager.DisplayListener(lines 126-128), delivered on the main looper.Two concurrent callers can interleave. The later-incremented generation can be written to
playbackRouteSnapshotfirst, and the earlier one can overwrite it._outputRouteGenerationandplaybackRouteSnapshot.routeGenerationcan then disagree, andplaybackRouteSnapshot.capabilitiescan disagree with_capabilities. That contradicts the documented guarantee of "one atomically published planning view".
routeSnapshotInitializedis also a plain field. A second thread is not guaranteed to observe the write, so the initialization guard on line 83 can fail.🔒️ Proposed fix using a single lock
+ private val routeLock = Any() `@Volatile` private var playbackRouteSnapshot = AudioPlaybackRouteSnapshot( sinkType = "unknown", routeGeneration = 0L, capabilities = AudioPassthroughCapabilities(), ) - private var routeSnapshotInitialized = false + private var routeSnapshotInitialized = false - private fun publishCapabilities(next: AudioPassthroughCapabilities) { + private fun publishCapabilities(next: AudioPassthroughCapabilities) = synchronized(routeLock) { val changed = _capabilities.value != next if (!changed && routeSnapshotInitialized) return- private fun bumpOutputRouteGeneration() { + private fun bumpOutputRouteGeneration() = synchronized(routeLock) { val generation = generationCounter.incrementAndGet()Note that
synchronizedchanges the return type of the expression-bodied form; keep the block bodies and addsynchronized(routeLock) { ... }inside instead if you preferUnitreturns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudioCapabilityManager.kt` around lines 73 - 114, Serialize the compound snapshot updates in publishCapabilities and bumpOutputRouteGeneration using a shared routeLock, wrapping each method body in synchronized(routeLock) while preserving block-bodied Unit returns. Guard all reads and writes of playbackRouteSnapshot, _capabilities, _outputRouteGeneration, generationCounter, and routeSnapshotInitialized within the lock so each planning view is published consistently and initialization is safely visible across callback threads.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileFreshSubtitleRestore.kt (1)
25-51: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve local downloaded subtitles during authoritative restore.
When
authoritativeInventoryis true,prepareMobileFreshSubtitleRestorereturns onlymountedSubtitles.PlayerViewModelpublishes that list directly, so local downloaded subtitles disappear during fresh playback and persistedSubtitleIdentity.Downloadedselections cannot resolve. Merge local downloads before publishing the fresh state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileFreshSubtitleRestore.kt` around lines 25 - 51, Update prepareMobileFreshSubtitleRestore so authoritativeInventory still loads and merges local downloaded subtitles before producing subtitleTracks; do not discard downloaded when the flag is true. Preserve mountedSubtitles as the authoritative base while ensuring persisted SubtitleIdentity.Downloaded selections can resolve in the published fresh state.
🧹 Nitpick comments (7)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.kt (1)
419-445: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not cache the fallback probe.
detectPlatformSoftwareAudioCodecscaches the result of a failed enumeration. The cache is process-wide and never invalidated. IfMediaCodecListthrows once, every laterdetect()call reusesexact = falseand the AAC/MP3 fallback list for the whole process lifetime. The device then reportsCAPABILITY_EVIDENCE_PLATFORM_ATTESTEDaudio permanently, and the server never grants passthrough on that run. Cache only the exact probe so a later call can retry.♻️ Proposed change
- cachedPlatformSoftwareAudioProbe = probe + if (probe.exact) { + cachedPlatformSoftwareAudioProbe = probe + } return probe🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.kt` around lines 419 - 445, Update detectPlatformSoftwareAudioCodecs so only successfully enumerated probes with exact = true are assigned to cachedPlatformSoftwareAudioProbe; return the AAC/MP3 non-exact fallback without caching when MediaCodecList enumeration fails, allowing subsequent calls to retry.androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/audiobook/AudiobookPlayerScreen.kt (1)
172-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe same
onPlayerErroroverride was added to both audiobook screens with a fully qualifiedPlaybackException. Both files already import the other Media3 types they use, includingPlayer, so the inline fully qualified name is the one inconsistency shared by the two copies. The error handling itself is correct in both places.
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/audiobook/AudiobookPlayerScreen.kt#L172-L175: addimport androidx.media3.common.PlaybackExceptionand change the parameter type toPlaybackException.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt#L203-L206: apply the identical import and parameter-type change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/audiobook/AudiobookPlayerScreen.kt` around lines 172 - 175, The onPlayerError overrides use an inconsistent fully qualified PlaybackException type. In androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/audiobook/AudiobookPlayerScreen.kt:172-175 and androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt:203-206, import androidx.media3.common.PlaybackException and use the unqualified type in each onPlayerError parameter, preserving the existing error handling.shared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReadyTest.kt (1)
50-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the delivery-validation branches.
The tests cover a valid
sidecarrow and an index gap. They do not cover the delivery rules inapplyAuthoritativeSubtitleReadyTrack:sidecarwith a blank URL must return null, andburn_in_onlywith a non-blank URL must return null. Both branches decide whether a malformed row enters playback state.♻️ Proposed test
+ `@Test` + fun mismatchedDeliveryAndUrlIsRejected() { + val sidecarWithoutUrl = decodePlaybackSubtitleReady( + buildJsonObject { + putJsonObject("track") { + put("track_id", "file:9:subtitle:0") + put("combined_index", 0) + put("source", "embedded") + put("delivery", "sidecar") + } + }, + ) + assertNull(applyAuthoritativeSubtitleReadyTrack(emptyList(), sidecarWithoutUrl)) + + val burnInWithUrl = decodePlaybackSubtitleReady( + buildJsonObject { + putJsonObject("track") { + put("track_id", "file:9:subtitle:0") + put("combined_index", 0) + put("source", "embedded") + put("delivery", "burn_in_only") + put("url", "/stream/s/subtitles/0.vtt") + } + }, + ) + assertNull(applyAuthoritativeSubtitleReadyTrack(emptyList(), burnInWithUrl)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReadyTest.kt` around lines 50 - 66, Add tests in PlaybackSubtitleReadyTest covering both delivery-validation branches in applyAuthoritativeSubtitleReadyTrack: a sidecar track with a blank URL and a burn_in_only track with a non-blank URL must each return null, while preserving the existing valid sidecar and index-gap coverage.Source: Coding guidelines
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt (1)
284-285: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
authoritativeSubtitleReadyRowsgrows without bound across sessions. Both players cache subtitle-ready rows in a map keyed by(sessionId, subtitleId)and never remove entries. Each replan, seek recovery, 404 renewal, and auto-advance episode mints a new session id, so entries for retired sessions accumulate for the ViewModel's whole life while only the current session id is ever read.
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt#L284-L285: drop entries whose session id no longer matches the active session. Clear the map inresetPlaybackRecoveryState()or at the start ofloadContent, alongsidependingAuthoritativeSubtitleDownloadId.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt#L1336-L1337: apply the same removal inresetSeekRecoveryForContentChange()orprepareSessionExit(), so retired-session rows do not survive a content change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt` around lines 284 - 285, Clear authoritativeSubtitleReadyRows whenever playback recovery or content changes reset the active session, alongside pendingAuthoritativeSubtitleDownloadId. In androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt:284-285, update resetPlaybackRecoveryState() or loadContent; in androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt:1336-1337, update resetSeekRecoveryForContentChange() or prepareSessionExit() so retired-session rows are removed.android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/AudiobookPlayerTeardownSourceTest.kt (2)
26-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an ordering assertion; the test name claims ordering but the body checks only presence.
The test is named "reports both timelines through the retained lifecycle without blocking". Lines 29-43 assert only that nine strings appear somewhere in
onClearedSource. They do not prove thatreportPositionruns beforestopAsync, which is the behavior that protects the persisted position.The sibling test on lines 50-51 already uses the correct
indexOfcomparison. Apply the same pattern here.💚 Proposed ordering assertion
assertTrue(onClearedSource.contains("playbackSessionLifecycle.stopAsync(")) + assertTrue( + "reportPosition must run before stopAsync", + onClearedSource.indexOf("playbackSessionLifecycle.reportPosition(") < + onClearedSource.indexOf("playbackSessionLifecycle.stopAsync("), + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/AudiobookPlayerTeardownSourceTest.kt` around lines 26 - 44, Add an indexOf-based ordering assertion to the test `onCleared reports both timelines through the retained lifecycle without blocking`, verifying that `playbackSessionLifecycle.reportPosition(` appears before `playbackSessionLifecycle.stopAsync(` in `onClearedSource`, matching the sibling test’s pattern.
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the source slices; a missing delimiter silently widens the slice.
substringAfterreturns the whole receiver when the delimiter is absent.substringBeforereturns the whole remainder when its delimiter is absent. Neither throws.Three changes break these slices without failing any test:
- A rename of
startSingleFileSession,startPartSession, orretireActiveSession.- A modifier change, for example
private suspend funtoprivate fun.- A reorder that no longer places
startPartSessiondirectly beforeretireActiveSession.After any of them the slice widens to cover unrelated code, the substring assertions still find their text elsewhere in the file, and the test reports success while guarding nothing.
Assert that each delimiter exists before slicing.
💚 Proposed helper that fails on a missing delimiter
+ private fun slice(after: String, before: String): String { + require(viewModelSource.contains(after)) { "Missing anchor: $after" } + val tail = viewModelSource.substringAfter(after) + require(tail.contains(before)) { "Missing anchor: $before" } + return tail.substringBefore(before) + } + private val singleFileStartSource = slice( after = "private suspend fun startSingleFileSession(", before = "private suspend fun startPartSession(", ) private val partStartSource = slice( after = "private suspend fun startPartSession(", before = "private suspend fun retireActiveSession(", )Apply the same helper to
onClearedSourceon lines 14-16.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/AudiobookPlayerTeardownSourceTest.kt` around lines 18 - 25, Make the source-slicing test fail when delimiters are missing by introducing a helper that validates both boundary markers before applying substringAfter and substringBefore. Use it for singleFileStartSource, partStartSource, and onClearedSource, preserving the existing delimiter strings and slice ranges.android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparerTest.kt (1)
16-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen both assertions so they discriminate against a trivial implementation.
Line 18 asserts only the absence of the removed token. It does not prove that the neutral v3 contract token is advertised, which is the stated requirement of this migration.
Line 29 sets
playerStartSeconds = 0.0, which is also the default ofPlaybackTimelineV3. The assertion on line 33 passes even ifcastPlayerStartPositionreturns a constant0.0or the timeline default. A non-zero player start separates the player-local value from both the source position and the default.💚 Proposed test changes
`@Test` fun castContextDoesNotAdvertiseThePreNeutralSidecarFeature() { + val features = playbackClientFeaturesV3(chromecastPlaybackContext("test")) assertFalse( - "external_text_sidecar_set_v1" in - playbackClientFeaturesV3(chromecastPlaybackContext("test")), + "external_text_sidecar_set_v1" in features, ) + // Replace with the actual neutral v3 contract token constant. + assertTrue(NEUTRAL_V3_CONTRACT_FEATURE in features) } `@Test` fun castUsesPlayerLocalStartInsteadOfSourceTimelinePosition() { val plan = plan( timeline = PlaybackTimelineV3( sourceStartSeconds = 90.0, - playerStartSeconds = 0.0, + playerStartSeconds = 12.5, ), ) - assertEquals(0.0, castPlayerStartPosition(plan, requested = 90.0)) + assertEquals(12.5, castPlayerStartPosition(plan, requested = 90.0)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparerTest.kt` around lines 16 - 34, Strengthen castContextDoesNotAdvertiseThePreNeutralSidecarFeature by additionally asserting that the neutral v3 contract token is present in playbackClientFeaturesV3(chromecastPlaybackContext("test")). Update castUsesPlayerLocalStartInsteadOfSourceTimelinePosition to use a non-zero playerStartSeconds distinct from sourceStartSeconds and assert that value is returned by castPlayerStartPosition, proving it does not use a constant or default timeline value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudiobookPlayerViewModel.kt`:
- Around line 244-249: Guard the stale-result cleanup in the collect block
around playback renewal by wrapping
playbackSessionManager.stopSession(start.session.sessionId) in runCatching,
matching the existing handling near the other stale-result cleanup. Also ensure
failures anywhere in this renewal observer’s collect body cannot cancel the
coroutine, while preserving the existing renewal and stale-result behavior.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt`:
- Around line 916-923: Update beginOutageRecovery to synchronize all outageJob
reads and writes using the existing recoveryJobLock, including the active-job
check and coroutine publication. Update cancelRecoveryJobs to cancel and clear
outageJob inside the same lock, preserving the existing recoveryJob
synchronization and preventing missed cancellation during teardown.
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.kt`:
- Line 49: Update the selected subtitle handling around
resolvedSelectedSubtitleIndex() so an unresolved optional
selectedTracks.subtitle never defaults to inventory index 0. Preserve the
CONVERT/RENDER artifact using a distinct non-colliding selection representation,
or reject the incomplete plan before deduplication, ensuring the selected
subtitle remains mountable when inventory index 0 exists.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/cast/SiloCastSessionManager.kt`:
- Around line 678-683: Update the subtitle ID selection in the session state
construction so a non-null plannedSubtitles value is authoritative: return the
selected subtitle’s receiver track ID when one is selected, otherwise return
null, without consulting subtitleOptions or activeIds. Only use the
receiver-derived fallback when plannedSubtitles itself is null.
- Around line 136-159: Update the progress-reporting logic in progressListener
to track whether reportProgress is already in flight, and skip launching another
report while the existing coroutine is pending. Set the in-flight state before
launching and clear it in a finally block so it resets on both success and
failure, while preserving the existing elapsed-time throttle and progress
values.
- Around line 389-401: Bound the recovery loop in the cast load-failure branch
of SiloCastSessionManager by tracking attempts for the pending playback spec and
stopping recovery after a finite maximum. Increment and check the counter before
calling recoverFromLoadFailure or prepareMedia(replacement), reset it when
starting a new spec or successful preparation, and preserve the existing pending
cleanup/stop behavior when recovery is exhausted or returns null.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt`:
- Around line 865-868: Guard the adapted versionId assignment in the
transaction-building code using the same media-file comparison as
withRebasedDownloads: only emit "adapted:<id>" when
playback.effectiveMediaFileId differs from liveContext.mediaFileId; otherwise
preserve liveContext.versionId. Update the versionId expression near mediaFileId
while leaving the existing fallback behavior intact.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileVideoPlaybackStarter.kt`:
- Around line 372-374: Preserve the nullable duration from
resolved.durationSeconds in the VideoPlaybackStartResult.Ready path instead of
defaulting it to 0.0, and carry that nullability through the player UI state.
Update PlayerProgressBar consumers as needed so unknown durations do not produce
synthetic one-second progress or false −0:00 remaining time.
- Around line 315-320: Update the StartParams construction in
MobileVideoPlaybackStarter to set subtitleTrackIndex from
initialTracks.subtitleTrackIndex when present, otherwise use
readyV3.plan.resolvedSelectedSubtitleIndex(). Preserve explicit -1 for subtitles
Off and null when the plan selected no subtitle.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt`:
- Around line 3409-3421: Update the pending-authoritative-subtitle handling
around pendingAuthoritativeSubtitleDownloadId so it is cleared only after added
resolves to a valid mobile subtitle identity and
mobileSubtitleTransactions.selectFromRefresh is invoked. Preserve the pending
marker when added is null or mobileSubtitleIdentity cannot resolve, allowing a
later ready event to complete auto-selection.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt`:
- Around line 1536-1544: The launchSubtitleTransaction method must serialize
subtitlePlaybackContext(state) and the subsequent transaction() as one atomic
operation. Add or reuse a Mutex around both updatePlaybackContext and
transaction execution, ensuring all subtitle transaction callers share the same
lock so suspended context construction cannot allow a newer request to be
overwritten.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.kt`:
- Around line 284-290: Update the duration handling in TvVideoPlaybackStarter so
an unknown duration remains unknown instead of being converted to 0.0. In
updateScrubPreview(), apply the upper-bound clamp only when duration is
positive; when it is zero or otherwise unknown, clamp the preview only to a
lower bound of zero so auto-seek and scrubber nudges can move.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/playback/PlaybackSubtitleIdentity.kt`:
- Around line 180-190: Update subtitleLabelIndicatesHearingImpaired so the
ambiguous bare “hi” token no longer matches language-only labels such as “hi” or
“EN - HI”; retain the unambiguous “cc” and “sdh” detection and existing phrase
checks. Hoist the hearing-impaired token Regex to a reusable declaration outside
the function so it is compiled once, then reuse it from
subtitleLabelIndicatesHearingImpaired.
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReady.kt`:
- Around line 31-36: Update the PlaybackSubtitleReady construction to safely
cast each payload field to JsonPrimitive before reading content, intOrNull, or
other values, so object and array fields resolve to null instead of throwing.
Preserve valid primitive parsing and ensure malformed subtitle_ready events do
not terminate collection or trigger reconnects.
---
Outside diff comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudioCapabilityManager.kt`:
- Around line 73-114: Serialize the compound snapshot updates in
publishCapabilities and bumpOutputRouteGeneration using a shared routeLock,
wrapping each method body in synchronized(routeLock) while preserving
block-bodied Unit returns. Guard all reads and writes of playbackRouteSnapshot,
_capabilities, _outputRouteGeneration, generationCounter, and
routeSnapshotInitialized within the lock so each planning view is published
consistently and initialization is safely visible across callback threads.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileFreshSubtitleRestore.kt`:
- Around line 25-51: Update prepareMobileFreshSubtitleRestore so
authoritativeInventory still loads and merges local downloaded subtitles before
producing subtitleTracks; do not discard downloaded when the flag is true.
Preserve mountedSubtitles as the authoritative base while ensuring persisted
SubtitleIdentity.Downloaded selections can resolve in the published fresh state.
---
Nitpick comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.kt`:
- Around line 419-445: Update detectPlatformSoftwareAudioCodecs so only
successfully enumerated probes with exact = true are assigned to
cachedPlatformSoftwareAudioProbe; return the AAC/MP3 non-exact fallback without
caching when MediaCodecList enumeration fails, allowing subsequent calls to
retry.
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/AudiobookPlayerTeardownSourceTest.kt`:
- Around line 26-44: Add an indexOf-based ordering assertion to the test
`onCleared reports both timelines through the retained lifecycle without
blocking`, verifying that `playbackSessionLifecycle.reportPosition(` appears
before `playbackSessionLifecycle.stopAsync(` in `onClearedSource`, matching the
sibling test’s pattern.
- Around line 18-25: Make the source-slicing test fail when delimiters are
missing by introducing a helper that validates both boundary markers before
applying substringAfter and substringBefore. Use it for singleFileStartSource,
partStartSource, and onClearedSource, preserving the existing delimiter strings
and slice ranges.
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparerTest.kt`:
- Around line 16-34: Strengthen
castContextDoesNotAdvertiseThePreNeutralSidecarFeature by additionally asserting
that the neutral v3 contract token is present in
playbackClientFeaturesV3(chromecastPlaybackContext("test")). Update
castUsesPlayerLocalStartInsteadOfSourceTimelinePosition to use a non-zero
playerStartSeconds distinct from sourceStartSeconds and assert that value is
returned by castPlayerStartPosition, proving it does not use a constant or
default timeline value.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/audiobook/AudiobookPlayerScreen.kt`:
- Around line 172-175: The onPlayerError overrides use an inconsistent fully
qualified PlaybackException type. In
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/audiobook/AudiobookPlayerScreen.kt:172-175
and
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.kt:203-206,
import androidx.media3.common.PlaybackException and use the unqualified type in
each onPlayerError parameter, preserving the existing error handling.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt`:
- Around line 284-285: Clear authoritativeSubtitleReadyRows whenever playback
recovery or content changes reset the active session, alongside
pendingAuthoritativeSubtitleDownloadId. In
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt:284-285,
update resetPlaybackRecoveryState() or loadContent; in
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt:1336-1337,
update resetSeekRecoveryForContentChange() or prepareSessionExit() so
retired-session rows are removed.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReadyTest.kt`:
- Around line 50-66: Add tests in PlaybackSubtitleReadyTest covering both
delivery-validation branches in applyAuthoritativeSubtitleReadyTrack: a sidecar
track with a blank URL and a burn_in_only track with a non-blank URL must each
return null, while preserving the existing valid sidecar and index-gap coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45357666-94f0-4dc5-a121-6e87fe6ebf35
📒 Files selected for processing (78)
.agents/skills/test-shield-playback/scripts/shield-testandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudioCapabilityManager.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudiobookPlayerViewModel.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackCapabilityDetector.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionManager.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/ReplayableSubtitleDataSource.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloLoadControl.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SubtitleManager.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SubtitleMountResolver.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/VideoPlayerMediaSpec.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparer.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractor.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/EpisodeSelectionHandoff.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PlaybackStartupStallDetector.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/VideoPlaybackStartRequest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/AudiobookPlayerTeardownSourceTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackPlanningSnapshotRegistryTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerSeekReanchorTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionManagerStagedReplanTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackV3SessionTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/SiloLoadControlTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/SubtitleManagerTrackSelectionTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparerTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractorTest.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/video/PlaybackStartupStallDetectorTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/cast/SiloCastSessionManager.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/audiobook/AudiobookPlayerScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileFreshSubtitleRestore.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleAutoSelection.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileVideoPlaybackStarter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlaybackRealtimeController.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileFreshSubtitleRestoreTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleAutoSelectionTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapterTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/SubtitleTrackSelectionTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/audiobook/TvAudiobookPlayerScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvPlaybackFormatting.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlaybackRealtimeController.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitlePolicy.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleIdentity.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleRemountReselection.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleTransactionAdapter.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/diagnostics/TvDiagnosticsPromptScreen.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/SubtitleRemountReselectionTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/SubtitleTransactionIntegrationTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlaybackExitSnapshotTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlaybackQualityOptionsTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerSubtitleIntegrationPolicyTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleRefreshOwnershipTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvSubtitleTransactionAdapterTest.ktdocs/playback/01-media3-only-player-architecture.mddocs/superpowers/plans/2026-08-02-instant-external-srt-switching-android.mdshared/src/androidUnitTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3ConformanceTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackModels.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackSubtitleChoices.ktshared/src/commonMain/kotlin/org/siloserver/silo/playback/PlaybackSubtitleIdentity.ktshared/src/commonMain/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReady.ktshared/src/commonMain/kotlin/org/siloserver/silo/playback/SubtitleCodecFamily.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackSubtitleChoicesTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleIdentityTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReadyTest.ktshared/src/commonTest/resources/playback/v3/SOURCEshared/src/commonTest/resources/playback/v3/capability_response.jsonshared/src/commonTest/resources/playback/v3/conformance_matrix.jsonshared/src/commonTest/resources/playback/v3/decision_response.json
🚧 Files skipped from review as they are similar to previous changes (20)
- shared/src/commonTest/resources/playback/v3/SOURCE
- shared/src/commonTest/resources/playback/v3/capability_response.json
- docs/superpowers/plans/2026-08-02-instant-external-srt-switching-android.md
- .agents/skills/test-shield-playback/scripts/shield-test
- shared/src/commonTest/resources/playback/v3/decision_response.json
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/VideoPlayerMediaSpec.kt
- shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PlaybackApiTest.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/ReplayableSubtitleDataSource.kt
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerScreen.kt
- android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/subtitle/PgsSupExtractorTest.kt
- androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/SiloLoadControl.kt
- docs/playback/01-media3-only-player-architecture.md
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerScreen.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/PlaybackStartupStallDetector.kt
- shared/src/androidUnitTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3ConformanceTest.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackModels.kt
- shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.kt
- android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycleTest.kt
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/cast/SiloCastSessionManager.kt (1)
403-414: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear local playback state after terminal load recovery.
Line 412 clears
pendingdirectly when recovery returnsnull. This bypasses the progress-job cancellation infinalizePending. It also retains the previousfileId, title, subtitle state, and position becausesyncCastStatefalls back to cached state whenpendingis null at Lines 685-690.Clear the active playback fields and cancel the progress job on this terminal path. Preserve the connected Cast route state. Otherwise, the phone can show an ended item and suppress a later auto-stage for that file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/cast/SiloCastSessionManager.kt` around lines 403 - 414, Update the terminal recovery branch in SiloCastSessionManager’s load-failure handling to use finalizePending instead of assigning pending = null directly. Ensure it cancels the progress job and clears the active file, title, subtitle, and position fields while preserving the connected Cast route state, so subsequent sync and auto-stage behavior see no ended local item.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparer.kt`:
- Around line 453-457: In CastPlaybackPreparer.kt:453-457, add a mutex-protected
success method that resets loadFailureRecoveryAttempts, and invoke it from
SiloCastSessionManager.kt:398-402 only after the receiver confirms the current
pending spec loaded successfully; do not reset the budget on failed or unrelated
loads.
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerProgressBar.kt`:
- Around line 74-89: Gate the intro and chapter timeline decoration calculations
and rendering on hasKnownDuration in the PlayerProgressBar composable. When
duration is unknown, skip both intro tint and chapter tick decorations entirely;
preserve their existing positioning and rendering when the duration is known.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReadyTest.kt`:
- Around line 70-84: Update malformedScalarFieldsDoNotAbortRealtimeDecoding to
include valid control values for the other playback subtitle fields and assert
they decode successfully while the malformed scalar becomes null. Add separate
test cases for malformed session_id, file_id, and subtitle_id so each field’s
isolated failure behavior is verified without allowing whole-payload discarding
to pass.
---
Outside diff comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/cast/SiloCastSessionManager.kt`:
- Around line 403-414: Update the terminal recovery branch in
SiloCastSessionManager’s load-failure handling to use finalizePending instead of
assigning pending = null directly. Ensure it cancels the progress job and clears
the active file, title, subtitle, and position fields while preserving the
connected Cast route state, so subsequent sync and auto-stage behavior see no
ended local item.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf49e3a0-6198-4cd2-b494-584b65562325
📒 Files selected for processing (24)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudiobookPlayerViewModel.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/cast/CastPlaybackPreparer.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/VideoPlaybackStartResult.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/video/VideoPlayerUiState.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/player/PlaybackV3SessionTest.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/cast/SiloCastSessionManager.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileVideoPlaybackStarter.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerProgressBar.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapterTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerProgressBarTest.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvScrubPreviewPolicyTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.ktshared/src/commonMain/kotlin/org/siloserver/silo/playback/PlaybackSubtitleIdentity.ktshared/src/commonMain/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReady.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.ktshared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleIdentityTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReadyTest.kt
🚧 Files skipped from review as they are similar to previous changes (13)
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/playback/PlaybackSubtitleReady.kt
- shared/src/commonTest/kotlin/org/siloserver/silo/playback/PlaybackSubtitleIdentityTest.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackV3Session.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/playback/PlaybackSubtitleIdentity.kt
- shared/src/commonTest/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3Test.kt
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileVideoPlaybackStarter.kt
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/MobileSubtitleTransactionAdapter.kt
- shared/src/commonMain/kotlin/org/siloserver/silo/model/playback/PlaybackProtocolV3.kt
- androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModel.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/AudiobookPlayerViewModel.kt
- androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt
- android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/PlaybackSessionLifecycle.kt
|
Final review and remediation are complete at
The PR description has been updated with the final validation evidence. The app is stopped cleanly and left on the Shield Home screen. |
The ceiling is derived so STEADY-STATE crossing lands near TRAVERSE_TARGET_SECONDS, but a hold does not start at the ceiling — it doubles every 900ms to reach it, and the early rungs cover almost nothing. A three-hour film spends 8.1s ramping and covers only ~920s of itself in that time, so the real cost is ~17.8s, not the ~10.5s the docs claimed. The test claimed to check this and could not: it computed duration / topRate, arithmetic the implementation never performs, so it reported 10.55s against a 15s tolerance and passed while the real behaviour was 17.75s. TvSeekRateLadder.traverseSeconds now models the ramp the code actually runs, the test asserts against it, and the documentation states the honest envelope: ~10.6s for a 22-minute episode to ~17.8s for a three-hour film. The property worth keeping is the SPREAD — under 2x across runtimes, versus 41s vs 338s before the ceiling was derived — not the absolute number, and that is now what is asserted. Mutation-checked: changing the ramp cadence fails it. Also pins the unknown-duration case. Protocol v3 (Silo-Server#200) declares duration server-side and deliberately refuses a Media3/catalog fallback, so an omitted duration now reaches the ladder as 0 and lands on MIN_TOP_RATE. That is more reachable than it was before v3, so it is worth a test. androidTvApp 1003 -> 1006, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ceiling is derived so STEADY-STATE crossing lands near TRAVERSE_TARGET_SECONDS, but a hold does not start at the ceiling — it doubles every 900ms to reach it, and the early rungs cover almost nothing. A three-hour film spends 8.1s ramping and covers only ~920s of itself in that time, so the real cost is ~17.8s, not the ~10.5s the docs claimed. The test claimed to check this and could not: it computed duration / topRate, arithmetic the implementation never performs, so it reported 10.55s against a 15s tolerance and passed while the real behaviour was 17.75s. TvSeekRateLadder.traverseSeconds now models the ramp the code actually runs, the test asserts against it, and the documentation states the honest envelope: ~10.6s for a 22-minute episode to ~17.8s for a three-hour film. The property worth keeping is the SPREAD — under 2x across runtimes, versus 41s vs 338s before the ceiling was derived — not the absolute number, and that is now what is asserted. Mutation-checked: changing the ramp cadence fails it. Also pins the unknown-duration case. Protocol v3 (Silo-Server#200) declares duration server-side and deliberately refuses a Media3/catalog fallback, so an omitted duration now reaches the ladder as 0 and lands on MIN_TOP_RATE. That is more reachable than it was before v3, so it is worth a test. androidTvApp 1003 -> 1005, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#209) * fix(tv): make the seek rate mean what the chip says, and stop it running away Hold-to-seek advanced 2.0 seconds of content on every 100ms tick, so the rate on the chip was a twentieth of the truth: "8×" moved at 160× real time, and the top speed of "32×" moved at 640×, crossing a 45-minute episode in four seconds. That is the whole reason it felt ungovernable rather than merely quick — the viewer aims with the number on screen, and the number was wrong by 20×. A rate is now exactly its own multiple of real time: rate × tick seconds per tick. 8× means 8×. Two things fell out of fixing that. The ramp reached the top speed after three seconds of holding, so a press meant to nudge forward a few seconds crossed the scene; the milestones are now 1.5s / 3s / 5s. And a sustained hold now stops at 16× — reaching 32× takes a deliberate repeat-press, so holding cannot fall into the fastest speed by accident. 1× is dropped from the ladder. It scans at exactly playback speed, so the first press looked like nothing had happened. Speeds and ramp move into TvSeekRateLadder as pure functions, and the test asserts the property that was violated: a rate advances exactly that multiple of real time. A magic multiplier in the tick now fails a test instead of shipping another chip that lies. Verified: :androidTvApp:testDebugUnitTest 983 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): derive the seek ceiling from runtime so the end is reachable The honest-rate fix made the labels true but left a fixed 32x ceiling, and a fixed ceiling cannot serve both ends of this control. Nudging past an intro wants single digits. Reaching the end of a 45-minute episode at 32x takes 84 seconds of holding, and a three-hour film takes five and a half minutes — that is not a seek. The top of the ladder now comes from the item's runtime, targeting about ten seconds to cross the whole thing: 22-min episode 256x 5.2s to cross (was 41s) 45-min episode 512x 5.3s (was 84s) 90-min film 1024x 5.3s (was 169s) 3h film 1024x 10.5s (was 338s) The ramp follows from the same place: it keeps doubling every 900ms until it reaches that item's ceiling, so a long film goes on accelerating past the point where a short episode has already topped out. Reaching the top takes 6-8s of deliberate holding, and the first step is still 4x, so the aimable half of the control is untouched. An unknown runtime falls back to 32x rather than guessing — live content and un-probed files both arrive as zero duration. Two corrections to the previous commit's tests. The traverse assertion caught a real bug: 512x was too low a cap for a three-hour film, which crossed in 21s against a 10s target, so the cap is 1024x. And the reverse-bump test asserted semantics the key handlers do not use — delta is a direction along the signed ladder, not "faster" — so it now pins the property that actually matters: a bump never crosses zero and flips direction mid-seek. Verified: :androidTvApp:testDebugUnitTest 985 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tv): make the traversal claim honest about the ramp The ceiling is derived so STEADY-STATE crossing lands near TRAVERSE_TARGET_SECONDS, but a hold does not start at the ceiling — it doubles every 900ms to reach it, and the early rungs cover almost nothing. A three-hour film spends 8.1s ramping and covers only ~920s of itself in that time, so the real cost is ~17.8s, not the ~10.5s the docs claimed. The test claimed to check this and could not: it computed duration / topRate, arithmetic the implementation never performs, so it reported 10.55s against a 15s tolerance and passed while the real behaviour was 17.75s. TvSeekRateLadder.traverseSeconds now models the ramp the code actually runs, the test asserts against it, and the documentation states the honest envelope: ~10.6s for a 22-minute episode to ~17.8s for a three-hour film. The property worth keeping is the SPREAD — under 2x across runtimes, versus 41s vs 338s before the ceiling was derived — not the absolute number, and that is now what is asserted. Mutation-checked: changing the ramp cadence fails it. Also pins the unknown-duration case. Protocol v3 (#200) declares duration server-side and deliberately refuses a Media3/catalog fallback, so an omitted duration now reaches the ladder as 0 and lands on MIN_TOP_RATE. That is more reachable than it was before v3, so it is worth a test. androidTvApp 1003 -> 1005, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tv): the scrubber no longer tops out at 32x The header still described the fixed ceiling this branch replaced. The rate now doubles to a runtime-derived ceiling — 256x for a 22-minute episode, 1024x for a feature — which is the whole point of the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: rxwatcher <rxwatcher@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Problem
Part of Silo-Server/silo-server#135
The Android phone/TV playback stack still depended on the pre-neutral v3 draft: client-derived attempt behavior, platform-shaped capability claims, and recovery paths that could lose track intent or restart indefinitely. The client also lacked a strict executable binding to the server's canonical contract corpus.
Approach
Coordinated release train:
Review follow-up
subtitle_ready.trackhandling, source-duration rules, server-ordered quality menus, and shared phone/TV subtitle identity migration.main(908466c108c00104ba76e965cc294aae30b4c4de) and re-ran the review and validation on the resolved head.5508a810.Risks and follow-up
Testing
./gradlew test lint :androidApp:assembleDebug :androidTvApp:assembleDebug: passed (371 tasks).79e3e761ad391b1aa9f2c280eeceeb23df9d3c81.git diff --check: passed.AI Disclosure
Summary by CodeRabbit
New Features
Bug Fixes