feat(moq-net)!: add a Latency type and standardize on latency_max - #2688
Merged
Conversation
kixelated
force-pushed
the
claude/latency-api-naming-f87982
branch
from
August 6, 2026 02:28
f72692c to
e838de7
Compare
`with_latency_max` was awkward English, and the `_max` suffix existed to leave
room for a future `latency_min` floor. A type carries that better than a suffix:
adding `min` later becomes an additive field rather than a second setter the
caller has to keep consistent with the first.
`moq_net::Latency` is that type, built via `Latency::max(d)` or
`Latency::REAL_TIME` (the default, which skips aggressively). It lives in moq-net
because that is where the concept is densest: `Subscription` carries the same
budget on the wire as `Subscriber Max Latency`, the publisher's cache already
enforces it in `evict_expired`, and `Subscription`'s own docs already described
the local receiver-side bound as the same budget. The reordering consumer that
enforces it locally is expected to follow moq-net-ward now that tracks are
timestamp-aware, so putting the type anywhere else meant moving it twice.
Two concepts share the `latency_max` spelling today and only one of them is this
type. `Subscription` is a *drift* budget (how stale before a group is skipped)
and becomes `Latency`; `track::Info::latency_max` is a *retention* bound (how
long the publisher keeps a group, the inverse of `Cache-Control: max-age`) and
stays a `Duration`. The draft separated these deliberately, so typing both alike
would re-conflate them. Being on the wire is not the criterion; which concept it
is, is. moq-srt's `latency` is likewise the SRT receive latency reused as a skip
threshold, so it stays a `Duration` and wraps at the boundary.
Everything consumer-side in moq-mux now takes `Latency`:
`container::Consumer::{with_latency, set_latency}`, the fmp4/mkv/ts/flv/h264/h265
exporters, `moq_{audio,video}::decode::Config`, and moq-rtmp, whose `latency` is
purely the moq-level skip ceiling. moq-mux re-exports the type.
No `impl From<Duration>`: a bare scalar collapses to a fixed jitter buffer in the
JS `Latency` union (floor included), while here it is a ceiling only, so the
conversion would mean two different things across the two languages.
`container::Producer::with_latency` is renamed `with_buffer`, since it is the one
knob that genuinely *adds* delay (publisher-side frame packing) and sat
confusingly next to the consumer ceiling under the same name.
Also standardizes the spelling on `latency_max`, which was already the 227-to-45
majority in Rust and matches the existing `latency_default` / `latency_bound` /
`--latency-max` / `latency-max=` names. The holdouts were the lite SUBSCRIBE
messages (and their js/net mirrors) plus a few libmoq and moq-cli internals; both
of those modules are private, so those renames are internal. No wire format, CLI
flag, or FFI record field changes, so no draft or binding updates are needed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kixelated
force-pushed
the
claude/latency-api-naming-f87982
branch
from
August 6, 2026 05:34
e838de7 to
ea30bc3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea30bc3bb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
kixelated
force-pushed
the
claude/latency-api-naming-f87982
branch
from
August 6, 2026 19:57
5484b88 to
de220b9
Compare
This was referenced Aug 12, 2026
kixelated
added a commit
that referenced
this pull request
Aug 12, 2026
`moq play` sets a `latency_max` field that no longer exists: #2688 renamed it to `latency` and retyped it as `Latency`, while #2697 added this call site against the old shape. Neither PR could see the other, and `dev` never runs the `--all-features` build outside a PR, so it landed red and now fails Check on every PR that targets it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
with_latency_maxwas awkward English, and the_maxsuffix existed only to leave room for a futurelatency_minfloor. A type carries that better than a suffix: addingminlater becomes an additive field rather than a second setter callers must keep consistent with the first.moq_net::Latency, built viaLatency::max(d)orLatency::REAL_TIME(the default, which skips aggressively). moq-mux re-exports it, so call sites there still readmoq_mux::Latency.impl From<Duration>: a bare scalar collapses to a fixed jitter buffer in the JSLatencyunion (floor included), while here it is a ceiling only, so the conversion would silently mean two different things across the two languages. The explicit constructor keeps that unrepresentable.container::Producer::with_latencybecomeswith_buffer. It is the one knob that genuinely adds delay (publisher-side frame packing) and sat confusingly next to the consumer ceiling under the same name.latency_max. It was already the 227-to-45 majority in Rust and matches the existinglatency_default/latency_bound/--latency-max/latency-max=names; there is nomin_latencyordefault_latencyanywhere, somax_latencywas an outlier rather than a competing convention.Why moq-net and not moq-mux
The concept is densest in moq-net, which already owns all three pieces:
Subscriptioncarries this budget on the wire asSubscriber Max Latency, the publisher's cache enforces it inevict_expired, andSubscription's own doc comment already described the local receiver-side bound as the same budget, just implemented a crate up. The reordering consumer that enforces it locally is expected to move moq-net-ward now that tracks are timestamp-aware (frame::Frame.timestamp,Info.timescale), so landing the type in moq-mux would have meant moving it twice, breaking the same six crates each time.Drift vs retention
Two things share the
latency_maxspelling and only one is this type:Subscription.latencyLatencyConsumerceilingLatencytrack::Info.latency_maxDurationThe draft separated these deliberately (it moved
Publisher Max Latencyto TRACK_INFO and redefined it as a retention bound, "the inverse of an HTTPCache-Control: max-age", whileSubscriber Max Latencystayed a delivery-time preference). Typing both alike would re-conflate them. Being on the wire is not the criterion; which concept it is, is. moq-srt'slatencyis likewise the SRT receive latency reused as a skip threshold, so it stays aDurationand wraps only at the moq-mux boundary.Latency::minis not added: nothing implements a floor yet, and shipping a field the consumer ignores would be dead config. Worth noting for later that a floor means holding frames that have arrived, which is playout pacing. If that turns out to be a player-only concern, this type stays ceiling-only and js/watch's{min, max}union is a genuinely different type that shares a name.Public API changes
Breaking (hence
dev):moq-net: newLatency(REAL_TIME,max(),merge(),pub maxfield,#[non_exhaustive]).track::Subscription::latency_max: Duration->latency: Latency;Subscription::with_latency_max->with_latency.track::Info::latency_maxandInfo::with_latency_maxare unchanged.moq-mux:container::Consumer::{with_latency_max, set_latency_max}->{with_latency, set_latency}, takingLatency.container::{fmp4, mkv, ts, flv}::Export::with_latencyandcodec::{h264, h265}::Export::with_latencychange parameter type.container::Producer::with_latency->with_buffer. Re-exportsmoq_net::Latency.moq-audio/moq-video:decode::Config::latency_max: Option<Duration>->latency: Latency(Latency::default()already means whatNonedid).moq-rtmp:DEFAULT_LATENCY,Config::latency,Play::with_latency,Client::with_latencychange toLatency.Internal only (not public surface):
moq_net::liteis a private module (mod lite;), andjs/netexports only./src/index.ts+./src/zod.tswith nolitere-export, so themax_latency->latency_max/maxLatency->latencyMaxrenames on the SUBSCRIBE messages break nothing downstream. Likewisemoq-cli'sSubscribeArgsandlibmoq'smoq_consume_{video,audio}parameter names (C has no named arguments).No wire format, no CLI flag, and no FFI record field changed.
--latency-maxwas already the flag name, andmoq_subscription.latency_max_ms/moq_track_info.latency_max_mskeep their flat_msshape (a tagged enum through uniffi and the C ABI is noise for a two-field bound).Cross-package sync
drafts/update. The rename is identifier-only; the spec's prose field names are untouched.moq-ffirecord change, sopy/,swift/,kt/,go/, anddoc/lib/{py,swift,kt,go,c}need nothing. Verified by grep: zero stale references underdoc/,py/,swift/,kt/,go/,cpp/,test/,demo/.rs/moq-net<->js/net: the message-level renames are mirrored.Subscription.latencyhas no JS counterpart to update, becausejs/net'strack.Subscriptionexposes onlypriorityand has never carried a drift budget; adding one is new feature work, not a mirror of this change.Rebase note
Rebased onto
origin/devafter #2630 landed.ExportSource::for_streamconflicted: #2630 switched it torequest_catalog()while this branch changed its latency parameter. Both are preserved. #2630 also added two tests in the same file passingDuration::ZEROintofor_video/for_video_raw/for_audio; since there is deliberately noFrom<Duration>, those are migrated toLatency::REAL_TIME.Test plan
just fix(no changes) andjust checkclean, including clippy and rustdoc at-D warnings.cargo clippy -p libmoq -p moq-ffi -p moq-gst --all-targets -- -D warningsclean (just checkdoes not select those).cargo nextest runover moq-net, moq-mux, moq-audio, moq-video, moq-cli, moq-rtmp, moq-srt, libmoq, moq-ffi, hang, moq-native: 1961 tests, all passing.@moq/net: 354 tests passing,bun checkclean.just test smoke-full. No wire, FFI, or gateway behavior changed, only identifiers and types.🤖 Generated with Claude Code
(written by Opus 5)