feat(playback): finalize platform-neutral protocol v3 - #567
Conversation
Supersedes the wire-contract sections of the 2026-07-12 v3 plan: server-owned attempt keys, delivery-keyed negotiation without Media3 engine names, tiered capability evidence, neutral device/output context, track/quality replan operations, audio-only planning, and coordinated no-back-compat rollout across server, Android, Apple, and web. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… with deliveries Contract core of the platform-neutral v3 finalization (plan sections 3.1 and 3.2), breaking on purpose — v3 is dark and all clients move together: - Every PlanV3 now carries plan_attempt_key, an opaque server-computed token clients store and echo in attempted_plan_keys; ReplanRequestV3 gains bounded local_mutations that the replan handler folds into the failed plan's key. Clients never hash anything. - KotlinName() is deleted from DeliveryV3, StreamProtocolV3 and SubtitleModeV3; the attempt-key canonical string now uses lowercase wire tokens, and PlanRecipeVersionV3 bumps to v3.3 so no key or plan ID computed under the old canonicalization can collide. - EngineV3 leaves the wire: ClientPlaybackContextV3.Engines (media3_*) becomes Deliveries keyed original_http|progressive|hls, with EngineCapabilityV3 renamed DeliveryCapabilityV3. PlanV3.Engine is removed; the planner, subtitle policy and quirk registry re-key on delivery class, and the media3_only feature token is deleted. - Validated-claim strings drop the prefix: media3_h264_decode -> h264_decode, media3_audio_decode -> audio_decode. - Golden fixtures in testdata/protocol_v3 are regenerated by Go and are now the cross-repo source of truth. Part of the playback protocol v3 neutral-contract train (steps 2-3 of docs/superpowers/plans/2026-07-30-playback-protocol-v3-neutral-contract.md). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implement plan sections 3.3 and 3.4 of the v3 neutral-contract pass: - ClientCodecCapabilitiesV3 gains required video_evidence and audio_evidence closed enums (exact | platform_attested | declared). Planner strictness follows the tier: exact keeps the strict decode-entry validation, platform_attested validates codec/resolution/bit-depth/ frame-rate but skips profile/level matching, declared grants copy routes from the flat codec lists. Only exact audio evidence earns passthrough claims. The detailed_decode_capabilities feature token is deleted (subsumed by video_evidence=exact), and evidence-blocked direct routes carry the new evidence_insufficient_for_direct reason/warning. - DeviceContextV3 is now platform/os_version/manufacturer/model plus a bounded platform_details map (<=16 entries, <=128 chars); the Android Build dump fields are gone. Fire TV quirks keep matching on manufacturer/model (brand fallback removed with the field). - output_route_generation (int64, dual-location) becomes an optional opaque output_context_id string on the output context; the dual-location consistency validation is deleted. Attempt keys, plan invalidation, route events, and the planstore column follow (new Goose migration). - Feature advertisement collapses to the top-level client_features list only; ClientPlaybackContextV3.Features is deleted and ReplanRequestV3 gains an optional client_features refresh. - PlanRecipeVersionV3 bumped v3.3 -> v3.4; fixtures re-keyed. Part of the playback protocol v3 neutral-contract finalization plan (docs/superpowers/plans/2026-07-30-playback-protocol-v3-neutral-contract.md). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…outes Protocol v3 could only replan after a failure, so changing the audio track or the quality still required the legacy audio PATCH and the client-recipe transcode start — the two endpoints v3 is meant to replace. Clients also had to own a resolution ladder to render a quality menu, and a source with no video track was terminaled by the video/HDR gates, keeping audiobooks on the legacy path. Add track_change and quality_change replan operations. They carry no failure classification and route through the existing replan transaction, so they inherit its idempotency, capacity reservation, and staged-successor commit for free. Because nothing failed, the previous route stays eligible: neither the attempted-key history nor the failed-plan exclusion applies to them. Publish the server ladder on the plan as available_qualities so the quality menu is server-owned; the rungs come from the same resolutionLabelV3 and ladderBitrateKbpsV3 helpers the planner itself uses, not a parallel table. Plan audio-only sources through their own reduced route family: original_http when the client decodes the codec, otherwise a progressive AAC conversion. The plan advertises audio/mp4 for that remux and the transport now serves the same value, because a declared-tier client probes the advertised MIME with isTypeSupported before attaching a source buffer, and "video/mp4" on a stream with no video track is exactly the mismatch that makes the probe lie. Name the protocol's string vocabulary (dynamic ranges, transformations, executors, validated claims, terminal reasons) as constants while touching these lines, so the wire values have one definition. Part of #135
…inals Protocol v3 exists only as Go code today, so the Android and Apple ports have no authority to implement against other than reading this repository. Publish the contract as a normative document, machine-checkable schemas, and generated golden fixtures, and fix the one place where the server's own wire output disagreed with the ordinal space it publishes. - docs/architecture/playback-protocol-v3.md is self-contained enough for a third-party client: endpoints and status codes, evidence tiers and their bound-matching rules, delivery classes, the timeline model, replan semantics, registries, track identity, plan identity, quality, and transformations. - docs/design/schemas/playback-v3/ carries JSON Schemas for the five wire shapes plus valid and invalid fixtures, following the client-diagnostics layout. internal/playback/contract validates every fixture against its schema, so a schema that drifts from the Go types fails the Go suite. - cmd/playbackfixtures generates internal/playback/testdata/protocol_v3 from the production planner. `make playback-fixtures` writes them and `make verify-playback-fixtures` (wired into CI) fails when they are stale. These files are what the client ports consume, so drift would otherwise surface as a playback bug on three platforms at once. The subtitle fix: combined ordinals are one dense space over externals, then embedded tracks, then downloaded ones, but the legacy URL builder skipped burn-in-only tracks while assigning indices, so every track after a DVD/DVB track was numbered one too low and resolved to its neighbour. Ordinal assignment now lives in playback.BuildSubtitleInventoryV3 and both the plan inventory and the legacy `subtitle_urls` shape project from it; the legacy shape still filters burn-in-only entries but keeps each track's real index. Part of #135
The web player was the last client still speaking the legacy start protocol: it picked its own file version from a codec probe, posted an ffmpeg recipe to start a transcode, PATCHed an endpoint to change audio tracks, and derived its own quality ladder. None of that survives a server-owned plan, and none of it produced telemetry the apps could be compared against. Video player: starts with a v3 request that advertises `declared` evidence from `isTypeSupported` probes and the three delivery classes, then consumes the returned plan for its URL, timeline, tracks and warnings. Quality and track changes become replans (`quality_change`, `track_change`), the quality menu renders `available_qualities` instead of computing rungs, and playback failures emit `route-events` so web failures land in the same diagnostics as Android and Apple. The duration comes from `source.duration_seconds` rather than the playback engine, and the "how was this delivered" overlay reads the plan's delivery and server transformations instead of comparing codec strings. Audiobook player: starts against the audio-only planner path with a single `original` rung, and takes its seek anchor from `timeline.player_start_seconds` so the progressive-remux route (which anchors the stream and restarts the player clock at zero) does not seek twice. Server side, `disable_progress_persistence` left the wire, so the rule it encoded is now derived. Resume state is keyed on the item, but every part of a multipart presentation shares that key while carrying its own file-local clock — persisting part 4's position would store "12 minutes in" as the book's resume point. `PresentationPartTotal > 1` expresses that directly and generalizes to multipart movies and split episodes, and a client can no longer forget to ask or lie about it. `useTranscodeQuality` and the legacy response types are deleted, and `WEBTEST_KNOWN_FAILURES` loses the audiobook entry along with its fix. Part of #135
Protocol v3 shipped behind a flag, alongside the legacy start path it was
designed to replace. Running both meant every planner change had to be made
twice, in two shapes that disagree about who decides the route: the legacy
body carried a decision the client had already made, while v3 asks the server
to make it. This deletes the legacy half.
Removed:
- `handleStartPlaybackLegacy` and its request/response bodies. The
`POST /playback/start` route stays, but the protocol-version dispatch
envelope is now a strict v3 decode — a body that does not declare
`protocol_version: 3` gets `426 client_upgrade_required` so an outdated app
can render a clear "update required" state instead of misreading a plan.
Deliberately not a `400`: the request may be well-formed for the protocol it
was written against.
- `POST /playback/transcode/start`, superseded by the `quality_change` replan
operation, and `PATCH /playback/{session_id}/audio`, superseded by
`track_change`. Both mutated a session without re-planning.
- The shadow planner and both rollout settings rows. With v3 the only
protocol, `playback.protocol_v3_enabled` would mean "no playback at all";
`playback.protocol_v3_shadow_enabled` gated a comparison against a path that
no longer exists. `409 protocol_disabled` on route-events goes with them, and
capability `enabled` is now constant `true` (the field stays — clients
feature-detect against it).
- Version-selection helpers in `internal/playback/resolver.go` that only legacy
start reached. `Resolve`/`ClientCapabilities`/`PlayDecision` stay: downloads
consumes them. `internal/jellycompat` has its own resolution surface and is
untouched.
Behaviour the legacy handlers owned and v3 now owns explicitly: series version
and audio-track preferences are persisted on start and on a `track_change`
replan (not on failure recovery, whose forced route is not a user choice); an
omitted `start_position` resolves to the profile's saved resume point; and an
omitted audio track resolves through the series preference, the profile audio
language, then the library override. Both are settled before planning, because
the plan's timeline is cut at the start position. Spec §2.2 documents this as
"omission is a request, not a default".
The encode-target clamp that lived in the deleted transcode handler is already
enforced in the planner, twice — `availableQualitiesV3` omits rungs at or above
the source height, and the encode path clamps `targetHeight` to it.
Unchanged: progress, stop, HLS manifest and segment delivery, the realtime
control socket, stream tokens and restart reconstruction, watch together,
downloads, jellycompat.
Every removal is recorded in the pre-lock removals table in
docs/architecture/v1-scope.md.
Part of #135
ffprobe reports embedded cover art as a video stream carrying
disposition.attached_pic. convertProbeData appended every "video" stream
to VideoTracks without consulting isMainVideoStream, the predicate that
already existed for duration decisions, so the picture was persisted as a
playable track. That misreports the file twice:
- An audio file with a cover picks up a video track, so it no longer
satisfies MediaFile.IsAudioOnly and the v3 planner routes an
audiobook through the video path instead of planAudioOnlyV3.
- When the picture is ordered ahead of the real stream, the flat
codec_video/resolution/hdr columns describe the poster: a 954x720
h264 episode was stored as mjpeg 480x480.
Filter attached_pic streams out of the track loop. The guard is the
disposition flag, not the codec name, so a genuine MJPEG video is still
probed as video — the library has one.
Already-probed rows self-heal on the next playback: NeedsCriticalProbeRepair
already reprobes tracks missing color_range, which covers 21 of the 23
affected rows, and applyProbeData overwrites VideoTracks wholesale. The
remaining two need a rescan; nothing persisted records attached_pic, and
keying repair off still-image codec names would reprobe the genuine MJPEG
file on every playback forever.
Part of the playback v3 neutral-contract work: it is what lets Android
drop AUDIOBOOK_COVER_ART_CODECS, which fabricated decode support the
client cannot honestly claim under video_evidence: "exact".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ubtitles off The v3 plan's subtitle inventory is the authoritative track list a client builds its subtitle menu from, but the handler only rewrote it with session-scoped URLs when a track was actually selected. A start or replan that resolved to `subtitle.mode: "off"` therefore returned the planner's URL-less inventory, so a client whose picker reads the inventory had a menu it could not fetch anything from. The Cast path hits this every time: it starts with subtitles off and needs the receiver's text tracks up front. attachSubtitleArtifactV3 now scopes and publishes the inventory unconditionally and gates only the artifact stamping on the selection. Spec §8 records that the `url` on a sidecar entry does not depend on the current selection. Part of the v3 neutral-contract finalization.
|
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:
📝 WalkthroughWalkthroughPlayback Protocol v3 is finalized across server and web playback. The change adds neutral contracts, delivery-based capability negotiation, server-owned plans and attempt keys, durable replanning, subtitle inventories, audio-only playback, software-decoding support, generated fixtures, migrations, and CI verification. ChangesPlayback Protocol v3
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WebPlayer
participant PlaybackAPI
participant PlaybackPlanner
participant PlanStore
participant StreamService
WebPlayer->>PlaybackAPI: POST /playback/start with capabilities and context
PlaybackAPI->>PlaybackPlanner: resolve delivery and create PlanV3
PlaybackPlanner->>PlanStore: persist plan and start response
PlaybackAPI-->>WebPlayer: return plan URL and playback metadata
WebPlayer->>PlaybackAPI: POST /playback/replan for quality, track, seek, or recovery
PlaybackAPI->>PlaybackPlanner: create replacement plan
PlaybackPlanner->>PlanStore: update durable start response
PlaybackAPI-->>WebPlayer: return replacement plan
WebPlayer->>StreamService: request server-provided stream URL
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code review — protocol v3 finalization (head
|
|
Addressed all 16 confirmed findings from the review at Resolution summary:
The finalized neutral additions are Validation:
PR remains draft as requested. |
… audio planning Server: - The deferred replan lease release is bounded by a 3s timeout so a saturated pool or DB outage cannot wedge a handler goroutine that holds the per-session store lock on an uncancellable context. - planAudioOnlyV3 honors the request bandwidth cap: an over-cap source skips the original_http direct route and converts to AAC with the same bandwidth_cap_applied warning and decision reason the video ladder uses. Unknown source bitrate never triggers the cap. - A copy-audio progressive plan rejected only by a per-delivery audio_decode_codecs subset retries as an AAC conversion instead of returning adaptation_unavailable, and the AAC recipe respects the delivery's max_channels. Web: - failure_recovery replans issued while another replan is in flight queue (superseding a pending seek reanchor) instead of being silently dropped with the fatal overlay already suppressed. - A terminal response to a fresh non-preserving start clears the previous plan and stops its session, so episode navigation cannot keep rendering the prior item under the new title. - A refused recovery replan for a transport-dead plan surfaces the error and re-arms the plan failure key, so transient recovery failures no longer strand an endless spinner; the audiobook player gets the same guard reset. - A track-less subtitle_translation_completed hands off to the refreshed persisted track once the inventory settles, clearing the live overlay, instead of pinning the synthetic live track forever. Part of #135. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review-bot comment disposition (41 items)All 39 inline comments plus the 2 outside-diff review-body items were verified against the PR head. Summary: 31 already addressed — the five commits 9 valid → fixed in
1 refuted — the 1 deferred — routing client-side sidecar subtitle swaps through Verification for 🤖 Triage and verification by Claude (Fable 5); fixes drafted by OpenAI Codex (gpt-5.6-sol), reviewed and verified by Claude before commit. Generated with Claude Code |
![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: 63782d7513
ℹ️ 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.
💡 Codex Review
silo-server/web/src/player/hooks/usePlaybackSession.ts
Lines 556 to 560 in c32dca1
Clear the previous session after replacement start failures
When requestKey changes while a session is active and the new start throws—such as during a network outage or HTTP 5xx—allowPreserveExistingSessionOnError is false, but this catch path only sets an error and leaves the previous plan, stream URL, and session refs active. The page can therefore continue playing the previous item under the new item's metadata, and the newly assigned playback-attempt ID is now paired with the old plan, breaking subsequent recovery requests; stop and clear the previous session here as the terminal-decision path does.
AGENTS.md reference: AGENTS.md:L12-L14
ℹ️ 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.
💡 Codex Review
silo-server/web/src/player/hooks/usePlaybackSession.ts
Lines 557 to 561 in ce2f3e6
Clear the old session after a failed replacement start
The terminal-decision path now clears the previous session, but when requestKey changes and the new start throws because of a network failure, 5xx, or malformed response, this catch only updates the error fields. Since allowPreserveExistingSessionOnError is false for that path, the old plan, streamUrl, and sessionId remain active while the page renders the newly selected item's metadata, and playbackAttemptIdRef already identifies the failed new attempt. Apply the same stop-and-clear behavior used for a non-adopted terminal response.
AGENTS.md reference: AGENTS.md:L12-L14
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Post-review closeout — 0bf85b9All 48 inline review threads now have dispositions and are resolved. This follow-up:
Two review dispositions did not require source changes:
Validation:
The body-only review items for reconstruction durability and failed replacement starts are included in 0bf85b9. The two migration nitpicks are covered by the disposition above. |
|
@coderabbitai review |
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Follow-up on the exact head 79e3e76:
|
1 participant
Problem
Part of #135
Fixes #574
Playback protocol v3 still exposed client-engine and platform details, let clients derive attempt identities, and split intent changes across legacy routes. That made cross-platform behavior drift-prone and left failure recovery, subtitle selection, terminal reporting, and hardware-transcode startup inconsistent in real Shield playback.
Approach
Coordinated client PRs:
Risks and follow-up
Testing
go test ./...on dev-builder: all Go packages passed for the final Go tree.make test-webon dev-builder: 267 files / 1,831 tests passed. The final source-only sync initially omitted the gitignored vendored Foliate package; after restoring it, the four affected files passed 67/67 and the other 263 files had passed 1,764/1,764.pnpm exec vitest run src/player/hooks/usePlaybackSession.test.ts: 19/19 passed at final head.pnpm run build: passed at final head.golangci-lint run --new-from-merge-base=origin/main ./...: 0 issues.pnpm run lint: 0 errors (157 unchanged baseline warnings).pnpm run format:check: passed.make verify-playback-fixtures,make verify-local-paths, andgit diff --check: passed.AI Disclosure
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation