Repository navigation
fix(watch,publish): keep the delay across a rename and close frames read during teardown; cover renames in media QA - #5635
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 12 minutes. View limit details
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. Comment |
|
Interop triage before merge (not merging yet):
Holding the merge for a maintainer call on whether to land with the leak as a follow-up or chase it first. (Written by Opus 5.5) |
![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: 380bcdd399
ℹ️ 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".
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. |
# Conflicts: # quest/m1/README.md # quest/m1/browser-rename-check.md
Two paths dropped a captured frame without closing it when a reader tore down while the frame was crossing the capture: - The encoder, preview, transcoder, and stats loops raced each read against effect teardown. A read that had already resolved lost the race to the synchronous abort, and its frame was never seen again. - A new reader's replay wrapper enqueued a frame its consumer cancelled while the read was resolving; the enqueue threw and the frame leaked. Read without racing and let the existing cancel end a pending read, close anything that resolves after teardown, and close the frame the replay wrapper can no longer deliver. A sweep test tears the encoder and the preview down at every microtask offset after a push. The media fixture also marks the codec-change generation once the catalog advertises VP8, rather than on request, so old-codec pictures never carry the new generation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da76a4239b
ℹ️ 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".
A codec change renames the video track. The replacement's container consumer restarted its jitter estimate from the cold-start prior, so the shared "auto" delay dropped at promotion (340ms to 100ms in the reproduction) and climbed back as the new track relearned the same lateness. The audio ring follows that delay: it skipped buffered audio on the drop and parked playback on each rise, which is the post-rename tone failure the media QA caught. The path's arrivals do not change with the track name, so a replacement now continues its predecessor's estimate. `Container.Consumer` takes a `predecessor`, whose learned histogram and startup ramp carry over through `Jitter.fork()`, while the reference window restarts so the new track's opening backlog is measured against itself. The video decoder hands the on-screen rendition's consumer to the one replacing it. A live-consumer test warms a real estimate, renames the track, and plays a postMessage audio ring that follows the shared delay: without the fix it skips 280ms and renders 13 silent ticks; with it, neither. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
@codex review The new commit (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd6e1f29d
ℹ️ 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".
A source switch or a rendition `broadcast` override can move video to another path, whose arrivals have nothing to do with what the outgoing consumer learned. Inheriting its histogram there held a high delay for the estimator's decay period, or underbuffered after a low-to-high switch. Only hand the outgoing consumer to a replacement read from the same broadcast path; another path starts from the prior. (Codex review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review
(Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # test/interop/clients/js/media.ts # test/interop/interop.sh
|
@codex review
(Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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 capture tests waited a 10ms timer per step, so the teardown sweeps spent over a second sleeping and assumed the pipeline settles within 10ms on a loaded worker. The pipeline runs on promises alone, so one macrotask turn drains it deterministically. The sweeps still fail without the leak fix, now at offset 2. (Codex review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review
(Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Merge summary for
(Written by Claude Opus 5.5) |
Problem
Unit tests do not prove that real WebCodecs playback follows the fresh track minted for a decoder identity change. This adds browser coverage, and fixes the two playback failures that coverage surfaced.
Audio stalls after a codec rename. Interop failed
audio tone: codec rename 71/81 samples. A codec change renames the video track, and the replacement'sContainer.Consumerrestarted its jitter estimate from the cold-start prior. The shared "auto" delay is the max over targets, and video's target follows that estimate. It dropped at promotion, by about 240 ms in the reproduction and about 560 ms in CI. It then climbed back as the new track relearned the same path's lateness. The audio ring follows that delay. It skipped buffered audio on the drop and parked playback on each rise. In CI the audio timestamp jumped 5144 to 5706 ms at the rename, followed by a 0.55 s stall.Unclosed
VideoFrame. CI loggedA VideoFrame was garbage collected without being closedfrom the publisher page once. That was a real@moq/publishleak with two causes:effect.race(reader.read()). Teardown aborts synchronously, so a read that had already resolved, but whose continuation had not run yet, lost the race. Nothing closed the frame it carried.Capture's per-reader wrapper replays the held picture to a new reader. It awaited a live read, then calledcontroller.enqueueafter its consumer had cancelled. The enqueue threw and the frame leaked.Both are reachable whenever a reader tears down while a frame is in flight: a subscriber leaving (demand gap), a codec rename swapping the track, or the preview being hidden.
Approach
Keep the estimate across a rename. The path's arrivals do not change with the track name, so a replacement consumer continues its predecessor's estimate.
Container.Consumertakes apredecessor. Its learned histogram and startup ramp carry over throughJitter.fork(), and the reference window restarts, so the new track's opening backlog is measured against itself rather than against the old track's arrivals. The video decoder hands the on-screen rendition's consumer to the one replacing it, only when both read the same broadcast path: a source switch or abroadcastoverride to another path starts from the prior (Codex review).doc/concept/audio-jitter.md(normative) now states this under "Across renditions".Leak fix. Read without racing. Each loop already cancels its reader on teardown, which ends a pending read. A frame that resolves after teardown is closed rather than encoded or kept. The replay wrapper records its cancellation and closes a frame it can no longer deliver.
Regression tests.
decoder.test.tswarms a real consumer on a manual clock: 33 ms frames flushed in bursts, which settles the delay above 300 ms. It then renames the track and plays a postMessageAudioRingBufferthat follows the shared delay in 20 ms steps. Without the fix it skips 280 ms and renders 13 silent ticks; with it, neither, and the delay never drops.decoder.test.tscase warms the same bursty path, then moves the media to another path with on-time frames and asserts the delay falls. Without the path guard it holds at 380 ms.jitter.test.tschecks that a fork keeps the estimate but measures the replacement's backlog against its own arrivals (copying the history instead fails it).capture.test.tssweeps teardown across every microtask offset after a push, for the encoder and for the preview, and asserts only the capture's held picture stays open. Each step settles with onesetImmediateturn rather than a timer, since the pipeline runs on promises alone (Codex review). Reverting the leak fixes fails both sweeps at offset 2.Media QA. Extend the existing deterministic media fixture with an AVC-to-VP8 change and a VP8 resize from 320x240 to 640x480. Check the catalog's resolved track, actual decoded and presented dimensions, frame progress and A/V sync. Source-generation bits in the presented pixels reject pictures from before each change. The codec-change generation advances only once the fixture's catalog advertises VP8, so pictures still encoded as AVC on the old track never carry it (Codex review).
The default media run includes these cases.
just test media --media-cases renameselects them independently, explicitly omitting cold-start grading and unrelated controls. Default grading and every tone, timing and skew threshold stay unchanged, including the 90% tone floor.Impact
@moq/hang): one new optional prop,ConsumerProps.predecessor?: Consumer.Jitter.fork()and theDecoderTrackfields are package-internal.@moq/watch: none.--media-casesselection and a source-generation marker in the internal fixture.Decisions
audio-jitter.mdsays the histogram is the only dynamic.fixPR.race(reader.read())loop overVideoFrames (encoder, preview, transcoder, stats tab), not only the encoder. The audio encoder loop reads plain sample arrays with nothing to close, so it is left alone.predecessor, notpreviousorreplaces: the replaced consumer's role.Validation
just checkpasses.@moq/watch372 and@moq/hang242 unit tests pass.skipped 13440samples,silent 13) and passes with it. The sweep tests fail without their fixes, as above.just test media --media-cases rename) one32f983ae, on a machine at load 47 on 24 cores:video/1, so the predecessor path is not involved: presented video fell behind wall clock and the delay rose with real lateness (325 to 1680 ms).SUBSCRIBE_UPDATEfor the oldvideotrack raced its finish and the relay's stream close (The stream was aborted by the remote server). That is a@moq/netrace, listed under follow-ups.e32f983aepassed both rename windows and failed the after-growth tone (48/61, 64/80, 71/81), each while the player logged late video frames (up to 204 ms) and the estimate rose with them.da76a4239, same load: one of three runs also failed after-growth tone (68/80), so that window's failure predates this change and tracks machine load. Across all eight runs on the fix, every run that reached the rename windows passed them (7/7).Follow-ups
predecessorhand-off.@moq/net: aSUBSCRIBE_UPDATEwrite that fails because the publisher already finished the track should defer to the track's clean end rather than fail it.moq-mux/hangconsumers that replace a track keep their estimate.quest/m1/browser-rename-check.mduntil smooth playback is established with the complete applicable oracle.quest/m1/publish-audio-channel-count.mdalready owns the unpinned cold-start tone symptom.(Written by GPT-6, updated by Claude Opus 5.5)
🤖 Generated with Claude Code