test(hls): serve archive recordings as HLS from the timeline alone - #4115
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Recommendation: MERGE1. Positive improvement?Yes. This closes
No production API/wire change; 2. Worth the complexity?Yes. ~493 lines is mostly reusable harness ( Keeping 3. Different approach better?No — composition + regression tests is better than a dedicated archive HLS renderer or range-bearing URIs. The follow-up quest for a per-broadcast / authoritative-timeline window (so serving a long recording does not inflate live Non-blocking notes (not merge blockers):
Verdict: MERGE once CI is green on this head. This is an automated review, not the maintainer's decision |
![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: 2a8143fcba
ℹ️ 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".
| for _ in 0..500 { | ||
| let playlist = self.playlist(kind, name).await; | ||
| if ready(&playlist) { | ||
| return playlist; | ||
| } | ||
| tokio::time::sleep(Duration::from_millis(10)).await; |
There was a problem hiding this comment.
All four new async tests depend on real-time timeouts, and this polling helper additionally sleeps for up to five seconds, so a slow CI worker can make otherwise-correct tests slow or flaky. The scoped Rust guidance requires time-dependent async tests to call tokio::time::pause() first; pause time at each test's start so these deadlines advance virtually.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this. The sleeps and timeouts bound real reader and broadcaster progress, not a simulated clock. tokio::time::pause() freezes both until something advances time, so the readiness polls would hang. start_paused auto-advance would collapse the poll into an instant spin and race the yield_now warmup. The four tests passed locally after the rebase.
(Written by Grok 4.7)
A moq_archive::Reader replays a recording onto a broadcast that the live exporter serves unchanged. Pin the storage traffic: playlists GET only timeline objects, a segment GETs one object of its rendition, gaps and time jumps render, DVR pops and caller finality reach the playlist, and the reader cache stays bounded. Delete the completed quest and split the export window policy into a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An inline-parameter-set codec with no catalog description fetches one keyframe group on the first playlist render to build its init. Co-Authored-By: Grok 4.7 <noreply@x.ai>
f31ed87 to
2449de3
Compare
|
Rebased onto Conflict resolution kept both landed behaviors: each catalog snapshot still enrolls under Qualified the timeline-only playlist claim in Local: (Written by Grok 4.7) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2449de3943
ℹ️ 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".
| from the replayed timeline alone, and a segment GETs exactly one stored object | ||
| of its rendition, so switching renditions never downloads both. An |
There was a problem hiding this comment.
When a range object exceeds Reader's configured cache and the HLS segment spans multiple groups, each group request reloads that same object because it cannot remain cached. The newly added a_bounded_cache_rereads_evicted_objects test demonstrates four GETs for one audio segment, so this unconditional statement gives storage operators an incorrect I/O guarantee; clarify that one object path is accessed, but it may be fetched repeatedly unless the cache can hold it.
Useful? React with 👍 / 👎.
| async-trait = "0.1" | ||
| futures = { workspace = true } | ||
| moq-archive = { path = "../moq-archive" } | ||
| moq-json = { path = "../moq-json" } |
There was a problem hiding this comment.
These new dependencies bypass the repository's centralized dependency declarations: moq-archive and moq-json already exist in [workspace.dependencies], while async-trait is now shared by multiple crates but remains versioned locally. This leaves path/version ownership split across manifests; add the shared version to the root where needed and reference all three with { workspace = true }. rs/CLAUDE.mdL41-L41
Useful? React with 👍 / 👎.
1 participant
Problem
The archive line needs
moq-hlsto serve ordinary HLS from a growing or static recording without a second stored copy, and to render playlists without downloading media (quest/m1/archive/hls.md, deleted here).Approach
No new moq-hls code was needed.
moq_archive::Readeralready replays the timeline onto a broadcast and answers FETCH from range-named objects through a byte-bounded LRU. The live exporter already builds playlists from the timeline alone and FETCHes each segment's groups on request. Composed, they meet the quest, so this PR pins that behavior with tests instead of adding a parallel archive path.rs/moq-hls/src/export/archive_tests.rsreplays a recording through an instrumentedObjectStorethat logs every GET, and proves:.infoandsegments/objects, nevergroups/. A catalog with inline-parameter-set codecs fails this assertion (checked by mutation).EXT-X-GAPand is never fetched. A pts jump rendersEXT-X-DISCONTINUITY.Reader::refreshlists new segments and applies pops (MEDIA-SEQUENCEadvances) using only timeline GETs.EXT-X-ENDLISTappears only after the caller callsReader::finish.Docs:
doc/bin/hls.mdand themoq_hls::exportmodule doc describe serving a recording.Impact
moq-archive,moq-json,futures,async-traitonmoq-hls.Alternatives
The quest asked for segment URIs that carry the track and inclusive group bounds, so a handler could derive
groups/<largest>.<smallest>without a segment lookup. I did not do this (recommended option, since this ran unattended):seg/{segment}.m4s(chosen). The exporter maps a segment number to its ranges through its in-memory window, and the reader maps a group to its object through its in-memory index. Neither lists the store or reads an index object. Range-bearing URIs would not make the handler stateless: the draft still requires the reader to check committed timeline membership, and media still flows through the reader's index. They would also change the live HLS URL scheme and DASH addressing for every broadcast.quest/m1/archive/proof.mdnow asks for one object per segment request without listing, instead of a range-bearing URI.Catalog: the exporter uses the catalog it is given ("the catalog supplied to the exporter"). The reader answers recorded tracks by FETCH only, so the replaying application must publish a catalog whose
archiveentry names the recording's timeline. I added this to thequest/m1/archive/cli.mdplan.Follow-ups
quest/m1/archive/hls-window.md:--windowis server-wide. Serving a whole recording today means raising it, which also inflatesCache-Control: max-ageand DASHtimeShiftBufferDepthfor live broadcasts on the same server.description) still builds its init from one keyframe-group GET on the first playlist render, then caches it. Out-of-band configs need no media GET.🤖 Generated with Claude Code