fix(scanner): accept corroborated long video durations - #545
Conversation
|
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:
📝 WalkthroughWalkthroughThe scanner accepts corroborated video durations up to 1,000,000 seconds. Playback limits synthetic HLS manifests to 50,000 segments and uses bounded native manifests for longer media. ChangesDuration validation
Synthetic manifest bounds
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Scanner
participant durationFromProbeMetadata
participant PacketDuration
participant ProbeRepair
Scanner->>durationFromProbeMetadata: validate format and primary stream durations
durationFromProbeMetadata-->>Scanner: return corroborated extended duration
Scanner->>ProbeRepair: check accepted long-video state
ProbeRepair-->>Scanner: return no repair required
Scanner->>PacketDuration: evaluate when metadata is not valid
PacketDuration-->>Scanner: return bounded packet-derived duration
sequenceDiagram
participant CompatibilityHandler
participant ensureTranscodeManifest
participant CanGenerateSyntheticManifest
participant FFmpeg
CompatibilityHandler->>ensureTranscodeManifest: request transcode manifest
ensureTranscodeManifest->>CanGenerateSyntheticManifest: check duration and segment count
CanGenerateSyntheticManifest-->>ensureTranscodeManifest: allow or reject synthetic generation
ensureTranscodeManifest->>FFmpeg: poll native manifest when rejected
FFmpeg-->>CompatibilityHandler: return bounded manifest
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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: 11a83159be
ℹ️ 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
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b4ef4747f
ℹ️ 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
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ceeb2dd6
ℹ️ 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".
| if strings.EqualFold(opts.TargetCodecVideo, "copy") || | ||
| !CanGenerateSyntheticManifest(opts.TotalDuration, opts.SegmentDuration) { |
There was a problem hiding this comment.
For encoded videos above the synthetic-manifest limit, this branch now serves FFmpeg's real sliding playlist instead of the full VOD playlist, but the v3 setup still marks every known-duration encoded transport as seek-anywhere (configureHLSTimelineV3 sets CanSeekAnywhere = durationSeconds > 0). In that context, clients are told to seek locally into segments that the live playlist does not list or produce yet, instead of reanchoring through the server as the copy path does, so long transcodes can stall during seeks or reconnects.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| if longVideoDurationsAgree(normalizedFormatDuration, normalizedStreamDuration) && | ||
| !durationLooksImplausible(raw, normalizedFormatDuration) { | ||
| return normalizedFormatDuration, true |
There was a problem hiding this comment.
Only absolute-end timestamp shapes should take this normalized return path. If ffprobe reports ordinary duration fields with a non-zero start_time (for example, both format and video stream have start_time=30 and duration=182930), the normalized values agree and are returned before the raw corroborated durations below, so the persisted runtime is shortened by 30 seconds even though the duration fields already matched.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2aede2d8c
ℹ️ 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".
| if strings.EqualFold(opts.TargetCodecVideo, "copy") || opts.TotalDuration <= 0 { | ||
| // Copy-video or unknown-duration sessions must use FFmpeg's real manifest. | ||
| if strings.EqualFold(opts.TargetCodecVideo, "copy") || | ||
| !CanGenerateSyntheticManifest(opts.TotalDuration, opts.SegmentDuration) { |
There was a problem hiding this comment.
When an encoded legacy transcode has a known duration above the synthetic-manifest limit, this new branch serves FFmpeg's bounded/growing playlist instead of the full VOD manifest, but the legacy start response still sets CanSeekAnywhere=true for every non-copy known-duration session (canSeekAnywhere in internal/api/handlers/playback.go). Those clients are told arbitrary local seeks are valid even though the manifest exposes only the produced window, so long legacy transcodes can stall on seek or reconnect instead of reanchoring through the server; gate the legacy flag on CanGenerateSyntheticManifest as well.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e47fbcf67d
ℹ️ 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".
| return !strings.EqualFold(req.TargetCodecVideo, "copy") && | ||
| playback.CanGenerateSyntheticManifest(float64(file.Duration), req.SegmentDuration) |
There was a problem hiding this comment.
When this new manifest-size check makes a non-copy legacy transcode use FFmpeg's real playlist, nonzero starts are still reported with streamOriginSeconds == 0 because the callers only set an origin for video-copy sessions. For a long encoded start at 18.261s with 2s segments, FFmpeg is launched at the aligned 16s/segment-8 window, but the response advertises player_start_seconds=18.261 and timeline_offset_seconds=0, so clients can seek inside a playlist that only exposes the new window and stall instead of starting at 18.261-16. Use the aligned transport seek as the origin for oversized encoded sessions too.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| startsLookLikeAbsoluteOffsets := durationStartOffsetIsMaterial(formatDuration, formatStart) && | ||
| durationStartOffsetIsMaterial(streamDuration, streamStart) | ||
| if normalizedDurationsAgree && (!rawDurationsAgree || startsLookLikeAbsoluteOffsets) && | ||
| !durationLooksImplausible(raw, normalizedFormatDuration) { | ||
| return normalizedFormatDuration, true |
There was a problem hiding this comment.
When both ffprobe duration fields already agree but an ordinary nonzero start_time is just over the 0.1% threshold (for example a 182,930s file with matching format/stream durations and a 300s start), startsLookLikeAbsoluteOffsets becomes true and this branch persists duration-start instead of the agreed duration. That shortens valid long videos by the start offset; a material start offset alone is not proof that the duration field is an absolute end timestamp, so the normalized value needs a stronger absolute-end signal before it wins.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 489fc18514
ℹ️ 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".
| // window. Out-of-window seeks should restart explicitly instead of relying | ||
| // on segment 404s to move FFmpeg. | ||
| return strings.EqualFold(req.TargetCodecVideo, "copy") || | ||
| !playback.CanGenerateSyntheticManifest(durationSeconds, req.SegmentDuration) |
There was a problem hiding this comment.
When this predicate makes a long encoded session use FFmpeg's real sliding playlist (for example a 1,000,000s file with 2s segments), the audio-switch restart path still only applies windowed timeline handling for copy-video sessions: it leaves restartStreamOriginSeconds at 0, passes that into the restarted opts/node request, and omits can_seek_anywhere/timeline fields unless legacyCopyRestart is true. The web clients default omitted can_seek_anywhere to true, so changing audio on these long encoded sessions can tell the player to seek locally inside a bounded/growing playlist instead of reanchoring, causing stalls or a reload at the wrong origin; the same real-manifest origin/response logic needs to cover audio-switch restarts too.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d32c03bcb
ℹ️ 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".
| ) | ||
| rawDurationsAgree := longVideoDurationsAgree(formatDuration, streamDuration) | ||
| normalizedDurationsAgree := longVideoDurationsAgree(normalizedFormatDuration, normalizedStreamDuration) | ||
| if normalizedDurationsAgree && !rawDurationsAgree && |
There was a problem hiding this comment.
When ffprobe reports both the format and primary video duration as the same absolute end timestamp, with the same nonzero start time, this guard skips the normalized span because rawDurationsAgree is also true and then falls through to returning the raw end timestamp below. Fresh evidence in this revision is that the normalized branch is gated on !rawDurationsAgree, so the previously discussed start_time=180000 / duration=350000 shape still persists 350000 seconds instead of the 170000-second runtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 058f9d814d
ℹ️ 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".
| // its own synthetic VOD manifest. Longer media waits for FFmpeg's bounded | ||
| // real playlist so one request cannot allocate hundreds of thousands of | ||
| // segment entries. | ||
| if shouldGenerateCompatFullManifest(source, h.compatSegmentDuration()) { |
There was a problem hiding this comment.
When a Jellyfin-compatible transcode has InitialSeekSeconds > 0 and the duration exceeds the synthetic-manifest bound, this predicate now falls through to returning FFmpeg's real playlist. That playlist is produced from SeekSeconds/StartSegmentNumber, so it starts at seg_K instead of preserving the full source timeline that generateFullManifest uses for resume; clients that also apply their resume seek can seek K seconds into an already K-shifted playlist and land near 2K or stall. Keep compat long manifests source-time aligned, or reset/advertise the offset explicitly for these clients.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| usesGrowingManifest := strings.EqualFold(videoCodec, "copy") || | ||
| !playback.CanGenerateSyntheticManifest(durationSeconds, segmentDuration) | ||
| if usesGrowingManifest { | ||
| plan.Timeline.PlayerStartSeconds = 0 |
There was a problem hiding this comment.
For oversized or unknown-duration encoded HLS starts that are not on a segment boundary, this growing-manifest branch now sets player_start_seconds to 0 after snapping FFmpeg's seek down to the prior segment. For example, a v3 start at 17.3s with 2s segments launches FFmpeg at 16s/segment 8 and advertises stream_origin_seconds=16, but player_start_seconds=0 tells clients to begin at the start of that real playlist rather than 1.3s into it, so playback resumes early; use requested - seek here like the legacy long-encoded response does.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
|
|
||
| best := packetSpan | ||
| if packetSpan > maxReasonableMediaDurationSeconds && | ||
| durationIsPositiveFinite(frameDuration) && |
There was a problem hiding this comment.
When the packet PTS span is a valid long duration but avg_frame_rate is malformed enough to produce an out-of-range frame-count estimate, this condition still treats that estimate as a contradiction. For example, after bad metadata forces packet scanning, a 182,930s packet span with 900 packets at 1/1000 fps makes frameDuration=900000, clears this finite check, zeroes best, and then fails the ordinary frame-duration cap, so repair persists no duration and repeats later; only sane bounded frame estimates should veto a packet-derived span.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e7bafec88
ℹ️ 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".
| if rawDurationsAgree && | ||
| !durationLooksImplausible(raw, formatDuration) { | ||
| return formatDuration, true |
There was a problem hiding this comment.
When both long raw durations agree and both starts satisfy matchingAbsoluteEnds, falling through here can still persist the raw end timestamp if the normalized spans miss the tighter post-subtraction tolerance. For example, start_time=180000 with format duration=350000.275 and stream duration=350300.196 passes the raw 0.1% agreement check and the dominant-start test, but normalized spans differ by ~300s against a ~170s normalized tolerance, so this branch returns ~350000s even though the same predicates identified the values as absolute end timestamps; in that case the code should normalize using the appropriate tolerance or reject for packet repair rather than accepting the inflated raw value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb03344408
ℹ️ 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".
| if requested <= 0 || !shouldGenerateCompatFullManifest(source, segmentDuration) { | ||
| return 0, 0 |
There was a problem hiding this comment.
For Jellyfin-compatible transcodes with InitialSeekSeconds > 0 whose duration is unknown or above the synthetic-manifest bound, this branch now resets FFmpeg to source time 0 and ensureTranscodeManifest serves FFmpeg's short real playlist instead of the synthetic playlist. Fresh evidence is that the current helper returns zero for every non-synthetic source, while the documented resume-safe path in generateFullManifest depends on listing the full source timeline and emitting #EXT-X-START; without that, clients that apply their negotiated resume position are seeking outside the tiny live window or simply start from the beginning.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
1 participant
Summary
Root cause
The scanner rejected every video duration above 100,000 seconds. That sent valid long videos into a full packet scan, but the packet estimator applied the same ceiling and discarded its result. The persisted duration therefore remained unknown and later full scans repeated the complete-file read.
This change does not blindly raise the metadata ceiling. Extended-range metadata is accepted only when the container and first real video stream agree within the larger of one second or 0.1%, and both remain below the bounded 1,000,000-second limit.
Impact and risk
Valid multi-day videos with corroborated metadata avoid the expensive packet fallback and persist a duration that later scan and request-time repair checks accept. Lone, inconsistent, non-finite, and over-limit timestamps still take the existing repair path.
The main risk is trusting two correlated metadata fields. The agreement check, primary-stream restriction, and hard ceiling limit that exposure; an authoritative packet scan can still recover a long duration when metadata is not trustworthy.
Validation
The local
make testsweep passed the changed scanner package but did not finish green on macOS: two unrelated Jellycompat process-identity tests fail because the local process start token is empty. There are no Jellycompat changes in this branch; GitHub Actions will run the full Go suite on Linux.Fixes #544
AI Disclosure
Summary by CodeRabbit
Bug Fixes
Tests