CMAF playback v4 #896
CMAF playback v4 #896
Conversation
Co-authored-by: Emil Santurio <[email protected]>
I delete <hang-meet> because it's pretty useless. We'll replace it with hang.live functionality soon.
WalkthroughAdds Media Source Extensions (MSE) playback and comprehensive CMAF (fMP4) encoding utilities. Introduces MSE muxer and MSE-based video/audio backends, WebCodecs decoders, AudioWorklet-based audio decoder/emitter, and a MultiBackend that unifies WebCodecs and MSE paths. Refactors the watch stack: new Backend interfaces, signal-based audio/video backends, Source/Decoder/Renderer adjustments, and a swap from a buffer-based sync model to a jitter-based sync. Updates demos and UI attributes to surface jitter, buffering, and timestamp signals; removes an older track-rewrite utility. 🚥 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: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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/broadcast.ts (1)
50-83:⚠️ Potential issue | 🟡 MinorReset
#announcedon disable/path unset to avoid stale state.
Line 52 and Line 65 return early without clearing, so togglingenabled/pathcan leave#announcedtrue and bypass the wait‑for‑announce semantics.🛠️ Proposed safeguard
- const enabled = effect.get(this.enabled); - if (!enabled) return; + const enabled = effect.get(this.enabled); + if (!enabled) { + effect.set(this.#announced, false, false); + return; + } @@ - const conn = effect.get(this.connection); - if (!conn) return; + const conn = effect.get(this.connection); + if (!conn) { + effect.set(this.#announced, false, false); + return; + } @@ - const path = effect.get(this.path); - if (path === undefined) return; + const path = effect.get(this.path); + if (path === undefined) { + effect.set(this.#announced, false, false); + return; + }js/hang-ui/src/watch/context.tsx (1)
144-167:⚠️ Potential issue | 🔴 CriticalRemove the redundant jitter effect that creates a feedback loop
The effect at lines 164-167 reads
watch.jitterand immediately writes it back viasetJitter(). This creates a circular dependency: the effect triggers on jitter changes, callssetJitter()which setswatch.jitteragain, triggering the effect anew. Sincejitteris already properly wrapped viasolid(watch.jitter)at line 53 and exposed in the context, this effect serves no purpose and should be deleted.🛠️ Suggested fix
-signals.effect((effect) => { - const jitter = effect.get(watch.jitter); - setJitter(jitter); -});
🤖 Fix all issues with AI agents
In `@js/hang-demo/src/index.html`:
- Line 44: The build config is missing mse.html as an entry; open vite.config.ts
and add the mse input key to rollupOptions.input so the HTML is built (e.g., add
an entry like mse: "mse.html" alongside the existing watch/publish/support keys
in the input object) ensuring the symbol rollupOptions.input in vite.config.ts
includes mse.
In `@js/hang/src/watch/backend.ts`:
- Around line 110-115: The `#buffering` Signal is never updated so consumers
always read false; wire it to the backend/MSE buffering lifecycle by setting
this.#buffering.value appropriately where buffering starts/stops (e.g., on
MediaSource/sourceBuffer events and backend fetch/append state). Locate the
backend functions that handle MediaSource open/close, SourceBuffer.updating,
appendBuffer start/complete, and any fetch/seek/error handlers and set
this.#buffering.value = true when buffering begins (e.g., sourceBuffer.updating
true, fetch started, or no buffered ranges) and this.#buffering.value = false
when buffering ends (e.g., sourceBuffer.updating false, data appended, or enough
buffered ranges); also initialize it from current MSE state during setup so it
reflects the real initial state. Ensure you reference the existing symbols
`#buffering` and readonly buffering when making these updates so consumers keep
receiving updates.
- Around line 124-129: The Video.Source and Audio.Source instances are
constructed without the supported checker, causing source.supported to be
undefined and `#runSupported`() to short-circuit; instead pass the supported
signal into the Source constructors so it is initialized inline. Update the
instantiation of this.#videoSource and this.#audioSource to include a supported
option (the same value later passed to source.supported.set(supported)), and
remove the out-of-band source.supported.set(supported) calls in Decoder/MSE
backends so supported is established during construction and `#runSupported`() can
read it immediately.
- Around line 185-206: The Video.Mse and Audio.Mse instances created in `#runMse`
are not closed on cleanup, leaking internal Effect signals; update the cleanup
registered via effect.cleanup in `#runMse` to call video.close() and audio.close()
(in addition to mse.close()) so that Video.Mse.close() and Audio.Mse.close() run
and their internal signals/sources are cleaned up; ensure this happens before or
alongside mse.close() to avoid dangling resources.
In `@js/hang/src/watch/element.ts`:
- Around line 58-75: The MultiBackend's internal Effect (accessible as
this.#backend.signals) is never torn down, leaking decoders/sources; in the
HangWatch cleanup hook (where observer.disconnect() is registered via
this.signals.cleanup) also call the backend's signals cleanup to release those
resources. Update the cleanup to invoke this.#backend.signals.cleanup() (guarded
if necessary) when the component is cleaned up so MultiBackend's signals are
properly disposed.
- Around line 152-162: attributeChangedCallback is setting this.sync.jitter (and
alias "latency") but playback uses the backend's jitter signal
(this.#backend.jitter), so update the attribute handling to set the backend
jitter signal instead (use this.#backend.jitter.set((newValue ?
Number.parseFloat(newValue) : 100) as Time.Milli)) for both "jitter" and
"latency"); ensure the public jitter getter still returns this.#backend.jitter
so they remain consistent; reference element method attributeChangedCallback,
fields this.sync and this.#backend, and backend class MultiBackend/Jitter signal
in backend.ts when making the change.
In `@js/hang/src/watch/mse.ts`:
- Around line 34-46: The jitter Signal from MuxerProps isn't propagated to the
Sync instance, so update Sync whenever this.jitter changes: in the constructor
add an effect that reads this.jitter and assigns it to this.#sync.jitter (e.g.
this.#signals.effect(() => { this.#sync.jitter = this.jitter.value; })),
ensuring you reference the existing this.jitter Signal, the Sync instance
this.#sync and the signaling system this.#signals.effect so MuxerProps.jitter
actually influences latency/skip behavior.
- Around line 79-94: The skip currently advances element.currentTime to the
buffer end (last + 0.1) which ignores the computed latency; change the seek
target in the effect.interval block so it respects latency by setting the new
time to last - latency + 0.1 (rather than last + 0.1), and guard it so you never
seek backwards (only set element.currentTime if newTime > element.currentTime)
and clamp newTime to at most last to avoid overshooting; update the code around
effect.interval, the latency variable usage, and the element.currentTime
assignment accordingly.
- Around line 134-146: The catch block in `#runPaused`(effect: Effect) currently
calls this.paused.set(false) when element.play() rejects, which keeps the UI
state incorrect; update the catch handler to set this.paused.set(true) so the
paused signal matches the element.paused state (and optionally include the error
in the console.error message) to avoid re-triggering play attempts while the
element remains paused.
In `@js/hang/src/watch/video/decoder.ts`:
- Around line 207-263: The CMAF path in `#runCmafTrack` doesn't race
sub.nextGroup() and group.readFrame() against effect.cancel, so reads can
continue after cancellations; update the loops to mirror `#runLegacyTrack` by
awaiting Promise.race([sub.nextGroup(), effect.cancel]) when getting group and
Promise.race([group.readFrame(), effect.cancel]) when reading frames, handle the
cancel result (break/return appropriately), and ensure group.close() remains in
finally so resources are cleaned up on cancellation.
🟡 Minor comments (12)
js/hang/src/container/cmaf/encode.ts-841-844 (1)
841-844:⚠️ Potential issue | 🟡 MinorESDS size field assumes descriptor fits in single byte.
The ES_Descriptor size is written as a single byte (line 843), which limits it to values < 128. For most AAC configurations this is fine, but if
audioSpecificConfigis large (e.g., with SBR/PS extensions), this could overflow.Consider adding a guard or using the expandable size encoding if this needs to support larger configs:
// ES_Descriptor esds[offset++] = 0x03; // tag + if (esDescSize >= 128) { + throw new Error("ES_Descriptor too large for single-byte size encoding"); + } esds[offset++] = esDescSize; // size (assuming < 128)js/hang/src/container/cmaf/encode.ts-914-922 (1)
914-922:⚠️ Potential issue | 🟡 MinorHardcoded Opus PreSkip value should query encoder-reported lookahead.
The PreSkip value of 312 samples assumes the default libopus encoder lookahead but is not mandated by the Opus specification (RFC 7845). Different encoder implementations and configurations can have different lookahead values. The Opus API explicitly recommends querying this value via
OPUS_GET_LOOKAHEADfrom the encoder rather than hardcoding a constant, to avoid audio timing mismatches at playback start. This fallback is only used when a description is not provided; when description is available, the correct OpusHead is used instead.js/hang/src/watch/sync.ts-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorUpdate comment/error text to
received().
Line 23 still says “buffer,” and Line 79 referencesupdate()after the rename.✏️ Text fixes
- // The buffer required, based on both audio and video. + // The latency required, based on jitter + audio/video delay. @@ - throw new Error("reference not set; call update() first"); + throw new Error("reference not set; call received() first");Also applies to: 79-80
js/hang/src/watch/audio/source.ts-97-117 (1)
97-117:⚠️ Potential issue | 🟡 MinorClear selection when no renditions remain.
Line 98 returns early on emptyavailablewithout clearing track/config/jitter, leaving stale selection.🧽 Suggested reset
- const available = effect.get(this.#available); - if (Object.keys(available).length === 0) return; + const available = effect.get(this.#available); + if (Object.keys(available).length === 0) { + effect.set(this.#track, undefined); + effect.set(this.#config, undefined); + effect.set(this.sync.audio, undefined); + return; + }js/hang/src/watch/audio/source.ts-66-74 (1)
66-74:⚠️ Potential issue | 🟡 MinorClear catalog when broadcast/catalog disappears.
Line 68 and Line 71 return without clearing#catalog, leaving stale data after disconnects.🧹 Proposed cleanup
- const broadcast = effect.get(this.broadcast); - if (!broadcast) return; + const broadcast = effect.get(this.broadcast); + if (!broadcast) { + effect.set(this.#catalog, undefined); + return; + } @@ - const catalog = effect.get(broadcast.catalog)?.audio; - if (!catalog) return; + const catalog = effect.get(broadcast.catalog)?.audio; + if (!catalog) { + effect.set(this.#catalog, undefined); + return; + }js/hang-demo/src/mse.html-22-25 (1)
22-25:⚠️ Potential issue | 🟡 MinorChange
bufferattribute tojitter.
Line 23 usesbuffer="150", but the hang-watch component only recognizesjitter(orlatencyas a legacy alias). Thebufferattribute will be ignored.Suggested fix
- <hang-watch id="watch" url="%VITE_RELAY_URL%" path="bbb" muted buffer="150" reload> + <hang-watch id="watch" url="%VITE_RELAY_URL%" path="bbb" muted jitter="150" reload>js/hang/src/watch/video/mse.ts-57-60 (1)
57-60:⚠️ Potential issue | 🟡 MinorCleanup order: abort before removeSourceBuffer.
Aborting a SourceBuffer after removal may throw. The safer order is to abort first, then remove.
🔧 Suggested fix
effect.cleanup(() => { + sourceBuffer.abort(); mediaSource.removeSourceBuffer(sourceBuffer); - sourceBuffer.abort(); });js/hang/src/watch/audio/mse.ts-64-67 (1)
64-67:⚠️ Potential issue | 🟡 MinorSame cleanup order issue: abort before removeSourceBuffer.
Same issue as in video/mse.ts - abort should happen before removal.
🔧 Suggested fix
effect.cleanup(() => { + sourceBuffer.abort(); mediaSource.removeSourceBuffer(sourceBuffer); - sourceBuffer.abort(); });js/hang/src/watch/audio/mse.ts-183-203 (1)
183-203:⚠️ Potential issue | 🟡 MinorIncomplete bidirectional sync for muted state.
The
volumechangeevent handler (line 200-202) only syncsvolumeback to the signal, but notmuted. If the user toggles mute via browser controls, themutedsignal won't update.🔧 Suggested fix
effect.event(element, "volumechange", () => { this.volume.set(element.volume); + this.muted.set(element.muted); });js/hang/src/watch/video/mse.ts-25-25 (1)
25-25:⚠️ Potential issue | 🟡 Minor
signalsshould be private for consistency.Other files use
#signalsas a private field. This appears to be an oversight.🔧 Suggested fix
- signals = new Effect(); + `#signals` = new Effect();And update the reference in
close():- this.signals.close(); + this.#signals.close();js/hang/src/watch/audio/decoder.ts-158-168 (1)
158-168:⚠️ Potential issue | 🟡 MinorMissing error handling for decoder.configure.
decoder.configure()can throw if the configuration is invalid. Whilesupported()is called beforehand, race conditions or catalog mismatches could still cause failures. Consider wrapping in try-catch.🛡️ Proposed fix
+ try { decoder.configure({ ...config, description, }); + } catch (e) { + console.error("[Audio Decoder] Configuration failed:", e); + return; + }js/hang/src/watch/mse.ts-52-56 (1)
52-56:⚠️ Potential issue | 🟡 MinorCapture the URL in a variable to ensure correct cleanup
When the
elementSignal changes,#runMediaSourceruns again and assigns a new URL toelement.src. Previous cleanup functions will revoke the wrong URL since they referenceelement.srcwhich now points to the new URL instead of the original one. Store the URL in a local variable.🛠️ Suggested fix
- const mediaSource = new MediaSource(); - - element.src = URL.createObjectURL(mediaSource); - effect.cleanup(() => URL.revokeObjectURL(element.src)); + const mediaSource = new MediaSource(); + const url = URL.createObjectURL(mediaSource); + + element.src = url; + effect.cleanup(() => URL.revokeObjectURL(url));
🧹 Nitpick comments (15)
justfile (1)
207-217: Consider adding error handling when variant playlists are not found.The master.m3u8 wait loop (lines 194-205) exits with an error if the file isn't generated, but this variant playlist wait loop silently continues if none of the playlists appear. This could lead to a less informative failure later when
hangtries to read the playlist.🔧 Suggested improvement for consistency
for i in {1..20}; do # Check if at least one variant playlist exists if [ -f "$OUT_DIR/v0/stream.m3u8" ] || [ -f "$OUT_DIR/v720/stream.m3u8" ] || [ -f "$OUT_DIR/v144/stream.m3u8" ] || [ -f "$OUT_DIR/vaudio/stream.m3u8" ]; then break fi sleep 0.5 done + + # Verify at least one variant playlist was created + if ! [ -f "$OUT_DIR/v720/stream.m3u8" ] && ! [ -f "$OUT_DIR/v144/stream.m3u8" ] && ! [ -f "$OUT_DIR/vaudio/stream.m3u8" ]; then + kill $FFMPEG_PID 2>/dev/null || true + echo "Error: No variant playlists generated in time" + exit 1 + fijs/hang/src/publish/audio/encoder.ts (1)
117-127: Consider removing unusedsourcefrom getAll.The
_sourcevariable is extracted but never used in this effect. If the intent is only to ensure the worklet is available, you could simplify:♻️ Suggested simplification
`#runConfig`(effect: Effect): void { - const values = effect.getAll([this.source, this.#worklet]); - if (!values) return; - const [_source, worklet] = values; + const worklet = effect.get(this.#worklet); + if (!worklet) return; const config = {However, if the dependency on
sourceis intentional (e.g., to re-run config when source changes), this is fine as-is.js/hang/src/container/cmaf/encode.ts (2)
236-239: Incomplete TODO comment.The TODO comment on line 237 appears truncated ("We could" with no completion). Consider completing or removing this comment.
if (!codedWidth || !codedHeight || !description) { - // TODO: We could throw new Error("Missing required fields to create video init segment"); }
786-789: Silent fallback for unknown sample rates.Unknown sample rates silently default to 44100Hz index. Consider logging a warning when the sample rate isn't in the lookup table, as this could cause audio/video sync issues.
- const freqIndex = sampleRateIndex[sampleRate] ?? 4; // Default to 44100 if not found + const freqIndex = sampleRateIndex[sampleRate]; + if (freqIndex === undefined) { + console.warn(`Unknown sample rate ${sampleRate}, defaulting to 44100Hz index`); + } + const resolvedFreqIndex = freqIndex ?? 4;js/hang/src/watch/audio/source.ts (1)
31-64: ExposetrackasGetterto keep selection read-only and align withvideo/source.ts.Line 42 exposes
trackas aSignal, which allows external mutation. The video source implementation exposes it as aGetter, maintaining consistency across the API surface.🔧 Suggested type tightening
- readonly track: Signal<string | undefined> = this.#track; + readonly track: Getter<string | undefined> = this.#track;js/hang-ui/src/watch/components/LatencySlider.tsx (1)
28-30: Avoid redundant signal reads in the same render cycle.
context.jitter()is called twice on line 30 - once for the conditional check and once for the display value. In reactive frameworks, each call may trigger a subscription. Consider storing the value once:♻️ Suggested optimization
- <span>{typeof context.jitter() !== "undefined" ? `${Math.round(context.jitter())}ms` : ""}</span> + <span>{(() => { + const jitter = context.jitter(); + return typeof jitter !== "undefined" ? `${Math.round(jitter)}ms` : ""; + })()}</span>Or use a derived signal/memo if SolidJS patterns prefer that approach.
js/hang/src/watch/video/renderer.ts (1)
124-129: Consider exposing flip as a direct property on Decoder.The deep property chain
this.decoder.source.catalog.peek()?.flipcouples Renderer tightly to the internal structure of Source and Catalog. If flip is commonly accessed, consider exposing it directly on Decoder for a cleaner API.js/hang-ui/src/shared/components/stats/types.ts (1)
28-32: Track the TODO for future simplification.The TODO suggests using
Hang.Watch.Backenddirectly instead of the customProviderPropsshape. Consider creating an issue to track this cleanup once the API stabilizes.Would you like me to open an issue to track this TODO for future API consolidation?
js/hang/src/watch/audio/decoder.ts (2)
49-51: Thesource.supported.set(supported)pattern needs revisiting.The "super hacky" comment acknowledges this is not ideal. Setting a function on the source from the decoder creates a tight coupling and unclear ownership. Consider a more explicit dependency injection or factory pattern.
110-118: Unused destructured variable and missing AudioContext suspension.Line 113 destructures
_which is unused. Additionally, the comment on line 117 suggests disconnecting/reconnecting the worklet when disabled, but there's no corresponding logic to suspend the AudioContext whenenabledbecomes false - only resume when enabled.♻️ Consider adding suspension logic
`#runEnabled`(effect: Effect): void { const values = effect.getAll([this.enabled, this.#context]); if (!values) return; - const [_, context] = values; + const [enabled, context] = values; - context.resume(); + if (enabled) { + context.resume(); + } else { + context.suspend(); + } - - // NOTE: You should disconnect/reconnect the worklet to save power when disabled. }js/hang/src/watch/video/mse.ts (2)
77-87: Simplify#appendBufferby removing redundant pre-wait.The initial
while (sourceBuffer.updating)loop (lines 78-80) is redundant ifappendBufferis always called sequentially. The post-append wait (lines 84-86) ensures completion. If concurrent appends are possible, consider using a queue instead.♻️ Simplified version
async `#appendBuffer`(sourceBuffer: SourceBuffer, buffer: Uint8Array): Promise<void> { - while (sourceBuffer.updating) { - await new Promise((resolve) => sourceBuffer.addEventListener("updateend", resolve, { once: true })); - } - sourceBuffer.appendBuffer(buffer as BufferSource); while (sourceBuffer.updating) { await new Promise((resolve) => sourceBuffer.addEventListener("updateend", resolve, { once: true })); } }If the pre-wait is intentional for concurrent safety, add a comment explaining the pattern.
189-191:supported()function in mse.ts doesn't need to be async.MediaSource.isTypeSupported()is synchronous, making the async wrapper unnecessary overhead.♻️ Simplified version
-async function supported(config: Catalog.VideoConfig): Promise<boolean> { - return MediaSource.isTypeSupported(`video/mp4; codecs="${config.codec}"`); -} +function supported(config: Catalog.VideoConfig): boolean { + return MediaSource.isTypeSupported(`video/mp4; codecs="${config.codec}"`); +}Note: This also requires updating the
Supportedtype definition insource.ts(line 10) to accept(config: Catalog.VideoConfig) => booleanin addition to the Promise variant, or using a separate type for MSE implementations. The decoder implementations should remain async as they call async APIs.js/hang/src/watch/audio/mse.ts (2)
87-95: Consider extracting shared#appendBufferhelper.The
#appendBufferimplementation is identical between audio/mse.ts and video/mse.ts. Consider extracting to a shared utility in the backend module to reduce duplication.
210-212: Same as video:supported()doesn't need async wrapper.
MediaSource.isTypeSupportedis synchronous.js/hang/src/watch/mse.ts (1)
120-132: Resolve the TODO on buffering eventsOnly
waiting/playing/seekingare wired. Consider coveringstalled/canplay(or a readyState poll) to avoid missed buffering transitions. Happy to help refine this.
| #buffering = new Signal<boolean>(false); | ||
| readonly buffering: Getter<boolean> = this.#buffering; | ||
|
|
||
| #timestamp = new Signal<number>(0); | ||
| readonly timestamp: Getter<number> = this.#timestamp; | ||
|
|
There was a problem hiding this comment.
buffering is never updated.
Line 110 initializes #buffering, but no effect updates it, so consumers will always see false. Please wire it to backend/MSE buffering state.
🤖 Prompt for AI Agents
In `@js/hang/src/watch/backend.ts` around lines 110 - 115, The `#buffering` Signal
is never updated so consumers always read false; wire it to the backend/MSE
buffering lifecycle by setting this.#buffering.value appropriately where
buffering starts/stops (e.g., on MediaSource/sourceBuffer events and backend
fetch/append state). Locate the backend functions that handle MediaSource
open/close, SourceBuffer.updating, appendBuffer start/complete, and any
fetch/seek/error handlers and set this.#buffering.value = true when buffering
begins (e.g., sourceBuffer.updating true, fetch started, or no buffered ranges)
and this.#buffering.value = false when buffering ends (e.g.,
sourceBuffer.updating false, data appended, or enough buffered ranges); also
initialize it from current MSE state during setup so it reflects the real
initial state. Ensure you reference the existing symbols `#buffering` and readonly
buffering when making these updates so consumers keep receiving updates.
| this.#videoSource = new Video.Source(this.#sync, { | ||
| broadcast: this.broadcast, | ||
| }); | ||
| this.#audioSource = new Audio.Source(this.#sync, { | ||
| broadcast: this.broadcast, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the backend.ts file to understand the context
cat -n js/hang/src/watch/backend.ts | head -150 | tail -50Repository: moq-dev/moq
Length of output: 1809
🏁 Script executed:
# Search for where Source.supported is set
rg -n "\.supported\s*=" js/hang/src/watch -g '*.ts' -C 3Repository: moq-dev/moq
Length of output: 1102
🏁 Script executed:
# Look for Source class definitions and initialization
rg -n "class.*Source" js/hang/src/watch -g '*.ts' -C 3Repository: moq-dev/moq
Length of output: 1085
🏁 Script executed:
# Search for selection logic that might depend on supported
rg -n "selection|select" js/hang/src/watch/backend.ts -C 3Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Check how Video.Source and Audio.Source are imported and used
rg -n "from.*Video|from.*Audio|import.*Video|import.*Audio" js/hang/src/watch/backend.ts -C 2Repository: moq-dev/moq
Length of output: 393
🏁 Script executed:
# Look at MSE backend implementation
find js/hang/src/watch -name "*mse*" -o -name "*decoder*" | head -20Repository: moq-dev/moq
Length of output: 211
🏁 Script executed:
# Search for where supported is set on sources (assignment, not just initialization)
rg -n "source\.supported\s*=" js/hang/src/watch -g '*.ts' -C 3Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Look for VideoBackend and AudioBackend to see how they use sources
rg -n "class.*Backend" js/hang/src/watch -g '*.ts' -C 2Repository: moq-dev/moq
Length of output: 2472
🏁 Script executed:
# Search for the selection/rendition selection logic
rg -n "#runSupported|runSupported" js/hang/src/watch -g '*.ts' -C 3Repository: moq-dev/moq
Length of output: 1910
🏁 Script executed:
# Look at the full `#runSupported` method in video/source.ts
cat -n js/hang/src/watch/video/source.ts | sed -n '75,95p'Repository: moq-dev/moq
Length of output: 851
🏁 Script executed:
# Look at the full `#runSupported` method in audio/source.ts
cat -n js/hang/src/watch/audio/source.ts | sed -n '76,96p'Repository: moq-dev/moq
Length of output: 842
🏁 Script executed:
# Check how video/decoder.ts uses the Source
cat -n js/hang/src/watch/video/decoder.ts | head -80Repository: moq-dev/moq
Length of output: 3614
🏁 Script executed:
# Check how video/mse.ts uses the Source
cat -n js/hang/src/watch/video/mse.ts | head -80Repository: moq-dev/moq
Length of output: 3052
Wire Source.supported during construction instead of post-hoc.
Video.Source and Audio.Source are initialized without the supported checker (lines 124–129), leaving source.supported undefined. While Decoder and MSE backends currently set it afterward with source.supported.set(supported) (marked "super hacky" in the code), the selection logic in #runSupported() returns early if supported is undefined. Pass supported via the Source constructor options to avoid relying on out-of-band initialization.
🤖 Prompt for AI Agents
In `@js/hang/src/watch/backend.ts` around lines 124 - 129, The Video.Source and
Audio.Source instances are constructed without the supported checker, causing
source.supported to be undefined and `#runSupported`() to short-circuit; instead
pass the supported signal into the Source constructors so it is initialized
inline. Update the instantiation of this.#videoSource and this.#audioSource to
include a supported option (the same value later passed to
source.supported.set(supported)), and remove the out-of-band
source.supported.set(supported) calls in Decoder/MSE backends so supported is
established during construction and `#runSupported`() can read it immediately.
| this.#backend = new MultiBackend({ | ||
| broadcast: this.broadcast, | ||
| }); | ||
| this.signals.cleanup(() => this.audio.close()); | ||
|
|
||
| // Watch to see if the canvas element is added or removed. | ||
| const setCanvas = () => { | ||
| this.canvas.set(this.querySelector("canvas") as HTMLCanvasElement | undefined); | ||
| const setElement = () => { | ||
| const canvas = this.querySelector("canvas") as HTMLCanvasElement | undefined; | ||
| const video = this.querySelector("video") as HTMLVideoElement | undefined; | ||
| if (canvas && video) { | ||
| throw new Error("Cannot have both canvas and video elements"); | ||
| } | ||
| this.#backend.element.set(canvas ?? video); | ||
| }; | ||
| const observer = new MutationObserver(setCanvas); | ||
|
|
||
| const observer = new MutationObserver(setElement); | ||
| observer.observe(this, { childList: true, subtree: true }); | ||
| this.signals.cleanup(() => observer.disconnect()); | ||
| setCanvas(); | ||
| setElement(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find MultiBackend class definition
rg -n "class MultiBackend" --type ts --type tsxRepository: moq-dev/moq
Length of output: 82
🏁 Script executed:
# Search for close method in backend files
fd -e ts -e tsx | xargs rg -l "MultiBackend" | head -10Repository: moq-dev/moq
Length of output: 112
🏁 Script executed:
# Look at element.ts to see connection and broadcast cleanup
rg -n "cleanup\|connection\|broadcast" js/hang/src/watch/element.ts -A 2 -B 2Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Search for MultiBackend class definition
rg -n "class MultiBackend" js/hang/src/watch/backend.tsRepository: moq-dev/moq
Length of output: 104
🏁 Script executed:
# Search for close method definitions in backend files
rg -n "close\(" js/hang/src/watch/backend.ts -A 3Repository: moq-dev/moq
Length of output: 405
🏁 Script executed:
# Read element.ts to see current cleanup patterns
cat -n js/hang/src/watch/element.ts | head -100Repository: moq-dev/moq
Length of output: 4094
🏁 Script executed:
# Read MultiBackend class to check for close() method
cat -n js/hang/src/watch/backend.ts | sed -n '95,250p'Repository: moq-dev/moq
Length of output: 4197
🏁 Script executed:
# Check full length of backend.ts
wc -l js/hang/src/watch/backend.tsRepository: moq-dev/moq
Length of output: 87
🏁 Script executed:
# Search for close method in entire backend.ts
rg -n "close" js/hang/src/watch/backend.tsRepository: moq-dev/moq
Length of output: 210
🏁 Script executed:
# Check if MultiBackend extends or implements something that provides close()
rg -n "class MultiBackend\|interface Backend" js/hang/src/watch/backend.ts -A 2Repository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Find Backend interface definition
rg -n "interface Backend" js/hang/src/watch/Repository: moq-dev/moq
Length of output: 241
🏁 Script executed:
# Search entire backend.ts for close() method definition (not just calls)
rg -n "^\s*close\(\)" js/hang/src/watch/backend.tsRepository: moq-dev/moq
Length of output: 37
🏁 Script executed:
# Read the end of backend.ts to see if there's a close method
tail -20 js/hang/src/watch/backend.ts | cat -nRepository: moq-dev/moq
Length of output: 730
🏁 Script executed:
# Read Backend interface definition in backend.ts
rg -n "interface Backend" js/hang/src/watch/backend.ts -A 20Repository: moq-dev/moq
Length of output: 621
🏁 Script executed:
# Check if MultiBackend has a signals.close() pattern like in element.ts
rg -n "signals\." js/hang/src/watch/backend.tsRepository: moq-dev/moq
Length of output: 172
🏁 Script executed:
# Check if signals property is public or private in MultiBackend
rg -n "signals" js/hang/src/watch/backend.ts -B 1Repository: moq-dev/moq
Length of output: 654
🏁 Script executed:
# Look at how Effect.close() works and if it's called elsewhere
rg -n "signals\.close\(\)" js/hang/src/watch/Repository: moq-dev/moq
Length of output: 1316
🏁 Script executed:
# Check what happens when element is removed - does `#runElement` cleanup?
rg -n "element" js/hang/src/watch/backend.ts -B 2 -A 2 | head -40Repository: moq-dev/moq
Length of output: 1513
Close #backend.signals when HangWatch is cleaned up
#backend owns a signals Effect that manages Video/Audio sources and decoders. This cleanup is missing while connection and broadcast are properly closed, creating a resource leak.
🛠️ Suggested fix
this.#backend = new MultiBackend({
broadcast: this.broadcast,
});
+ this.signals.cleanup(() => this.#backend.signals.close());Note: MultiBackend does not have a close() method; the cleanup must target the signals property instead.
🤖 Prompt for AI Agents
In `@js/hang/src/watch/element.ts` around lines 58 - 75, The MultiBackend's
internal Effect (accessible as this.#backend.signals) is never torn down,
leaking decoders/sources; in the HangWatch cleanup hook (where
observer.disconnect() is registered via this.signals.cleanup) also call the
backend's signals cleanup to release those resources. Update the cleanup to
invoke this.#backend.signals.cleanup() (guarded if necessary) when the component
is cleaned up so MultiBackend's signals are properly disposed.
| constructor(sync: Sync, props?: MuxerProps) { | ||
| this.element = Signal.from(props?.element); | ||
| this.paused = Signal.from(props?.paused ?? false); | ||
| this.jitter = Signal.from(props?.jitter ?? (100 as Time.Milli)); | ||
| this.#sync = sync; | ||
|
|
||
| this.#signals.effect(this.#runMediaSource.bind(this)); | ||
| this.#signals.effect(this.#runSkip.bind(this)); | ||
| this.#signals.effect(this.#runTrim.bind(this)); | ||
| this.#signals.effect(this.#runBuffering.bind(this)); | ||
| this.#signals.effect(this.#runPaused.bind(this)); | ||
| this.#signals.effect(this.#runTimestamp.bind(this)); | ||
| } |
There was a problem hiding this comment.
Muxer.jitter is currently a no‑op
The jitter signal never updates Sync, so MuxerProps.jitter won’t affect latency/skip behavior. Wire it into sync.jitter (or drop the prop).
🛠️ Suggested fix
this.jitter = Signal.from(props?.jitter ?? (100 as Time.Milli));
this.#sync = sync;
+ this.#signals.effect((effect) => {
+ effect.set(this.#sync.jitter, effect.get(this.jitter));
+ });
this.#signals.effect(this.#runMediaSource.bind(this));🤖 Prompt for AI Agents
In `@js/hang/src/watch/mse.ts` around lines 34 - 46, The jitter Signal from
MuxerProps isn't propagated to the Sync instance, so update Sync whenever
this.jitter changes: in the constructor add an effect that reads this.jitter and
assigns it to this.#sync.jitter (e.g. this.#signals.effect(() => {
this.#sync.jitter = this.jitter.value; })), ensuring you reference the existing
this.jitter Signal, the Sync instance this.#sync and the signaling system
this.#signals.effect so MuxerProps.jitter actually influences latency/skip
behavior.
| // Use the computed latency (catalog jitter + user jitter) | ||
| const latency = effect.get(this.#sync.latency); | ||
|
|
||
| effect.interval(() => { | ||
| // Skip over gaps based on the effective latency. | ||
| const buffered = element.buffered; | ||
| if (buffered.length === 0) return; | ||
|
|
||
| const last = buffered.end(buffered.length - 1); | ||
| const diff = last - element.currentTime; | ||
|
|
||
| // We seek an extra 100ms because seeking isn't free/instant. | ||
| if (diff > latency && diff > 0.1) { | ||
| console.warn("skipping ahead", diff, "seconds"); | ||
| element.currentTime += diff + 0.1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n js/hang/src/watch/mse.ts | sed -n '75,100p'Repository: moq-dev/moq
Length of output: 1050
Skip should respect the target latency
The seek jumps to last + 0.1, ignoring the latency variable entirely and seeking to the buffer end. This contradicts the condition if (diff > latency) which only triggers when behind by more than the target latency. Seek to last - latency + 0.1 instead to maintain the intended gap.
🛠️ Suggested fix
- if (diff > latency && diff > 0.1) {
- console.warn("skipping ahead", diff, "seconds");
- element.currentTime += diff + 0.1;
- }
+ if (diff > latency && diff > 0.1) {
+ console.warn("skipping ahead", diff, "seconds");
+ const target = Math.max(0, last - latency + 0.1);
+ element.currentTime = target;
+ }🤖 Prompt for AI Agents
In `@js/hang/src/watch/mse.ts` around lines 79 - 94, The skip currently advances
element.currentTime to the buffer end (last + 0.1) which ignores the computed
latency; change the seek target in the effect.interval block so it respects
latency by setting the new time to last - latency + 0.1 (rather than last +
0.1), and guard it so you never seek backwards (only set element.currentTime if
newTime > element.currentTime) and clamp newTime to at most last to avoid
overshooting; update the code around effect.interval, the latency variable
usage, and the element.currentTime assignment accordingly.
Introduces a CMAF player for hang and CMAF containers.
A lot of restructuring was done to make the API tree-shakable. I basically had to invert the ownership, such that leaf nodes ended up owning parents containers.
It's not fully complete and the MSE player will need some more iterations. But hopefully somebody else can do that lul.