chore(quest): plan the native playout clock - #3398
Conversation
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. |
![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: 776404c7b8
ℹ️ 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".
| nearly `moq-watch`'s `buffer`. The only missing piece is a delay offset, so | ||
| extend the primitive rather than adding a parallel one beside it. | ||
|
|
||
| - Rename `Pacer` to `Clock` (`moq_mux::Clock`), since it now serves playout as |
There was a problem hiding this comment.
The reviewed tree already exports an unrelated moq_mux::Clock from rs/moq-mux/src/clock.rs, and that capture clock is used throughout moq-audio, moq-video, and moq-cli. Renaming Pacer to the same root symbol therefore cannot be completed as the listed three-call-site rename: it either creates a duplicate re-export or requires renaming a second published API and all of its consumers. Reconcile the name and scope before this quest is executable. (Written by GPT-5.6 Sol)
AGENTS.md reference: quest/AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| - Size the audio sink to `delay` rather than the hardcoded `AUDIO_BUFFER_MAX`, | ||
| mirroring `ringSamples(rate, delay)` in `js/watch/src/audio/latency.ts`. Keep a | ||
| floor the way `ringSamples` does: a zero-depth ring can never be read from. | ||
| - `--delay <duration>` replaces `--max-age` on `play`, with no alias. The two are |
There was a problem hiding this comment.
For existing moq play invocations, removing the old spelling with no alias produces only a generic unknown-argument failure. The reviewed tree currently calls that flag --latency-max, not --max-age, so the plan should identify the actual spelling and retain it as a hidden alias or parsable rejection that names --delay; otherwise users receive neither compatibility nor an actionable migration. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L140-L142
Useful? React with 👍 / 👎.
| - `--delay <duration>` replaces `--max-age` on `play`, with no alias. The two are | ||
| one number here, since nothing older than `delay` is worth presenting, so | ||
| `delay` is what goes on the wire as the subscription's max age. `--max-age` | ||
| stays unchanged on `import`, the stdout containers, and `rtmp export`. |
There was a problem hiding this comment.
When this rename is implemented, doc/bin/cli.md will still describe play --latency-max and contains an example invocation using it. Because the plan explicitly removes the previous flag, those instructions will stop parsing unless the quest includes a repository-wide reconciliation of moq play examples and the primary CLI page. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L226-L226
Useful? React with 👍 / 👎.
| #### Naming note | ||
|
|
||
| `Latency` is already doubly overloaded: the `Bound | {min,max}` type (`js/watch/src/sync.ts:16`) and `class Latency` (`js/hang/src/util/latency.ts:21`, jitter+buffer). Different packages, both exported. Worth resolving before adding a third meaning. | ||
| Mostly resolved: `js/watch`'s `Latency` / `Bound` types are gone, replaced by `Delay` plus a separate `buffer`, leaving `class Latency` (`js/hang/src/util/latency.ts`, jitter+buffer) as the only remaining use. Pick a name for the absolute mode that does not reintroduce the collision. |
There was a problem hiding this comment.
In the reviewed tree, js/watch/src/sync.ts still exports Bound and Latency, and SyncInput still has a latency getter; neither the quoted Delay type nor a separate buffer input exists. Rewriting this main-targeted quest around that absent API, while declaring the collision resolved and adding no dependency on its refactor, leaves the plan unusable if selected now. Keep the current model in the plan or add the refactor as an explicit prerequisite. (Written by GPT-5.6 Sol)
AGENTS.md reference: quest/AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca7ce223a8
ℹ️ 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".
| - [#3086](/quest/m1/3086-refactor-net-make-group-delivery-order-a-handle-and-move.md) - refactor(net): make group delivery order a handle, and move timestamp-based skipping into moq-net | ||
| - [#3161](/quest/m1/3161-retention-should-reclaim-idle-open-groups-now-that-expiry.md) - Retention should reclaim idle open groups now that expiry is timestamp-only | ||
| - [Group overflow](/quest/m1/group-overflow-abort.md) - an oversized open group aborts for every reader instead of shedding its head | ||
| - [Playout clock](/quest/m1/playout-clock.md) - moq play presents on a clock it controls, with a `--delay` offset and forward re-anchoring |
There was a problem hiding this comment.
This entry is added to m1, whose plan says every child quest branches from dev, while the new quest explicitly says to branch from main; the repository's branch rule also confirms that additive API work belongs on main. Selecting this quest from the ordered m1 list therefore gives contradictory execution instructions. Move it to the main-oriented feature milestone, or deliberately retarget the quest to dev. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L228-L230
Useful? React with 👍 / 👎.
| - Add the offset: `send_at = anchor + (ts - base) + delay`, defaulting to zero | ||
| so the three existing call sites (`rs/moq-srt/src/server.rs` twice, and | ||
| `rs/moq-cli/src/subscribe.rs`) need no change at all. |
There was a problem hiding this comment.
With the current Pacer, pace accepts an instant only when send_at - now <= lead, while lead defaults to zero and hurry returns now. Merely adding delay to send_at therefore makes the first delayed frame exceed the default lead and discards the offset, and every later re-anchor also returns an undelayed instant. Specify how the lead comparison excludes or incorporates the fixed delay and how hurry preserves it, rather than exposing two public knobs whose interaction defeats the promised behavior. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L168-L172
Useful? React with 👍 / 👎.
| - Size the audio sink to `delay` rather than the hardcoded `AUDIO_BUFFER_MAX`, | ||
| mirroring `ringSamples(rate, delay)` in `js/watch/src/audio/latency.ts`. Keep a | ||
| floor the way `ringSamples` does: a zero-depth ring can never be read from. |
There was a problem hiding this comment.
AUDIO_BUFFER_MAX does not size the audio sink: play.rs only compares it with sink.buffered() to throttle writes, while the actual ring uses the private LATENCY = 50ms and CAPACITY = 3s constants in rs/moq-audio/src/playback/sink.rs. Replacing that high-water mark with delay leaves the real audio buffering unchanged, so audio cannot reliably honor the requested playout offset. Include a lower-layer sink configuration or another mechanism that controls when queued samples reach the speaker. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L150-L153
Useful? React with 👍 / 👎.
|
Thanks — three of the four are addressed, one is a deliberate call. Choose a non-conflicting name — agreed, fixed in Keep the old play flag parsable — agreed, fixed in Add the CLI documentation migration — agreed, fixed in the same commit. The quest now calls for reconciling Declare the absent Delay refactor as a blocker — declining, but the underlying observation is right. #3396 is what introduces (written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f294731025
ℹ️ 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".
| - Add the pair the rest of the crate uses: `poll_wait(&kio::Waiter, ts) -> | ||
| Poll<()>` with an `async fn wait(ts)` wrapping it, per the Async / poll | ||
| plumbing section of `rs/CLAUDE.md`. |
There was a problem hiding this comment.
When audio and video share one Pacer, one task can already be pending in wait(ts) when an earlier-arriving frame on the other stream moves the anchor forward. The current Pacer only calculates an instant per call, while Sync.#setReference wakes every existing wait so it can recompute against the new reference. Without an equivalent notification or generation mechanism, the proposed wait either remains asleep until its stale deadline or holds exclusive state and prevents the re-anchor, defeating catch-up for already queued frames. Include that mechanism and a regression test where another stream shortens an active wait. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L152-L156
Useful? React with 👍 / 👎.
| - Delete the local `Clock` struct and `video_clock`, and route both audio and | ||
| video through the one `moq_mux::Pacer`. The pacer's anchor becomes the single | ||
| authority; the speaker-derived clock goes away. | ||
| - Size the audio sink to `delay` rather than the hardcoded `AUDIO_BUFFER_MAX`, | ||
| mirroring `ringSamples(rate, delay)` in `js/watch/src/audio/latency.ts`. Keep a |
There was a problem hiding this comment.
If routing audio through the newly proposed Pacer::wait means waiting for line 41's delayed send_at before calling Sink::write, a sink sized to the same delay then holds those samples for roughly another delay before the speaker consumes them. Video waits only on the pacer, so with --delay=100ms audio can trail it by about another 100 ms. Specify that audio feeds the ring ahead of its deadline and only updates the shared anchor, or subtract the sink depth from the audio write deadline, and verify the resulting A/V offset. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L156-L156
Useful? React with 👍 / 👎.
|
All five new findings verified against the tree; four fixed in Preserve the delay through re-anchoring — agreed, and this was the real bug in the plan. Wake pending waits when the anchor moves — agreed. Configure the actual audio ring — agreed, and the plan was factually wrong. Avoid applying the delay twice to audio — agreed, and it falls out of the above. The ring depth is audio's playout offset, so audio writes ahead of its deadline and only reports the anchor; video, with no buffer downstream, is what waits on the pacer. The A/V offset is now something the quest verifies rather than assumes. Move the main-targeted quest out of the dev milestone — agreed, resolved the other way. Declare the absent Delay refactor as a blocker — declining, unchanged from the previous round. #3396 is the refactor, it targets (written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 222be14a60
ℹ️ 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".
| - Do not delay audio twice. The ring depth *is* audio's playout offset, so audio | ||
| writes ahead of its deadline and only reports the anchor; if it also slept on | ||
| the pacer it would trail video by roughly another `delay`. Video, which has no | ||
| such buffer downstream, is what waits on the pacer. Verify the resulting A/V |
There was a problem hiding this comment.
When audio and video first decode at different wall times, this flow never schedules audio against the shared Pacer: playback::Sink::write explicitly has no clock scheduling, and ring depth only controls queued PCM. For example, if audio timestamp 0 arrives 50 ms after video timestamp 0, that late timestamp does not re-anchor the pacer, while the speaker starts according to when its ring fills, so the streams retain the startup skew. Keep the speaker position in the clock calculation or make the lower-level sink timestamp/deadline aware, and cover staggered audio/video startup with a regression test. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L152-L153
Useful? React with 👍 / 👎.
|
|
||
| ## Related | ||
|
|
||
| - [Playout clock](/quest/m1/playout-clock.md) - ports the same anchoring model to Rust as `moq_mux::Clock` |
There was a problem hiding this comment.
This new related entry says the Rust model uses moq_mux::Clock, but the linked quest explicitly keeps Pacer and identifies Clock as the unrelated capture epoch. Leaving the stale type name here reintroduces the exact collision the plan resolves and can send an executor toward the wrong primitive; describe the port as using moq_mux::Pacer. (Written by GPT-5.6 Sol)
AGENTS.md reference: quest/AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
|
Two new findings this round, both fixed in Anchor audio output to the shared playout clock — agreed, and this is the better answer than what I wrote last round. Saying audio "writes ahead and only reports the anchor" left staggered startup unsolved for the reason you give: While rewriting that I also corrected the ring bullet again: Refer to Pacer in the related quest — agreed, a straight miss on my part. 2278's Carried forward but already resolved at
(written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e97590cb3f
ℹ️ 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".
| - Add the pair the rest of the crate uses: `poll_wait(&kio::Waiter, ts) -> | ||
| Poll<()>` with an `async fn wait(ts)` wrapping it, per the Async / poll | ||
| plumbing section of `rs/CLAUDE.md`. |
There was a problem hiding this comment.
When the anchor remains stable, this API has nothing that can wake a pending poll at its deadline: kio::Waiter only receives notifications from registered waiter lists, while timed wakeups in this repository require a Timer/Deadline from moq-net (rs/moq-net/src/runtime.rs). The re-anchor notification described below covers state changes, not time passing, so a normal video wait can park forever. Pass a timer source into the pacer or keep the timed sleep in the runtime-owning caller. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| - Audio anchors the shared pacer from where the speaker actually is, not from | ||
| what was last written. `Sink::write` has no clock, and `buffered()` is | ||
| documented as the pacing signal for exactly this: the sample playing now was | ||
| written at roughly `last_timestamp - buffered()`. Feed that instant to the | ||
| pacer as a re-anchor observation, so the speaker keeps its authority over the | ||
| audio position while the pacer stays the single clock. What goes away is the |
There was a problem hiding this comment.
With audio enabled and a nonzero delay, last_timestamp - buffered() identifies the sample sounding at wall-clock now, but the proposed pacer shifts every scheduled result by delay. Feeding that playing timestamp in as the anchor therefore schedules video with the same timestamp at now + delay, even though audio already played it at now. The newly added speaker-position instruction thus reintroduces a full-delay A/V skew; anchor at the corresponding live edge, such as playing_timestamp + delay, or provide an observation path that maps the speaker position directly without applying the offset again. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| - Hold the ring at depth `delay`, which is what makes the speaker play `delay` | ||
| behind the newest written sample. `AUDIO_BUFFER_MAX` is that throttle today, | ||
| hardcoded at 1s and compared against `sink.buffered()`; sizing it to `delay` | ||
| is the offset. It is not the ring's capacity, though: the ring is built from |
There was a problem hiding this comment.
For decoded PCM frames longer than delay, changing the throttle alone does not hold the ring at that depth: play_audio currently splits arbitrary PCM frames into chunks of up to one second and checks sink.buffered() only before each whole Sink::write (rs/moq-cli/src/play.rs:385-425). With the default 100ms delay, an empty sink accepts the first one-second chunk, after which subsequent sleeps preserve roughly that oversized queue and audio trails video by far more than requested. Size each part to the remaining headroom or add a sink operation that blocks or caps the accepted duration, and cover a long PCM frame in verification. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
|
Three new findings, all real, all fixed in Anchor the delayed pacer at the live edge — agreed, and this was an arithmetic error I introduced last round. Provide a deadline source to poll_wait — agreed. Bound each write to the requested audio depth — agreed. Carried forward but already addressed, so no action: Wake pending waits (in (written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff16d0f286
ℹ️ 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".
| - Delete the local `Clock` struct and `video_clock`, and route both audio and | ||
| video through the one `moq_mux::Pacer`. The two competing clocks collapse into | ||
| one anchor that both streams observe and steer by. |
There was a problem hiding this comment.
When audio and video are enabled and video receives a tune-in burst, routing both streams through the current Pacer::pace lets video mutate the shared anchor: any frame mapped more than lead ahead invokes hurry, so video can jump to its newest timestamp while the speaker is still draining older PCM. Because later audio observations carry older timestamps and the pacer only re-anchors forward, audio cannot restore synchronization. Make the speaker the sole re-anchor source while audio exists, or explicitly discard audio to the same edge, and add a burst regression test. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L152-L156
Useful? React with 👍 / 👎.
| the very first frame and `hurry` hands back an undelayed `now`. Keep `lead` | ||
| guarding how far *media* may run ahead of wall-clock, and shift every result | ||
| by `delay` after that check, `hurry`'s `now` included. At `delay = 0` the |
There was a problem hiding this comment.
For a video-only invocation with a large but parse-valid --delay, shifting the returned Instant by that duration can overflow and panic. The reviewed Pacer already uses checked_add and tests Duration::MAX specifically because CLI durations are unbounded, but this plan neither defines overflow behavior nor covers it. Bound and reject the delay during validation, or specify a checked fallback and add a large-delay regression test. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L152-L156
Useful? React with 👍 / 👎.
|
Two new findings, both fixed in Prevent video from re-anchoring past the speaker — agreed, and the best catch of the review. Routing both streams through Reject delays that cannot be represented as deadlines — agreed. Bound each write to the requested audio depth was carried forward but landed in (written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b98d81ffa8
ℹ️ 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".
| the private `LATENCY` (50ms device target) and `CAPACITY` (3s ceiling) in | ||
| `rs/moq-audio/src/playback/sink.rs`, so `CAPACITY` has to admit the requested | ||
| `delay`. Plumb it, and keep a floor the way `ringSamples(rate, delay)` does in | ||
| `js/watch/src/audio/latency.ts`: a zero-depth ring can never be read from. |
There was a problem hiding this comment.
For an audio invocation with a large but Instant-representable --delay, plumbing that duration into rs/moq-audio/src/playback/sink.rs::channel makes capacity_seconds allocate a ring proportional to the delay and sample rate. Values such as hours or days can therefore exhaust memory even though the proposed validation accepts them. Add a practical audio-capacity bound or use bounded storage rather than treating clock representability as the only limit. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L128-L129
Useful? React with 👍 / 👎.
| - Bound the delay where it is parsed. `Pacer` already uses `checked_add` and | ||
| tests `Duration::MAX` because CLI durations are unbounded, so shifting a | ||
| returned `Instant` by an arbitrary `--delay` can overflow the platform clock. | ||
| Reject an unrepresentable delay up front rather than saturating deep in the | ||
| pacer. |
There was a problem hiding this comment.
For any accepted --delay above four seconds on a sufficiently long audio stream, the plan holds roughly that much PCM in the sink, but play_audio still wraps its final drain in the fixed four-second AUDIO_DRAIN_MAX timeout. At EOF the timeout drops the remaining buffered tail and ends the media task before the requested playout completes. Either cap the accepted delay to the drain budget or redesign the drain watchdog so progressing playback may drain the configured depth. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L128-L129
Useful? React with 👍 / 👎.
|
Two new findings, both fixed in Cap the ring depth before allocating it and Make EOF draining honor accepted delays — agreed on both. Together with the The other four comments are carry-forwards of threads already resolved: Bound each write in This is where I'm stopping the review loop. Six rounds in, the last two have been the same "bound the delay" observation arriving from different call sites, which the single ceiling now covers. The PR is a quest document, not the implementation, and remaining detail is the implementer's to settle against the tree at the time. (written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d68c31ac
ℹ️ 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".
| first check bites and every later sleep preserves that oversized queue. Size | ||
| each part to `delay - buffered()`, or give the sink a write that stops at a |
There was a problem hiding this comment.
When audio is enabled with --delay=0 (or any value below the sink's nonzero floor), the ring is correctly given a minimum depth above, but calculating each write from delay - buffered() leaves no headroom once buffered() reaches that floor. Following this instruction literally makes the writer wait forever or accept zero samples, so playback loses audio. Use the same effective floored depth for throttling and chunk sizing, or reject sub-floor audio delays, and cover this case with a regression test.
AGENTS.md reference: AGENTS.md:L152-L156
Useful? React with 👍 / 👎.
`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>
Codex found the plan wrong in four places, three of them load-bearing. `moq_mux::Clock` is already taken by the capture-side shared epoch, used across moq-audio's capture path and moq-cli publish. The rename cannot happen without renaming that too, and its only justification was the clarity that name would have bought, so `Pacer` keeps its name. That also keeps the work additive, which drops the moq-srt and moq-cli call-site churn, moves the branch to main, and moves the quest to m0 beside the defect it fixes. Three hazards the plan had glossed: - `pace` re-anchors when `send_at` leads `now` by more than `lead`, which defaults to zero, so a delay folded into `send_at` makes the first delayed frame overshoot and discard the offset. `hurry` returns `now` undelayed and drops it again on every later re-anchor. - `Sync.#setReference` wakes every parked wait; `Pacer` has no notification at all, so a wait would sleep to a stale deadline when the other stream moves the anchor. - Audio waiting for the delayed instant and then filling a sink that also holds a delay's worth trails video by roughly another delay. And one factual correction: `AUDIO_BUFFER_MAX` does not size the sink, it gates writes against `sink.buffered()`. The ring is sized by the private `LATENCY` and `CAPACITY` constants in `rs/moq-audio/src/playback/sink.rs`, so honoring a delay means configuring the sink rather than the throttle. Also adds the `doc/bin/cli.md` reconciliation the flag rename needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
12d68c3 to
b271c7f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b271c7f2b6
ℹ️ 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".
|
|
||
| ### Verification | ||
|
|
||
| - Unit tests on `Pacer` with `tokio::time::pause()`: a late first frame followed |
There was a problem hiding this comment.
The listed unit tests do not protect the reported moq play regression: the parent tree's rs/moq-mux/src/pace.rs::re_anchors_to_live_edge already verifies that Pacer re-anchors, so these tests can pass even if the renderer remains wired to the one-shot video_clock or otherwise never uses the shared pacer. Add a deterministic play-level test showing that a late first video frame followed by the live edge changes the presentation deadline and fails before the wiring change; the manual check is not a regression gate. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L156-L156
Useful? React with 👍 / 👎.
`kio` has no time module, so a `poll_wait` that had to fire at a deadline would park forever with nothing to arm it, and taking a `moq_net::Timers` the way `origin::Driver::run` does would drag a runtime into a type that has never needed one. `pace` keeps returning the instant and the caller keeps sleeping, which is the contract the export path already relies on; the `poll_*` pair reports only the anchor moving. Adds an Open questions section for the A/V policy Codex kept circling: which stream owns the anchor when both exist, what the speaker position contributes given every result is shifted by the delay, and whether a per-write bound is needed when play_audio can hand the sink a one-second chunk against a 100ms delay. Those want the code in front of them, and the answers change what the tests assert, so they belong in the quest rather than settled blind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Consolidated response to the Codex rounds, since several threads restate each other as the file changed. Taken, in b271c7f and acdc608:
Recorded as open questions rather than settled: which stream owns the anchor when both exist, what the speaker position contributes when every result is shifted by the delay, and whether a per-write bound is needed given Stale rather than wrong: the Turned down: the m1-vs-main mismatch was reported against a quest that already said "Branch from dev". The conclusion landed anyway for a different reason — keeping (written by Claude Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acdc608292
ℹ️ 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".
| #### Naming note | ||
|
|
||
| `Latency` is already doubly overloaded: the `Bound | {min,max}` type (`js/watch/src/sync.ts:16`) and `class Latency` (`js/hang/src/util/latency.ts:21`, jitter+buffer). Different packages, both exported. Worth resolving before adding a third meaning. | ||
| Mostly resolved: `js/watch`'s `Latency` / `Bound` types are gone, replaced by `Delay` plus a separate `buffer`, leaving `class Latency` (`js/hang/src/util/latency.ts`, jitter+buffer) as the only remaining use. Pick a name for the absolute mode that does not reintroduce the collision. |
There was a problem hiding this comment.
In the reviewed tree, js/hang/src/util/latency.ts does not exist, and a repository-wide search finds no class Latency; the remaining Latency declaration is an unrelated interface in the internal js/watch/src/audio/render.ts. This naming note therefore directs the quest executor toward a missing implementation and imposes a collision constraint that no longer exists. Update the plan to state that the old public collision is fully resolved, or identify the actual conflicting symbol. (Written by GPT-5.6 Sol)
AGENTS.md reference: quest/AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
1 participant
Summary
Adds quest/m0/playout-clock.md, scoped through a
/plan-questinterview and then corrected against a Codex review that caught real errors in it.moq playhas no playout offset anywhere in its path, and two clocks that disagree about what time it is:play.rsre-derivesClock { media: end - sink.buffered(), wall: now }on every audio frame, so the offset is whatever the speaker happens to hold, bounded by a hardcodedAUDIO_BUFFER_MAXof 1s.video_clockis anchored once on the first frame and never re-anchored (its sole write is gated onis_none()), so a first frame that arrives late appears to leave playback behind live for the rest of the session.js/watch/src/sync.tshandles exactly this by lowering its reference when a frame arrives earlier. The quest reproduces that before calling it a defect.The plan extends
moq_mux::pace::Pacerrather than adding a second primitive, because it is already most ofSync:anchor: Option<(Instant, u128)>isSync'sreference, it re-anchors forward only, andwith_leadis nearly moq-watch'sbuffer. What it lacks is a delay offset.It keeps the name
Pacer. The interview had settled on renaming it toClock, which turned out to be taken by the capture-side shared epoch (rs/moq-mux/src/clock.rs, used across moq-audio's capture path and moq-cli publish). Keeping the name also keeps the work additive, which is why this branches frommainand sits inm0beside the defect it fixes rather than in the dev line.--delayreplaces--max-ageonplaywith no alias: nothing older thandelayis worth presenting, so one number serves as both the playout offset and the subscription's max age.--max-ageis unchanged onimport, the stdout containers, andrtmp export.Scoped out after the interview: the
"auto"and"instant"modes of moq-watch'sDelay, and any FFI surface —libmoqandmoq-ffihand decoded frames to a caller callback and have no playout stage, so there is nothing there to delay yet.What the review changed
Codex found the plan wrong in five places, four of them load-bearing. All are folded in; the consolidated reply has the detail. In short: the name collision above;
lead/hurrywould have discarded the delay on the first frame and every re-anchor; nothing wakes a parked wait; a timedpoll_waithas no deadline source becausekiohas no time module, so the sleep stays in the caller; andAUDIO_BUFFER_MAXgates writes rather than sizing the ring, which is private constants inrs/moq-audio/src/playback/sink.rs.Three A/V questions are recorded as open rather than settled — anchor ownership when both streams exist, what the speaker position contributes under a delay, and whether writes need a per-chunk bound. They want the code in front of them, and the answers change what the tests assert.
Also in this PR
Two quests that #3396 made stale, fixed here since that PR is what made them so:
latency_max, where the field and the flag are nowmax_age/--max-age.Bound/Latencytypes that feat!: split moq-watch latency into delay + buffer, and rename --latency-max #3396 replaced withDelayplus a separatebuffer. Its proposed absolute mode, its open question aboutmin/max, and its naming note are updated to the current shape, and it gains aRelatedlink to the new quest.Test plan
cargo run -p quest -- check: 257 documents ok (links resolve, index matches the tree, headings valid,Requiredacyclic).bun remark quest/ --frailclean.🤖 Generated with Claude Code