Skip to content

fix(watch): keep media holes on the current timeline - #6903

Merged
kixelated merged 5 commits into
mainfrom
quest/m1/cut-audit/watch-audio-hole
Oct 27, 2026
Merged

kixelated merged 5 commits into
mainfrom
quest/m1/cut-audit/watch-audio-hole

Conversation

@kixelated

@kixelated kixelated commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

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 gap and end reads 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:

  • Startup coverage: received media is summed across holes, never counting the missing spans. The video decode gate and the shared startup floor read the same sum.
  • Decoded storage: video stops decoding once its input sits maxDelay past 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

  • Video ownership P1 (zero-duration single-picture groups split by holes retained unbounded parked pictures during startup), picked by the maintainer:
    1. ✅ Separate decoded-storage backpressure from startup coverage, keeping the rule that missing media is not counted as received buffered media.
    2. Change startup coverage so inferred holes count as playable silence or held pictures.
    3. Defer the larger design and keep this draft.
  • Choice made without the maintainer: the storage bound is maxDelay (delay + buffer) past the opening picture rather than a picture count or delay. It reuses the bound every other stage holds, leaves buffer of slack for holes inside the opening window, while a delay bound would stop decoding before received media could make up for any hole inside the first floor.

Impact

  • Public API: unchanged. Wire: unchanged. No version bump.
  • Audio plays through a hole using its retained timestamp-indexed PCM; video keeps pictures already waiting on the same clock.
  • Same-instance codec/container changes preserve compatible PCM and Sync. Channel/rate changes still rebuild an incompatible graph, explicit instant mode still clears audio, and caller-requested Player/element reset remains unchanged.
  • A track whose holes keep its received span below the floor still does not start (missing media is not buffered), now with bounded decoded storage while it waits.

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

  • Before the fix, seven deterministic preservation/endpoint cases failed. Tests cover legacy and CMAF, actual missing groups through Track and Container, codec/container handover, endpoints across holes, parked video, and declared restarts.
  • Audio accounting: six delayed 20 ms inputs filling a 100 ms floor, then a gap, blocked the resumed input forever before the fix (legacy and CMAF).
  • Video storage: 64 single-picture groups split by holes, with another track pending, submitted all 64 pictures before the fix (legacy and CMAF); now 29, ending at the first picture 1100 ms past the opening one, independent of input count.
  • Chromium 156 with native Opus AudioDecoder and AudioWorklet: one omitted 20 ms group produced an inferred 40 ms container gap; the decoded PCM span survived. Codec resets: 1; audio-ring resets: 0; Sync resets: 0; resumed native PCM began at 140000 µs with nonzero tone energy, and a parked Sync wait later presented.
  • just fix and just check pass on the merged head.

Follow-ups

  • The decoded-storage bound, like the container consumer, is a PTS span: a hostile publisher packing many frames into one timestamp is bounded by neither. Consider a quest for a density bound at the consumer, which already owns encoded storage.
  • Consider a release backport; no publication or deployment is included here.

(Written by Claude Opus 5.5)

kixelated and others added 2 commits October 26, 2026 18:25
@kixelated kixelated changed the title A media hole in the player keeps the timeline fix(watch): keep media holes on the current timeline Oct 27, 2026
@kixelated
kixelated marked this pull request as ready for review October 27, 2026 01:51
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8a0a48a9-c561-40d9-a3cc-ac237fc695a8
Commits

Reviewing files that changed from the base of the PR and between 000e109 and f7dfc4a.

Files selected for processing: 9
  • js/watch/src/audio/buffer.ts
  • js/watch/src/audio/decoder.test.ts
  • js/watch/src/audio/decoder.ts
  • js/watch/src/audio/terminal.test.ts
  • js/watch/src/audio/terminal.ts
  • js/watch/src/video/decoder.test.ts
  • js/watch/src/video/decoder.ts
  • quest/m1/cut-audit/README.md
  • quest/m1/cut-audit/watch-audio-hole.md
Files with no reviewable changes: 2
  • quest/m1/cut-audit/README.md
  • quest/m1/cut-audit/watch-audio-hole.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Walkthrough

Audio gaps now re-prime the codec while retaining buffered PCM and Sync state. A frame in a group later than a declared endpoint starts a new timeline. Video readers track endpoints across gaps, and startup coverage sums buffered durations without counting holes, subject to a maximum-delay threshold. Tests cover these behaviors, plus audio container and codec changes on an existing instance.

Priority: Normal

Merge Risk: Minimal · up to f7dfc

The reviewed gap handling has no identified merge-blocking issue; normal checks remain appropriate.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly identifies the primary change: preserving the current timeline across media holes in the watch pipeline.
Description check Passed The description directly explains the media-hole problem, the audio and video timeline-preservation approach, storage bounds, validation, and impact.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 7 files.

Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T19:50:04.468466Z f7dfc4a Draft marked ready
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread js/watch/src/audio/decoder.ts Resolved
Comment on lines +763 to +764
if (read.kind === "gap") {
if (latest !== undefined) this.#resetPending = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pending Backpressure.#decoding entries. 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 existing ring.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: retaining first and 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)

@kixelated
kixelated marked this pull request as draft October 27, 2026 02:17
…sion

Co-Authored-By: GPT-6 <noreply@openai.com>
kixelated and others added 2 commits October 27, 2026 12:43
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
kixelated marked this pull request as ready for review October 27, 2026 19:48

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 through ring.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 maxDelay window even when those ranges remain empty.
  • Known limit remains, not a new finding: js/watch/src/video/decoder.ts:750–755 bounds 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)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary for head f7dfc4a:

  • Codex reviewed this head with no findings (thumbs up), and the automated review found no new actionable bugs in the delta: the audio accounting and video startup findings are fixed.
  • Decision: the maintainer picked option 1, separating decoded-storage backpressure (maxDelay past the opening picture) from startup coverage, which still never counts missing media as buffered.
  • Known limit, left as a follow-up: the storage bound is a PTS span, so many frames packed into one timestamp are not bounded. A density bound at the container consumer is suggested as a separate quest.
  • quest check passes against the current main; the finished quest is deleted.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Oct 27, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 27, 2026
@kixelated

Copy link
Copy Markdown
Collaborator Author

Re-landing the unchanged head f7dfc4a. It left the merge queue only because of the order-dependent @moq/boy element test on main (unhandled error between tests in merge group run 40579958993), which moq-dev/moq#6935 fixes and is queued ahead of this PR. Codex reviewed this head with no findings, and the remaining open thread was answered by f7dfc4a.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Oct 27, 2026
Merged via the queue into main with commit e37b109 Oct 27, 2026
12 checks passed
@kixelated
kixelated deleted the quest/m1/cut-audit/watch-audio-hole branch October 27, 2026 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant