@moq/watch: fix playback stalls and frame-rate beating#1373
Open
skirsten wants to merge 3 commits intomoq-dev:mainfrom
Open
@moq/watch: fix playback stalls and frame-rate beating#1373skirsten wants to merge 3 commits intomoq-dev:mainfrom
skirsten wants to merge 3 commits intomoq-dev:mainfrom
Conversation
Promise.race against a never-settling Promise (Effect#closed,
Effect#cancel) attaches a .then reaction to that long-lived promise on
every call. Because the promise never settles, those reactions never run
and never get GC'd. Each leaked reaction's closure retains the awaiter
state, which keeps the resolved {frame, group} alive.
Over a multi-minute playback session this accumulates one retained Frame
per consumed video frame (~400MB after 9 minutes at 60fps) and grows the
PromiseReaction chain to tens of thousands of nodes, which inflates major
GC pauses to 100–200ms. That's the cause of the periodic ~700ms stalls.
Three changes, no API impact:
* @moq/hang Consumer.next() waits via an AbortSignal listener registered
with { once: true } and explicitly removed when notify wins, instead of
Promise.race([wait, signals.closed]).
* @moq/watch video legacy decoder loop drops the outer
Promise.race([consumer.next(), effect.cancel]). When the effect closes,
effect.cleanup runs consumer.close(), which makes next() return
undefined naturally and the loop exits.
* @moq/watch video and audio CMAF paths track in-flight groups in a Set.
effect.cleanup closes them all, so recvGroup/readFrame terminate
without racing against effect.cancel.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughConsumer wait/notification in the legacy container was rewritten to use AbortSignal listeners instead of a Promise.race. Sync removed its private coordination promise and deleted the wait(timestamp) method. Video and audio decoders now queue decoded frames and track open CMAF groups for explicit closure on cleanup; in-flight reads no longer race effect cancellation. Decoder removes the frame signal, adds consume() to select the newest ready VideoFrame via source.sync.now(), and DecoderTrack manages an internal queue. The renderer switched to a requestAnimationFrame loop that calls decoder.consume() and takes ownership of returned frames. MultiBackend.close() now closes video/audio sources and sync. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/watch/src/video/renderer.ts (1)
107-145:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis makes pull mode unconditional instead of adding the requested mode switch.
The PR objective calls for
mode: "push" | "pull"with"push"as the default and only the WebCodecs path opting into pull. This loop removes the old push behavior for every renderer instance, and there’s no API surface here to preserve existing callers’ scheduling semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/video/renderer.ts` around lines 107 - 145, `#runRender` currently always uses a RAF-driven "pull" loop which removes the original "push" semantics; modify `#runRender`(effect: Effect) to respect a mode ("push" | "pull", default "push") sourced from the renderer instance (e.g., this.mode) or the effect/options, and branch behavior: for mode === "pull" keep the requestAnimationFrame loop and RAF cleanup as implemented (tick, rafId, cancelAnimationFrame), but for mode === "push" remove the RAF loop and instead subscribe to the decoder's push callback/event (e.g., decoder.onframe or equivalent) to immediately call this.#draw(ctx, frame), update this.frame and this.timestamp, and on effect.cleanup remove that listener; ensure ownership/close semantics remain identical when handling frames in both branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/watch/src/video/renderer.ts`:
- Around line 111-135: The previewShown flag must be set any time we actually
paint a frame so a later empty vsync cannot paint a future preview; inside
tick() after you draw a frame (the branch where "if (frame) { ... }") set
previewShown = true (this covers frames returned from decoder.consume() and
cloned previews from decoder.frame.peek()), ensuring previewShown is not only
toggled in the preview branch; update references to decoder.consume,
decoder.frame.peek, previewShown, and the frame drawing block accordingly.
---
Outside diff comments:
In `@js/watch/src/video/renderer.ts`:
- Around line 107-145: `#runRender` currently always uses a RAF-driven "pull" loop
which removes the original "push" semantics; modify `#runRender`(effect: Effect)
to respect a mode ("push" | "pull", default "push") sourced from the renderer
instance (e.g., this.mode) or the effect/options, and branch behavior: for mode
=== "pull" keep the requestAnimationFrame loop and RAF cleanup as implemented
(tick, rafId, cancelAnimationFrame), but for mode === "push" remove the RAF loop
and instead subscribe to the decoder's push callback/event (e.g.,
decoder.onframe or equivalent) to immediately call this.#draw(ctx, frame),
update this.frame and this.timestamp, and on effect.cleanup remove that
listener; ensure ownership/close semantics remain identical when handling frames
in both branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c87f457-6582-45a2-91e1-5a8eaf9819d6
📒 Files selected for processing (5)
js/hang/src/container/legacy.tsjs/watch/src/audio/decoder.tsjs/watch/src/sync.tsjs/watch/src/video/decoder.tsjs/watch/src/video/renderer.ts
Previously, frame pacing happened in the decoder via Sync.wait(), which slept on a setTimeout / #update race per in-flight frame to wake near vsync. The renderer then ran a single-shot rAF on every frame signal update to paint. Two pacing layers, two places that could stall, and systematic beating when the display refresh and content fps line up (the wait resolves a fraction of a ms past vsync, the signal-driven rAF schedules against the next vsync, so you display every other frame). Move pacing to the renderer: - DecoderTrack queues decoded frames immediately into #queue: VideoFrame[]. The output callback no longer awaits anything. - Decoder.consume() returns the newest queued frame whose PTS is ≤ sync.now(), closing any older entries; caller takes ownership. Late-frame drop and decode-buffer trim move here. - The renderer's rAF calls decoder.consume() each vsync. - Drop Sync.wait() and the #update notification machinery — buffer changes now take effect on the next vsync (≤16ms), which is fine for live media. Drop the "first-paint preview" path and Decoder.frame mirror that fed it. The preview was painting decoder.frame.peek() while the buffer filled, which saved 20–100ms of black canvas at first paint but added a special case to the render loop and held a cloned VideoFrame resident at all times for every active track. Replace with two narrower signals: DecoderTrack.display, set once from the first decoded frame's displayWidth/displayHeight as a catalog-display fallback; and #runBuffering now reacts to consume-time `timestamp` instead of decode-time frame presence. Net: one rAF loop in the system. Pacing happens at paint time, where vsync is. Stalls show up as held frames instead of dropped frames.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
js/watch/src/video/renderer.ts (1)
107-131: ⚡ Quick winPause-aware rAF gating would reduce idle render wakeups.
Once paused and a preview frame already exists, this loop still schedules every vsync. Consider short-circuiting rAF in that state and re-entering when
paused/framechanges.Possible refactor
`#runRender`(effect: Effect) { const ctx = effect.get(this.#ctx); if (!ctx) return; + const paused = effect.get(this.paused); + const currentFrame = effect.get(this.frame); + if (paused && currentFrame) return; let rafId: number | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/watch/src/video/renderer.ts` around lines 107 - 131, The render loop in `#runRender` continues scheduling requestAnimationFrame even when paused and a preview frame exists, causing unnecessary wakeups; update `#runRender` to gate rAF: before scheduling the next raf check a paused flag and whether this.frame.get() (or the equivalent preview frame) exists, and only call requestAnimationFrame(tick) when not paused or when no preview frame is present; also subscribe to changes on the pause state and the frame store so that when paused becomes false or the frame is cleared you re-enter the raf loop (use the same tick closure), and keep the existing effect.cleanup to cancel any outstanding rafId via cancelAnimationFrame(rafId) to avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@js/watch/src/video/renderer.ts`:
- Around line 107-131: The render loop in `#runRender` continues scheduling
requestAnimationFrame even when paused and a preview frame exists, causing
unnecessary wakeups; update `#runRender` to gate rAF: before scheduling the next
raf check a paused flag and whether this.frame.get() (or the equivalent preview
frame) exists, and only call requestAnimationFrame(tick) when not paused or when
no preview frame is present; also subscribe to changes on the pause state and
the frame store so that when paused becomes false or the frame is cleared you
re-enter the raf loop (use the same tick closure), and keep the existing
effect.cleanup to cancel any outstanding rafId via cancelAnimationFrame(rafId)
to avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7952ba1-2fb7-4ae4-9ea5-9cba34d379dc
📒 Files selected for processing (3)
js/watch/src/sync.tsjs/watch/src/video/decoder.tsjs/watch/src/video/renderer.ts
MultiBackend.close() only closed its own #runElement signal, leaving the Sync and the two Source instances (constructed as fields, not inside an Effect) alive. Each owns its own Effect with several .run() handlers that never get cleaned up, surfacing as "Signals was garbage collected without being closed" warnings whenever a watch component unmounts. Close them explicitly, ordered so inner Decoder/Renderer/Emitter (spawned via the #runElement effect) finish before the sources/sync they reference are torn down.
Collaborator
|
Can you make separate PRs for these commits? The 1st and 3rd make a lot of sense and should get merged. The 2nd one with the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detailed description of both fixes is in the commits.
closes #1367