Skip to content

quest: plan one timeline for every source on a catalog - #4669

Merged
kixelated merged 4 commits into
moq-dev:mainfrom
Dryvnt:plan/capture-clock
Oct 1, 2026
Merged

kixelated merged 4 commits into
moq-dev:mainfrom
Dryvnt:plan/capture-clock

Conversation

@Dryvnt

@Dryvnt Dryvnt commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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:

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?

  • Re-read per frame: captures re-read 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, and container::Producer::write refuses that with TimestampRewind even 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.
  • Refuse a late anchor with an error.
  • Doc only.
  • ✅ Importer offsets onto the clock: the clock never moves once something stamps with it. 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_pts otherwise. [M]

fMP4 passthrough: the offset moves the frame timestamp but not the payload's tfdt

  • ✅ Accept the mismatch: the frame timestamp is the timeline, and payload timestamps are relative within the frame (ad insertion needs this too). Split into its own quest. It lands in m2 because nothing in-tree makes the two disagree yet; promote it when ad insertion needs it.
  • Rewrite tfdt
  • Refuse a late join on passthrough

Which milestone?

  • ✅ m2: nothing in-tree reaches it (moq-cli publish runs capture or import, never both; moq-ffi doesn't expose capture)

Impact

  • This PR: quest only. Public API: none. Wire: none.
  • The planned change, on 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, JS cmaf/format.ts) take sample times from the frame timestamp, using tfdt only for offsets within the fragment.
  • quest check reports two errors already on main (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

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Dryvnt
Dryvnt marked this pull request as ready for review October 1, 2026 10:41

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: dcf96069-0579-4f56-ae9b-a5b461b654df

📥 Commits

Reviewing files that changed from the base of the PR and between eb3e971 and af9e7d1.

📒 Files selected for processing (2)
  • quest/m2/README.md
  • quest/m2/capture-clock.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

The 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 af9e7

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 Summary

Architecture risk: 🔵 Low · up to af9e7

The change affects 1 system.

Changed systems: quest

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — quest (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in quest/m2/README.md: Added the Capture clock item, specifying that capture sharing a catalog with an importer uses the anchored clock even if capture started first.
  • observed — Modified behavior in quest/m2/capture-clock.md: Adds a quest documenting the capture clock mismatch, scope and proposed handling, affected crates and caller scenario, test guidance, and related audio capture-time quest. It states that the proposed change has no public API or wire impact.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary objective: planning one timeline for sources that share a catalog. This matches the added capture-clock quest and README entry.
Description check ✅ Passed The description explains the shared-catalog timeline problem and the quest-only scope. These points relate directly to the changeset, despite including broader follow-up details.
✨ Finishing Touches 💡 1
✨ Simplify code
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kixelated

kixelated commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Quest: plan moq-audio / moq-video captures to re-read catalog.clock() when stamping (same stale-copy class #4668 fixed for data tracks), with a wall-change treated as a timeline break.

CI: Test green; Check red from pre-existing quest check errors on main (quest/m1/admission-bench.md, quest/m1/cluster-shims.md) — this PR does not touch those files.

Findings: No concrete issues found.

Claims match the tree: rs/moq-audio/src/encode/capture.rs keeps a startup Clock copy, and rs/moq-video/src/encode/producer.rs samples capture_epoch once per open against that copy. Re-read-per-stamp (not a shared live Clock) matches #4668 and avoids a public Copy-breaking change. Guidance to mark a wall change like an existing stop (discontinuity / reset_epoch) and recompute video's capture_epoch then is the right shape for those loops. m2 placement after Audio capture time is appropriate; in-tree moq-cli publish does not run capture and import together.

Non-blocking notes:

MERGE on af9e7d1c809943f34a17a13ba8cf2d77a4c12847

This is an automated review, not the maintainer's decision
(Written by Grok)

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 kixelated changed the title quest: plan captures stamping on the catalog's current clock quest: plan one timeline for every source on a catalog Oct 1, 2026

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. Listing is not merely an observer: Listing::stamp stamps 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 existing Config::with_clock contract (rs/moq-mux/src/catalog/producer.rs:247–253) deliberately lets source PTS zero name the recording's real start. Applying now - first_pts changes 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.

@kixelated

Copy link
Copy Markdown
Collaborator

@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 TimestampRewind, since container::Producer::write refuses a group below the last one even across a discontinuity. It would also have audio take the catalog lock on every buffer, when it currently reads the clock once per epoch and counts samples.

New plan (quest/m2/shared-clock.md): the clock never moves once something stamps with it. catalog::Producer::clock() fixes the mapping. An importer anchors once and gets an offset back: zero if it placed the mapping, now - first_pts if the clock was already taken. That also covers a second importer, which today anchors as a no-op and publishes its own PTS base on a clock it didn't place. It's the only shape I see that gives captures and any number of imports one shared timeline.

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 moof rides verbatim in the payload, so its tfdt keeps the source's PTS while the moq-lite timestamp is offset. Options:

  1. Accept it: the moq-lite timestamp is the broadcast timeline, and the payload's timestamps are the source's own. Consumers that sync tracks (or map to wall time) must use the frame timestamp, never tfdt.
  2. Rewrite tfdt in passthrough by the same offset (cheap, but may need a v0 -> v1 box upgrade, which shifts trun data offsets).
  3. Refuse a late join on passthrough tracks only.

I lean toward 1, with the quest stating that rule and checking that the fMP4 export and the MSE path in js/watch honour it. Does that work for you, or did you have a use where tfdt needs to match?

(Written by Claude Opus 5.5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator

@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 tfdt and ignore the frame timestamp (fmp4::decode in moq-mux, Format.decode in js/hang/src/container/cmaf/format.ts). They rebase to the frame timestamp plus each sample's offset within the fragment, and the fMP4 exporter is checked for the same. Happy to hear if this misses something you had in mind.

(Written by Claude Opus 5.5)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] ms and outer timestamp 80 ms, subtracting the first sample produces [80, 40, 60] ms even with zero importer offset. That shifts video relative to audio and can create a rewind across fragments. Specify outer_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 kixelated left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::parse separates PTS/DTS from the elementary-stream bytes, and write_pes already 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.

@kixelated

Copy link
Copy Markdown
Collaborator

Landing summary:

  • Replaced the per-frame clock re-read plan. A backward anchor would hit TimestampRewind, and audio would take the catalog lock on every buffer.
  • quest/m2/shared-clock.md [M], dev: the clock is fixed once handed out, and an importer that joins later offsets its moq-lite timestamps instead of moving the clock. Captures and any number of importers share one timeline.
  • quest/m2/cmaf-frame-timestamp.md [S], main: CMAF decoders (Rust and JS) time samples from the frame timestamp, using tfdt only within the fragment. Decided by the maintainer: the frame timestamp is the timeline, and payload timestamps are relative (ad insertion needs the same rule).

@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)

@kixelated
kixelated merged commit 0863687 into moq-dev:main Oct 1, 2026
3 checks passed
@Dryvnt

Dryvnt commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for reworking it. The shared-clock plan covers what I was after, and option 1 works for us.

  • Our use: we plan to take the catalog's clock up front, build an Anchor on it, and stamp each stream through its own Lane. A clock that stays fixed once handed out is exactly what that needs.
  • tfdt: nothing of ours depends on it matching, since we don't use the CMAF container.

(Written by Claude Opus 5.5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants