Add a buffer visualization #897
Conversation
- Add buffer visualization tool with timeline display - Improve audio decoder with better sync handling - Enhance MSE support for video playback - Add ring buffer improvements and tests - Update latency slider and watch controls Co-Authored-By: Claude Opus 4.5 <[email protected]>
kixelated
force-pushed
the
buffer-visualization
branch
from
b0d06bc to
257b9f2
Compare
WalkthroughThis pull request refactors the media playback system to introduce typed timing values and enhanced buffering mechanisms. Changes include replacing buffer parameters with jitter in HTML demos, updating the consumer API to expose group information via a new next() method instead of per-frame group fields, introducing BufferedRange types and buffered range tracking throughout audio and video pipelines, adding stalled and timestamp signals to decoders, renaming ring buffer's refill state to stalled, relocating timestamp and stalled properties from MultiBackend to VideoBackend, implementing a new BufferControl UI component for interactive buffer visualization, updating type signatures to use Moq.Time.Milli for temporal values, and extending the Setter API with an update() method for functional state updates. 🚥 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. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
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/backend.ts (1)
182-194:⚠️ Potential issue | 🟡 MinorDuplicate
mse.close()call in cleanup.
mse.close()is registered as cleanup on line 182, then called again inside the cleanup block at line 193. This results inclose()being invoked twice when the effect cleans up.Suggested fix
- effect.cleanup(() => mse.close()); - const video = new Video.Mse(mse, this.#videoSource); const audio = new Audio.Mse(mse, this.#audioSource, { volume: this.audio.volume, muted: this.audio.muted, }); effect.cleanup(() => { video.close(); audio.close(); mse.close(); });js/hang-demo/src/index.html (1)
20-31:⚠️ Potential issue | 🟡 MinorUpdate the attribute list to match the new
jitterattribute.Line 26 still documents
buffer, but Line 31 now usesjitter. Align the list to avoid confusion.✏️ Suggested doc fix
- - `buffer`: The (additional) buffer size in milliseconds. + - `jitter`: The target jitter buffer size in milliseconds.
🤖 Fix all issues with AI agents
In `@js/hang-ui/src/watch/components/BufferControl.tsx`:
- Around line 63-80: The document-level "mousemove" and "mouseup" listeners
added in onMouseDown (and removed in onMouseUp) are not cleaned up if the
component unmounts; add SolidJS onCleanup in the component to always remove
document.removeEventListener("mousemove", onMouseMove) and
document.removeEventListener("mouseup", onMouseUp") and call
setIsDragging(false) on unmount; import onCleanup if not present and place the
cleanup so it references the existing onMouseDown, onMouseMove, onMouseUp,
isDragging, and updateBufferFromMouseX handlers to ensure listeners are removed
regardless of component lifecycle.
In `@js/hang/src/watch/audio/decoder.ts`:
- Around line 344-360: The `#addDecodeBuffered` method currently returns
immediately after extending a matching range, which can leave that extended
range overlapping with later ranges; update the logic in `#addDecodeBuffered`
(operating on this.#decodeBuffered) so that after you extend a found range you
continue scanning subsequent ranges and merge any that now overlap or are
adjacent (adjusting start/end as needed and removing merged entries), then sort
or maintain the merged list before returning; ensure the early return is
replaced with merging logic that collapses all overlapping/adjacent ranges into
a single range.
🧹 Nitpick comments (3)
js/hang/src/container/legacy.ts (1)
67-72: Duplicate type definition withjs/hang/src/watch/backend.ts.
BufferedRangeandBufferedRangesare also defined injs/hang/src/watch/backend.ts(lines 9-14). Consider consolidating to a single source of truth to avoid drift.Options:
- Export from one location and import in the other
- Create a shared types module
js/hang-ui/src/watch/context.tsx (1)
159-167: Redundant effect for jitter synchronization.The effect at lines 164-167 reads the jitter signal and calls
setJitterwith the same value, which seems redundant sincejitteron line 53 already usessolid(props.hangWatch.jitter)to create a reactive accessor. This effect would causesetJitterto be called every time jitter changes externally, butsetJitterwrites back to the same signal — potentially creating unnecessary work.Consider whether this effect is needed, or if it's a leftover from a previous implementation.
js/hang/src/watch/video/decoder.ts (1)
392-409: Consider consolidating multiple overlapping ranges in#addBuffered.The current implementation merges a new range with the first overlapping range found, but if the new range spans multiple existing ranges, they won't be consolidated. For incremental decode buffer tracking, this is likely fine since ranges are added sequentially, but worth noting if behavior changes.
| const onMouseDown = (e: MouseEvent) => { | ||
| setIsDragging(true); | ||
| updateBufferFromMouseX(e.clientX); | ||
| document.addEventListener("mousemove", onMouseMove); | ||
| document.addEventListener("mouseup", onMouseUp); | ||
| }; | ||
|
|
||
| const onMouseMove = (e: MouseEvent) => { | ||
| if (isDragging()) { | ||
| updateBufferFromMouseX(e.clientX); | ||
| } | ||
| }; | ||
|
|
||
| const onMouseUp = () => { | ||
| setIsDragging(false); | ||
| document.removeEventListener("mousemove", onMouseMove); | ||
| document.removeEventListener("mouseup", onMouseUp); | ||
| }; |
There was a problem hiding this comment.
Missing cleanup for document event listeners on component unmount.
If the component unmounts while the user is dragging (e.g., navigation or conditional rendering), the mousemove and mouseup listeners will remain attached to the document. Consider using SolidJS's onCleanup to remove listeners when the component unmounts.
🛠️ Proposed fix
+import { createMemo, createSignal, For, onCleanup, Show } from "solid-js";
...
const onMouseUp = () => {
setIsDragging(false);
document.removeEventListener("mousemove", onMouseMove);
document.removeEventListener("mouseup", onMouseUp);
};
+
+ // Cleanup listeners on unmount
+ onCleanup(() => {
+ document.removeEventListener("mousemove", onMouseMove);
+ document.removeEventListener("mouseup", onMouseUp);
+ });🤖 Prompt for AI Agents
In `@js/hang-ui/src/watch/components/BufferControl.tsx` around lines 63 - 80, The
document-level "mousemove" and "mouseup" listeners added in onMouseDown (and
removed in onMouseUp) are not cleaned up if the component unmounts; add SolidJS
onCleanup in the component to always remove
document.removeEventListener("mousemove", onMouseMove) and
document.removeEventListener("mouseup", onMouseUp") and call
setIsDragging(false) on unmount; import onCleanup if not present and place the
cleanup so it references the existing onMouseDown, onMouseMove, onMouseUp,
isDragging, and updateBufferFromMouseX handlers to ensure listeners are removed
regardless of component lifecycle.
| return ( | ||
| <div class="bufferControlContainer"> | ||
| {/* Buffer Visualization - interactive, click/drag to set buffer */} | ||
| <div | ||
| class={`bufferVisualization ${isDragging() ? "dragging" : ""}`} | ||
| ref={containerRef} | ||
| onMouseDown={onMouseDown} | ||
| role="slider" | ||
| tabIndex={0} | ||
| aria-valuenow={context.jitter()} | ||
| aria-valuemin={MIN_RANGE} | ||
| aria-valuemax={maxRange()} | ||
| aria-label="Buffer jitter" | ||
| > |
There was a problem hiding this comment.
Missing keyboard support for slider accessibility.
The element has role="slider" and tabIndex={0} but lacks keyboard event handling. Users navigating with keyboard cannot adjust the jitter value. Consider adding onKeyDown to handle arrow keys.
♿ Proposed fix for keyboard accessibility
+ const onKeyDown = (e: KeyboardEvent) => {
+ const current = context.jitter();
+ let newValue = current;
+ if (e.key === "ArrowRight" || e.key === "ArrowUp") {
+ newValue = Math.min(maxRange(), current + RANGE_STEP) as Moq.Time.Milli;
+ } else if (e.key === "ArrowLeft" || e.key === "ArrowDown") {
+ newValue = Math.max(MIN_RANGE, current - RANGE_STEP) as Moq.Time.Milli;
+ } else {
+ return;
+ }
+ e.preventDefault();
+ context.setJitter(newValue);
+ };
return (
<div class="bufferControlContainer">
<div
class={`bufferVisualization ${isDragging() ? "dragging" : ""}`}
ref={containerRef}
onMouseDown={onMouseDown}
+ onKeyDown={onKeyDown}
role="slider"| #addDecodeBuffered(start: Time.Milli, end: Time.Milli): void { | ||
| if (start > end) return; | ||
|
|
||
| this.#decodeBuffered.mutate((current) => { | ||
| for (const range of current) { | ||
| // Extend range if new sample overlaps or is adjacent (1ms tolerance for float precision) | ||
| if (start <= range.end + 1 && end >= range.start) { | ||
| range.start = Math.min(range.start, start) as Time.Milli; | ||
| range.end = Math.max(range.end, end) as Time.Milli; | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| current.push({ start, end }); | ||
| current.sort((a, b) => a.start - b.start); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Range merging can leave overlapping ranges after extension.
When a new sample extends an existing range, the updated range might now overlap with subsequent ranges, but those aren't merged. For example, with ranges [(0,5), (10,15)] and adding (4,11):
- First range extends to
(0,11) - Function returns early
- Result:
[(0,11), (10,15)]— overlapping ranges remain
This could cause buffer visualization to show incorrect/duplicate coverage.
🔧 Proposed fix to merge all overlapping ranges
`#addDecodeBuffered`(start: Time.Milli, end: Time.Milli): void {
if (start > end) return;
this.#decodeBuffered.mutate((current) => {
- for (const range of current) {
- // Extend range if new sample overlaps or is adjacent (1ms tolerance for float precision)
- if (start <= range.end + 1 && end >= range.start) {
- range.start = Math.min(range.start, start) as Time.Milli;
- range.end = Math.max(range.end, end) as Time.Milli;
- return;
- }
- }
-
- current.push({ start, end });
- current.sort((a, b) => a.start - b.start);
+ // Add the new range and re-merge everything
+ current.push({ start, end });
+ current.sort((a, b) => a.start - b.start);
+
+ // Merge overlapping/adjacent ranges in-place
+ let write = 0;
+ for (let read = 1; read < current.length; read++) {
+ if (current[read].start <= current[write].end + 1) {
+ current[write].end = Math.max(current[write].end, current[read].end) as Time.Milli;
+ } else {
+ write++;
+ current[write] = current[read];
+ }
+ }
+ current.length = write + 1;
});
}🤖 Prompt for AI Agents
In `@js/hang/src/watch/audio/decoder.ts` around lines 344 - 360, The
`#addDecodeBuffered` method currently returns immediately after extending a
matching range, which can leave that extended range overlapping with later
ranges; update the logic in `#addDecodeBuffered` (operating on
this.#decodeBuffered) so that after you extend a found range you continue
scanning subsequent ranges and merge any that now overlap or are adjacent
(adjusting start/end as needed and removing merged entries), then sort or
maintain the merged list before returning; ensure the early return is replaced
with merging logic that collapses all overlapping/adjacent ranges into a single
range.
Helps a lot with debugging.