Refactor/video pacing rAF#1401
Open
skirsten wants to merge 3 commits into
Open
Conversation
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.
Extract the queue-selection logic from DecoderTrack.consume into a
top-level consumeFrame() helper that takes any { timestamp, close() }
queue. The DecoderTrack method now wraps it and applies the side
effects (timestamp signal, decode-buffer trim).
The new decoder.test.ts exercises the behavior with a StubFrame across:
- empty queue / all-future / boundary (pts === now)
- newest-ready selection with older frames closed
- monitor refresh < content fps (the reviewer's concern: older frame
closed, newer painted)
- monitor refresh > content fps (no frame returned between content
ticks; renderer is expected to hold)
- late frame still selectable when nothing newer is queued
- 60fps × 60Hz steady state: one paint per vsync, no queue growth
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR replaces reactive frame delivery with a pull-based consumption model. The 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![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: 1
🧹 Nitpick comments (2)
js/watch/src/video/decoder.ts (1)
504-505: 💤 Low valueNit: stray blank line.
Line 505 adds an extra empty line between
consumeFrameandsupported. Not a blocker; the project's formatter probably handles it on next save.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/video/decoder.ts` around lines 504 - 505, There's an extra blank line between the functions consumeFrame and supported; remove the stray empty line so consumeFrame and supported are adjacent and keep file formatting consistent by deleting the lone blank line separating the two functions (look for the function declarations consumeFrame(...) and supported(...)).js/watch/src/video/renderer.ts (1)
134-156: 💤 Low valueOptional: tighten
#drawsignature now that the clear branch is unreachable.The only caller (line 116) is inside
if (frame), soframe?: VideoFrameand theif (!frame) { ... clear ...; return; }branch are effectively dead. If you don't plan to call#draw(ctx)externally (e.g. on close to blank the canvas), making the parameter required would make intent clearer.Proposed simplification
- `#draw`(ctx: CanvasRenderingContext2D, frame?: VideoFrame) { - if (!frame) { - // Clear canvas when no frame - ctx.fillStyle = "#000"; - ctx.fillRect(0, 0, ctx.canvas.width, ctx.canvas.height); - return; - } - + `#draw`(ctx: CanvasRenderingContext2D, frame: VideoFrame) { // Prepare background and transformations for this draw ctx.save();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/video/renderer.ts` around lines 134 - 156, The private renderer method `#draw` currently accepts an optional frame and contains an unreachable clear branch; change the signature to require frame (e.g., `#draw`(ctx: CanvasRenderingContext2D, frame: VideoFrame)) and remove the if (!frame) { ... clear ...; return; } block. If you still need an explicit canvas-clear outside normal draws, extract that code into a new clearCanvas(ctx: CanvasRenderingContext2D) helper and call it where appropriate instead of keeping an optional parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/watch/src/video/decoder.test.ts`:
- Around line 119-140: The test's closed assertion is vacuous because
consumeFrame splices closed frames out of queue before you inspect it; to fix,
track frames out-of-band by keeping a separate createdFrames (or inputFrames)
list of the StubFrame instances when you construct them and then inspect that
list for .closed instead of iterating queue; update the loop that currently does
"for (const f of queue) ..." to iterate createdFrames (or check specific frame
ids/timestamps) so closed detection reflects frames removed by consumeFrame, and
assert against that out-of-band list alongside the existing painted and queue
assertions to ensure no frames were incorrectly closed/dropped by consumeFrame.
---
Nitpick comments:
In `@js/watch/src/video/decoder.ts`:
- Around line 504-505: There's an extra blank line between the functions
consumeFrame and supported; remove the stray empty line so consumeFrame and
supported are adjacent and keep file formatting consistent by deleting the lone
blank line separating the two functions (look for the function declarations
consumeFrame(...) and supported(...)).
In `@js/watch/src/video/renderer.ts`:
- Around line 134-156: The private renderer method `#draw` currently accepts an
optional frame and contains an unreachable clear branch; change the signature to
require frame (e.g., `#draw`(ctx: CanvasRenderingContext2D, frame: VideoFrame))
and remove the if (!frame) { ... clear ...; return; } block. If you still need
an explicit canvas-clear outside normal draws, extract that code into a new
clearCanvas(ctx: CanvasRenderingContext2D) helper and call it where appropriate
instead of keeping an optional parameter.
🪄 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: 4bae9da4-229a-400e-a144-34b7d26542d9
📒 Files selected for processing (4)
js/watch/src/sync.tsjs/watch/src/video/decoder.test.tsjs/watch/src/video/decoder.tsjs/watch/src/video/renderer.ts
- import type Time in decoder.test.ts (useImportType) - drop stray blank line in decoder.ts (formatter) - track frames out-of-band in 60Hz/60fps test so the closed assertion actually observes frames consumeFrame has spliced out of the queue - drop renderer #draw unreachable !frame branch and tighten signature Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
1 participant
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.
Summary
Consolidate video frame pacing into the renderer's
requestAnimationFrameloop. Previously pacing happened twice: in the decoder viaSync.wait()(a setTimeout/#updaterace per in-flight frame, trying to wake near vsync) and again in the renderer via a single-shot rAF triggered by a frame signal. Two layers, two places to stall, and systematic beating when the display refresh and content fps lined up (the wait would resolve a fraction of a ms past vsync, the signal-driven rAF would schedule against the next vsync, so every other frame got dropped).This PR moves all pacing to paint time:
DecoderTrackenqueues decoded frames into#queue: VideoFrame[]immediately; the output callback no longer awaits anything.Decoder.consume()returns the newest queued frame whose PTS is<= sync.now(), closing older entries and transferring ownership to the caller. Late-frame drop and decode-buffer trim move here.decoder.consume()each vsync.Sync.wait()and the#updatenotification machinery are removed. Buffer changes take effect on the next vsync (~<=16ms), which is fine for live media.The "first-paint preview" path and the
Decoder.framemirror that fed it are dropped. The preview saved 20-100ms of black canvas at first paint, but it added a special case to the render loop and held a clonedVideoFrameresident at all times for every active track. It's replaced with two narrower signals:DecoderTrack.display(set once from the first decoded frame's display dimensions, as a catalog-display fallback) and#runBufferingnow reacting to consume-timetimestampinstead of decode-time frame presence.Net: one rAF loop in the system. Stalls now show up as held frames instead of dropped frames.
Test plan
just check(biome + tsc + clippy + cargo check, etc.)js/watch/src/video/decoder.test.tscoversconsumeFrame():pts === now)