fix(playback): trust the server's media runtime end to end - #482
Merged
Conversation
The duration-plausibility rule only rejected videos of 10 seconds or less, so a feature film that probed as 61 seconds passed untouched and persisted. Clients then had nothing trustworthy to anchor on: Android's grow-only duration ratchet has no floor to hold when the catalog value is wrong, so the playback engine's growing-HLS-window duration won and a 90-minute movie displayed as ~1 minute. Size and duration together pin an implied bitrate, which separates the two cases the absolute floor conflates. A genuine short clip has an ordinary bitrate; a 100 GB file claiming 61 seconds implies ~13 Gbps. The ceiling sits far above any real medium, so legitimate content cannot trip it — and unlike the absolute floor, it does not false-positive on a genuine high-bitrate short. Also bump the repair-rule revision marker so rows judged by the previous, weaker rule are re-checked once under this one. Without that bump an improved rule never reaches the rows it was written for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… copy seek window Two defects with one root: a v3 plan described where playback sits without ever stating how long the media is. Add source.duration_seconds. It is the file's full runtime, never `total - source_start` and never adjusted by timeline_offset_seconds, and it is omitted rather than null when unknown — clients that coerce null to a numeric default would read it as zero, the exact value this field exists to stop them inventing. It is set in SourceDescriptorFromFileV3, the single place every delivery already flows through, so direct play, progressive remux, HLS remux and HLS transcode all carry it. Until now the v3 plan omitted duration entirely, so clients fell back to the playback engine. On an HLS copy remux the server intentionally serves FFmpeg's still-growing playlist, so the engine reports the length produced so far. With no server-supplied runtime to anchor on, a feature film played back as a couple of minutes. The legacy protocol already answered this correctly via fileDurationSeconds; this restores parity. Separately, the copy branch published seek_window_end_seconds as the media runtime. That made the window look *complete*, which clients read as proof that any target inside it is locally seekable, so they native-seek past the produced head of a growing playlist instead of asking for a reanchor. Leave the end open: an incomplete window plus can_seek_anywhere=false routes every seek through the server, which is what legacy did before v3 added the bound. Advertise plan_source_duration_v1 so a client can distinguish "this server does not populate the field" from "this server knows the runtime is genuinely unknown" — without it, both look like an absent field and a client cannot tell whether its own catalog fallback is still required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ent duration The player's exit state converts its position to media time but took the duration from the video element, which is player-local. On a remux or transcode stream the element only covers the window produced so far, so the two values live in different coordinate systems. Resuming a movie 50 minutes in makes that concrete: the exit position is ~3060s of media time while the element reports ~120s. The progress cache then evaluates `position >= duration`, marks the item completed, latches the watched badge, and — because completion clears the resume point — resets position to 0. Exiting a resumed movie destroyed the resume point and claimed it had been watched. The server's runtime is authoritative and already expressed in media time, so prefer it and fall back to the element only when no server value exists. The rule moves into mediaTimeline.ts next to the coordinate conversions it depends on, which is also what makes it testable — VideoPlayer itself has no test harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughPlayback v3 now exposes validated source runtimes, keeps copy-remux seek windows open-ended, and selects authoritative player exit durations. Probe duration validation adds bitrate-based plausibility checks and applies them to legacy repair decisions. ChangesPlayback duration semantics
Probe duration validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/scanner/probe_duration_test.go (1)
290-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a boundary case for the short/min-bytes threshold.
No case tests
duration == 10(i.e.,implausiblyShortVideoMaxSeconds) combined withsize == 100MiBexactly, to pin down the inclusive<=/>=boundary behavior of the new rule.✅ Suggested boundary case
{name: "unknown duration is not this rule's job", duration: 0, size: 100 * gib, want: false, hasVideo: true}, + {name: "exact short/min-bytes boundary", duration: 10, size: 100 * 1024 * 1024, want: true, hasVideo: true}, }🤖 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 `@internal/scanner/probe_duration_test.go` around lines 290 - 305, Add a boundary test case to the duration probe table in the relevant test function, using duration 10 seconds and size exactly 100MiB, and set its expected result to match the rule’s inclusive <= and >= thresholds. Keep the case as a video input and preserve the existing scenarios.
🤖 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.
Nitpick comments:
In `@internal/scanner/probe_duration_test.go`:
- Around line 290-305: Add a boundary test case to the duration probe table in
the relevant test function, using duration 10 seconds and size exactly 100MiB,
and set its expected result to match the rule’s inclusive <= and >= thresholds.
Keep the case as a video input and preserve the existing scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fea1df4-353d-4614-8557-2efa170290d9
📒 Files selected for processing (11)
internal/api/handlers/playback_v3.gointernal/api/handlers/playback_v3_test.gointernal/playback/capabilities_v3.gointernal/playback/protocol_v3.gointernal/playback/protocol_v3_test.gointernal/scanner/probe.gointernal/scanner/probe_duration_test.gointernal/scanner/probe_repair.goweb/src/player/components/VideoPlayer.tsxweb/src/player/utils/mediaTimeline.test.tsweb/src/player/utils/mediaTimeline.ts
![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: ebec4141a8
ℹ️ 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".
| // re-converge on the improved rule. Last bumped when the implied-bitrate | ||
| // ceiling was added, which catches durations the absolute floor missed — | ||
| // a feature film probing as 61 seconds passed the old rule untouched. | ||
| var legacyProbeDurationFixTime = time.Date(2026, time.July, 26, 0, 0, 0, 0, time.UTC) |
There was a problem hiding this comment.
Any server running the previous binary after 2026-07-26T00:00:00Z can persist a duration that passes the old rule but fails the new implied-bitrate rule; after upgrading, its ProbeUpdatedAt is not before this cutoff, so legacyDurationRepairNeeded returns false and the stable-file scanner never repairs it. Since this commit itself was created after the cutoff and deployments may happen much later or roll gradually, use a persisted probe-rule revision (or another deployment-safe marker) rather than the start of the authoring day.
Useful? React with 👍 / 👎.
| if durationSeconds <= implausiblyShortVideoMaxSeconds && sizeBytes >= implausiblyShortVideoMinBytes { | ||
| return true | ||
| } | ||
| return impliedBitrateBps(sizeBytes, durationSeconds) > implausibleVideoBitrateBps |
There was a problem hiding this comment.
For legitimate sources whose aggregate bitrate exceeds 1 Gbps, this rejects an otherwise correct ffprobe duration; an 8K/4320p ProRes 4444 XQ source can exceed this threshold even though the repository explicitly handles 4320p media. ProbeFile consequently falls back to enumerating every video packet in what can be a hundreds-of-gigabytes file, blocking normal scans and potentially exhausting the playback repair's one-minute timeout. Use a codec/resolution-aware bound or a narrower malformed-timestamp signature rather than treating every source above this fixed ceiling as impossible.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
This was referenced Jul 26, 2026
Pukabyte
pushed a commit
to Pukabyte/silo-server
that referenced
this pull request
Jul 26, 2026
…er#482) * fix(scanner): reject durations that imply an impossible bitrate The duration-plausibility rule only rejected videos of 10 seconds or less, so a feature film that probed as 61 seconds passed untouched and persisted. Clients then had nothing trustworthy to anchor on: Android's grow-only duration ratchet has no floor to hold when the catalog value is wrong, so the playback engine's growing-HLS-window duration won and a 90-minute movie displayed as ~1 minute. Size and duration together pin an implied bitrate, which separates the two cases the absolute floor conflates. A genuine short clip has an ordinary bitrate; a 100 GB file claiming 61 seconds implies ~13 Gbps. The ceiling sits far above any real medium, so legitimate content cannot trip it — and unlike the absolute floor, it does not false-positive on a genuine high-bitrate short. Also bump the repair-rule revision marker so rows judged by the previous, weaker rule are re-checked once under this one. Without that bump an improved rule never reaches the rows it was written for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(playback): publish source runtime in v3 plans and stop faking the copy seek window Two defects with one root: a v3 plan described where playback sits without ever stating how long the media is. Add source.duration_seconds. It is the file's full runtime, never `total - source_start` and never adjusted by timeline_offset_seconds, and it is omitted rather than null when unknown — clients that coerce null to a numeric default would read it as zero, the exact value this field exists to stop them inventing. It is set in SourceDescriptorFromFileV3, the single place every delivery already flows through, so direct play, progressive remux, HLS remux and HLS transcode all carry it. Until now the v3 plan omitted duration entirely, so clients fell back to the playback engine. On an HLS copy remux the server intentionally serves FFmpeg's still-growing playlist, so the engine reports the length produced so far. With no server-supplied runtime to anchor on, a feature film played back as a couple of minutes. The legacy protocol already answered this correctly via fileDurationSeconds; this restores parity. Separately, the copy branch published seek_window_end_seconds as the media runtime. That made the window look *complete*, which clients read as proof that any target inside it is locally seekable, so they native-seek past the produced head of a growing playlist instead of asking for a reanchor. Leave the end open: an incomplete window plus can_seek_anywhere=false routes every seek through the server, which is what legacy did before v3 added the bound. Advertise plan_source_duration_v1 so a client can distinguish "this server does not populate the field" from "this server knows the runtime is genuinely unknown" — without it, both look like an absent field and a client cannot tell whether its own catalog fallback is still required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(web): pair the exit position with the media runtime, not the element duration The player's exit state converts its position to media time but took the duration from the video element, which is player-local. On a remux or transcode stream the element only covers the window produced so far, so the two values live in different coordinate systems. Resuming a movie 50 minutes in makes that concrete: the exit position is ~3060s of media time while the element reports ~120s. The progress cache then evaluates `position >= duration`, marks the item completed, latches the watched badge, and — because completion clears the resume point — resets position to 0. Exiting a resumed movie destroyed the resume point and claimed it had been watched. The server's runtime is authoritative and already expressed in media time, so prefer it and fall back to the element only when no server value exists. The rule moves into mediaTimeline.ts next to the coordinate conversions it depends on, which is also what makes it testable — VideoPlayer itself has no test harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
cursor Bot
pushed a commit
to Prairie-Server/prairie-server
that referenced
this pull request
Jul 26, 2026
…er#482) * fix(scanner): reject durations that imply an impossible bitrate The duration-plausibility rule only rejected videos of 10 seconds or less, so a feature film that probed as 61 seconds passed untouched and persisted. Clients then had nothing trustworthy to anchor on: Android's grow-only duration ratchet has no floor to hold when the catalog value is wrong, so the playback engine's growing-HLS-window duration won and a 90-minute movie displayed as ~1 minute. Size and duration together pin an implied bitrate, which separates the two cases the absolute floor conflates. A genuine short clip has an ordinary bitrate; a 100 GB file claiming 61 seconds implies ~13 Gbps. The ceiling sits far above any real medium, so legitimate content cannot trip it — and unlike the absolute floor, it does not false-positive on a genuine high-bitrate short. Also bump the repair-rule revision marker so rows judged by the previous, weaker rule are re-checked once under this one. Without that bump an improved rule never reaches the rows it was written for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(playback): publish source runtime in v3 plans and stop faking the copy seek window Two defects with one root: a v3 plan described where playback sits without ever stating how long the media is. Add source.duration_seconds. It is the file's full runtime, never `total - source_start` and never adjusted by timeline_offset_seconds, and it is omitted rather than null when unknown — clients that coerce null to a numeric default would read it as zero, the exact value this field exists to stop them inventing. It is set in SourceDescriptorFromFileV3, the single place every delivery already flows through, so direct play, progressive remux, HLS remux and HLS transcode all carry it. Until now the v3 plan omitted duration entirely, so clients fell back to the playback engine. On an HLS copy remux the server intentionally serves FFmpeg's still-growing playlist, so the engine reports the length produced so far. With no server-supplied runtime to anchor on, a feature film played back as a couple of minutes. The legacy protocol already answered this correctly via fileDurationSeconds; this restores parity. Separately, the copy branch published seek_window_end_seconds as the media runtime. That made the window look *complete*, which clients read as proof that any target inside it is locally seekable, so they native-seek past the produced head of a growing playlist instead of asking for a reanchor. Leave the end open: an incomplete window plus can_seek_anywhere=false routes every seek through the server, which is what legacy did before v3 added the bound. Advertise plan_source_duration_v1 so a client can distinguish "this server does not populate the field" from "this server knows the runtime is genuinely unknown" — without it, both look like an absent field and a client cannot tell whether its own catalog fallback is still required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(web): pair the exit position with the media runtime, not the element duration The player's exit state converts its position to media time but took the duration from the video element, which is player-local. On a remux or transcode stream the element only covers the window produced so far, so the two values live in different coordinate systems. Resuming a movie 50 minutes in makes that concrete: the exit position is ~3060s of media time while the element reports ~120s. The progress cache then evaluates `position >= duration`, marks the item completed, latches the watched badge, and — because completion clears the resume point — resets position to 0. Exiting a resumed movie destroyed the resume point and claimed it had been watched. The server's runtime is authoritative and already expressed in media time, so prefer it and fall back to the element only when no server value exists. The rule moves into mediaTimeline.ts next to the coordinate conversions it depends on, which is also what makes it testable — VideoPlayer itself has no test harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An Android user reported a 90-minute movie playing back as
0:37 / 1:01. Tracing it surfaced one root cause and two independent defects along the same path.A wrong duration persists in the database. The scanner's plausibility rule only rejected video durations of 10 seconds or less (
probe.go), so a feature film that probed as 61 seconds passed as "reasonable" and was stored. The repair layer reuses the same predicate, so such a row was never re-probed.The v3 plan never carried a runtime.
TimelineV3describes where playback sits but never how long the media is, so clients fell back to the playback engine. On an HLS copy remux the server intentionally serves FFmpeg's still-growing playlist (BuildPlaybackManifest), so the engine reports the length produced so far. Android's grow-only ratchet is a correct defense, but with a wrong catalog value it had no floor to hold. The legacy protocol already answered this correctly viafileDurationSeconds— v3 regressed on it.Two further defects found while verifying, both independent of the display bug:
seek_window_end_secondsas the media runtime. That makes the window look complete, which clients read as proof that any target inside it is locally seekable — so they native-seek past the produced head of a growing playlist instead of requesting a reanchor. Legacy published no window here and reanchored correctly; v3 added the bound that defeats the guard.position >= durationmarked the item completed, latched the watched badge, and — because completion clears the resume point — reset position to 0. Exiting a resumed movie destroyed the resume point and claimed it had been watched.Approach
Fix the data first. Size and duration together pin an implied bitrate, which separates the two cases the absolute floor conflates: a genuine short clip has an ordinary bitrate, a 100 GB file claiming 61 seconds implies ~13 Gbps. The ceiling sits far above any real medium (UHD Blu-ray peaks near 150 Mbps), so legitimate content cannot trip it — and unlike the absolute floor it does not false-positive on a genuine high-bitrate short. The repair-rule revision marker is bumped so rows judged by the previous rule re-converge; without that an improved rule never reaches the rows it was written for.
Then make the contract self-sufficient.
source.duration_secondsis added to the v3 plan — a fact about the media file, not a claim about the transport. It lands onSourceDescriptorV3rather thanTimelineV3for two reasons:SourceDescriptorFromFileV3is the single site every delivery already flows through (timeline finalization is scattered across four sites, andoriginal_httphas none — which is exactly whyseek_window_end_secondsis populated on 1 of 4 deliveries today), and the descriptor carriesmedia_file_id, so the value is self-labelling when the server resolves a different effective file.It is omitted, not null, when unknown. Android's
SiloJsonsetscoerceInputValues, so an explicit null against a non-nullable field becomes0.0— the exact value this field exists to stop clients inventing.Considered and rejected: a
manifest_completenessenum. It is time-varying data in an immutable document — FFmpeg writesEXT-X-ENDLISTon exit, anddecisionResponseFromAttemptV3replays the stored plan verbatim on idempotent retry, so a frozen"growing"becomes a lie. It is also derivable fromdelivery+can_seek_anywhere. Once the runtime is authoritative the client never trusts the engine's duration, so it never needs to branch.Also deliberately not done: making duration genuinely "unknown" client-side. All three clients collapse unknown to
0, which would kill D-pad scrubbing on Android TV, shrink the phone seek bar to a 1-second slider, and make a web seek-bar click jump to the start. Clients keep treating 0 as unknown internally.Scope
Part of #431.
Server-side only. The client halves are separate PRs in
silo-androidandsilo-apple(branchfix/playback-source-durationin both), each consumingsource.duration_seconds. This PR is independently correct and safe to merge first — it is purely additive on the wire, and older clients ignore the new field.Risks
seek_window_end_secondsbecomesnilfor copy remuxes. Any client using it as a duration source would lose that value — none do; Android consumes it only indecideSeek. This is why the runtime field ships in the same change.Verification
All three reproduce at
10394b0a(main) at identical line numbers and are untouched by this diff. Zero findings attributable to these changes.The 6 web failures are in
SeasonContent.test.tsxandServerStorageStep.test.tsxand reproduce identically with this branch stashed. Unrelated to the player.Failures were checked against a baseline captured before these changes; no new failures. All three are pre-existing:
TestHandleReplanPlaybackV3SeekFailureRecoveryNeverChangesMediaVersionmain. Consistent, pre-existing.TestServeDirectPlayChangedEntityRejectsOldIfRangemain(1 failure in 4 runs). Rewrites a same-size file and relies on coarse-grained Linux ctime changing between two writes in the same tick.TestRemoveJellyfinCompatWebDisablesWebSettingNew tests added: 3 scanner cases (impossible implied bitrate, genuine short clip preserved, table-driven predicate), 1 updated HLS timeline assertion, 6 web
mediaTimelinecases.AI Disclosure
TimelineV3toSourceDescriptorV3— the timeline's fields are all positions in named clocks, so an unqualified duration there has no defined clock, and timeline finalization is scattered across four sites while the descriptor has one. (2) The proposedmanifest_completenessenum was dropped as time-varying data frozen into a replayed immutable document, and redundant withdelivery+can_seek_anywhere. (3) The proposed client rule "represent unknown duration" was withdrawn after review showed all three clients collapse unknown to0, which would break Android TV scrubbing and make a web seek-bar click jump to the start — worse than the bug being fixed. (4) Review surfaced the copy-mode seek-window bug and the web resume-point bug, neither of which was in the original report; both are fixed here. One review claim was checked and corrected: it cited the legacycanSeekAnywhereatplayback.go:529for a v3 conclusion; the v3 equivalent isplayback_v3.go:2021, same conclusion. A single test failure that appeared once in final verification was traced rather than dismissed — the predicate returnsfalsefor that fixture's exact values (2,341 bps against a 1,000,000,000 bps ceiling), proving the change is behaviourally identical there.Follow-ups (not in this PR)
duration: 0withforce_overwrite: trueon stop, and the server unconditionally clobbers a good stored duration with it (progress.go:97→pgstore/progress.go:127). Live watch-state corruption path, independent of this work.userstore/threshold.go).timing_origin_secondsis computed and sent by the server but read by no client; on a re-anchored copy stream every sidecar subtitle cue is offset by the anchor.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes