Improve video rendering and sync #871
Conversation
The b-frames "support" was well intentioned, but introduced a lot of latency. The main problem is that #ref is computed AFTER queuing, we only pop from the queue after sleeping for some amount. This means we were rendering frames significantly later than intended. Instead of moving the ref computation before adding to the queue, I'd like to remove the queue. However, this means that frames are rendered in parallel. This is annoying but also means we support b-frames automatically. In fact maybe they just work now, who knows. But there are undoubtedly some bugs with the "buffering" detection as a result. However, that's probably just wrong anyway, as we shouldn't be deciding if we're buffering when deciding how long to sleep, as that implies we have a frame.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces a new Sync module to centralize playback timing synchronization. The changes refactor Audio and Video sources to depend on Sync for latency management instead of maintaining local buffering logic. BufferProvider is simplified to a placeholder, and corresponding test files and type definitions are cleaned up by removing syncStatus and latency properties from VideoSource. Broadcast is updated to instantiate and wire Sync through its component dependencies. The Signals library gains a promise() method on Getter and Signal types. Watch UI logic is simplified to rely solely on bufferStatus instead of syncStatus for visibility determination. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/hang/src/watch/video/source.ts (1)
208-244: Race condition: parallel frame processing can cause out-of-order rendering.The late-frame check (line 213) occurs before
sync.wait(), but multiple frames can be waiting in parallel. If a later frame'ssync.wait()completes before an earlier frame's, the timestamp and frame can regress:
- Frame A (ts=100) and Frame B (ts=200) both pass the late-frame check
- Both call
sync.wait()- Frame B completes first, sets
#timestamp = 200- Frame A completes, overwrites with
#timestamp = 100Consider rechecking after
sync.wait()completes:🐛 Proposed fix
const wait = this.sync.wait(timestamp).then(() => true); const ok = await Promise.race([wait, effect.cancel]); if (!ok) return; + // Recheck after waiting - another frame may have rendered + if (timestamp < (this.#timestamp.peek() ?? 0)) { + return; + } + this.#timestamp.set(timestamp);js/hang/src/watch/broadcast.ts (1)
163-172: Add missingthis.sync.close()cleanup call and implement close() method in Sync class.The
Syncclass has asignalsEffect field that manages state tracking and effects, but lacks aclose()method. All other components (audio,video,location,chat,preview,user) follow a consistent pattern of implementingclose()to clean up theirsignalsEffect. Thebroadcast.tsclose() method must callthis.sync.close()once Sync implements this method to prevent resource leaks from unclosed effects.Required changes
- Add close() method to Sync class in
js/hang/src/watch/sync.ts:+ close() { + this.signals.close(); + }
- Call sync.close() in broadcast.ts close() method:
close() { this.signals.close(); this.audio.close(); this.video.close(); this.location.close(); this.chat.close(); this.preview.close(); this.user.close(); + this.sync.close(); }
🧹 Nitpick comments (6)
js/signals/src/index.ts (1)
144-148: Potential memory leak: dispose callback fromchanged()is never invoked.The
changed()method returns aDisposefunction to unregister the callback, butpromise()discards it. If the caller abandons the promise (e.g., viaPromise.racelosing) or the signal is never updated again, the callback remains registered.Consider storing and invoking the dispose function when the promise resolves:
♻️ Proposed fix
async promise(): Promise<T> { - return new Promise((resolve) => { - this.changed(resolve); - }); + return new Promise((resolve) => { + const dispose = this.changed((value) => { + dispose(); + resolve(value); + }); + }); }js/hang-ui/src/shared/components/stats/providers/buffer.ts (1)
11-20: Incomplete implementation:BufferProvideris now a placeholder.The
setupmethod now only sets"TODO"as display data, but the class docstring still claims it provides "buffer metrics (fill percentage, latency)". This will show "TODO" in the stats panel.Is this intentional as part of the refactor, with a follow-up planned to implement proper buffer metrics using the new
Syncmodule? If so, consider updating the docstring or adding a TODO comment explaining the planned work.Would you like me to open an issue to track reimplementing the buffer metrics using the new
Sync.latencysignal?js/hang/src/watch/sync.ts (3)
97-112: Accumulatedslepttime may be inaccurate when sleep is interrupted.When
#updateresolves before the timeout completes (e.g., due to reference or latency changes),sleptis still incremented by the full requestedsleepduration (line 108), not the actual time elapsed. This could cause the returned value to overestimate how long the frame actually waited.If the return value is used for metrics or decisions, consider tracking actual elapsed time:
♻️ Proposed fix for accurate timing
async wait(timestamp: Time.Milli): Promise<Time.Milli> { let slept = Time.Milli.zero; if (!this.#reference) { throw new Error("reference not set; call update() first"); } for (;;) { const now = performance.now(); const ref = (now - timestamp) as Time.Milli; const sleep = this.#reference - ref + this.#latency.peek(); if (sleep <= 0) return slept; + const start = performance.now(); const wait = new Promise((resolve) => setTimeout(resolve, sleep)).then(() => true); const ok = await Promise.race([this.#update, wait]); - slept = (slept + sleep) as Time.Milli; + slept = (slept + (performance.now() - start)) as Time.Milli; if (ok) return slept; } }
30-42: Missing cleanup:signalsEffect is never closed.The
signalsEffect is initialized in the constructor but there's noclose()method to clean it up. IfSyncinstances are created and discarded, the Effect and its subscriptions will leak.Consider adding a
close()method:♻️ Proposed fix
+ close(): void { + this.signals.close(); + } }
25-28: Consider using the newSignal.promise()method.The manual
#update/#resolvepattern could be simplified using thepromise()method added toSignalin this PR. However, since the current pattern notifies on both reference and latency changes from different sources, keeping it manual may be intentional.js/hang/src/watch/audio/source.ts (1)
19-21: Unusual import placement.The
Syncimport at line 19 is placed between class fields and other imports, which is unconventional. Consider moving it to the top with the other imports for consistency.♻️ Proposed fix
import type * as Moq from "@moq/lite"; import type { Time } from "@moq/lite"; import { Effect, type Getter, Signal } from "@moq/signals"; import type * as Catalog from "../../catalog"; import * as Container from "../../container"; import * as Hex from "../../util/hex"; import * as libav from "../../util/libav"; import type * as Render from "./render"; +import type { Sync } from "../sync"; export type SourceProps = { // Enable to download the audio track. enabled?: boolean | Signal<boolean>; }; export interface AudioStats { bytesReceived: number; } -import type { Sync } from "../sync"; // Unfortunately, we need to use a Vite-exclusive import for now. import RenderWorklet from "./render-worklet.ts?worker&url";
jdreetz
left a comment
jdreetz
left a comment
There was a problem hiding this comment.
I think these changes are good. Just left a few nits about tweaking names/comments.
| // Computed latency = catalog.minBuffer + buffer | ||
| #latency: Latency; | ||
| readonly latency: Getter<Time.Milli>; | ||
| // Additional buffer in milliseconds (on top of catalog's minBuffer). |
There was a problem hiding this comment.
is this comment accurate?
| } | ||
| if (this.frame.peek() === undefined) { | ||
| // Render something while we wait for the sync to catch up. | ||
| this.frame.set(frame.clone()); |
| // TODO: This cause the `syncStatus` to be racey especially | ||
| await new Promise((resolve) => setTimeout(resolve, sleep)); | ||
| } | ||
| const wait = this.sync.wait(timestamp).then(() => true); |
There was a problem hiding this comment.
i'd leave a comment about what's this is doing.
| video?: Catalog.VideoConfig | Signal<Catalog.VideoConfig | undefined>; | ||
| } | ||
|
|
||
| export class Sync { |
There was a problem hiding this comment.
what is this syncing from/to? Broadcast? Should it be called BroadcastSync?
There was a problem hiding this comment.
Right now yes, but for tree-shaking reasons I want to move stuff out of Broadcast. You can technically use this Sync class across multiple broadcasts too if they decide to use the same timestamp clock.
There was a problem hiding this comment.
ex. the MSE.Video and WebCodecs.Video modules would take Catalog and Sync modules. Inverting the hierarchy means we can shake.
There was a problem hiding this comment.
modules would take Catalog and Sync modules
Probably makes testing easier too with dependency injection.
The b-frames "support" was well intentioned, but introduced a lot of latency. The main problem is that #ref is computed AFTER queuing, we only pop from the queue after sleeping for some amount. This means we were rendering frames significantly later than intended.
Instead of moving the ref computation before adding to the queue, I'd like to remove the queue. However, this means that frames are rendered in parallel. This is annoying but also means we support b-frames automatically. In fact maybe they just work now, who knows.
The
syncStatusis racey because of this change, and it was pretty wrong in the first place. I believe it would only flag when a frame needed to be rendered in the future, which is the exact opposite of what we wanted it to do anyway. Any buffering detection can't be keyed off frames received anyway; it should instead spin if frames are not forthcoming.