Skip to content

feat!: JSON writes take a broadcast-clock timestamp in every binding - #7691

Merged
kixelated merged 11 commits into
mainfrom
quest/m1/json-required-timestamp
Nov 3, 2026
Merged

kixelated merged 11 commits into
mainfrom
quest/m1/json-required-timestamp

Conversation

@kixelated

@kixelated kixelated commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #7573

Problem

moq-c stamped a JSON snapshot or stream write that had a NULL timestamp with the catalog clock's now(). C media is written on the publisher's own timeline, which C had no way to read the broadcast clock for. Each JSON track takes a seat in the catalog's write Order, so every media track advertised the gap between the two timelines, about 8 s, as delay (moq-dev/moq#7573). Rust and moq-ffi left a null timestamp untimed instead, so the bindings disagreed on what a missing timestamp means.

The maintainer decided (2026-11-03, quest/m1/json-required-timestamp.md) that every track in a broadcast uses one epoch: every JSON write takes a required broadcast-clock timestamp, and nothing auto-stamps one.

Approach

  • moq-mux: json::Snapshot::update(&value, timestamp) and json::Stream::append(&value, timestamp) take a required moq_net::Timestamp. A track without a timescale would refuse every timestamped write, so json_snapshot / json_stream refuse it up front with TimestampMismatch and advertise nothing.
  • moq-ffi: MoqJsonSnapshotProducer::update, MoqJsonStreamProducer::append, and MoqJsonWindowProducer::push / pop take a required MoqTimestamp, documented against MoqMediaCatalogProducer::now. The window constructor refuses a track without a timescale, as snapshot and stream do. (moq-mux has no window API; moq-ffi wraps moq_json::window directly.)
  • moq-c: a NULL timestamp on moq_publish_json_snapshot_update, _stream_append, _window_push, and _window_pop fails with MOQ_ERROR_INVALID_POINTER and publishes nothing. The new moq_publish_catalog_now(broadcast, moq_timestamp *dst) mirrors moq-ffi's now(), and doc/lib/c tells a C publisher to stamp media and JSON from it. Nothing in moq-c stamps a write any more, so its data producers no longer keep a clock.
  • Wrappers (Python, Swift, Kotlin, Dart, Go): the snapshot update, stream append, and window push / pop take a required timestamp. The Dart raw binding is regenerated. Tests, doc/lib pages, and each package's MIGRATE are updated.
  • Tests: a moq-c test publishes Opus and a JSON snapshot, both stamped from moq_publish_catalog_now, and asserts the media advertises no delay. Stamping the media from 0 instead reproduces the bug (10 s delay), which I checked locally. Tests also cover NULL failing on every C JSON write, and catalog_now refusing a NULL dst or a closed broadcast. moq-mux and moq-ffi test that an untimed JSON or window track is refused.

Impact

  • moq-mux (breaking): json::Snapshot::update / json::Stream::append take (&T, moq_net::Timestamp) instead of impl Into<Timed<&T>>. Migrate update(&v) / update(Timed::from(&v).at(at)) to update(&v, at), using catalog.clock().now() or .capture(instant)? for at. json_snapshot / json_stream refuse a track without a timescale: create it with with_timescale(Timescale::MILLI).
  • moq-ffi and every generated binding (C, C++, Go, Dart, Python, Swift, Kotlin) (breaking): the JSON snapshot update, stream append, and window push / pop timestamp is required instead of optional, and their constructors refuse an untimed track. Pass MoqMediaCatalogProducer::now().
  • moq-c (breaking): NULL timestamp on moq_publish_json_snapshot_update, moq_publish_json_stream_append, moq_publish_json_window_push, and moq_publish_json_window_pop returns MOQ_ERROR_INVALID_POINTER. Added moq_publish_catalog_now.
  • Wrappers (breaking): Python update(value, timestamp) / append(value, timestamp) / push(value, timestamp) / pop(count, timestamp), Swift timestamp: Timestamp, Kotlin timestamp: Timestamp, Dart required MoqTimestamp timestamp, Go moq.Timestamp (not a pointer).
  • Wire: none.

Choices made without asking

  • Untimed JSON tracks are refused at construction. With a required timestamp, every write on a track without a timescale would fail with TimestampMismatch. Refusing it in json_snapshot / json_stream fails early instead, and no catalog entry is left advertising a track that can never carry a value.
  • Rust signature is two arguments (&value, timestamp) rather than a Timed<&T> whose at is checked at runtime. A missing timestamp then cannot be expressed, and it mirrors the bindings' update(value, timestamp).
  • NULL maps to MOQ_ERROR_INVALID_POINTER. moq-c has no separate invalid-argument code, and other NULL required pointers already return this one.

Alternatives

Decisions

  • JSON windows (maintainer, 2026-11-03, on this PR): options were refuse NULL in moq-c windows only; ✅ both windows now; leave windows to time-addressing. moq-c's moq_publish_json_window_push / _pop refuse a NULL timestamp, moq-ffi's MoqJsonWindowProducer::push / pop take a required timestamp, and the window constructor refuses a track without a timescale.

Follow-ups

  • None.

Checks

  • origin/main merged at cb4ceba (MIGRATE conflicts with moq-dev/moq#7697 in eight packages, resolved by keeping both entries), and earlier at c7ceeda and 1c1e383. Regenerating the Dart binding after the merge reproduces the committed one apart from its import blank lines, which follow main's formatting.
  • just check at cb4ceba: every module passes, including all 4466 selected Rust tests and Dart.
  • Swift ran no tests (Linux host). just test interop --all was not run: no interop client calls the changed JSON producers, and there is no wire change.

Review

  • Reviewer: Codex (quest review), gpt-6.1-sol, medium effort.
  • Round 1 on 48e86b2 against origin/main (9ac02f9): no findings.
  • Round 2 on c7ceeda against origin/main (fb9a4fb): no findings.
  • Round 3 on head cb4cebae1f5390b2fb32494e0a9ca35c6ef7fcfc against origin/main (151ed38): no findings.
  • Online review: one thread (pullfrog) on py/moq-rs/README.md, which still said omitting time leaves data untimed. Fixed in b8a774d, together with the same stale sentence in five doc/lib pages. Replied and resolved.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 5 commits November 2, 2026 23:21
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title JSON writes take a broadcast-clock timestamp in every binding feat!: JSON writes take a broadcast-clock timestamp in every binding Nov 3, 2026
kixelated and others added 3 commits November 3, 2026 09:13
…d-timestamp

# Conflicts:
#	dart/moq/MIGRATE.md
#	rs/moq-ffi/MIGRATE.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 3, 2026 17:51
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 93dc22e6-fed4-4aaf-b896-990dedf2aaef
Commits

Reviewing files that changed from the base of the PR and between 151ed38 and cb4ceba.

Files selected for processing: 44
  • c/moq/MIGRATE.md
  • cpp/moq/MIGRATE.md
  • dart/moq/MIGRATE.md
  • dart/moq/lib/json.dart
  • dart/moq_ffi/lib/src/moq.dart
  • doc/lib/c/index.md
  • doc/lib/dart/index.md
  • doc/lib/go/index.md
  • doc/lib/kt/index.md
  • doc/lib/py/index.md
  • doc/lib/swift/index.md
  • go/wrapper/MIGRATE.md
  • go/wrapper/README.md
  • go/wrapper/data_timestamps_test.go
  • go/wrapper/json/json.go
  • go/wrapper/json/json_test.go
  • kt/moq/MIGRATE.md
  • kt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/json/Tracks.kt
  • kt/moq/src/jvmAndAndroidTest/kotlin/dev/moq/SmokeTest.kt
  • py/moq-rs/MIGRATE.md
  • py/moq-rs/README.md
  • py/moq-rs/moq/json.py
  • py/moq-rs/tests/test_data_timestamps.py
  • py/moq-rs/tests/test_local.py
  • quest/m1/README.md
  • quest/m1/json-required-timestamp.md
  • rs/moq-c/MIGRATE.md
  • rs/moq-c/c-tests/timestamps.c
  • rs/moq-c/src/api.rs
  • rs/moq-c/src/publish.rs
  • rs/moq-c/src/test.rs
  • rs/moq-ffi/MIGRATE.md
  • rs/moq-ffi/src/json.rs
  • rs/moq-ffi/src/test.rs
  • rs/moq-mux/MIGRATE.md
  • rs/moq-mux/src/catalog/producer.rs
  • rs/moq-mux/src/catalog/tracks.rs
  • rs/moq-mux/src/container/fmp4/import_test.rs
  • rs/moq-mux/src/json.rs
  • rs/moq-transcode/src/catalog.rs
  • swift/MIGRATE.md
  • swift/README.md
  • swift/Sources/Moq/Json.swift
  • swift/Tests/MoqTests/SmokeTests.swift
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

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 commit: 48e86b2 against base 9ac02f9.

[P1] Migrate the in-tree CARLA status publisher (rs/moq-mux/src/json.rs:153–156, py/moq-rs/moq/json.py:99–104). The new timescale check and required timestamp break an existing consumer in this repository: demo/carla/sim/moq_carla_sim/publish.py:186–188 still constructs the status snapshot on TrackInfo(compress=True) without a timescale, so StatusPublisher construction now returns TimestampMismatch when the worker starts. Even if that track is made timed, publish.py:202 still calls update(payload) without the required timestamp. The CARLA worker installs the bindings from this checkout (demo/carla/sim/pyproject.toml:3–5), and its runtime constructs StatusPublisher for each publication, so this is not just an external migration. Supply a timed track and sample the same broadcast catalog clock for each update; update the CARLA fake and add a real-binding or contract test so the mock cannot hide signature or timescale drift.

Static GitHub-only review; I did not run CARLA or its tests. The test stub at demo/carla/sim/tests/conftest.py:36–47 still accepts the old call, so the existing mocked test alone would not expose this regression.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ Minor suggestions only.

Reviewed changes

  • moq-mux: json::Snapshot::update / json::Stream::append now take (&T, moq_net::Timestamp); prepare refuses a track without a timescale up front with TimestampMismatch, so json_snapshot / json_stream advertise nothing for it.
  • moq-ffi: JSON snapshot/stream update/append and window push/pop take a required MoqTimestamp; the window constructor refuses an untimed track (snapshot/stream refusal comes transitively from moq-mux).
  • moq-c: NULL timestamp on every JSON write returns MOQ_ERROR_INVALID_POINTER; added moq_publish_catalog_now; Data<T> no longer keeps a clock.
  • Wrappers/docs: Python, Swift, Kotlin, Dart, and Go signatures and call sites updated; the Dart raw binding is regenerated; MIGRATE and doc/lib pages are updated; tests cover the shared-epoch no-delay case (a_shared_epoch_gives_media_no_delay) plus NULL and untimed refusals.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread py/moq-rs/README.md Resolved
kixelated and others added 3 commits November 3, 2026 11:32
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d-timestamp

# Conflicts:
#	cpp/moq/MIGRATE.md
#	dart/moq/MIGRATE.md
#	go/wrapper/MIGRATE.md
#	kt/moq/MIGRATE.md
#	py/moq-rs/MIGRATE.md
#	rs/moq-c/MIGRATE.md
#	rs/moq-ffi/MIGRATE.md
#	swift/MIGRATE.md

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The new commits are documentation-only and read correctly, but the in-tree CARLA status publisher still calls the old untimed JSON API. That worker fails at startup once this lands.

Reviewed changes

Reviewed the two commits since the prior pullfrog review (48e86b2): b8a774d6c (docs) and the origin/main merge at c7ceeda86.

  • Replaced the stale "A missing data timestamp stays untimed" sentence in py/moq-rs/README.md and doc/lib/{py,go,kt,swift,dart}/index.md with wording that separates raw frames (may stay untimed on an untimed track) from JSON writes (always take a broadcast-clock timestamp).
  • Merged origin/main into the branch (bench and moq-audio perf commits); the changed files carried no conflicts.

⚠️ The in-tree CARLA status publisher still calls the old untimed JSON API

demo/carla/sim/moq_carla_sim/publish.py builds the status track with moq.TrackInfo(compress=True) and calls self._producer.update(payload) with no timestamp. SnapshotProducer now refuses a track without a timescale and update requires a timestamp, so StatusPublisher.__init__ fails with TimestampMismatch when the worker starts (and the update call would raise TypeError). This was raised in the prior automated review and is unchanged; the mocked stub in demo/carla/sim/tests/conftest.py still accepts the one-argument call, so the unit tests cannot catch it. The worker runs the bindings from this checkout, so this is an in-tree regression, not an external migration.

Technical details
# CARLA status publisher on the old JSON API

## Affected sites
- `demo/carla/sim/moq_carla_sim/publish.py:187` — `broadcast.publish_track("status", moq.TrackInfo(compress=True))` leaves the track untimed; constructing `moq.json.SnapshotProducer` on it now raises `TimestampMismatch` (`moq_mux::json::Snapshot::prepare` refuses a track with no timescale, surfaced through the moq-ffi constructor).
- `demo/carla/sim/moq_carla_sim/publish.py:202` — `self._producer.update(payload)` calls the now two-argument `SnapshotProducer.update(value, timestamp)` with one argument.
- `demo/carla/sim/tests/conftest.py:46` — the stub's `update(self, value: object) -> None` still mirrors the old signature, so the mocked tests pass and hide the drift.
- `demo/carla/sim/pyproject.toml:3-5` — the worker installs the bindings from this checkout, so the demo runs against this PR's API.

## Required outcome
- The `status` track must be timed (for example `moq.TrackInfo(timescale=1_000, compress=True)`).
- Every `StatusPublisher.publish` call must pass a broadcast-clock timestamp read from the same `media.CatalogProducer` epoch the rest of the truck uses (for example `catalog_producer.now()`), so the JSON track does not advertise a timeline gap as catalog `delay`.
- Update the `conftest.py` stub to the new signature so the mocked tests fail when call sites drift. Note the type-check lives in `just carla check` (`demo/carla/justfile:79-82`), not the root `just check` (which only lints/formats the worker), so this PR's stated gate did not exercise it.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@kixelated
kixelated added this pull request to the merge queue Nov 3, 2026
@pullfrog

pullfrog Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

The model never responded, so Pullfrog stopped this run after 2 minutes. The request went out but nothing came back. This is usually temporary, so re-trigger to try again.

Pullfrog  | Rerun failed job ➔ | View workflow run | via Pullfrog | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS)

@pullfrog

pullfrog Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

The failing check was the pullfrog review run, whose finding was real: #7691 broke the in-tree CARLA worker, which still created an untimed status track and called SnapshotProducer.update(payload) with no timestamp. Fixed on pullfrog/7691-carla-json-timestamp (8a02fdf): the status track gets timescale=1_000, every publish is stamped from moq.media.CatalogProducer.now(), and the test stub now requires the timestamp and exposes the catalog clock. pyright, pytest -q (169 passed), and ruff pass.

I could not push to this PR: it is in the merge queue, and GitHub rejected the update with GH006: Protected branch update failed ... A pull request for this branch has been added to a merge queue. Branches that are queued for merging cannot be updated. The fix is therefore in a stacked draft PR: #7755. Once #7691 lands, that PR narrows to the CARLA fix alone.

Task list (6/6 completed)
  • Checkout PR branch
  • Fetch and inspect failing check suite logs
  • Verify failure introduced by this PR
  • Reproduce and fix failure
  • Verify with exact CI command
  • Commit and push

Pullfrog  | View workflow run | via Pullfrog | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No new code changes in this delta; one documentation ordering nit inline. The previously reported CARLA regression is unchanged.

Reviewed changes

  • Resolved the origin/main merge across the eight MIGRATE.md guides, keeping both the #7691 and #7697 entries and deleting the stale duplicate "JSON window edits take an optional timestamp" heading in rs/moq-c.

⚠️ The in-tree CARLA status publisher still calls the old JSON API

The prior review raised this and the merge commits did not touch it: demo/carla/sim/moq_carla_sim/publish.py still builds the status track with moq.TrackInfo(compress=True) and calls self._producer.update(payload) with one argument, so the worker fails at startup once this lands.

Technical details
# CARLA status publisher on the old JSON API

## Affected sites
- `demo/carla/sim/moq_carla_sim/publish.py:187` — `publish_track("status", moq.TrackInfo(compress=True))` leaves `timescale` null; `moq.json.SnapshotProducer` now refuses it (`moq_mux::json::Snapshot::prepare` returns `TimestampMismatch`).
- `demo/carla/sim/moq_carla_sim/publish.py:202` — `self._producer.update(payload)` now needs `(value, timestamp)`.
- `demo/carla/sim/tests/conftest.py:46` — the stub's `update(self, value)` still accepts the old call, hiding the drift.

## Required outcome
- Give the `status` track a timescale and pass a timestamp from the same broadcast clock the media uses (`media.CatalogProducer.now()`).
- Update the test stub so the mocked suite fails when call sites drift.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Comment thread rs/moq-c/MIGRATE.md
broadcast clock, converted to track milliseconds. NULL fails, as above.
Recompile callers against the regenerated `moq.h`.

### Subscriptions take a `moq_delay` ([#7697](https://github.com/moq-dev/moq/pull/7697))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

main keeps #7697 above the #7614 JSON-window entry, and this file is ordered newest first; the merge moved it below. Move this block directly after the #7691 entry so landing the PR does not reorder main's changelog.

Merged via the queue into main with commit 90980d8 Nov 3, 2026
9 of 10 checks passed
@kixelated
kixelated deleted the quest/m1/json-required-timestamp branch November 3, 2026 21:12
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.

moq-c: a JSON write without a timestamp is ordered on the broadcast clock, which C media can't read, so every media track advertises ~8 s delay

1 participant