refactor(net)!: replace the Latency type with a plain max_age duration - #2955
Conversation
`Latency` (added in #2688) wrapped a single `Duration` field. 125 of its 164 uses were `Latency::max(..)`, a wrapper applied at the call site and unwrapped at the comparison sites, and `merge` had exactly one caller. Its remaining justification was room for a future `min`, but a minimum belongs at presentation, where there is a render clock to hold data against. A delivery layer has no instant to enforce it at, and holding arrived bytes back there only adds delay while blinding the receiver's own buffer. So the type collapses to the duration it always was, and the name follows what the field measures. `latency` promised an end-to-end budget the field cannot carry: a subscriber's total is the publisher's emission cadence plus this value, so `latency: 300ms` against a 2s-cadence stream is 2.3s. `max_age` claims nothing about the total, and the implementation already reached for the name on its own (`evict_expired(max_age)`). - `Subscription::latency: Latency` -> `max_age: Duration` - `track::Info::latency_max` -> `max_age`, `DEFAULT_LATENCY_MAX` -> `DEFAULT_MAX_AGE` - `origin::Info::latency_default` -> `default_max_age` (`cache_duration` keeps its name: it pairs with `pool` as the origin's two cache budgets) - `Latency::REAL_TIME` -> `Duration::ZERO`, `Latency::merge` -> `Duration::max` - moq-lite `Subscriber/Publisher Max Latency` -> `Max Age` in the draft, wire bytes unchanged; mirrored in js/net The type's docs move onto `Subscription::max_age`, which is where a reader looking for them now lands. The moq-rtmp gateway configs carried both fields under one name once renamed, so they split into `import_max_age` (retention on tracks the gateway mints) and `export_max_age` (the FLV muxer's skip threshold). `import`/`export` rather than `play`/`publish` because those invert between the server and the dialing client. CLI flags are untouched: the two `#[arg(long)]` fields that derived their spelling from the field name now pin `long = "latency-max"` explicitly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parent commit renamed `latency_max` to `max_age` in moq-net, but stopped at the binding boundary: `moq-ffi` and `libmoq` kept calling the same value `latency_max_ms`. That left the FFI naming a field after a budget it does not carry, and disagreeing with the crate it wraps. - `MoqTrackInfo`, `MoqSubscription`, `MoqAudioDecoderOutput`, `MoqVideoDecoderOutput`: `latency_max_ms` -> `max_age_ms` - `moq_track_info`: `latency_max_ms`/`latency_max_valid` -> `max_age_ms`/`max_age_valid` - `moq_subscription`, `moq_video_decoder_output`, `moq_audio_decoder_output`: `latency_max_ms` -> `max_age_ms` - `moq_consume_video` / `moq_consume_audio` take `max_age_ms` The Python, Swift, Kotlin, and Go bindings are generated and gitignored, so they pick the rename up on the next build. The hand-written wrappers on top of them are not, so `py/moq-rs`, `go/wrapper`, and `swift/Sources/Moq` are updated here, along with `doc/lib/c` and the smoke client. Two doc comments promised a future `latency_min_ms` to justify the `_max` suffix. A minimum here is a jitter-buffer floor, not a staleness bound, so they now name it `min_buffer_ms` and say why the two differ. This is a source-breaking change for C consumers: the struct layouts are unchanged, but the field names move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lid`
`_valid` was doing two unrelated jobs in one header.
On the read-back structs it is right: `moq_connection_stats` has the library
set `rtt_valid` to say the backend does not report RTT, so the value is
meaningless. That is a statement about the data.
On the input structs it is wrong. The caller sets `max_age_valid`, and is not
claiming the value is valid; it is saying "I supplied this, use it instead of
your default." Validity never enters into it, and `group_start`'s own doc
comment already said "whether `group_start` is present" while the field said
`_valid`.
So the four caller-supplied flags become `_present` and the nine library-filled
stats flags keep `_valid`, which turns an overloaded suffix into a signal for
which direction a field flows:
- `moq_track_info`: `max_age_valid`/`timescale_valid` ->
`max_age_present`/`timescale_present`
- `moq_subscription`: `group_start_valid`/`group_end_valid` ->
`group_start_present`/`group_end_present`
`_default` was the other candidate and is a trap: a zero-initialized struct
would read `max_age_default = false`, which says "not the default" when `{0}`
has to mean exactly that. Fixing it needs inverted polarity, which breaks the
documented guarantee that a zeroed struct gets defaults.
Source-breaking for C consumers alongside the `max_age_ms` rename in the parent
commit, so the two land together and callers edit once.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf67d8de0f
ℹ️ 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".
| pub use moq_net::Latency; | ||
| pub use source::Source; | ||
| /// Re-export of [`std::time::Duration`], the drift budget every consumer-side knob here takes. | ||
| pub use std::time::Duration; |
There was a problem hiding this comment.
All affected APIs now accept the standard std::time::Duration, which callers can import directly, so re-exporting it as moq_mux::Duration adds a second, misleading public spelling with no abstraction benefit. Because downstream users may adopt that spelling, removing it later becomes another breaking API change; keep the crate's public surface limited to its own types. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L143-L147
Useful? React with 👍 / 👎.
bf67d8d to
434e41f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 434e41f8ff
ℹ️ 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".
| Use ``output.max_age_ms`` to | ||
| control how aggressively stalled groups get skipped. That's | ||
| the congestion-control knob. (Named ``_max`` to leave room for | ||
| a future ``latency_min_ms`` jitter-buffer floor.) |
There was a problem hiding this comment.
After renaming the option to max_age_ms, this published Python docstring still says its name reserves room for a future latency_min_ms. That rationale belongs to the removed latency naming and now contradicts the updated FFI docs, which describe a distinct future min_buffer_ms presentation knob. Remove the migration rationale so consumers are not guided toward an obsolete API shape. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L104-L105
Useful? React with 👍 / 👎.
| - Restricted the GOAWAY New Session URI to servers, specified a duplicate GOAWAY as a protocol violation, and recommended scheme continuity and sticky redirects. | ||
| - Exempted a ceiling-cost serving path from the actively-carrying cost discount: a relay whose serving path costs the saturation ceiling (primarily a session that received a GOAWAY) advertises the ceiling instead of 0, so the drain propagates downstream instead of being re-masked by each carrying hop. Keyed on the value, not the reason, which does not travel on the wire. | ||
| - Added the Error Codes section, defining separate session and stream code spaces and listing the codes moq-lite uses, reused unchanged from moq-transport. Codes 64+ are the application's; 32-63 are reserved and MUST NOT be interpreted, pending a future revision. Previously the codes were unspecified, so an endpoint could neither send one a peer would understand nor safely interpret one it received. Note this renumbers every code an existing implementation sent, and that a stream reset of 0x0 is now INTERNAL_ERROR rather than a cancellation (CANCELLED is 0x1). | ||
| - Renamed `Subscriber Max Latency` to `Subscriber Max Age` and `Publisher Max Latency` to `Publisher Max Age`. Both already measured a group's age, and neither bounds end-to-end latency: a subscriber's total is the publisher's emission cadence plus this budget, so the old name promised something the field does not carry. |
There was a problem hiding this comment.
For the in-progress moq-lite-06 changelog, the second sentence adds design motivation about emission cadence and end-to-end latency instead of only recording the rename. Reduce this bullet to the factual name change, leaving that rationale to the surrounding specification or PR description as required by the draft conventions. (Written by GPT-5.6 Sol)
AGENTS.md reference: drafts/AGENTS.md:L77-L81
Useful? React with 👍 / 👎.
Three fixes from the Codex review pass on #2955: - `moq-mux` no longer re-exports `Duration`. The mechanical sweep rewrote `pub use moq_net::Latency;` into `pub use std::time::Duration;`, which added `moq_mux::Duration` as a second public spelling of a std type with no abstraction behind it. Nothing referenced it, and every affected API now takes `std::time::Duration` directly. - The Python `decode_audio` docstring still justified the `_max` suffix with a future `latency_min_ms`. The Rust FFI docs were updated to `min_buffer_ms` and this one was missed, so the two published surfaces disagreed. - The moq-lite-06 changelog bullet carried two sentences of design rationale. `drafts/CLAUDE.md` asks for what changed, not why; the reasoning lives in the commit message and PR description. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Collapses
Latencyinto the plainDurationit always wrapped, and renames it to say what it measures.Why the type goes
Latency(#2688) had a singlemax: Durationfield. On currentdev:Latency::max(..)— a wrapper applied at the call site and unwrapped at the comparison sites.mergehas exactly one caller, and it isDuration::max.min. A minimum belongs at presentation, where there is a render clock to hold data against. A delivery layer has no instant to enforce one at, and holding already-arrived bytes there only adds latency while blinding the receiver's own buffer, which is better placed because it knows its render clock. Theminis not coming, so the struct is ceremony.The type's docs are not lost: #2890 had grown them into a real explanation of where the budget is enforced and how age is measured, so they move onto
Subscription::max_age.Why
max_agelatencypromised an end-to-end budget the field cannot carry. A subscriber's total is the publisher's emission cadence plus this value, solatency: 300msagainst a 2s-segment stream is 2.3s end-to-end.max_ageclaims nothing about the total, and the implementation had already reached for the name on its own (evict_expired(max_age)).Cache-Control: max-ageis the same concept, same units, same zero-means-immediately-stale convention — and unlikeExpires:it is a delta, not a date.It also makes the clamp self-explaining: you cannot wait longer than the publisher keeps it.
Renames
Subscription::latency: LatencySubscription::max_age: Durationtrack::Info::latency_maxtrack::Info::max_agetrack::DEFAULT_LATENCY_MAXtrack::DEFAULT_MAX_AGEorigin::Info::latency_defaultorigin::Info::default_max_ageLatency::REAL_TIME/mergeDuration::ZERO/Duration::maxwith_latency/with_latency_maxwith_max_agelatency_max_msmax_age_mslatency_max_validmax_age_presentorigin::Info::cache_durationkeeps its name: it pairs withpoolas the origin's two cache budgets (age and bytes).The moq-rtmp gateway configs carried two of these fields, which collided once both became
max_age. They split intoimport_max_age(retention on tracks the gateway mints) andexport_max_age(the FLV muxer's skip threshold) —import/exportrather thanplay/publish, because those two invert between the server and the dialing client.Wire and bindings
Wire bytes are unchanged.
draft-lcurley-moq-literenamesSubscriber/Publisher Max LatencytoMax Ageunder the WIP moq-lite-06 section; published changelog sections keep their historical names.js/netmirrors the rename so both implementations of the spec agree.The Python, Swift, Kotlin, and Go bindings are generated and gitignored, so they pick this up on the next build. The hand-written wrappers over them are not, so
py/moq-rs,go/wrapper,swift/Sources/Moq,doc/lib/c, and the smoke client are updated here._valid->_presenton the C input structsWhile renaming
latency_max_valid, it became clear_validwas doing two unrelated jobs in one header.On the read-back structs it is correct:
moq_connection_statshas the library setrtt_validto say the backend does not report RTT, so the value is meaningless. That is a statement about the data.On the input structs it is wrong: the caller sets
max_age_valid, and is not claiming the value is valid — they are saying "I supplied this, use it instead of your default."group_start's own doc comment already read "whethergroup_startis present" while the field said_valid.So the four caller-supplied flags become
_present(max_age,timescale,group_start,group_end) and the nine library-filled stats flags keep_valid. The suffix now tells you which direction a field flows._defaultwas the other candidate and is a trap: a zeroed struct would readmax_age_default = false, i.e. "not the default", when{0}must mean exactly that. Fixing it requires inverted polarity, which breaks the documented guarantee that a zero-initialized struct gets defaults.Breaking
moq-net,moq-mux, the gateways,moq-ffi, andlibmoq.libmoqconsumers.#[arg(long)]fields derived their spelling from the field name, so they now pinlong = "latency-max"explicitly. Renaming the flags is a separate user-facing decision, and under the fix(cli)!: refuse the renamed flags with a migration instead of ignoring them #2915 policy it would need the old spellings to keep parsing.Deliberately out of scope
The catalog
jitterfield. It is not a latency bound at all — it is the publisher's worst-case emission gap, a measured stream property, and a player must add it to its own buffer. Renaming it (cadence) is a catalog wire change acrosshang,moq-msf, and the hang draft, so it gets its own PR. WithLatencygone it is the last name in the stack pointing at the wrong concept.Testing
just checkandjust testpass, plusjust py check/test(52 pass),just go check, all 15 JS packages, andjust drafts check.(written by Opus 5)