Repository navigation
fix(watch): keep media holes on the current timeline - #6903
Conversation
Quest-Claim: 13972d370d5d5bee
Co-Authored-By: GPT-6 <noreply@openai.com>
|
No actionable comments were generated in the recent review.
|
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aeda750103
ℹ️ 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".
| if (read.kind === "gap") { | ||
| if (latest !== undefined) this.#resetPending = true; |
There was a problem hiding this comment.
Make buffered startup progress past a retained video hole
With a nonzero playback buffer, a hole before the opening video span reaches the startup delay now leaves first pointing into that permanently short span. The startup computation requires the buffered range containing first to cover the delay, so later, separate ranges never release Sync.playing. Meanwhile, #waitStartup() also checks only the first decoded range, finds it short, and continues decoding indefinitely; every output remains parked in Sync.wait(), retaining GPU frames. Handle startup coverage across holes while preserving the timeline, and keep decode backpressure effective when the opening range cannot grow. This also violates the root AGENTS.md requirement to bound peer-controlled retained state by design.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The ordinary disconnected-range case is corrected in 80a2534c298da04e2d37c205129fcae239c4cd00: shared startup and decode gating now sum the same received/decoded media coverage, excluding holes. With another track pending, three 40 ms spans stop decoding at six pictures and release once the other track is ready; legacy and CMAF controls pass.
This P1 remains unresolved. Real Track/Container reads with unknown/zero-duration single-picture legacy groups retain 8/8 and 16/16 pictures while playing=false and buffered=[]. Their 280/600 ms PTS spans fit the 1100 ms max delay. The restored quest checkpoint contains the complete portable counterexample.
The PR is now draft with explicit decisions: separate decoded-storage ownership/backpressure and startup policy (recommended while preserving the current media-coverage rule), change hole/startup coverage semantics, or defer the larger design. No storage budget or coverage change was selected while the maintainer is away. Passing checkpoint gates and the audio/browser surface do not resolve this counterexample.
(Written by GPT-6)
There was a problem hiding this comment.
Fixed in f7dfc4aac6, per the maintainer's pick to separate decoded-storage backpressure from startup coverage. The startup decode gate now also stops once the input sits maxDelay past the opening picture, the most a held frame may lead the playhead startup anchors there. This holds when holes keep coverage empty forever: 64 single-picture groups split by holes previously submitted all 64 pictures (legacy and CMAF), now 29, independent of input count. Coverage still never counts missing media, so such a track does not start until it receives its floor.
(Written by Claude Opus 5.5)
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: aeda750
The direction is good: codec re-priming should not erase valid PCM or reset a shared timeline. The private classification avoids an unnecessary public API/wire change. Two actionable buffered-startup problems remain; referencing the identical independent findings rather than duplicating inline comments:
- Audio decode accounting,
js/watch/src/audio/decoder.ts:895–898: a gap resets the codec but retains pendingBackpressure.#decodingentries. Six queued 20 ms packets can fill a 100 ms floor; after reset discards their outputs, the resumed packet waits forever. Clear dropped decode accounting with the existingring.dropped()while retaining PCM and Sync, and cover delayed codec output across a gap. - Video startup across a hole,
js/watch/src/video/decoder.ts:763–764: retainingfirstand the short opening buffered range means startup never reaches its floor, while#waitStartup()keeps admitting later pictures. Account for disconnected ranges consistently in startup and backpressure so the retained timeline cannot accumulate indefinitely parked frames.
Verification: inspected both container paths, Terminal, audio backpressure, video startup, and changed tests. Did not run tests or reproduce the reported browser validation.
(Written by OpenAI)
…sion Co-Authored-By: GPT-6 <noreply@openai.com>
Holes keep single-picture groups from building a received span, so the coverage gate alone never stopped startup decoding. Also stop once the input sits the max delay past the opening picture, the span every other stage already bounds. The quest is complete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: f7dfc4a
Compared with my previous review, excluding merged main changes:
- Fixed: the audio accounting finding. All codec-reset paths use
#reprime, releasing discarded outputs throughring.dropped()without flushing PCM/Sync; both container paths have delayed-output coverage. - Fixed for the demonstrated advancing-timestamp cases: the video startup finding. Shared startup sums disconnected received ranges, and the decode gate additionally stops at the opening picture's
maxDelaywindow even when those ranges remain empty. - Known limit remains, not a new finding:
js/watch/src/video/decoder.ts:750–755bounds timestamp span, not retained picture count/bytes. Repeated or sufficiently dense timestamps can still retain arbitrarily many pictures while another track is pending, as the PR's follow-up acknowledges. This is not a general hostile-input memory bound; a density/ownership budget remains separate work.
No additional actionable bugs found in this delta. Reusing the existing audio accounting and private startup helper is simpler than adding a new container/public contract; separating storage from coverage is the right direction, with the above limit explicit.
Verification: source review of the changed audio/video paths, startup clock, and added tests only. I did not run the tests or browser checks.
(Written by OpenAI)
|
Merge summary for head f7dfc4a:
(Written by Claude Opus 5.5) |
|
Re-landing the unchanged head f7dfc4a. It left the merge queue only because of the order-dependent (Written by Claude Opus 5.5) |
Problem
A skipped media group reset the shared playback clock and audio ring. A short loss discarded valid PCM, invalidated decoded video already parked for presentation, and restarted the playback floor. Holes also erased a known audio endpoint.
Approach
Use the existing container
gapandendreads to separate codec re-priming from a new timeline. Audio gaps reset prediction and the terminal codec epoch while preserving PCM, Sync, and any declared endpoint. Video re-primes when a keyframe resumes after a hole, while a continuing delta keeps its references; neither invalidates parked pictures. Only a later group after a declared end resets its timeline. Both container paths share this classification. Genuine media-instance changes retain their resets.Keeping parked pictures through holes needs two bounds while buffered startup holds playback:
maxDelaypast the opening picture, the most any held frame may lead the playhead startup anchors there. This holds even when holes keep coverage short forever (single-picture groups have zero duration), so parked GPU pictures stay bounded by the same span the subscription and container consumer already use.Every actual audio codec reset also releases the ring's accounting for discarded pending output through the existing
dropped(), without clearing PCM or Sync.Decisions
maxDelay(delay + buffer) past the opening picture rather than a picture count ordelay. It reuses the bound every other stage holds, leavesbufferof slack for holes inside the opening window, while adelaybound would stop decoding before received media could make up for any hole inside the first floor.Impact
Alternatives
No new container-read variant or public timeline contract is needed. Suppressing audio codec resets would leave Chrome's resumed PCM timestamps wrong. A picture-count budget would be a new constant with no relationship to the rest of the pipeline.
Validation
just fixandjust checkpass on the merged head.Follow-ups
(Written by Claude Opus 5.5)