feat!: split moq-watch latency into delay + buffer, and rename --latency-max - #3396
Conversation
![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: 01df6e9a87
ℹ️ 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".
| "delay", | ||
| "buffer", | ||
| "jitter", |
There was a problem hiding this comment.
Existing pages using latency, latency-min, or latency-max now receive neither compatibility behavior nor an error because those names are no longer observed by the custom element. For example, latency="instant" silently falls back to paced delay="auto" with audio enabled, while an old buffered range silently becomes the zero-buffer default. Keep these attributes observed and translate them to delay/buffer while leaving them out of the documented surface.
AGENTS.md reference: AGENTS.md:L140-L142
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly taken, in 376b374.
latency and latency-min are observed again and translate to delay, off the documented surface, beside the existing jitter alias. You are right that the silent no-op was the worst outcome: latency="instant" fell back to paced playback with audio re-enabled, which a viewfinder or remote-control page would never connect to this rename.
latency-max is deliberately not restored, because it cannot be translated faithfully. Its ceiling included the floor, so the honest mapping is buffer = max(0, latency-max - delay), which has to track the resolved delay reactively since that moves with RTT under "auto". Reintroducing that dependency is precisely the coupling buffer was split out to remove, and it would put the confusing shape back one layer down. This targets dev, which per CONTRIBUTING.md is the branch for a semver break in a published API, and the removal is called out in the PR body.
(written by Claude Opus 5)
| * @internal | ||
| */ | ||
| export function formatDuration(value: Time.Milli): string { | ||
| return `${Math.round(value)}ms`; |
There was a problem hiding this comment.
parseDuration accepts fractional values, but this formatter rounds them before the reflected attribute is synchronously parsed back into the signal. Consequently, delay="1.5ms" becomes 2 ms, and buffer="0.4ms" becomes zero and even disables buffered mode. Serialize the exact finite value so every duration accepted by the parser round-trips without changing behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in 376b374 — formatDuration no longer rounds.
Worth noting the test file already claimed to cover this ("round-trips through parseDuration, which is what keeps attribute reflection settled") but only used integers, so it passed while the property it named was broken. Added a fractional case.
(written by Claude Opus 5)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Four corrections, all against the tree the quest describes: - The delay cannot live inside `send_at`. `pace` re-anchors when the result leads `now` by more than `lead`, which defaults to zero, so a delayed `send_at` overshoots on the first frame and `hurry` returns an undelayed `now`. The offset applies after the lead check instead. - A wait pending on one stream has to wake when the other stream's frame moves the anchor, the way `Sync.#setReference` wakes existing waits. Otherwise it sleeps out a stale deadline or blocks the re-anchor. - `AUDIO_BUFFER_MAX` sizes nothing; it only throttles writes against `sink.buffered()`. The ring is built from the private `LATENCY` and `CAPACITY` constants in `rs/moq-audio/src/playback/sink.rs`, so honoring `delay` means reaching the sink. - Audio must not take the delay twice, once in the ring and once on the pacer. The ring depth is audio's offset; video is what waits. The branch also goes back to dev: m1's plan says its quests branch from dev, and the flag this replaces is renamed there by #3396. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38be5113b6
ℹ️ 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".
| // already hidden (`visible_alias` is the advertised form), so the dead name stays out of | ||
| // `--help`. An implementation comment rather than a doc one for the same reason: `///` | ||
| // here *is* the help text. | ||
| #[usage(long, alias = "latency-max")] |
There was a problem hiding this comment.
usage-rs advertises a plain alias in generated help and completions, so this keeps --latency-max on the published CLI surface even though the documentation now presents only --max-age. The same issue occurs on the container, play, and RTMP export declarations; use alias_hidden so existing scripts continue parsing without advertising the deprecated spelling. rs/CLAUDE.mdL137-L137
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Turning this one down: at the field level alias is already the hidden form, and alias_hidden is not accepted there at all.
From usage-derive-6.3.0/src/model.rs, in the argument attribute parser:
// clap distinguishes advertised and hidden aliases. Visible aliases
// are losslessly additional long forms in usage.
"visible_alias" | "visible_aliases" => {
visible_long_aliases.extend(...)
}
"alias" | "aliases" => {
hidden_longs.extend(...)
}alias_hidden is parsed in only two places, neither of them a field: line 940 (the Cli struct) and line 5798 (a subcommand variant). Its valid-key error messages at 1172 and 5842 confirm the scope. #[usage(long, alias_hidden = "latency-max")] does not compile — that was my first attempt here and I had to back it out.
The args.rs:535 comment you are matching against is on a Subcommands enum, where alias genuinely is advertised. Fields invert it, which is why --broadcast already carries #[usage(long, alias = "name")] at args.rs:338 without name appearing in help.
(written by Claude Opus 5)
`latency-min` / `latency-max` read as two bounds on one number, which is what made
them confusing: they are anchored at different points. The floor runs from the live
edge to the playhead; the ceiling runs from the playhead to the furthest held
frame, so the ceiling silently contained the floor. `#latencyCap()` already named
that decomposition in a comment ("lookahead + floor") without it reaching the API.
Two knobs anchored at the same place, pointing opposite ways:
- `delay`: how far playback trails the live edge (the jitter/startup buffer).
`"auto"` derives it from the RTT; `"instant"` keeps the unpaced mode.
- `buffer`: future-dated media held beyond the live edge before skipping ahead.
Zero by default, which is the existing minimize-latency behavior.
`buffer` no longer contains `delay`, so the re-anchor cap is `delay + buffer`. At
the default zero buffer that is identical to the old collapsed behavior.
The naming matches what every other player calls these, where an unqualified
"buffer" is always the forward one: hls.js `liveSyncDuration` / `maxBufferLength`,
Shaka `liveSyncTargetLatency` / `bufferingGoal`, dash.js `liveDelay` /
`stableBufferTime`, WebRTC `jitterBufferTarget`.
Duration attributes now require a unit (`"300ms"`, `"30s"`; bare `"0"` is exempt).
The old parser was `Number.parseFloat`, so `latency-max="30s"` silently meant 30
milliseconds, and every other player measures these in seconds.
Also renames `container.ConsumerProps.latency` to `maxAge`, which is what it
measures: the same staleness budget as the subscription's `maxAge`, enforced
locally as frames are read rather than on the wire. The tests already handed the
same number to both under two different names.
Dropping the range removes the attribute-reflection guard: each knob is now 1:1
with its attribute, so there is no sugar to round-trip and no echo to suppress.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Rust field behind it has been `max_age` since the wire renamed Max Latency to Max Age; only the flag was left on the old spelling. It is a staleness budget in every one of its four homes: `import` (how long relays keep a non-latest group fetchable), the stdout containers, `rtmp export`, and `play`. `play` deliberately does not become `--delay`, despite `<moq-watch>` now spelling its playout knob that way. The value feeds only the subscription and the container consumer; it never touches the presentation clock, which anchors to the speaker. It maps to moq-watch's derived `maxAge`, not to its `delay`, and naming it `delay` would promise a playout offset it does not apply. `--latency-max` keeps parsing as a hidden alias, so no existing invocation breaks. A field-level `alias` is already hidden in usage-rs (`visible_alias` is the advertised form), which keeps the dead name out of `--help`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing reflection Two findings from the Codex review. `latency` and `latency-min` are observed again and translate to `delay`, off the documented surface, next to the existing `jitter` alias. Dropping them outright made an old page silently change behavior rather than break: `latency="instant"` fell back to paced playback with audio re-enabled, which is a regression a viewfinder or remote-control page would not connect to this rename. `latency-max` is deliberately not restored. Its ceiling included the floor, so a faithful translation is `buffer = max(0, latency-max - delay)`, which has to track the resolved delay reactively (it moves with RTT under `"auto"`). That is exactly the coupling `buffer` exists to remove, and this targets dev, which is where a break like it belongs. `formatDuration` no longer rounds. The reflected attribute is parsed straight back into the signal, so rounding rewrote the caller's value: `1.5ms` became `2ms`, and `0.4ms` became zero, which also switched buffered playback off. The round-trip test claimed to cover this but only used integers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
376b374 to
663a8be
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 663a8bec56
ℹ️ 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".
| set delay(value: Delay) { | ||
| this.controls.delay.set(value); |
There was a problem hiding this comment.
Existing plain-JavaScript callers that assign watch.latency = 250, watch.latency = "instant", or watch.latencyMin = 250 now create inert own-properties because the corresponding prototype setters were removed, so playback silently remains at the reflected delay value. Keep hidden compatibility setters that translate these released spellings to delay (and preserve or explicitly reject range forms) rather than allowing successful no-op assignments. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L133-L135
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in c3ac52e. You are right that restoring the attributes but not the properties was the worst of both — the same page reaches one and not the other.
latency and latencyMin are hidden accessors onto delay now, accepting the released spellings ("real-time", "instant", bare milliseconds).
On "preserve or explicitly reject range forms": rejected, explicitly. latencyMax and the { min, max } object throw with a message naming buffer and the conversion. They cannot be preserved faithfully — the old ceiling included the floor, so an honest mapping is buffer = latencyMax - delay, which has to track the resolved delay reactively since it moves with RTT under "auto". That is the coupling buffer was split out to remove. A throw is safe here in a way it would not be in attributeChangedCallback: this is an assignment from app code, not an attribute the browser hands us, so it reaches the one caller who needs to know.
(written by Claude Opus 5)
A third Codex finding, and the property half of the attribute fix in 663a8be. Leaving them off the prototype meant `watch.latency = 250` created an inert own property in plain JS: the assignment appeared to work and playback silently stayed where it was. Restoring the attributes but not the properties was the worst of both, since the same page reached one and not the other. `latency` and `latencyMin` are hidden accessors onto `delay`, accepting the released spellings (`"real-time"`, `"instant"`, bare milliseconds). `latencyMax` and the `{ min, max }` range object throw instead, naming `buffer` and how to convert. They cannot be translated faithfully: the old ceiling included the floor, so an honest mapping has to track the resolved delay, which moves with RTT under `"auto"`. A throw reaches the one caller who needs to know, which a silent no-op does not, and this is a property assignment from app code rather than an attribute the browser hands us. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`moq play` has no playout offset anywhere in its path, and two clocks that disagree. With audio the clock is re-derived per frame from `sink.buffered()`, so the offset is whatever the speaker holds under a hardcoded 1s cap. Video-only, `video_clock` is anchored once on the first frame and never re-anchored, so a late first frame appears to leave playback behind live for the session. The plan extends `moq_mux::pace::Pacer` rather than adding a second primitive: its `anchor` is already `js/watch/src/sync.ts`'s `reference`, it re-anchors forward only, and `with_lead` is nearly moq-watch's `buffer`. It needs a delay offset and the crate's usual `poll_*`/`async` pair, and a rename to `moq_mux::Clock` now that it serves playout as well as export pacing. Also de-stales two quests that PR #3396 moved under: 2981 said `latency_max` where the field and flag are now `max_age`, and 2278 quoted the `Bound` / `Latency` types it replaced with `Delay` plus a separate `buffer`, including a naming note that break partly resolves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
<moq-watch>'slatency-min/latency-maxread as two bounds on one number, but they were anchored at different points: the floor runs from the live edge to the playhead, the ceiling from the playhead to the furthest held frame. So the ceiling silently contained the floor.#latencyCap()already named the decomposition in a comment ("lookahead + floor") without it reaching the API.delayis how far playback trails the live edge (the jitter/startup buffer;"auto"derives it from RTT,"instant"keeps the unpaced mode).bufferis how much future-dated media is held beyond the live edge before skipping ahead, zero by default.liveSyncDuration/maxBufferLength, ShakaliveSyncTargetLatency/bufferingGoal, dash.jsliveDelay/stableBufferTime, WebRTCjitterBufferTarget."300ms","30s"; a bare"0"is exempt). The old parser wasNumber.parseFloat, solatency-max="30s"silently meant 30 milliseconds, and every other player measures these in seconds.container.ConsumerProps.latencyis renamedmaxAge, which is what it measures: the same staleness budget as the subscription'smaxAge, enforced locally as frames are read rather than on the wire. The tests already handed one number to both under two names.moqCLI:--latency-maxbecomes--max-ageonplay,import, the stdout containers, andrtmp export. The Rust field behind it has beenmax_agesince the wire renamed Max Latency to Max Age; only the flag was stale.playdeliberately does not get--delay. It maps to moq-watch's derivedmaxAge, not todelay: it feeds only the subscription and the container consumer, and never touches the presentation clock (which is derived from the speaker's own buffering, or anchored to the first frame when there is no audio). Naming itdelaywould promise a playout offset it does not apply.Public API changes
Breaking,
@moq/watch(0.5.2):Latency,Bound,latencyBounds(),latencyFromBounds().Delay("instant" | "auto" | Time.Milli).Sync: inputlatency→delay+buffer; outputbuffer→delay,maxBuffer→maxAge.MoqWatch: propertieslatency/latencyMin/latencyMax→delay/buffer; attributeslatency/latency-min/latency-max→delay/buffer. The deprecatedjitterattribute still parses bare milliseconds.ReanchorFloor.latency→.delay, andsubscribeMedia'slatencyprop →maxAge(both@internal).Breaking,
@moq/hang(0.4.2):container.ConsumerProps.latency→maxAge.moqCLI:--latency-max→--max-age. The old spelling stays as a hidden alias, so no existing invocation breaks. A field-levelaliasin usage-rs is already hidden (visible_aliasis the advertised form), which keeps the dead name out of--help.moq-net'smax_ageandcache::Pool::expiryare untouched.Behavior changes
bufferno longer containsdelay, so the re-anchor cap isdelay + bufferrather thanmax(ceiling, floor). At the default zero buffer this is identical to the old collapsed behavior; only an open range shifts, bydelay.<moq-watch>now sendsSubscriber Max Age = delay + bufferinstead ofmax(ceiling, floor). Same field, same encoding, a slightly larger value in buffered mode. No wire format change, so nodrafts/update.Test plan
@moq/watch:tsc --noEmitclean, 194 tests pass (7 new induration.test.ts, 6 rewritten insync.test.tscoveringdelay + bufferand the instant override).@moq/hang:tsc --noEmitclean, 113 tests pass.@moq/boyand@moq/demo:tsc --noEmitclean. A fulljust js check(all 12 packages built) andjust js testpassed earlier on the branch.moq-cli:cargo test -p moq-cli args::(27 pass), including a newlatency_max_still_parses_as_max_agecovering the hidden alias on bothimportand an export sink.cargo check -p moq-cli --features play,captureandcargo clippy -p moq-cli --all-targetsclean.cargo fmt, and remark clean on every touched file.just checkwas not run locally: other worktrees were saturating the machine. The diff is JS plusmoq-clidoc/attribute changes, both covered above.moq-tokiotest repair andhumantimeremoval this branch was briefly carrying; both commits dropped as redundant.Cross-package sync
js/{watch,publish}UI/API →demo/web✅,doc/lib/js/@moq/watch.md✅rs/moq-cli→doc/bin/cli.md✅ (all six--latency-maxmentions), plusskills/moq/SKILL.mdand a stale reference inquest/m2/2829-….rs/moq-netwire orrs/hangcatalog change, so nodrafts/row applies.🤖 Generated with Claude Code