Repository navigation
fix(mux): data tracks stamp on the catalog's current clock - #4668
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A JSON or binary producer copied the catalog clock at creation, so one created before an importer's first frame never saw the anchor and kept stamping on the catalog's starting clock. Listing now reads the catalog clock on every write, and resets its estimator when the wall mapping changes so a re-anchor is not measured as jitter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome of the quest-start session:
(Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 9638803
No actionable bugs found in this diff.
Direction: centralizing current-clock stamping in the crate-private Listing is a focused simplification for all four data producers. Recording against the same clock snapshot keeps each write internally consistent, and reusing Estimator::discontinuity is justified to avoid measuring the re-anchor as jitter. I don’t see a simpler alternative that preserves those properties.
Verification: static inspection of the full diff, clock/catalog/estimator integration, downstream JSON/binary producers, and the new regression tests. I did not execute tests or just check. GitHub’s Test, Check, Windows, and macOS checks are still in progress for this commit; Android succeeded.
|
Merge status for 9638803:
(Written by Claude Opus 5.5) |
|
fix(mux): data tracks re-read the catalog clock on each stamp and reset the estimator when an importer re-anchors, so JSON/binary tracks created before the first frame share the media timeline. CI: Android, Check, Test, Windows, macOS all green on Findings: No concrete issues found.
Non-blocking notes:
MERGE on This is an automated review, not the maintainer's decision |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Added the JSON version of (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: c45a5ea
No new actionable bugs found. The only change since my previous review (9638803) adds the JSON anchored-clock regression test in rs/moq-mux/src/json.rs:516–551. It checks both an untimed snapshot and a captured stream record, creates both producers before anchoring, and verifies the actual frame timestamps with millisecond-safe bounds.
Direction: this is useful coverage of the JSON entry points without changing production behavior. The shared Listing::stamp design remains focused; a new test abstraction would add little value here.
Verification: static inspection of the delta and surrounding JSON producer/clock paths. I did not run tests or just check. GitHub's Android, Check, and Platform workflows are still in progress for this head.
|
Merging c45a5ea.
Auto-merge (squash) enabled on this head. (Written by Claude Opus 5.5) |
Completes quest/m1/data-track-clock.md.
Problem
A JSON or binary data producer copied the catalog's
Clockwhen it was created. Since #4543, a container importer re-anchors the catalog clock on its first frame, so a data track created before that frame kept stamping on the catalog's starting clock (reads 10 s at creation) while the media read the stream's own PTS. A test with a stream starting at 3600 s saw the data track stamp at 10.002 s.Fix
catalog::Listing(already owned by every data producer) now stamps payloads: it reads the catalog's current clock on each write, the way media tracks already do throughRendition::timestamp. The four producers (json::{Snapshot,Stream},binary::{Snapshot,Stream}) drop their cached clock.Following the anchor exposed a second problem. When the clock steps backwards (common, since fMP4 and FLV sources start near PTS 0), a data track that wrote captured payloads before and after the anchor would measure the step as lateness. Its advertised
jitterwould then latch at about 10 s, becausejitter.maxnever decays.Listingtherefore resets its estimator (Estimator::discontinuity) when the clock's wall mapping changes.Scope notes
doc/lib/rs/moq-mux.md.moq-audiocapture (encode/capture.rs) andmoq-videoencode (encode/producer.rs) also copycatalog.clock()once at startup. That is the same stale-copy problem for a local encoder sharing a catalog with an importer. It is out of scope here and suggested as a follow-up.Public API and wire impact
Listingis crate-private, and the removed fields were private.Tests
binary::test::a_write_follows_the_anchored_clock: snapshot and stream tracks created beforeanchor(3600 s)stamp on the anchored clock. Fails without the fix (10002 ms vs 3600000 ms).json::test::a_write_follows_the_anchored_clock: the same check forjson::{Snapshot,Stream}, so a later split of the JSON path stays covered.binary::test::an_anchor_is_not_jitter: captured writes across an anchor back to 0 advertise no jitter. Fails with the clock fix but without the estimator reset.cargo test -p moq-mux(945 tests) andjust checkpass.Follow-ups
moq-audiocapture,moq-videoencode) read the catalog clock per frame instead of a startup copy.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code