Refactor/video pacing rAF#1401
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces reactive frame delivery with a pull-based consumption model. The Decoder no longer signals decoded frames; DecoderTrack maintains an internal PTS-ordered queue and enqueues frames synchronously from VideoDecoder.output. A new consumeFrame() helper and Decoder.consume() select and return the newest eligible frame (caller-owned). The Renderer drives rendering from decoder.consume() inside rAF, transferring ownership and closing previous frames. Sync.wait() and related 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
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
skirsten
force-pushed
the
refactor/video-pacing-rAF
branch
2 times, most recently
from
0f47448 to
583540a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
js/watch/src/video/decoder.test.ts (1)
2-3: 💤 Low valueImport package mismatch with implementation.
The test imports
Timefrom@moq/lite, butdecoder.tsimports it from@moq/net. While this works today since it's a type-only import and the branded types are likely identical, using different source packages could cause type divergence if these packages evolve independently.Suggested fix
import { expect, test } from "bun:test"; -import type { Time } from "`@moq/lite`"; +import type { Time } from "`@moq/net`"; import { consumeFrame } from "./decoder.ts";🤖 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.test.ts` around lines 2 - 3, The test imports the Time type from a different package than the implementation which risks type divergence; update the import in this test to import Time from the same package used by decoder.ts (i.e., replace the current Time import source with the one used in decoder.ts) so the types for Time used alongside consumeFrame remain consistent across files.
🤖 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.
Nitpick comments:
In `@js/watch/src/video/decoder.test.ts`:
- Around line 2-3: The test imports the Time type from a different package than
the implementation which risks type divergence; update the import in this test
to import Time from the same package used by decoder.ts (i.e., replace the
current Time import source with the one used in decoder.ts) so the types for
Time used alongside consumeFrame remain consistent across files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63041e5d-678e-484e-b601-1905d2e635c1
📒 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
skirsten
force-pushed
the
refactor/video-pacing-rAF
branch
2 times, most recently
from
8b84e97 to
9bee308
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/renderer.ts`:
- Around line 103-104: The preview frame is cached then decoding is disabled
(effect.get(this.frame) + this.decoder.enabled.set(!frame)), but no redraw is
performed when the canvas is resized/replaced so the paused view can stay blank;
after computing frame and disabling the decoder, explicitly paint the cached
frame (call the existing frame-draw routine — e.g., paintFrame/ renderFrame/
drawPreview — or trigger the same rendering path the consumer uses) so the
preview is repainted immediately, and apply the same change to the other block
in the render loop (the code around lines 111-130) so canvas changes force a
redraw even when no new frames arrive (alternatively track a canvasVersion and
force a redraw when it changes).
🪄 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: 45856bf5-884e-4288-9070-d51ea23448e5
📒 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
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
skirsten
force-pushed
the
refactor/video-pacing-rAF
branch
from
9bee308 to
89056be
Compare
| this.frame.set(frame.clone()); | ||
| } | ||
|
|
||
| const wait = this.source.sync.wait(timestamp).then(() => true); |
There was a problem hiding this comment.
This is really important for higher latency playback.
There was a problem hiding this comment.
I did some tests on Chrome and Firefox.
It's the same peak frame count either way (latency * fps). Old code holds them in awaiting output() closures, new in the queue. Same VRAM either way, and decodeQueueSize stays at 0 so the decoder isn't backpressuring us implicitly.
Only real way to shrink this would be capping the queue and pausing decoder.decode() when it's full, so we hold encoded bytes instead of decoded frames. Container.Consumer already buffers at that layer, so would probably be better to do it there 🤔
But playback seems to be the fine even at high latency:
https://pr-31-moq-monitor.skirsten.workers.dev/?relay=https%3A%2F%2Fcdn.moq.dev%2Fdemo&watch=bbb
There was a problem hiding this comment.
Hmm, actually it depends on the type of decoder used. If chrome is using hardware decoding it only stores max 10 frames, if software there does not seem to be a limit as far as I can see...
Anyway, probably makes sense to add a limit from our side: https://github.com/skirsten/moq/pull/5/changes
Should I include this in this PR?
2 participants
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)