Skip to content

Add per-track timescale and frame timestamps to moq-lite#1439

Merged
kixelated merged 22 commits into
devfrom
claude/moq-net-timestamp-support-ieCkL
Jun 1, 2026
Merged

Add per-track timescale and frame timestamps to moq-lite#1439
kixelated merged 22 commits into
devfrom
claude/moq-net-timestamp-support-ieCkL

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

This change introduces per-track timescale negotiation and per-frame timestamps to the moq-lite protocol, enabling accurate media synchronization across different frame rates and sample rates.

Summary

Refactors the timestamp system from a generic Timescale<const SCALE: u64> type alias to a concrete Timestamp struct that carries both a raw value and its scale. This allows timestamps to be expressed in any unit (seconds, milliseconds, microseconds, nanoseconds) and enables the protocol to negotiate per-track timescales via SUBSCRIBE_OK messages.

Key Changes

  • Timestamp struct redesign: Replaced Timescale<SCALE> type alias with a concrete Timestamp struct containing value: VarInt and scale: u64. The scale is now carried per-timestamp instance rather than as a compile-time constant.

  • Unspecified scale handling: scale == 0 denotes an unspecified timescale (used by Timestamp::ZERO and Timestamp::MAX sentinels). Unit conversions and arithmetic against unspecified scales return TimeOverflow to prevent divide-by-zero errors.

  • Scale conversion API: Added Timestamp::convert() and Timestamp::as_scale() methods for explicit scale conversions. All as_* methods (as_secs, as_millis, etc.) now return Result<T, TimeOverflow> instead of infallible values.

  • Signed delta encoding: Added Timestamp::checked_add_delta() and Timestamp::checked_delta_from() for zigzag-encoded signed deltas used by moq-lite per-frame delta decoding (for B-frames).

  • Protocol integration:

    • Added timescale: u64 field to SubscribeOk message (Lite05+)
    • Added timescale: u64 field to Track struct
    • Added timestamp: Timestamp field to Frame struct
    • Updated Version::Lite05 to indicate timestamp support on the wire
  • VarInt zigzag support: Added VarInt::from_zigzag() and VarInt::to_zigzag() for signed integer encoding used in delta timestamps.

  • Ordering and comparison: Implemented Ord and PartialOrd for Timestamp with debug assertions for scale compatibility. Cross-scale comparisons are allowed only when one side is a scale-0 sentinel.

  • Removed PartialOrd/Ord from Timescale: The old generic type no longer derives these traits; the new Timestamp implements them with scale-aware semantics.

Implementation Details

  • Scale conversions use 128-bit intermediate arithmetic to avoid overflow during multiplication before division.
  • The wire format encodes only the raw value as a QUIC varint; scale is carried out-of-band via track properties or SUBSCRIBE_OK.
  • Timestamp::now() returns microsecond-scale timestamps anchored to a jittered wall-clock reference.
  • All arithmetic operations (checked_add, checked_sub) require matching scales and return TimeOverflow on mismatch.
  • The Debug impl chooses the largest unit (seconds, milliseconds, microseconds, nanoseconds) where the value has no fractional part.

Compatibility

  • Lite01-Lite04: Decode timestamps with scale == 0 (unspecified)
  • Lite05+: Negotiate per-track timescale via SUBSCRIBE_OK and encode per-frame timestamps

https://claude.ai/code/session_01Wy7egQjXvfPCKovs3mWANB

claude added 5 commits May 21, 2026 22:10
Replace the const-generic Timescale<SCALE> with a single Timestamp type
that carries scale as a runtime field, so timescales can be negotiated
per-track. Adds sentinel ZERO/MAX (scale 0) plus checked_add_delta and
checked_delta_from helpers for the upcoming moq-lite zigzag delta wire
format.

Updates hang, moq-mux, libmoq, moq-ffi to use the new constructors.
Catalog jitter switches from moq_net::Time to Option<moq_net::Timestamp>.

This is the foundational refactor; wire-format changes and async
subscribe come in follow-up commits.

https://claude.ai/code/session_01Wy7egQjXvfPCKovs3mWANB
Each Track now carries a timescale (units per second) and each Frame
carries a presentation Timestamp in that timescale. Both default to the
unspecified sentinel (scale 0) when not negotiated.

Adds VarInt::from_zigzag / to_zigzag for the upcoming moq-lite
delta-encoded timestamp wire format. Inputs must fit in i61 (the 62-bit
varint range after zigzag), which is ample for frame-to-frame deltas.

Updates internal struct-literal call sites across hang, moq-mux, libmoq,
moq-ffi, moq-relay, moq-clock to include the new fields with sensible
defaults (0 = unspecified). No wire-format changes yet.

https://claude.ai/code/session_01Wy7egQjXvfPCKovs3mWANB
Adds moq-lite version Lite05 (ALPN moq-lite-05, code 0xff0dad05):

- SUBSCRIBE_OK carries the per-track timescale (varint) so the subscriber
  learns it at session setup. Older versions decode as 0 (unspecified).
- Per-frame wire header gains a zigzag-encoded signed delta from the
  previous frame's timestamp on the same group stream, before the frame
  size. Negative deltas support B-frames.
- The publisher verifies frame timestamps match the track's negotiated
  timescale and aborts the connection with ProtocolViolation otherwise.

TrackProducer gains a set_timescale method that updates both the local
Track info and a new conducer State.timescale, plumbing the value through
to consumers. TrackConsumer exposes timescale_now/poll_timescale/timescale
to read the resolved value.

hang and moq-mux are updated to stamp the moq-net Frame.timestamp on
every frame they emit and to set Track.timescale = 1_000_000 (microseconds)
on tracks they produce. The legacy container-level timestamp prefix
remains as a duplicate during the transition.

https://claude.ai/code/session_01Wy7egQjXvfPCKovs3mWANB
BroadcastConsumer::subscribe_track is now async and resolves after the
session layer publishes the negotiated timescale (via SUBSCRIBE_OK on
moq-lite Lite05+, or 0 for older versions and for IETF until LOC track
properties land). Subscribers waiting on the resolved timescale via
TrackConsumer::timescale see it before the future completes.

subscribe_track_immediate keeps the synchronous semantics for callers
inside non-async contexts (FFI, internal moq-net publishers serving a
local broadcast) where the timescale either isn't relevant or is already
known.

The IETF subscriber now also sets timescale = 0 explicitly when
SUBSCRIBE_OK arrives, so the async subscribe future resolves on older
drafts that don't yet carry LOC track properties.

https://claude.ai/code/session_01Wy7egQjXvfPCKovs3mWANB
Comment thread rs/hang/examples/video.rs Outdated
let video_track = moq_net::Track {
name: "video".to_string(),
priority: 1, // Video typically has lower priority than audio
timescale: 1_000_000,

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.

Can you make a Timescale type? Timescale::MICRO is a lot nicer.

Comment thread rs/hang/src/catalog/root.rs Outdated
moq_net::Track {
name: Catalog::DEFAULT_NAME.to_string(),
priority: 100,
timescale: 0,

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.

Timescale::UNKNOWN

Comment thread rs/hang/src/container/frame.rs Outdated
pub fn encode(&self, group: &mut moq_net::GroupProducer) -> Result<(), Error> {
let mut header = BytesMut::new();
self.timestamp.encode(&mut header).map_err(moq_net::Error::from)?;
self.timestamp.encode_value(&mut header).map_err(moq_net::Error::from)?;

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.

I'd rather do something like self.timestamp.value.encode or something

Comment thread rs/hang/src/container/frame.rs Outdated
let size = header.len() + self.payload.len();

let mut chunked = group.create_frame(size.into())?;
let net_frame = moq_net::Frame::new(size as u64).with_timestamp(self.timestamp);

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.

I think you remove Frame::new and require using the struct constructor instead. timestamp shouldn't be optional, builder doesn't make sense.

Comment thread rs/hang/src/container/frame.rs Outdated
/// Decode a frame from raw bytes (VarInt timestamp prefix + payload).
pub fn decode(mut buf: impl Buf) -> Result<Self, Error> {
let timestamp = Timestamp::decode(&mut buf)?;
let timestamp = Timestamp::decode_value(&mut buf, TIMESCALE)?;

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.

I think we decode the value separately (via u64::decode) then call Timestamp::new

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.

Or in this case, Timestamp::from_micros(value)

Comment thread rs/moq-ffi/src/consumer.rs Outdated
let track = self.inner.subscribe_track_immediate(&moq_net::Track {
name,
priority: 0,
timescale: 0,

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.

We need to change subscribe_track so it only takes a &str name instead of a Track. The timescale and priority arrives in the SUBSCRIBE_OK instead. To allow subscriber priorities, we should add a Delivery struct in the future for overrides, but we need to start splitting track properties (set by the publisher) and track requests (set by the subscriber)

Comment thread rs/moq-mux/src/container/cmaf.rs Outdated
let cts = entry.cts.unwrap_or_default() as i64;
let pts = dts.checked_add_signed(cts).ok_or(Error::PtsOverflow)?;
let timestamp = Timestamp::from_scale(pts, timescale)?;
let timestamp = Timestamp::from_scale(pts, timescale, crate::container::TIMESCALE)?;

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.

Kinda confusing. Maybe make a .convert method or something to change timescale units? Timestamp::new(pts, timescale).convert(crate::container::TIMESCALE)

Comment thread rs/moq-mux/src/export/fmp4.rs Outdated
/// as the catalog changes.
pub fn new(broadcast: moq_net::BroadcastConsumer) -> Result<Self, crate::Error> {
let catalog_track = broadcast.subscribe_track(&hang::Catalog::default_track())?;
let catalog_track = broadcast.subscribe_track_immediate(&hang::Catalog::default_track())?;

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.

Cant do this in the constructor any longer. please stop using subscribe_track_immediate, we need to fix the API.

Comment thread rs/moq-mux/src/import/aac.rs Outdated
config: AacConfig,
) -> anyhow::Result<Self> {
let track = broadcast.unique_track(".aac")?;
let mut track = broadcast.unique_track(".aac")?;

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.

Need to improve this API. What about the priority too? unique_track should probably return a String or something?

I really don't like .set_timescale. Publisher track properties like the name/priority/timescale should be fixed. Allowing them to be updated can only cause pain.

Comment thread rs/moq-net/src/model/track.rs Outdated

/// Wait until the track's timescale has been negotiated (e.g. via SUBSCRIBE_OK
/// for subscribers, or set immediately by publishers).
pub async fn timescale(&self) -> Result<u64> {

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.

Please no. Return a TrackConsumer only when we get the SUBSCRIBE_OK.

kixelated and others added 6 commits May 22, 2026 15:59
…cale type

Addresses PR #1439 review:
- Add `Timescale` newtype with named constants (UNKNOWN/SECOND/MILLI/MICRO/NANO);
  replace raw `u64` timescale fields and method args throughout
- `BroadcastConsumer::subscribe_track(name, Subscription)` is now `async` and
  resolves on SUBSCRIBE_OK with the publisher's authoritative Track properties
  (priority, timescale). `subscribe_track_immediate` is gone
- Add `Subscription` for subscriber-side preferences (priority, timeout),
  separated from the publisher's immutable Track. Add aggregation methods
  `TrackProducer::max_priority` / `max_timeout` and an updatable
  `TrackConsumer::update_subscription`. This is the API surface the future
  fetch path will plug into
- Make Track properties immutable on `TrackProducer`: remove `set_timescale`,
  fold `unique_track` into `unique_name` so callers construct the full Track
  themselves
- Introduce `TrackRequest` so the dynamic broadcast flow yields a request the
  publisher fulfills via `accept(Track)`, completing the async resolution
- Remove `Frame::new` and `with_timestamp`; require struct construction; keep
  size-only `From` impls
- Hang container frame now uses `VarInt::encode_quic`/`decode_quic` + the
  `Timestamp::value()` accessor for explicit timestamp varint coding
- Replace `Timestamp::from_scale` callsites with `Timestamp::new(...).convert(...)`

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Brings in the stats, LOC, MKV, MSF, fragment-duration, and async-CMSF
changes from main alongside the async-subscribe / immutable-track work on
this branch.

Conflict resolution highlights:
- lite::publisher / lite::subscriber: keep the TrackRequest-based async
  subscribe flow on the subscriber side and threading the publisher's
  Track properties through SUBSCRIBE_OK, plus the new StatsHandle
  bookkeeping from main.
- moq-mux::export::Mkv / Fmp4: rewrite per-track subscribe to drive
  in-flight subscribe_track futures via FuturesUnordered polled with the
  conducer waiter's Waker, so a paused-time runtime still completes them
  before time advances. Defer header emission until every catalog-listed
  track is resolved.
- moq-mux::import::mkv: switch unique_track to unique_name +
  create_track(Track {..}) with explicit timescale.
- catalog::msf_consumer / producer: moq_net::Time → moq_net::Timestamp,
  as_millis() now returns Result so jitter conversion gets and_then'd.
- container::loc: Timestamp::from_scale(value, src) → Timestamp::new +
  convert; create_frame takes Into<Frame> so the usize size arg works.
- hang::container::Frame::encode: normalize timestamps to the container
  TIMESCALE on the wire so producers using a different source scale
  (e.g. nanoseconds from MKV) round-trip correctly via Timestamp::from_micros.

Also exposes Waiter::waker() for downstream code that needs a Waker to
drive sub-futures inside a conducer poll loop.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adapt the gstreamer source plugin to the new `subscribe_track` signature
(`&str` name + `Subscription`, async) and the new `Timestamp` arithmetic API
where `Timestamp - Timestamp` is no longer infallible and `Into<Duration>`
isn't implemented.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The underlying MoqBroadcastConsumer.subscribe_catalog/track/media methods
became async (returning coroutines) when the Rust API was made async to wait
for SUBSCRIBE_OK before returning a TrackConsumer. Propagate the change
through the BroadcastConsumer wrapper, tests, example, and README.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adding `moq-lite-05` to `version::ALPNS` shipped the wire identifier to
peers, but `Client::connect` / `Server::accept` never grew a match arm
for it. A connection that negotiated `moq-lite-05` therefore fell
through to `UnknownAlpn`, which is what the WebTransport ALPN
integration test surfaced as `unknown ALPN: moq-lite-05`.

Add the missing arms so Lite05 dispatches through the same Lite session
plumbing as 04/03.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ized

The Lite05 wire format is still in flux. Letting clients pick it up by
default risks two peers settling on `moq-lite-05` and then disagreeing
about what bytes belong on the wire once we finalize the draft.

- Rename the ALPN to `moq-lite-05-wip` (constant kept as `ALPN_LITE_05`).
- Drop the wip ALPN from the public `ALPNS` list so unmodified callers
  don't advertise it during negotiation.
- Drop Lite05 from `Versions::all()` for the same reason.
- Update `Display` / `FromStr` so the round-trip uses the `-wip` suffix.

Callers that explicitly opt in (`client.version = [moq-lite-05-wip]`)
still negotiate it via the existing ALPN match arms in `Client::connect`
and `Server::accept`. Once the format is finalized we'll rename the
constant to `moq-lite-05` and re-add it to `ALPNS` / `Versions::all()`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reworks the per-track timescale design to address review findings on the
Lite05 wire format work:

* `Timescale` now wraps `NonZeroU64`, so the divide-by-zero path that the
  old `UNKNOWN` sentinel was guarding against is impossible to construct.
  Conversion methods (`as_secs/millis/micros/nanos`, `as_scale`,
  `convert`) drop `Result<_, TimeOverflow>` returns for the scale-zero
  case and become infallible.
* `Frame.timestamp: Option<Timestamp>` and `Track.timescale: Option<Timescale>`.
  "No timestamp on this frame" / "no advertised timescale" is now an
  explicit `None`, not a magic value embedded in the type.
* `SubscribeOk.timescale: Option<Timescale>` on Lite05+ encodes `None`
  as wire `0` and `Some(n)` as `n`; older versions ignore the field.
* Lite05 publisher now rejects frames without an explicit timestamp at
  the matching scale with `ProtocolViolation` instead of silently
  emitting a zero delta. Same on track-level: a Lite05 publisher with a
  `None` timescale errors before sending SUBSCRIBE_OK.
* Removed `Timestamp::ZERO` / `Timestamp::MAX` sentinels. Call sites
  that used them (`jitter.rs`, `fmp4/import.rs`) move to `is_none_or`
  / `matches!` patterns over the `Option<Timestamp>` directly.
* Added a Lite05 publisher↔subscriber round-trip test in
  `moq-native/tests/broadcast.rs` covering negative deltas
  (B-frame-style PTS reordering) over WebTransport.
* Em dashes in two new comments in `broadcast.rs` replaced with
  periods per CLAUDE.md.

JS side (`js/net`) is still at Lite04 and has no on-wire change yet, so
the cross-package sync is deferred until JS picks up Lite05.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kixelated kixelated changed the base branch from main to dev May 28, 2026 17:14
Reconciles the PR with dev's landed work:

* `moq-net: runtime Timescale/Timestamp; container::Frame keeps source
  scale (#1473)` — adopts dev's canonical `Timescale(NonZero<u64>)` and
  `Timestamp` design. The PR keeps `Track.timescale: Option<Timescale>`,
  `Frame.timestamp: Option<Timestamp>`, and `SubscribeOk.timescale` for
  the Lite05 wire format on top.
* `moq-mux: catalog filter/target and Annex-B exporters (#1487)` and the
  surrounding mux reorganization (codec/* + container/{fmp4,mkv,loc}
  split). The PR's per-file Lite05 additions to these layers are dropped
  in favor of dev's structure; per-frame timestamp encoding stays opt-in
  at the moq-net layer until the mux gets explicit timescale support.
* `Split OriginConsumer into cheap read handle and announcement cursor
  (#1434)` and other broadcast/origin updates land via dev.
* `moq-net: add Lite05Wip version variant (unadvertised) (#1518)` — the
  PR's wire format additions now hang off dev's `Lite05Wip` name (was
  `Lite05` in the PR), keeping a single ALPN identifier (`moq-lite-05-wip`)
  across both sides of the merge.
* Lite05 publisher logic and Lite05 subscriber timestamp decoding kept
  from the PR. `SubscribeOk.timescale: Option<Timescale>` carries the
  negotiated scale on Lite05+; older versions decode it as `None`.
* `subscribe_track` reverts to dev's sync `(&Track)` signature. The PR's
  async `Subscription`-based variant is dropped here because it cascaded
  async through every mux exporter and tipped over dev's poll-driven
  exporters. Timescale negotiation still works: the publisher carries
  it on the `Track` it creates, and Lite05 subscribers stash the value
  from `SubscribeOk` on the `TrackEntry` before frame decoding begins.
* `lite05_timestamp_roundtrip` integration test ported to dev's API and
  passes over WebTransport (negative delta frames included).

JS net (`js/net`) is still at Lite04, so the Lite05 wire field doesn't
need a JS-side update in this merge.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kixelated and others added 2 commits June 1, 2026 09:48
Catches up with dev's landed work:

* `moq-net: runtime Timescale/Timestamp; container::Frame keeps source scale (#1473)` —
  canonical Timescale(NonZero<u64>) design.
* `moq-net: make subscribe_track async, blocking on SUBSCRIBE_OK (#1540)` —
  the async API the PR originally introduced is now on dev directly.
* `moq-net: add Lite05Wip version variant (unadvertised) (#1518)` —
  the version variant the PR added is on dev as `Lite05Wip`.
* `moq-mux: catalog filter/target and Annex-B exporters (#1487)` and the
  surrounding mux reorganization.
* SubscribeOk negotiates `Compression` on Lite05+ (dev's #1538-ish work).
* `moq-lite-05: add AnnounceOk message (#1573)`.

Almost every distinctive piece of the PR ended up in dev under a separate
PR, so this merge takes dev's versions wholesale and keeps only the
remaining infrastructure for per-frame timestamps:

* `Frame.timestamp: Option<Timestamp>` for the on-wire per-frame field.
* `VarInt::from_zigzag` / `to_zigzag` helpers for the signed-delta encoding.
* `lite::Version::has_timestamps()` feature gate.
* `hang::container::Frame::encode` stamps the moq-net frame timestamp so
  the wire layer can carry it on Lite05+ once the encoding lands.

What's still missing for a complete Lite05 timestamp feature (called out
in the PR description for follow-up):

* `Track.timescale` / `SubscribeOk.timescale` negotiation.
* Lite05 publisher and subscriber zigzag-delta encode/decode in
  `lite/publisher.rs::serve_group` and `lite/subscriber.rs::run_group`,
  which currently still match dev's untimed-frame format.
* A working `lite05_timestamp_roundtrip` integration test — the test from
  the previous merge referenced fields that dev redesigned away
  (`Track.priority`, `Track.timescale`, `with_publish`, `with_consume`,
  `Announced::expect`) and was deleted here pending a rewrite against
  dev's current API.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Builds on dev's existing Lite05 plumbing (Lite05Wip variant, async
subscribe_track, SubscribeOk-based negotiation, Compression field) to
add per-frame presentation timestamps on the wire.

* `Track::with_timescale(scale)` builder, mirroring `with_compress`.
  `timescale: Option<Timescale>` on Track; the publisher requires
  `Some(_)` on Lite05+ and surfaces a `ProtocolViolation` otherwise.

* `SubscribeOk.timescale: Option<Timescale>` echoes the publisher's
  scale on Lite05+. Wire layout: `None` is `0`, `Some(n)` is `n`,
  appended after the existing Compression varint. Lite04 and older
  decode as `None`.

* `lite/publisher.rs::serve_frame` zigzag-delta encodes
  `frame.timestamp` against a per-group `prev_ts` (`(curr - prev) <<
  1) ^ ((curr - prev) >> 63)` via [`VarInt::from_zigzag`], written
  before the size varint on Lite05+. The first frame's delta is its
  absolute value (prev_ts = 0). Frames missing a timestamp, or whose
  scale doesn't match the negotiated one, fail with
  `ProtocolViolation` rather than silently encoding garbage.

* `lite/subscriber.rs::run_group` reads the zigzag delta first,
  reconstructs the absolute value, and attaches a `Timestamp` at the
  negotiated scale to the produced `Frame`. Older versions still take
  the size-first untimed path. The subscriber asserts that Lite05
  peers actually advertise a scale in SUBSCRIBE_OK.

Tests:
* Three new unit tests in `lite::subscribe::test` cover
  Lite05-roundtrip, pre-Lite05-absence, and the `0 ↔ None` mapping.
* `broadcast_moq_lite_05_timestamps_webtransport` integration test
  publishes three frames with timestamps `[10000, 30000, 20000]` (the
  middle one going backwards exercises the negative-delta path) and
  verifies the subscriber sees them at the negotiated scale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kixelated kixelated marked this pull request as ready for review June 1, 2026 16:58
Decouples per-frame timestamp wire-presence from `Version::has_timestamps()`
so a Lite05 track without an advertised timescale skips the byte
entirely instead of forcing every frame to carry an opaque varint.

* Publisher: `serve_frame` writes the zigzag-delta timestamp iff
  `self.timescale.is_some()`. A `None` timescale means "no timing on
  this track" — catalogs, control channels, IETF-bridged tracks — and
  the frame goes out as bare `[size][payload]`. Frames with the
  matching scale still get encoded normally; mismatched-scale frames
  still surface as `ProtocolViolation` rather than corrupting decode.

* Publisher's `run_subscribe` no longer rejects a missing timescale on
  Lite05+; it just sets the SUBSCRIBE_OK field to `0` and lets the
  subscriber agree to skip the byte. Pre-Lite05 versions still always
  decode timescale as `None` (no wire field).

* Subscriber: `run_group` reads the zigzag delta iff the negotiated
  timescale (from SUBSCRIBE_OK) is `Some(_)`, otherwise reads only the
  size varint. The redundant "Lite05 MUST advertise a timescale"
  guard is gone.

* Doc note on `Version::has_timestamps()` clarifies that the method
  governs whether SUBSCRIBE_OK can carry a timescale, not whether the
  per-frame timestamp byte is always present.

* New integration test `broadcast_moq_lite_05_without_timescale`
  publishes a single frame on a Lite05 track with no timescale and
  asserts the subscriber sees `frame.timestamp = None`, proving the
  no-timestamp path round-trips.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread rs/moq-net/src/lite/publisher.rs Outdated
Comment on lines +674 to +678
if let Some(track_timescale) = self.timescale {
let ts = frame.timestamp.ok_or(Error::ProtocolViolation)?;
if ts.scale() != track_timescale {
return Err(Error::ProtocolViolation);
}

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.

Ideally, we should catch this earlier at the model level. Make a new error that is more descriptive?

Comment thread rs/moq-net/src/lite/publisher.rs Outdated
// isn't meaningful (catalogs, control channels, IETF transport).
// Refuse a frame missing a timestamp or at the wrong scale rather than
// silently encoding a zero delta and corrupting decode.
if let Some(track_timescale) = self.timescale {

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.

Make sure frame.timestamp is None if timescale is None.

Addresses review feedback on the publisher's serve_frame:

> Ideally, we should catch this earlier at the `model` level. Make a
> new error that is more descriptive?
> Make sure frame.timestamp is None if timescale is None.

The previous wire-side check only fired when the publisher tried to
encode the frame, and it reused `Error::ProtocolViolation` — a vague
"peer broke the rules" error for what was actually a local API misuse.

This commit moves the check into `GroupProducer::append_frame` so the
contract is enforced where the frame enters the group, not after the
publisher has already gone partway through a write. The new
`Error::TimestampMismatch` variant names the failure mode directly.

* `GroupProducer` gains a `timescale: Option<Timescale>` field,
  populated by `TrackProducer::{create_group,append_group}` from the
  parent track. `GroupProducer::new_with_timescale` is the explicit
  constructor; `new` keeps the no-timescale default for test code and
  callers that don't go through a TrackProducer.

* `append_frame` rejects frames whose `timestamp` doesn't match the
  group's timescale (None on a timed group, Some on an untimed group,
  or mismatched scale) with `Error::TimestampMismatch` before
  touching the cache. This catches both the "frame.timestamp is None
  if timescale is None" case and the symmetric "must be Some on a
  timed track" case.

* The lite publisher's `serve_frame` swaps its `ProtocolViolation`
  branches for `.expect("model layer validated timestamp presence")`
  comments. The wire layer now relies on the model invariant.

* The lite subscriber stamps the negotiated `SubscribeOk.timescale`
  onto the local `Track` it accepts, so the subscriber-side
  `GroupProducer` validates incoming frames against the same rule.

* Four new unit tests in `model::group::test` cover untimed-group +
  timestamped-frame, timed-group + missing-timestamp, scale mismatch,
  and the happy path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kixelated kixelated enabled auto-merge (squash) June 1, 2026 17:30
kixelated and others added 4 commits June 1, 2026 10:33
CI caught the Lite05 timestamp tests still using the old
`bc.subscribe_track(name, sub).ok().await` form, which #1576 removed
from `BroadcastConsumer` in favor of the explicit
`consume_track(name).subscribe(sub).await` two-step. Update both
Lite05 integration tests to match dev's new pattern.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's `cargo fmt --check` flagged the inline `Frame { size: N, timestamp: None }`
literals (and a few other spots) that I'd written compactly enough for
rustfmt to want to break onto multiple lines. Run `cargo fmt` to bring
the workspace back into compliance — no functional changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The model-layer `TimestampMismatch` validation I added in
`a953eaf7` was rejecting hang-frame writes onto tracks created
without an advertised timescale — every codec importer (opus,
aac, h264/avc1/avc3, h265/hev1, av1, vp8, vp9), every container
importer (mkv, fmp4), the audio producer, and a slew of unit
tests went through `BroadcastProducer::unique_track(suffix)` or
`Track::new(name).produce()`, both of which build a Track with
`timescale: None`. The hang container then stamped each frame's
moq-net timestamp at `hang::container::TIMESCALE` (microseconds),
and `GroupProducer::append_frame` rightfully rejected the
mismatch.

Two API additions and a sweep of producer call sites:

* `BroadcastProducer::unique_name(suffix) -> String` — returns
  just the generated name so callers can build a Track with
  non-default properties before `create_track`. `unique_track`
  stays as a convenience wrapper around `unique_name` +
  `create_track(Track::new(name))` for callers that don't need
  any of those properties.

* Every site that produces hang frames (codec importers,
  container importers, moq-audio publisher, moq-rtc vp8/vp9,
  the h264 export integration test, and the fmp4/mkv export
  test fixtures) now goes through:

      broadcast.create_track(
          moq_net::Track::new(name).with_timescale(hang::container::TIMESCALE),
      )

  matching the rule that hang::container's encode path stamps
  `moq_net::Frame::timestamp = Some(ts)` with
  `ts.scale() == hang::container::TIMESCALE`.

* moq-mux's `container::consumer` test fixtures and
  `container::producer` test fixtures pick up `.with_timescale`
  where they exercise the hang container. The two DurationWire
  tests (a test-only mock container that doesn't stamp moq-net
  frame timestamps) stay untimed.

This also addresses the user's earlier review comment "unique_track
should probably return a String or something" — the new
`unique_name` is the String-returning variant, and codec importers
no longer need a mutating setter to attach the timescale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kixelated kixelated merged commit e7ba559 into dev Jun 1, 2026
1 check passed
@kixelated kixelated deleted the claude/moq-net-timestamp-support-ieCkL branch June 1, 2026 19:13
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