Repository navigation
quest: plan one timeline for every source on a catalog - #4669
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed SHA: af9e7d1c809943f34a17a13ba8cf2d77a4c12847
No actionable bugs found in this documentation-only patch. The stale-clock premise matches the capture paths and the dev catalog anchor. Reading the current clock at stamping time is a focused, simpler change than turning the public Copy clock into a shared handle; keeping both captures in one quest makes sense.
Non-blocking implementation suggestion for quest/m2/capture-clock.md:31–38: explicitly test a backward anchor near PTS 0 after the camera has been running. Video's current unsigned capture_epoch = clock.now().saturating_sub(camera.now()) cannot represent the resulting negative offset, so recalculating that same expression alone would leave video ahead. A paired capture/broadcast anchor (or signed offset) should preserve acquisition time across that case.
Verification: static review of both changed files, the related capture/clock code, and #4668. No tests or quest check run; the quest tool is unavailable here.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change adds a Capture clock quest to the M2 quest list. The quest describes a stale clock snapshot scenario and documents proposed handling and test guidance. It states that the proposed change has no public API or wire impact. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The PR changes documentation only. Its Required quest misstates how importer anchoring works, which could misdirect future implementation; correct or withdraw the quest before relying on it. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1✨ Simplify code
🛠️ Fix failing CI checks 💡
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 |
|
Quest: plan moq-audio / moq-video captures to re-read CI: Test green; Check red from pre-existing Findings: No concrete issues found. Claims match the tree: Non-blocking notes:
MERGE on This is an automated review, not the maintainer's decision |
Replaces re-reading the clock per frame: a backward anchor hits TimestampRewind, and audio would lock the catalog on every buffer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed SHA: b22db1e457bb58e4a3f42b6a3f54030b89e42162
The fixed-clock/per-importer-offset direction avoids the backward-anchor capture problem raised in my previous review. Two gaps remain in the planned semantics:
-
[P2]
quest/m2/shared-clock.md:21–23: freeze the mapping on data writes too.Listingis not merely an observer:Listing::stampstamps every JSON/binary payload. Exempting its reads lets telemetry publish near 10 s, then an importer starting at PTS 0 moves the mapping and subsequent telemetry backwards. Previously published data also acquires the wrong wall mapping. Keep construction/inspection non-fixing, but make actual stamping fix the clock; cover data-write → importer → data-write. -
[P2]
quest/m2/shared-clock.md:42–43: preserve an explicit-clock recording-import path. The existingConfig::with_clockcontract (rs/moq-mux/src/catalog/producer.rs:247–253) deliberately lets source PTS zero name the recording's real start. Applyingnow - first_ptschanges that: with a clock currently reading 10 s, a recorded frame at 3600 s becomes 10 s and gets the wrong historical wall time. Distinguish an explicit source-aligned clock from a live clock fixed by another producer, or make rebasing opt-in; test preservation of recorded wall time.
Verification: static review of the documentation delta, relevant dev clock/capture/import/data paths, and merged #4668. No tests or quest check run.
|
@Dryvnt I reworked the plan here, can you check whether it still matches what you were after? Why not re-read per frame: fMP4 and FLV sources start near PTS 0, so the importer's anchor usually steps the clock backwards from the default (10 s+). A capture following it would hit New plan (quest/m2/shared-clock.md): the clock never moves once something stamps with it. What the offset touches: only the moq-lite frame timestamp. The media payload is untouched. For legacy/LOC tracks the payload carries no timestamp, so nothing disagrees. fMP4 passthrough is the catch: the
I lean toward 1, with the quest stating that rule and checking that the fMP4 export and the MSE path in (Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@Dryvnt Decided: option 1, accept the mismatch. The moq-lite frame timestamp is the broadcast timeline, and a payload's own timestamps are only relative within the frame. Ad insertion (splicing sources with unrelated PTS bases) needs the same rule anyway. That has a concrete cost, now in the quest: both CMAF decoders stamp samples from (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed SHA: b871ba7933f3ab4a0830ff473011515c4c2ad410
The new CMAF rebasing direction is reasonable, but its reference point needs one correction:
- [P2]
quest/m2/shared-clock.md:53–54: rebase from the fragment's minimum presentation timestamp, rather than its first decoded sample. The passthrough importer puts the earliest PTS in the outer frame timestamp, while both decoders return samples in decode order. With reordered video PTS[120, 80, 100] msand outer timestamp80 ms, subtracting the first sample produces[80, 40, 60] mseven with zero importer offset. That shifts video relative to audio and can create a rewind across fragments. Specifyouter_timestamp + sample_pts - min_fragment_pts, preserving composition offsets; cover a B-frame fragment whose first decoded sample is not its earliest presentation in Rust, JS, and export.
The two findings from the previous review remain unchanged.
Verification: static review of the one-commit documentation delta and relevant dev importer, decoder, export, and clock/data paths. No tests or quest check run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed SHA: f5d52939cc4b32e9f6bc0ddb780158f1b5fa03f7
Splitting CMAF timing into a prerequisite is sensible. One new correction:
- [P2]
quest/m2/cmaf-frame-timestamp.md:26–27: preserve verbatim PES payloads; they do not contain the PES timestamp header.PesStart::parseseparates PTS/DTS from the elementary-stream bytes, andwrite_pesalready regenerates PTS from the frame timestamp. Looking for a PES timestamp inside that opaque payload would reject valid private streams or modify their media bytes. Replace the rewrite/refusal requirement with a shifted-frame regression test confirming regenerated PES PTS and byte-identical payloads; treat embedded timing in section-framed streams separately.
Prior finding status: the CMAF reference-point issue remains at lines 20–22. The new observation about fmp4::encode is correct, but passthrough Import stamps the minimum PTS instead. Reconcile both producers' anchor convention with the decoder and test both paths; changing the decoder's reference alone cannot preserve both existing conventions for reordered fragments. The data-write freezing and explicit-clock recording findings are unchanged.
Verification: static review of the one-commit documentation delta and relevant main-branch CMAF/TS paths. No tests or quest check run.
|
Landing summary:
@Dryvnt thanks for the original plan; feel free to follow up if the reshaped one misses your use case. (Written by Claude Opus 5.5) |
|
Thanks for reworking it. The shared-clock plan covers what I was after, and option 1 works for us.
(Written by Claude Opus 5.5) |
Plans a follow-up of #4668: every source sharing a catalog (moq-audio/moq-video captures and any number of container importers) lands on one forward timeline, whichever starts first.
Adds two m2 quests:
quest/m2/cmaf-frame-timestamp.md[S], onmain: CMAF decoders (Rust and JS) time samples from the moq-lite frame timestamp, usingtfdtonly within the fragment.quest/m2/shared-clock.md[M], ondev: requires the first.Problem
The first importer's first frame re-anchors the catalog clock. A capture that started earlier keeps its copy of the old clock. A second importer's anchor is a no-op, so it publishes its own PTS base on a clock it didn't place.
Decisions
How should a source joining a clock already in use behave?
catalog.clock()when stamping and mark a break on a change. Rejected. fMP4 and FLV start near PTS 0, so the anchor usually steps the clock backwards, andcontainer::Producer::writerefuses that withTimestampRewindeven across a discontinuity. Every audio buffer would also take the catalog lock to watch for an event that should never happen while it is live.catalog::Producer::clock()fixes the mapping. An importer anchors once and gets an offset: zero if it placed the mapping (PTS stays verbatim),now - first_ptsotherwise. [M]fMP4 passthrough: the offset moves the frame timestamp but not the payload's
tfdttfdtWhich milestone?
moq-cli publishruns capture or import, never both; moq-ffi doesn't expose capture)Impact
dev: no new items.catalog::Producer::clock()now fixes the mapping, and an importer whose clock was already taken shifts its PTS instead of publishing it verbatim. CMAF decoders (fmp4::decode, JScmaf/format.ts) take sample times from the frame timestamp, usingtfdtonly for offsets within the fragment.quest checkreports two errors already onmain(quest/m1/admission-bench.md,quest/m1/cluster-shims.md). This PR doesn't touch them.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code