Repository navigation
Conversation
External subtitles injected as WebVTT renditions on a remote HLS source (superuser404notfound#316) appeared early when the origin's media timestamps do not start at the playlist's time 0. An ffmpeg MPEG-TS transcode cut with -copyts -max_delay 5000000 puts the video 10 s after source time, and every cue showed 10 s early; direct play was in sync. The served .vtt was whole-program cues in source time with no X-TIMESTAMP-MAP, so AVPlayer pinned cue time 0 to MPEG-TS timestamp 0 while it placed the A/V on the playlist timeline from the segments' own timestamps. After building the rewritten master, the proxy now reads the head of the segment the load opens on (the one AVPlayer fetches first, so an on-demand transcoder is not sent to another position): the first video PES PTS for MPEG-TS, tfdt over the track's mdhd timescale for fMP4. The anchor is that timestamp minus the segment's playlist start, and the renditions are served with X-TIMESTAMP-MAP=MPEGTS:<anchor>,LOCAL:00:00:00.000 and unchanged cue times. The probe runs in the background under one deadline that covers all of its reads (the segment head and, for fMP4, the init segment), through the authorizing relay when the load has one, and the .vtt handler waits for it inside its existing wait. An anchor near zero, a byte-range or encrypted playlist, an unresolvable EXT-X-DEFINE reference, or a failed probe keeps the plain WebVTT. Tests cover a +10 s MPEG-TS origin (MPEGTS:900000 with unchanged cues), a zero-offset origin and a failed probe (plain body), start-segment selection with EXT-X-DEFINE substitution, fMP4 tfdt parsing, the negative-anchor map, the authorized probe's headers and range, and an fMP4 probe whose segment arrives late and whose init segment stalls returning within the budget.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
![silo-kody[bot]](https://avatars.githubusercontent.com/u/287058192?s=60&v=4){kind=small}
| } else if size == 0 { | ||
| size = range.upperBound - offset | ||
| } | ||
| guard size >= header, offset + size <= range.upperBound else { break } |
There was a problem hiding this comment.
Integer overflow in offset + size traps when an origin supplies an extended-size MP4 box with size Int.max after any complete preceding box, despite passing the large <= UInt64(Int.max) check. Reject sizes larger than range.upperBound - offset before addition to prevent a malformed segment or init response from terminating the player process.
guard size >= header, size <= range.upperBound - offset else { break }Prompt for LLM
File Sources/AetherEngine/Native/RemoteHLSTimestampAnchor.swift:
Line 328:
Integer overflow in `offset + size` traps when an origin supplies an extended-size MP4 box with size `Int.max` after any complete preceding box, despite passing the `large <= UInt64(Int.max)` check. Reject sizes larger than `range.upperBound - offset` before addition to prevent a malformed segment or init response from terminating the player process.
Suggested Code:
guard size >= header, size <= range.upperBound - offset else { break }
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Fixed in d57205a: the box walk now checks size <= range.upperBound - offset instead of adding, so an extended size of Int.max ends the walk and the rendition keeps the plain WebVTT. Covered by oversizedExtendedBoxEndsTheWalk (it trapped before the fix).
There was a problem hiding this comment.
Thanks—this addresses the reported overflow. The size <= range.upperBound - offset check rejects an oversized box before evaluating offset + size, preventing the trap.
The reported oversizedExtendedBoxEndsTheWalk regression test covers the original failure case. With the fix in d57205a6, this finding is addressed; no further changes are needed for this suggestion.
| let seconds = Double(decodeTime) / Double(track.timescale) | ||
| if track.isVideo { return seconds } |
There was a problem hiding this comment.
Timestamp misalignment occurs because the fMP4 anchor uses tfdt decode time as presentation time without applying the first video sample's trun composition offset, shifting B-frame fragments' timestamp maps by their presentation-minus-decode offset and desynchronizing every injected cue. Derive the presentation timestamp before subtracting the playlist start, consistently with the MPEG-TS PTS path.
Prompt for LLM
File Sources/AetherEngine/Native/RemoteHLSTimestampAnchor.swift:
Line 305 to 306:
Timestamp misalignment occurs because the fMP4 anchor uses `tfdt` decode time as presentation time without applying the first video sample's `trun` composition offset, shifting B-frame fragments' timestamp maps by their presentation-minus-decode offset and desynchronizing every injected cue. Derive the presentation timestamp before subtracting the playlist start, consistently with the MPEG-TS PTS path.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Not changing. The error is bounded by the encoder's reorder delay, which sits under the 0.1 s threshold this anchor already uses to absorb it. An ffmpeg fMP4 HLS encode with -bf 3 at 24 fps gives a first-sample trun composition offset of 1024/12288 (83 ms) on every fragment, so the map is at most 83 ms off. That is not visible on a subtitle. Adding only the trun offset would not be correct either: presentation time also depends on the edit list in the init segment (that same file has an 83 ms empty edit and media_time 1024), and on trun version 1 signed offsets. Getting it exactly right means parsing more untrusted structure for a correction below the threshold.
There was a problem hiding this comment.
The high-severity recommendation was too strong for the evidence here. Your measured 83 ms offset supports treating this as an acceptable approximation for that ffmpeg output, rather than requiring additional parsing for subtitle synchronization.
You’re also right that adding the first trun composition offset alone is not a complete presentation-timeline correction: edit-list mapping and signed version-1 offsets need consideration.
One distinction matters: negligibleSeconds suppresses anchors near zero; it does not bound the error of a nonzero anchor. Likewise, the measured 83 ms reorder delay is not a universal bound for all fMP4 encoders. The defensible conclusion is that this approximation is acceptable for the tested output—not that every possible origin stays below 100 ms.
Given that tradeoff, I would not treat this finding as a high-severity blocker or insist on the proposed change. No code or issue status has been changed.
There was a problem hiding this comment.
Agreed on both corrections: negligibleSeconds only drops anchors near zero, and 83 ms is what one encoder produced, not a bound for every origin. The error does stay within the first frame's reorder delay, a few frames for common B-frame settings. Leaving the tfdt anchor as is.
There was a problem hiding this comment.
Agreed. Leaving the tfdt anchor unchanged is a reasonable tradeoff for this fix: it accepts a reorder-delay-scale approximation rather than adding presentation-timeline parsing for subtitle precision.
I would not treat the original suggestion as a high-severity blocker. No code or issue status has been changed.
| guard anchorSeconds >= 0 else { | ||
| return "X-TIMESTAMP-MAP=MPEGTS:0,LOCAL:\(timestamp(-anchorSeconds))" | ||
| } | ||
| let ticks = Int64((anchorSeconds * 90_000).rounded()) & ((Int64(1) << 33) - 1) |
There was a problem hiding this comment.
Integer conversion overflow in timestampMap crashes the process because it converts origin-controlled anchorSeconds * 90,000 to Int64 before wrapping to 33 bits; a version-1 tfdt of UInt64.max with an mdhd timescale of 1 passes the probe's parsing checks but traps during subtitle response construction. Reject nonfinite values and reduce finite tick values modulo 2^33 before converting to Int64.
Prompt for LLM
File Sources/AetherEngine/Subtitles/WebVTTBuilder.swift:
Line 60:
Integer conversion overflow in `timestampMap` crashes the process because it converts origin-controlled `anchorSeconds * 90,000` to `Int64` before wrapping to 33 bits; a version-1 `tfdt` of `UInt64.max` with an `mdhd` timescale of 1 passes the probe's parsing checks but traps during subtitle response construction. Reject nonfinite values and reduce finite tick values modulo 2^33 before converting to `Int64`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Fixed in d57205a: timestampMap wraps the tick count to 33 bits while it is still a Double, so a tfdt of UInt64.max over timescale 1 yields a valid map. A non-finite anchor is dropped in anchorSeconds (plain body), and timestampMap answers one with the identity map. Covered by hugeAnchorWrapsWithoutTrapping.
There was a problem hiding this comment.
Thanks—based on your description, d57205a6 addresses the reported crash:
- Wrapping the tick count to 33 bits before converting to
Int64avoids the overflow fortfdt = UInt64.maxwith timescale 1. - Dropping non-finite anchors preserves the plain-WebVTT fallback, while the identity map protects direct calls to
timestampMap. hugeAnchorWrapsWithoutTrappingcovers the reported extreme-value case.
That resolves the original finding as described; I haven’t independently inspected the updated diff.
Two inputs from a broken or malicious HLS origin could crash the player while the subtitle timestamp anchor was read or written. The MP4 box walk added a 64-bit extended box size to the current offset. A size of Int.max after any complete box passed the Int.max check and then overflowed on the addition. The walk now compares the size with the bytes left instead, so such a box ends the walk and the rendition keeps the plain WebVTT body. X-TIMESTAMP-MAP converted the anchor's 90 kHz tick count to Int64 before wrapping it to 33 bits. A version-1 tfdt of UInt64.max over a timescale of 1 is finite but far past Int64, and the conversion trapped. The ticks are now wrapped while still a Double. An anchor that is not finite (a playlist whose EXTINFs sum to NaN) is dropped by the probe, and timestampMap answers one with the identity map.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
1 participant
Summary
External subtitles that the engine injects as WebVTT renditions into a remote HLS master (superuser404notfound#316) showed every cue 10 s early on Silo's MPEG-TS transcodes; direct play was in sync. The served
.vtthad noX-TIMESTAMP-MAP, so AVPlayer tied cue time 0 to MPEG-TS timestamp 0, while ffmpeg's-copyts -max_delay 5000000output starts at source time plus 10 s. The proxy now reads the origin's starting timestamp and serves the renditions with a matching map.No issue filed; found during Silo's iOS Series validation (Silo-Server/silo-apple#337, case C10).
What changed
tfdtover the track'smdhdtimescale for fMP4.X-TIMESTAMP-MAP=MPEGTS:<anchor>,LOCAL:00:00:00.000and unchanged cue times; a negative anchor moves to theLOCALside..vtthandler waits for it inside its existing wait.EXT-X-DEFINEreference, or a failed probe keeps the plain WebVTT body, as before.CHANGELOG.md,docs/architecture.mdanddocs/formats.mddescribe the behaviour.Test plan
-copyts). The subtitle comes from the server as WebVTT and is injected by the proxy.MPEGTS:900000with unchanged cues), a zero-offset origin and a failed probe (plain body), start-segment selection withEXT-X-DEFINEsubstitution, fMP4tfdtparsing, the negative-anchor map, the authorized probe's headers and range, and an fMP4 probe whose segment arrives late and whose init segment stalls, returning within the budget.Checklist
CHANGELOG.mdupdatedfeat(...),fix(...),chore(...))Notes
A draft silo-apple PR moves the app's engine pin to this commit once it merges: Silo-Server/silo-apple#642.
AI Disclosure
Note
Anchor injected HLS subtitle renditions to origin media timestamps
X-TIMESTAMP-MAPheader via WebVTTBuilder.swift when the offset is non-negligible. Zero offsets, probe failures, and timeouts fall back to the plain body.HLSOriginRelaygains a rangedfetchHead, and starting subtitle decoding no longer cancels a running probe.RemoteHLSSubtitleProvider.startFillno longer tears down the whole provider on decode restart — check thatcancelFillstill covers probe and decode cleanup; probes over relays use authorizer requests with a byte range, verified in RefreshableHLSAuthorizationTests.swift.Macroscope summarized d57205a.