feat(hang): captions as standalone text tracks - #2533
Conversation
Add a `text` section to the hang catalog for caption/subtitle tracks, rendered in the browser and published via GStreamer. Wire format: one cue per group in the legacy container (frame timestamp = cue start on the media clock, payload = a self-contained WebVTT segment). Formats are `vtt` (primary), `utf8`, and `ttml`; roles `subtitle` and `caption`. The section is omitted when empty, so caption-less catalogs stay byte-identical. Documented in draft-lcurley-moq-hang. Watch: a Text.Source/Text.Renderer pair parses cues with media-captions and draws them into an overlay above the canvas, scheduled against the same Sync clock as audio/video (no <track>, since playback is canvas-only). A Captions tab in the settings UI toggles/selects the track. Publish: moqsink accepts a text/x-raw pad and publishes it as a hang text rendition, one WebVTT cue per group from the buffer PTS + duration. The GStreamer demux path is used because ffmpeg cannot mux subtitles into fragmented MP4; qtdemux hands us decoded UTF-8 cues on the shared media clock, so captions stay aligned with no separate pacing. moq-mux's catalog also gains the `text` field so the native path can carry it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughTimed text support was added to the hang catalog with text formats, roles, rendition metadata, priorities, serialization, and public APIs. Broadcast and mux catalog paths now publish and preserve text renditions. GStreamer accepts subtitle buffers and emits timed WebVTT cues. The watch package derives available caption tracks, parses and renders VTT or UTF-8 cues, and adds caption selection controls and an overlay. Demo recipes and documentation describe caption publishing and consumption. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ 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 |
- moq-gst: box both `Sink` variants (clippy::large-enum-variant under -D warnings), and fix the four `push_buffer` test calls that still passed two arguments (E0061). - watch: import the media-captions stylesheets (captions + regions) so cues are actually positioned/styled; without them the overlay rendered nothing visible. Mounted as a sibling of the overlay, since the renderer clears the overlay's children on every track change. - watch: destroy() the CaptionsRenderer on cleanup so its ResizeObserver is disconnected (removing the overlay alone leaks it). - watch: select the frame format from the rendition's container (legacy vs LOC) instead of always legacy; skip unsupported containers. - hang: add the `jitter` field to TextConfig (Rust + JS) to match the draft's shared common fields. - hang (js): a malformed `text` section now falls back to undefined rather than failing the whole catalog parse, so a catalog that used the key for something else still plays video/audio. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ition guard Follow-up review fixes on the standalone text-track captions work. An unrecognized `role` value made the entire catalog fail to decode in Rust, taking audio and video down with it, because `TextRole` was a closed serde enum while `TextFormat` already preserved unknown values. The role vocabulary is borrowed from draft-ietf-moq-msf and is expected to grow, so it now mirrors `TextFormat`: an `Unknown(String)` variant that round-trips verbatim, with the JS schema relaxed to a plain string the same way `format` already was. The draft spells out that a consumer must not reject an unrecognized role. moq-gst's subtitle pad inserted its catalog entry by hand instead of going through moq-mux's rendition machinery, so it neither held the reservation that gates the first catalog snapshot nor removed the entry on drop: a failed or finished pad left the catalog advertising a track whose producer was gone. Add `RenditionConfig for TextConfig` plus a `Reserved::text()` shorthand and hold the guard for the pad's lifetime, exactly like the codec paths. Cue text went into the WebVTT payload unescaped, so `<` opened a tag, a blank line truncated a multi-line caption at the first paragraph break, and a `-->` line read as another cue's timing. Escape the markup characters and drop blank lines; an empty result publishes nothing. The pad also declared every text track `role: caption`, asserting a transcription of non-speech audio that a demuxed subtitle track gives no evidence for. It now uses the `subtitle` default. The demo gst recipe wired `demux.subtitle_0` unconditionally. gst-launch builds every named branch up front, so on a file without subtitles that queue never receives EOS and the pipeline will not shut down: Ctrl-C hangs. Probe with ffprobe and only add the branch when there is a text track. Switching the media branches back to parsebin also restores the codec generality the hardcoded h264parse/aacparse dropped. Watch-side, the renderer read groups strictly sequentially, so one stalled group delayed every later cue; it now spawns a task per group like `Container.Consumer` does. Cue insertion is order-independent (groups arrive in delivery order, so the last-pushed cue is not necessarily the previous one on the clock), and a VTT payload whose timing disagrees with its frame timestamp warns once instead of misplacing every cue in silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… playback A second review pass over the caption path, all five findings reproduced before fixing. `text` was an ordinary application section before captions reserved the name, and the new field deserialized eagerly, so any legacy value (a string, an array, an object of another shape) failed the whole catalog and took audio and video with it. Every shape tried failed, not just the exotic ones, because `renditions` is a required field. Both catalog roots now decode the section through `hang::catalog::deserialize_text`, which falls back to empty, matching what the JS parser already did with `z.catch`. Cue pruning shifted off the front of an array ordered by start time, not end time, so a single long-running early cue stopped the sweep and let every expired cue behind it accumulate for the life of the stream, with each commit copying the whole array. It now sweeps the full array. Captions were gated only on the element being connected, while the caption clock runs off wall time: pausing left text scrolling over a frozen frame. They now follow `paused` like audio and video do. An empty `utf8` payload means "clear the caption" in the draft, but the renderer returned on any empty payload before looking at the format, leaving stale accessibility text on screen until the 30s linger expired. The clear is now scheduled as its own event. Text renditions declare a `jitter` that Sync never saw, since it summed audio and video alone. A publisher whose cue flush delay exceeded the A/V buffer delivered cues after their display window, so they were never shown. The text source now exposes its jitter and Sync folds it into the buffer, which is why it is constructed before Sync like the other two sources. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
js/hang/src/catalog/index.ts (2)
16-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the
.tsextension in this newhangexport.This lower-level package requires extensions on relative imports. Change the new export to
./text.ts; do not extend the existing extensionless pattern in this changed line.As per coding guidelines, lower-level packages such as
hangmust include.tsextensions in relative imports.Proposed fix
-export * from "./text"; +export * from "./text.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/hang/src/catalog/index.ts` at line 16, Update the new export in the catalog index to use the explicit "./text.ts" relative path. Limit the change to this export and preserve all existing exports unchanged.Source: Coding guidelines
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefresh the catalog module documentation.
The new export adds timed text types, but the module doc still describes only audio/video WebCodecs configurations. Update the doc block so the public entrypoint accurately describes the exported catalog surface.
As per coding guidelines, comments and documentation must describe the current code state, and entrypoints must include a top-level
@moduleblock.🤖 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/hang/src/catalog/index.ts` at line 16, Update the catalog module’s documentation near the public exports to add a top-level `@module` block and describe the complete exported surface, including timed text types from the text module alongside the existing audio/video WebCodecs configurations.Source: Coding guidelines
js/hang/src/catalog/priority.ts (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported priority map.
PRIORITYis a public TypeScript export whose surface now includestext, but it has no JSDoc. Add a concise description of the track-kind priorities.As per coding guidelines, every exported JavaScript or TypeScript symbol and notable public member must be documented.
🤖 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/hang/src/catalog/priority.ts` around lines 3 - 8, Add concise JSDoc above the exported PRIORITY constant describing that it maps track kinds, including text, audio, and video, to their priority values. Keep the existing priority entries and values unchanged.Source: Coding guidelines
rs/moq-gst/src/sink/pad.rs (2)
375-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
push_buffer's docstring for the newdurationparameter.The docstring wasn't updated when
duration: Option<gst::ClockTime>was added. Its behavior is non-obvious: it's ignored for media frames but required (dropped-with-a-warning ifNone) for subtitle cues.📝 Proposed doc addition
/// Import one buffer into the producer. A failed or producer-less pad drops the buffer; a timeline /// drop is logged. A bad bitstream (or an oversized frame, rejected by moq-net) invalidates only this /// pad. + /// `duration` is only meaningful for subtitle cues (an explicit cue end); it's ignored for media + /// frames, and a subtitle cue with no duration is dropped rather than pinned on screen forever. /// Returns `true` the first time a buffer is dropped because the pad has no TIME segment, so the /// caller can surface it once on the bus: without a timeline the pad can never publish.🤖 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 `@rs/moq-gst/src/sink/pad.rs` around lines 375 - 380, Update the documentation for push_buffer to describe the duration parameter: media frames ignore it, while subtitle cues require it and are dropped with a warning when it is None.
53-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why the container
Frame.durationis intentionally omitted.
duration_microsis computed and embedded in the WebVTT cue's-->end timestamp, butFrame.durationis alwaysNone. That's presumably deliberate (the payload self-describes its end time; nothing downstream readsFrame.durationfor thevttformat), but nothing here says so, inviting a future "fix".📝 Proposed comment
self.producer.write(moq_mux::container::Frame { timestamp: moq_net::Timestamp::from_micros(start_micros)?, + // The cue's end time is embedded in the WEBVTT payload itself (see the `-->` line + // above); the container-level duration is left unset since nothing reads it for `vtt`. duration: None, payload: Bytes::from(payload.into_bytes()), keyframe: true, })?;🤖 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 `@rs/moq-gst/src/sink/pad.rs` around lines 53 - 76, Add an inline comment next to `Frame.duration: None` in `write` explaining that the WebVTT payload embeds the cue end time using `duration_micros`, so the container frame duration is intentionally omitted for this format.js/watch/src/text/renderer.ts (2)
153-170: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFront-only pruning can strand expired cues behind a longer one.
cuesis sorted bystartTime, notendTime. If an earlier-starting cue has a longer duration than one that follows it, thewhile (cues[0].endTime < cutoff)loop stops at index 0 and never reaches the later, already-expired cue — defeating the stated goal of bounding memory on a long stream.♻️ Proposed fix: filter by endTime instead of shifting only the front
const cutoff = (now - PRUNE_BEHIND) / 1000; - let pruned = false; - while (cues.length > 0 && cues[0].endTime < cutoff) { - cues.shift(); - pruned = true; - } - if (pruned) commit(); + const before = cues.length; + for (let i = cues.length - 1; i >= 0; i--) { + if (cues[i].endTime < cutoff) cues.splice(i, 1); + } + if (cues.length !== before) commit();🤖 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/text/renderer.ts` around lines 153 - 170, Update the pruning logic in the tick callback so it removes every cue whose endTime is earlier than cutoff, rather than only shifting expired cues from the front of the startTime-sorted cues array. Preserve non-expired cues, set pruned when any cue is removed, and call commit only when pruning occurred.
211-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the microsecond-to-second conversion into a named constant.
1e6is the only inline numeric timing literal in the file;PRUNE_BEHIND,UTF8_LINGER, andSKEW_TOLERANCEare all named constants. As per coding guidelines, "Avoid using magic numbers; use named constants instead."♻️ Proposed fix
+// The scale of `sample.timestamp`: microseconds. +const MICROS_PER_SECOND = 1e6; + async `#ingest`( ... - const start = (sample.timestamp as number) / 1e6; + const start = (sample.timestamp as number) / MICROS_PER_SECOND;🤖 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/text/renderer.ts` at line 211, Replace the inline 1e6 divisor in the timestamp conversion within the renderer with a descriptive module-level named constant for the microsecond-to-second factor, alongside PRUNE_BEHIND, UTF8_LINGER, and SKEW_TOLERANCE, and use that constant in the calculation.Source: Coding guidelines
🤖 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 `@doc/bin/cli.md`:
- Around line 102-105: Revise the documentation paragraph to scope the
fragmented MP4 subtitle limitation specifically to the `moq import` path, rather
than stating that ffmpeg cannot mux subtitles into fragmented MP4 generally.
Preserve the explanation that `moqsink` handles captions through the GStreamer
path and retain the existing GStreamer reference.
In `@drafts/draft-lcurley-moq-hang.md`:
- Line 383: Update the container section’s introductory wording to state that
audio, video, and text tracks use the container framing contract. Keep the
existing text payload and format definitions unchanged.
- Around line 263-267: Clarify the format documentation for vtt and ttml to
define the authoritative cue start timestamp and require embedded absolute
timing to match the containing frame timestamp. Update the corresponding
repeated wording noted at lines 283-284, preserving the existing utf8 semantics.
In `@rs/hang/src/catalog/root.rs`:
- Around line 420-428: The test unknown_text_role_keeps_the_catalog should also
serialize the parsed catalog and assert that the unrecognized “commentary” role
remains present verbatim in the serialized output. Keep the existing
catalog-preservation assertion and verify the rendition is not coerced or
omitted during reserialization.
---
Nitpick comments:
In `@js/hang/src/catalog/index.ts`:
- Line 16: Update the new export in the catalog index to use the explicit
"./text.ts" relative path. Limit the change to this export and preserve all
existing exports unchanged.
- Line 16: Update the catalog module’s documentation near the public exports to
add a top-level `@module` block and describe the complete exported surface,
including timed text types from the text module alongside the existing
audio/video WebCodecs configurations.
In `@js/hang/src/catalog/priority.ts`:
- Around line 3-8: Add concise JSDoc above the exported PRIORITY constant
describing that it maps track kinds, including text, audio, and video, to their
priority values. Keep the existing priority entries and values unchanged.
In `@js/watch/src/text/renderer.ts`:
- Around line 153-170: Update the pruning logic in the tick callback so it
removes every cue whose endTime is earlier than cutoff, rather than only
shifting expired cues from the front of the startTime-sorted cues array.
Preserve non-expired cues, set pruned when any cue is removed, and call commit
only when pruning occurred.
- Line 211: Replace the inline 1e6 divisor in the timestamp conversion within
the renderer with a descriptive module-level named constant for the
microsecond-to-second factor, alongside PRUNE_BEHIND, UTF8_LINGER, and
SKEW_TOLERANCE, and use that constant in the calculation.
In `@rs/moq-gst/src/sink/pad.rs`:
- Around line 375-380: Update the documentation for push_buffer to describe the
duration parameter: media frames ignore it, while subtitle cues require it and
are dropped with a warning when it is None.
- Around line 53-76: Add an inline comment next to `Frame.duration: None` in
`write` explaining that the WebVTT payload embeds the cue end time using
`duration_micros`, so the container frame duration is intentionally omitted for
this format.
🪄 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 Plus
Run ID: 791e0b23-7266-4dda-ad4a-e87243ee969c
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockjs/watch/src/ui/icons/captions.svgis excluded by!**/*.svg
📒 Files selected for processing (32)
demo/pub/justfiledoc/bin/cli.mddoc/concept/layer/hang.mddrafts/draft-lcurley-moq-hang.mdjs/hang/src/catalog/index.tsjs/hang/src/catalog/priority.tsjs/hang/src/catalog/root.tsjs/hang/src/catalog/text.tsjs/publish/src/broadcast.tsjs/publish/src/rendition.tsjs/watch/package.jsonjs/watch/src/element.tsjs/watch/src/index.tsjs/watch/src/text/index.tsjs/watch/src/text/renderer.test.tsjs/watch/src/text/renderer.tsjs/watch/src/text/source.tsjs/watch/src/ui/components/captions.tsjs/watch/src/ui/components/settings-panel.tsjs/watch/src/ui/icons.tsjs/watch/src/ui/state.tsrs/hang/src/catalog/mod.rsrs/hang/src/catalog/priority.rsrs/hang/src/catalog/root.rsrs/hang/src/catalog/text/format.rsrs/hang/src/catalog/text/mod.rsrs/moq-gst/src/sink/imp.rsrs/moq-gst/src/sink/pad.rsrs/moq-mux/src/catalog/consumer.rsrs/moq-mux/src/catalog/hang/ext.rsrs/moq-mux/src/catalog/mod.rsrs/moq-mux/src/catalog/tracks.rs
| Subtitles ride in the source container. `moq import` reads media from stdin, and ffmpeg cannot mux a | ||
| subtitle track into fragmented MP4, so captions arrive over the GStreamer path instead: `moqsink` | ||
| accepts a decoded text pad and publishes it as a caption track. See | ||
| [GStreamer](gstreamer.md). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
ffmpeg subtitle muxing mov mp4 fragmented documentation TTML WebVTT
💡 Result:
FFmpeg's support for muxing subtitles into fragmented MP4 (fMP4) varies by format, with recent advancements improving the ability to handle TTML [1][2]. TTML in Fragmented MP4: FFmpeg has implemented support for muxing TTML into fragmented MP4 (e.g., for DASH/ISMV workflows) [2][3]. Historically, this was restricted due to challenges in synchronizing subtitle-only fragments with other media tracks [1][3]. Recent updates have addressed these synchronization requirements, allowing TTML to be muxed into fragmented MP4 containers [2][3]. When using TTML in fragmented MP4, it is typically signaled with the stpp codec identifier [4]. WebVTT in Fragmented MP4: While FFmpeg can mux WebVTT into a standalone WebVTT file (outputting.vtt) [5][6], native muxing of WebVTT directly into a fragmented MP4 container is not a standard, primary feature of the ffmpeg CLI muxer in the same way as video or audio streams. In professional streaming workflows (such as generating MPEG-DASH or HLS), it is common practice to use specialized packaging tools to encapsulate WebVTT into the fragmented MP4 (using the wvtt codec identifier) [4][7][8]. Tools like Bento4 (mp4dash/mp4fragment) or GPAC (MP4Box) are frequently used in combination with FFmpeg to finalize the fragmented MP4 structure for DASH/HLS [7][9][8]. Summary for Documentation: 1. TTML: FFmpeg supports fragmented TTML muxing in MP4/ISMV [2]. Ensure that the subtitle stream is correctly mapped and appropriate time bases are set [2][3]. 2. WebVTT: For fragmented MP4 workflows, standard practice involves using external packagers (e.g., Bento4, MP4Box) to ingest WebVTT and wrap it into an fMP4 container [7][9][8], as native FFmpeg fragmented MP4 muxing is more optimized for media tracks (audio/video) [10]. For the most up-to-date and specific implementation details regarding fragmentation flags or codec tags, refer to the FFmpeg formats documentation and the specific FATE test suite references within the source code, which often provide working examples of complex muxing configurations [2][11][10].
Citations:
- 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2023-December/318031.html
- 2: https://ffmpeg.org/pipermail/ffmpeg-devel/2023-December/318244.html
- 3: https://ffmpeg.org/pipermail/ffmpeg-devel/2023-December/318018.html
- 4: https://docs.unified-streaming.com/documentation/vod/subtitles.html
- 5: https://www.ffmpeg.org/doxygen/trunk/libavformat_2webvttenc_8c_source.html
- 6: https://svn.ffmpeg.org/doxygen/trunk/libavformat_2webvttenc_8c.html
- 7: https://stackoverflow.com/questions/59494814/adding-multiple-audio-tracks-and-subtitles-to-dash-manifest-mpd-with-ffmpeg
- 8: https://github.com/matmoi/create-DASH-HLS/
- 9: https://biblio.telecom-paristech.fr/cgi-bin/download.cgi?id=14196
- 10: https://ffmpeg.org/ffmpeg-all.html
- 11: https://ffmpeg.org/ffmpeg-formats.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate cli.md =="
fd -a '^cli\.md$' . | sed 's#^\./##'
echo "== relevant doc section =="
cli="$(fd '^cli\.md$' . | head -n1)"
if [ -n "${cli:-}" ]; then
sed -n '90,115p' "$cli" | cat -n
fi
echo "== references to ffmpeg, moq import, fragmented/fmp4, subtitle/mux =="
rg -n "ffmpeg|moq import|fragmented|fmp4|fMP4|fragment|subtitle|capt|mux" doc src .github 2>/dev/null | head -n 120Repository: moq-dev/moq
Length of output: 13502
Scope the fragmented MP4 subtitle limitation to moq import.
The current path does not preserve subtitle tracks when moq import writes fragmented MP4, so captions are handled via the GStreamer path. FFmpeg supports subtitle muxing into segmented/fragmented MP4 in some cases, so the absolute wording is too broad.
Proposed wording
-ffmpeg cannot mux a
-subtitle track into fragmented MP4, so captions arrive over the GStreamer path instead:
+the current `moq import` path does not preserve subtitle tracks in fragmented MP4, so captions
+arrive over the GStreamer path instead:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Subtitles ride in the source container. `moq import` reads media from stdin, and ffmpeg cannot mux a | |
| subtitle track into fragmented MP4, so captions arrive over the GStreamer path instead: `moqsink` | |
| accepts a decoded text pad and publishes it as a caption track. See | |
| [GStreamer](gstreamer.md). | |
| Subtitles ride in the source container. `moq import` reads media from stdin, and the current `moq import` path does not preserve subtitle tracks in fragmented MP4, so captions arrive over the GStreamer path instead: `moqsink` | |
| accepts a decoded text pad and publishes it as a caption track. See | |
| [GStreamer](gstreamer.md). |
🤖 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 `@doc/bin/cli.md` around lines 102 - 105, Revise the documentation paragraph to
scope the fragmented MP4 subtitle limitation specifically to the `moq import`
path, rather than stating that ffmpeg cannot mux subtitles into fragmented MP4
generally. Preserve the explanation that `moqsink` handles captions through the
GStreamer path and retain the existing GStreamer reference.
| The `format` field selects the cue serialization, and tells a consumer how to parse each frame's payload: | ||
|
|
||
| - `vtt`: WebVTT ([W3C WebVTT](https://www.w3.org/TR/webvtt1/)). Each payload is a self-contained `WEBVTT` segment whose cues carry absolute timing. | ||
| - `ttml`: TTML / IMSC1 ([W3C IMSC](https://www.w3.org/TR/ttml-imsc1.1/)) fragment (XML) with absolute timing. | ||
| - `utf8`: raw UTF-8 text with no embedded timing or styling. The cue is shown from its frame timestamp until the next cue; an empty payload clears it. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Define the authoritative timestamp for VTT and TTML cues.
These lines define embedded absolute cue times while also making the outer frame timestamp the cue start, but never require them to agree or specify which one consumers use. Require the embedded timing to match the frame timestamp, or explicitly define one as authoritative. The current GStreamer writer emits the same start in both places (rs/moq-gst/src/sink/pad.rs:48-77).
Proposed clarification
-Each payload is a self-contained `WEBVTT` segment whose cues carry absolute timing.
+Each payload is a self-contained `WEBVTT` segment whose cue timing MUST match the enclosing frame timestamp.Also applies to: 283-284
🤖 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 `@drafts/draft-lcurley-moq-hang.md` around lines 263 - 267, Clarify the format
documentation for vtt and ttml to define the authoritative cue start timestamp
and require embedded absolute timing to match the containing frame timestamp.
Update the corresponding repeated wording noted at lines 283-284, preserving the
existing utf8 semantics.
| The remainder of the payload is codec specific; see the WebCodecs specification for specifics. | ||
|
|
||
| For example, h.264 with no `description` field would be annex.b encoded, while h.264 with a `description` field would be AVCC encoded. | ||
| For a text track, the remainder is the cue in the track's declared `format` (for example a `WEBVTT` segment). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include text in the container framing contract.
The container section still says that only audio and video tracks use a container, while this new rule defines the text payload inside the legacy container. Update that introductory wording to include text, otherwise implementations can treat text as outside the container rules.
🤖 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 `@drafts/draft-lcurley-moq-hang.md` at line 383, Update the container section’s
introductory wording to state that audio, video, and text tracks use the
container framing contract. Keep the existing text payload and format
definitions unchanged.
| #[test] | ||
| fn unknown_text_role_keeps_the_catalog() { | ||
| // A future `role` value must not take down the whole catalog: audio and video have to keep | ||
| // playing even when a caption rendition is classified with a vocabulary we don't know yet. | ||
| let json = r#"{"video":{"renditions":{}},"audio":{"renditions":{}},"text":{"renditions":{"subs":{"format":"vtt","role":"commentary"}}}}"#; | ||
|
|
||
| let catalog = Catalog::from_str(json).expect("unknown role rejected the catalog"); | ||
| assert_eq!(catalog.text.renditions.len(), 1); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert that unknown roles survive serialization.
The draft requires unrecognized roles to be preserved verbatim, but this test only checks that one rendition exists. It would pass if "commentary" were coerced to "subtitle" or dropped during reserialization.
Proposed regression assertion
let catalog = Catalog::from_str(json).expect("unknown role rejected the catalog");
assert_eq!(catalog.text.renditions.len(), 1);
+ let output = catalog.to_json().expect("failed to reserialize");
+ assert!(output.contains(r#""role":"commentary""#));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn unknown_text_role_keeps_the_catalog() { | |
| // A future `role` value must not take down the whole catalog: audio and video have to keep | |
| // playing even when a caption rendition is classified with a vocabulary we don't know yet. | |
| let json = r#"{"video":{"renditions":{}},"audio":{"renditions":{}},"text":{"renditions":{"subs":{"format":"vtt","role":"commentary"}}}}"#; | |
| let catalog = Catalog::from_str(json).expect("unknown role rejected the catalog"); | |
| assert_eq!(catalog.text.renditions.len(), 1); | |
| } | |
| #[test] | |
| fn unknown_text_role_keeps_the_catalog() { | |
| // A future `role` value must not take down the whole catalog: audio and video have to keep | |
| // playing even when a caption rendition is classified with a vocabulary we don't know yet. | |
| let json = r#"{"video":{"renditions":{}},"audio":{"renditions":{}},"text":{"renditions":{"subs":{"format":"vtt","role":"commentary"}}}}"#; | |
| let catalog = Catalog::from_str(json).expect("unknown role rejected the catalog"); | |
| assert_eq!(catalog.text.renditions.len(), 1); | |
| let output = catalog.to_json().expect("failed to reserialize"); | |
| assert!(output.contains(r#""role":"commentary""#)); | |
| } |
🤖 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 `@rs/hang/src/catalog/root.rs` around lines 420 - 428, The test
unknown_text_role_keeps_the_catalog should also serialize the parsed catalog and
assert that the unrecognized “commentary” role remains present verbatim in the
serialized output. Keep the existing catalog-preservation assertion and verify
the rendition is not coerced or omitted during reserialization.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rs/hang/src/catalog/root.rs (1)
427-441: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winStrengthen the legacy compatibility assertion.
The fixture has empty video and audio rendition maps, and the test only checks
catalog.text.is_empty(). It would still pass if a regression discarded the sibling sections. Reuse a non-empty video/audio fixture and assert those renditions survive for each legacy shape.As per coding guidelines, critical compatibility behavior should have regression coverage.
🤖 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 `@rs/hang/src/catalog/root.rs` around lines 427 - 441, Strengthen the test legacy_text_section_keeps_the_catalog by using non-empty video and audio rendition fixtures instead of empty maps, then assert those rendition entries remain present for every legacy text shape while retaining the assertion that catalog.text is empty.Source: Coding guidelines
js/watch/src/sync.ts (1)
34-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported
SyncInputtype.
SyncInputis an exported TypeScript symbol, but the public type has no type-level JSDoc. Add a concise doc comment above the declaration; the field comments do not document the public API as a whole.As per coding guidelines, every exported TypeScript symbol must be documented.
Proposed documentation
+/** Inputs used to calculate the shared playback buffer. */ export type SyncInput = {🤖 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/sync.ts` around lines 34 - 47, Add a concise type-level JSDoc comment immediately above the exported SyncInput declaration to document the public API as a whole, while preserving the existing field comments and type definition.Source: Coding guidelines
🤖 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/sync.ts`:
- Around line 34-47: Add a concise type-level JSDoc comment immediately above
the exported SyncInput declaration to document the public API as a whole, while
preserving the existing field comments and type definition.
In `@rs/hang/src/catalog/root.rs`:
- Around line 427-441: Strengthen the test legacy_text_section_keeps_the_catalog
by using non-empty video and audio rendition fixtures instead of empty maps,
then assert those rendition entries remain present for every legacy text shape
while retaining the assertion that catalog.text is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c2de96d-74d2-48a9-8028-4250b29d6407
📒 Files selected for processing (8)
js/watch/src/element.tsjs/watch/src/sync.tsjs/watch/src/text/renderer.test.tsjs/watch/src/text/renderer.tsjs/watch/src/text/source.tsrs/hang/src/catalog/root.rsrs/hang/src/catalog/text/mod.rsrs/moq-mux/src/catalog/hang/ext.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rs/hang/src/catalog/text/mod.rs
- js/watch/src/text/source.ts
- rs/moq-mux/src/catalog/hang/ext.rs
- js/watch/src/element.ts
- js/watch/src/text/renderer.ts
The control bar gains a CC toggle next to the settings gear. It selects the first text rendition when there is more than one (the Captions tab in the settings panel is where a viewer picks between them) and drops out of layout entirely when the broadcast publishes no text section, so a caption-less stream gets no dead control. It sets `style.display` rather than the `hidden` attribute: `.control` sets `display: flex`, which beats the user-agent rule for `[hidden]`, so the button stayed on screen at full size. The live badge already hides itself this way. Also reverts the demo gst recipe's media branches to `h264parse`/`aacparse`. Swapping them for `parsebin` was meant to restore codec generality but broke video outright: parsebin preserves qtdemux's `stream-format=avc` caps behind its own capsfilter, while the moqsink pad template requires `byte-stream`, so the video pad never negotiated and the catalog published with no video rendition. A bare `h264parse` converts because it negotiates against what is downstream. The ffprobe gate around the subtitle branch is unaffected and still does its job: bbb builds a two-queue pipeline with no sink_2, bbbcc a three-queue one. Verified end to end against a local relay: bbbcc publishes video 0.avc3, audio 0.aac and text 0.vtt (format vtt, role subtitle), and cues render on the overlay within ~130 ms of their media timestamp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Toggling captions in the player re-anchored audio and video. The caption rendition advertised a jitter, the watch element folds a text rendition's jitter into the shared Sync buffer, and raising that floor makes the audio ring reset() to refill while video holds longer: a visible reinitialization from turning captions on. The advertised value was meaningless. The estimator's jitter is the smallest gap between consecutive frames, which for a codec is the frame duration and a real flush delay, but for cues is just how close two subtitles happen to sit. Big Buck Bunny has cues at 19.833s and 20.374s, so it published 541ms, and the figure would keep shrinking (republishing the catalog) as tighter cue pairs arrived. Each cue here is written and cut immediately, so an absent jitter states the truth: flushed as produced. The Sync wiring stays. Honoring a jitter a publisher deliberately declares is still correct, and an absent field now contributes zero, so toggling captions leaves the buffer untouched. Verified against a local relay: the catalog carries no text jitter, the sync buffer holds at 142ms across captions on and off, and cues still arrive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Adds caption/subtitle support to
hangas a standalonetextcatalog section: rendered in the browser and published via GStreamer.Wire format
Captions are their own MoQ track, kept out of the video bitstream so the relay stays media-agnostic. One cue per group in the legacy container: the frame timestamp is the cue's start on the media clock, and the payload is a self-contained WebVTT segment. Formats are
vtt(primary),utf8, andttml; rolessubtitleandcaption. The section is omitted from the wire when empty, so caption-less catalogs stay byte-identical. Specified indraft-lcurley-moq-hang(## Text).MSF was the starting point but only reserves the
rolevocabulary and an embedded-CEAaccessibilityfield; it defines no standalone-text framing, so this invents it (and borrowsrole: subtitle|caption).Watch (render)
A
Text.Source/Text.Rendererpair parses cues withmedia-captionsand draws them into an overlay above the canvas, scheduled against the sameSyncclock as audio/video. It deliberately does not use<track>/TextTrack— playback is canvas-only (WebCodecs), so there's no<video>element to attach a text track to. A Captions tab in the settings UI toggles/selects the track.Publish (GStreamer, not ffmpeg)
moqsinkaccepts atext/x-rawpad and publishes it as a hang text rendition: one WebVTT cue per group, built from the buffer's PTS + duration.Why GStreamer and not the
moq import fmp4stdin path: ffmpeg (8.1.1) cannot mux subtitles into fragmented MP4.webvtt/wvttis rejected by the muxer outright,mov_text+ CMAF flags aborts on an assertion, and copying a pre-embedded track through the fragmenter silently drops it. GStreamer'sqtdemuxhands us decoded UTF-8 cues (text/x-raw) with PTS + duration already resolved, on the same clock as audio/video — so captions stay aligned with no separate pacing, no lead/loop knobs, and no in-band box parsing.moq-mux's catalog gains the matchingtextfield so the native path can carry it too.Verified end-to-end
Published the captioned Big Buck Bunny (demuxed/big-buck-captions SRT, embedded as a soft
mov_texttrack) through the real GStreamer pipeline into a live relay:video: 0.avc3,audio: 0.aac,text: 0.vtt(format vtt, role caption)00:00:35.916 --> 00:00:37.249 [BUNNY SNORES]against a playhead of35.9s— ~16 ms alignment, becauseidentity sync=truepaces all three streams off one clock.Watch-side rendering (renderer →
CaptionsRenderer) was traced live: cues subscribe, parse, and commit to the overlay.Follow-ups (not in this PR)
bbb.mp4isn't uploaded tovid.moq.devyet, sojust pub gst bbbwon't show captions until it is.just pub subtitles <name> <srt>embeds a track into a local test video (needs+faststartformultifilesrclooping).just devruns the ffmpegbbbrecipe, which can't carry captions; it would need to point at the gst recipe to show them by default.moq-mux(stillError::UnsupportedSubtitle) is a separate, larger task.🤖 Generated with Claude Code
(Written by Claude Opus 4.8)