Repository navigation
feat!: JSON writes take a broadcast-clock timestamp in every binding - #7691
Conversation
Quest-Claim: 7350a1f8b5646919
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>
…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>
|
Warning Review limit reachedYou'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. View limit details
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. CodeRabbit Verify
|
kixelated
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ℹ️ Minor suggestions only.
Reviewed changes
- moq-mux:
json::Snapshot::update/json::Stream::appendnow take(&T, moq_net::Timestamp);preparerefuses a track without a timescale up front withTimestampMismatch, sojson_snapshot/json_streamadvertise nothing for it. - moq-ffi: JSON snapshot/stream
update/appendand windowpush/poptake a requiredMoqTimestamp; the window constructor refuses an untimed track (snapshot/stream refusal comes transitively from moq-mux). - moq-c: NULL
timestampon every JSON write returnsMOQ_ERROR_INVALID_POINTER; addedmoq_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/libpages are updated; tests cover the shared-epoch no-delay case (a_shared_epoch_gives_media_no_delay) plus NULL and untimed refusals.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
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
There was a problem hiding this comment.
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.mdanddoc/lib/{py,go,kt,swift,dart}/index.mdwith wording that separates raw frames (may stay untimed on an untimed track) from JSON writes (always take a broadcast-clock timestamp). - Merged
origin/maininto 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.deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
|
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.
|
|
The failing check was the pullfrog review run, whose finding was real: #7691 broke the in-tree CARLA worker, which still created an untimed I could not push to this PR: it is in the merge queue, and GitHub rejected the update with Task list (6/6 completed)
|
There was a problem hiding this comment.
ℹ️ No new code changes in this delta; one documentation ordering nit inline. The previously reported CARLA regression is unchanged.
Reviewed changes
- Resolved the
origin/mainmerge across the eightMIGRATE.mdguides, keeping both the#7691and#7697entries and deleting the stale duplicate "JSON window edits take an optional timestamp" heading inrs/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.deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
| 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)) |
There was a problem hiding this comment.
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.

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 writeOrder, so every media track advertised the gap between the two timelines, about 8 s, asdelay(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
json::Snapshot::update(&value, timestamp)andjson::Stream::append(&value, timestamp)take a requiredmoq_net::Timestamp. A track without a timescale would refuse every timestamped write, sojson_snapshot/json_streamrefuse it up front withTimestampMismatchand advertise nothing.MoqJsonSnapshotProducer::update,MoqJsonStreamProducer::append, andMoqJsonWindowProducer::push/poptake a requiredMoqTimestamp, documented againstMoqMediaCatalogProducer::now. The window constructor refuses a track without a timescale, as snapshot and stream do. (moq-mux has no window API; moq-ffi wrapsmoq_json::windowdirectly.)moq_publish_json_snapshot_update,_stream_append,_window_push, and_window_popfails withMOQ_ERROR_INVALID_POINTERand publishes nothing. The newmoq_publish_catalog_now(broadcast, moq_timestamp *dst)mirrors moq-ffi'snow(), anddoc/lib/ctells 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.update, streamappend, and windowpush/poptake a required timestamp. The Dart raw binding is regenerated. Tests,doc/libpages, and each package's MIGRATE are updated.moq_publish_catalog_now, and asserts the media advertises nodelay. 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, andcatalog_nowrefusing a NULLdstor 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::appendtake(&T, moq_net::Timestamp)instead ofimpl Into<Timed<&T>>. Migrateupdate(&v)/update(Timed::from(&v).at(at))toupdate(&v, at), usingcatalog.clock().now()or.capture(instant)?forat.json_snapshot/json_streamrefuse a track without a timescale: create it withwith_timescale(Timescale::MILLI).moq-ffiand every generated binding (C, C++, Go, Dart, Python, Swift, Kotlin) (breaking): the JSON snapshotupdate, streamappend, and windowpush/poptimestampis required instead of optional, and their constructors refuse an untimed track. PassMoqMediaCatalogProducer::now().moq-c(breaking): NULLtimestamponmoq_publish_json_snapshot_update,moq_publish_json_stream_append,moq_publish_json_window_push, andmoq_publish_json_window_popreturnsMOQ_ERROR_INVALID_POINTER. Addedmoq_publish_catalog_now.update(value, timestamp)/append(value, timestamp)/push(value, timestamp)/pop(count, timestamp), Swifttimestamp: Timestamp, Kotlintimestamp: Timestamp, Dartrequired MoqTimestamp timestamp, Gomoq.Timestamp(not a pointer).Choices made without asking
TimestampMismatch. Refusing it injson_snapshot/json_streamfails early instead, and no catalog entry is left advertising a track that can never carry a value.(&value, timestamp)rather than aTimed<&T>whoseatis checked at runtime. A missing timestamp then cannot be expressed, and it mirrors the bindings'update(value, timestamp).MOQ_ERROR_INVALID_POINTER. moq-c has no separate invalid-argument code, and other NULL required pointers already return this one.Alternatives
Decisions
moq_publish_json_window_push/_poprefuse a NULL timestamp, moq-ffi'sMoqJsonWindowProducer::push/poptake a required timestamp, and the window constructor refuses a track without a timescale.Follow-ups
Checks
origin/mainmerged atcb4ceba(MIGRATE conflicts with moq-dev/moq#7697 in eight packages, resolved by keeping both entries), and earlier atc7ceedaand1c1e383. Regenerating the Dart binding after the merge reproduces the committed one apart from its import blank lines, which followmain's formatting.just checkatcb4ceba: every module passes, including all 4466 selected Rust tests and Dart.just test interop --allwas not run: no interop client calls the changed JSON producers, and there is no wire change.Review
quest review), gpt-6.1-sol, medium effort.48e86b2againstorigin/main(9ac02f9): no findings.c7ceedaagainstorigin/main(fb9a4fb): no findings.cb4cebae1f5390b2fb32494e0a9ca35c6ef7fcfcagainstorigin/main(151ed38): no findings.py/moq-rs/README.md, which still said omitting time leaves data untimed. Fixed inb8a774d, together with the same stale sentence in fivedoc/libpages. Replied and resolved.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code