refactor(watch)!: let Sync own the instant latency - #3068
Conversation
`"instant"` was implemented in three places at once: the element mapped it to a zero buffer for `Sync`, cleared a `paced` flag on the video decoder, and the decoder skipped its wait. `Sync` can do the whole thing itself. Zero buffer and no pacing are separate. A zero floor still holds the rendition's own delay, a frame interval at 60fps, and `wait()` still sleeps because the sleep comes from the reference rather than the buffer. So `"instant"` now zeroes the buffer outright and returns from `wait()` without sleeping, checked inside the loop too, so parked frames wake through the existing `#update` resolve. Deletes `DecoderInput.paced`, `DecoderTrack.#latency` (`maxBuffer` is already zero in the mode), `#park` (nothing left to race), and the element's `#videoPaced` and `#syncLatency`. The `Paced` type goes with them: one `Latency` again, since the split only existed to keep a value away from a component that can now honor it. Audio stays the element's business. A ring with no depth underruns, and `Sync` has no way to reach the audio decoder. Supersedes #3052. 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: 817e038891
ℹ️ 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".
Zeroing the buffer changes `Sync.now()`, which the caption renderer uses as its playhead, so captions now track the picture rather than trailing it. Two neighbouring claims went stale with it: the mode does not merely stop pacing, and the wait is not the only thing skipped. Also spell out why neither half is redundant, since that is the whole point: a zero floor still holds the rendition's own delay and still sleeps on the anchor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`latency="instant"` makes `Sync.out.buffer` zero, and the audio decoder sized its ring straight off that. `AudioRingBuffer` rejects a non-positive latency, so the worklet's construction threw and left it with no backend. Every later resize is gated on the backend existing, so audio stayed silent even after switching back to a paced latency. The worklet is built while audio is disabled (deliberately, to make unmuting instant), so an element that merely starts in the mode hits this. `setLatency` had the same problem from the other direction: an existing ring resized to zero throws `empty buffer`. An AudioWorkletProcessor renders in fixed 128-sample quanta, so a ring shallower than one can never be read from. Floor both conversions there. Audio owning its own floor is the point: its depth should not derive verbatim from a buffer whose meaning is "video holds nothing". 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: aa4626f2cd
ℹ️ 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".
| const RENDER_QUANTUM = 128; | ||
|
|
||
| /** The ring depth for a target latency, floored at one render quantum. */ | ||
| function ringSamples(rate: number, latency: Time.Milli): number { |
There was a problem hiding this comment.
When latency="instant" initializes the postMessage audio path, this floor is what prevents AudioRingBuffer from throwing on zero latency and leaving the worklet without a backend. The existing ring-buffer tests only confirm that zero latency throws; none exercises ringSamples or verifies that decoder initialization normalizes zero to one render quantum, so this bug fix lacks the required regression test and can be removed without a failing check. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L137-L137
Useful? React with 👍 / 👎.
Move `ringSamples` next to `reanchorFloor`, where it is reachable from a test without exporting anything purely for one. Removing the floor fails the two new cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Supersedes #3061 and closes #3052, both of which addressed the same wart the harder way.
Why
"instant"was implemented in three places at once. The element mapped it to a zero buffer before handing it toSync, separately cleared apacedflag on the video decoder, and the decoder skipped its wait.Synccan just do it.The reason it was split up was the belief that
Synccouldn't honor the mode. That is only true of the audio half. The timing half is entirelySync's.The two things that are not the same
A zero buffer is not no pacing, which is what made this look harder than it is:
max(video, audio)in#runBuffer, the selected rendition's own delay. That's a frame interval at 60fps.wait()still sleeps at a zero floor, because the sleep comes from the reference (sleep = currentRef - ref + floor), which holds an early frame until its timestamp comes up. That is exactly whylatency-min="0"was never instant.So
"instant"now does both:#runBufferreturns zero outright, andwait()returns without sleeping. The check sits insidewait()'s loop as well as at the top, so frames already parked wake through the existing#updateresolve and leave.What that deletes
DecoderInput.paced— the decoder no longer knows the mode exists.DecoderTrack.#latencyand its effect —maxBufferis already zero in the mode via#runRange, so the mirror had nothing left to do. The three call sites readsync.out.maxBufferdirectly again.#park— inlined back toPromise.race([wait, effect.cancel]). With nopacedinput there is nothing extra to race, and the disposable-registration bookkeeping goes with it.#videoPacedand#syncLatencyin the element.Pacedtype. It existed only to keep"instant"away from a component that can now honor it.Net: 110 lines removed, 48 added.
What stays
Audio remains the element's business: a ring with no target depth underruns, and
Synchas no way to reach the audio decoder. The disable and ring flush are unchanged.The rewind fix from #3048 stays and is now unconditional, since there is no
pacedbranch to nest it in: an unanchored reference at output time means areset()landed after the frame'sreceived(), so the frame predates the current timeline and is dropped rather than painted.Reviewer notes
dev: removingDecoderInput.pacedand thePacedexport are semver breaks.just fix,just check, andjust testall pass.VideoDecoder, which bun's test environment lacks.(Written by claude-opus-5)