Add Matroska/WebM import and export support#1438
Conversation
Adds `import::Mkv` to ingest Matroska / WebM container files into a moq broadcast, supporting both bounded files and live (unknown-size Segment / Cluster) streams. Built on `webm-iterable`. The parser restarts from the start of the retained buffer on each `decode()` call and tolerates orphan child tags via the iterator's `HierarchyProblems` allowance; per-track timestamp dedup keeps block emission idempotent across replays. Supported codecs: VP8, VP9, H.264, H.265, AV1, Opus, AAC. Unsupported codecs (Vorbis, AC3, subtitles, etc.) are warned and dropped. All produce `Legacy` container tracks so payloads pass through to subscribers unchanged. Wired into `StreamFormat` and `FramedFormat` with aliases `mkv`, `webm`, `matroska`. Synthetic tests cover catalog population, chunked input, unsupported-codec skipping, and timestamp scaling. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
Adds `export::Mkv` to subscribe to a moq broadcast and produce a streaming Matroska / WebM byte stream. Mirrors the `Fmp4` exporter's `next()` / `poll_next()` API. Emits an unknown-size Segment (live mode) so the stream never has to be closed; one Cluster + SimpleBlock per frame, hand-encoded as EBML to avoid the WebmWriter's parent-context validation. DocType is "webm" when every codec is WebM-allowed (VP8/VP9/AV1/Opus), "matroska" otherwise. CodecPrivate is reconstructed per codec: AVCDecoderConfigurationRecord / HEVCDecoderConfigurationRecord / AV1CodecConfigurationRecord / AAC AudioSpecificConfig are passed through from the catalog `description` field; Opus rebuilds OpusHead from sample_rate + channels. CMAF-container tracks are rejected with a clear error since unwrapping moof+mdat fragments back into raw samples isn't implemented; this matches what the importer produces (Legacy tracks). Round-trip test: import → broadcast → export → re-import, asserting the catalog repopulates with the same codecs. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds Matroska/WebM import and export to moq-mux: a streaming Mkv importer that parses EBML and publishes per-track frames, an Mkv exporter that emits EBML headers and Cluster bytes ordered by cross-track timestamps, Avc1/Hvc1 transforms for Annex‑B ↔ length‑prefixed NAL handling, Fmp4 fragment-duration batching, CLI wiring for MKV output, and comprehensive unit/integration tests. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
rs/moq-mux/src/export/mkv.rs (1)
87-91: 🏗️ Heavy liftDon't expose
anyhow::Resultfrom this public library API.
Mkv::nextandMkv::poll_nextare new public entry points in a library crate. Returning erasedanyhowerrors here makes the API harder to consume consistently thancrate::Erroror a dedicatedthiserrorenum with#[from]conversions.As per coding guidelines,
Use thiserrorwith#[from]for library crates,anyhowfor binaries`.🤖 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-mux/src/export/mkv.rs` around lines 87 - 91, The public API currently exposes anyhow::Result via Mkv::next and Mkv::poll_next; change both signatures to return your crate error type (e.g., crate::Error or a new thiserror enum) instead of anyhow::Result and update the internal conversion paths to use #[from] conversions so errors map into that type; update the poll signature (Poll<anyhow::Result<Option<Bytes>>>) to Poll<Result<Option<Bytes>, crate::Error>> and ensure conducer::wait invocation and any intermediate helper functions propagate/convert errors into the crate error (add From impls or #[from] variants in the thiserror enum) so the public surface no longer exposes anyhow.rs/moq-mux/src/export/mkv_test.rs (1)
14-15: ⚡ Quick winPause Tokio time explicitly in these async tests.
These tests use
timeout, but they rely onstart_paused = trueinstead of callingtokio::time::pause()at the start of the body as required here.As per coding guidelines,
Async Rust tests that sleep should call tokio::time::pause() at the start to simulate time instantly.Also applies to: 139-140, 210-211
🤖 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-mux/src/export/mkv_test.rs` around lines 14 - 15, The tests currently use #[tokio::test(start_paused = true)] (e.g. export_header_roundtrip_vp9_opus) but must explicitly pause time inside the async body; remove the start_paused attribute and add tokio::time::pause() as the first statement in each async test that uses timeout to simulate time instantly. Locate the test functions (export_header_roundtrip_vp9_opus and the other async tests in this file that rely on timeout) and replace the attribute usage with a normal #[tokio::test] and an initial call to tokio::time::pause() at the top of each test body.
🤖 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 `@rs/moq-mux/src/export/mkv_test.rs`:
- Around line 269-270: The test uses a non-existent SimpleBlock::new_uncheked
constructor; replace that call in the simple_block closure with the
crate-supported path for creating a SimpleBlock/MatroskaSpec (do not call
new_uncheked/new_unchecked). Locate the simple_block closure and produce a
MatroskaSpec SimpleBlock via the public conversion/variant API (e.g., construct
the MatroskaSpec::SimpleBlock variant or use any provided
MatroskaSpec::from/into helper for SimpleBlock) supplying the same track,
rel_ts, keyframe and payload values so the closure returns the correct
MatroskaSpec without using a private/incorrect constructor.
In `@rs/moq-mux/src/export/mkv.rs`:
- Around line 152-210: The update_catalog implementation currently allows
adding/removing renditions after the MKV header (Tracks) is emitted; change
update_catalog to reject any track-layout changes once self.header_emitted is
true by comparing the set of rendition names in the incoming catalog to the keys
in self.tracks and returning an error (e.g. anyhow::anyhow!("track layout
changed after header emitted")) if there are any additions or removals; do not
insert new tracks or call self.tracks.retain when header_emitted is true. Locate
this logic inside update_catalog and use the existing symbols
(self.header_emitted, self.tracks, build_header, subscribe, ensure_legacy,
MkvTrack, tracks.retain) to implement the early check and early return.
In `@rs/moq-mux/src/import/mkv.rs`:
- Around line 201-204: Make processing of MatroskaSpec::Tracks idempotent: in
the MatroskaSpec::Tracks(Master::Full(children)) branch (and/or at the start of
handle_tracks), check self.tracks_seen and return early if true to avoid
re-adding tracks on partial-input restarts; if you prefer, move the
self.tracks_seen = true assignment before or add a guard inside handle_tracks to
skip adding/updating self.tracks when tracks_seen is already set, ensuring
existing rendition names in self.tracks are preserved rather than replaced.
---
Nitpick comments:
In `@rs/moq-mux/src/export/mkv_test.rs`:
- Around line 14-15: The tests currently use #[tokio::test(start_paused = true)]
(e.g. export_header_roundtrip_vp9_opus) but must explicitly pause time inside
the async body; remove the start_paused attribute and add tokio::time::pause()
as the first statement in each async test that uses timeout to simulate time
instantly. Locate the test functions (export_header_roundtrip_vp9_opus and the
other async tests in this file that rely on timeout) and replace the attribute
usage with a normal #[tokio::test] and an initial call to tokio::time::pause()
at the top of each test body.
In `@rs/moq-mux/src/export/mkv.rs`:
- Around line 87-91: The public API currently exposes anyhow::Result via
Mkv::next and Mkv::poll_next; change both signatures to return your crate error
type (e.g., crate::Error or a new thiserror enum) instead of anyhow::Result and
update the internal conversion paths to use #[from] conversions so errors map
into that type; update the poll signature (Poll<anyhow::Result<Option<Bytes>>>)
to Poll<Result<Option<Bytes>, crate::Error>> and ensure conducer::wait
invocation and any intermediate helper functions propagate/convert errors into
the crate error (add From impls or #[from] variants in the thiserror enum) so
the public surface no longer exposes anyhow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93efed1d-b014-4b93-b711-92f51f0c5122
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
rs/moq-mux/Cargo.tomlrs/moq-mux/src/export/mkv.rsrs/moq-mux/src/export/mkv_test.rsrs/moq-mux/src/export/mod.rsrs/moq-mux/src/import/framed.rsrs/moq-mux/src/import/mkv.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/stream.rsrs/moq-mux/src/import/test/mkv.rsrs/moq-mux/src/import/test/mod.rs
| let simple_block = |track: u64, rel_ts: i16, keyframe: bool, payload: &[u8]| -> MatroskaSpec { | ||
| SimpleBlock::new_uncheked(payload, track, rel_ts, false, None, false, keyframe).into() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does webm-iterable0.6 providewebm_iterable::matroska_spec::SimpleBlock::new_uncheked, or is the constructor named new_unchecked?
💡 Result:
In webm-iterable 0.6.4, the SimpleBlock constructor is not named new_uncheked (that spelling doesn’t match any API). There is no evidence in the public docs we found that webm_iterable::matroska_spec::SimpleBlock provides either new_uncheked or new_unchecked; instead, the SimpleBlock type is exposed as a struct with (at least) methods like read_frame_data/raw_frame_data, and conversions via TryFrom<&MatroskaSpec>/Into are mentioned in the crate docs.[1][2] So, based on the authoritative API docs and crate documentation for the 0.6.x line, the correct answer is: it does not provide SimpleBlock::new_uncheked, and we cannot confirm the existence of SimpleBlock::new_unchecked either (it does not appear to be the named constructor in the published docs).[1][2]
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and surrounding occurrences
rg -n "SimpleBlock::new_uncheked|SimpleBlock::new_unchecked|new_uncheked|new_unchecked" -S . || true
# Confirm exact dependency version
rg -n "webm-iterable|webm_iterable" Cargo.lock **/Cargo.toml || trueRepository: moq-dev/moq
Length of output: 459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get exact webm-iterable version block from Cargo.lock
python3 - <<'PY'
import re, itertools
path="Cargo.lock"
target="name = \"webm-iterable\""
with open(path, "r", encoding="utf-8") as f:
lines=f.readlines()
for i,l in enumerate(lines):
if l.strip()==target:
start=max(0,i-5)
end=min(len(lines), i+25)
print("---- Cargo.lock context ----")
for j in range(start,end):
print(f"{j+1}:{lines[j].rstrip()}")
break
else:
print("No webm-iterable entry found")
PY
# Search repo for any local definition/shim of `SimpleBlock` or `new_uncheked`
rg -n --hidden --no-ignore-vcs "new_uncheked" .
rg -n --hidden --no-ignore-vcs "(SimpleBlock|matroska_spec::SimpleBlock).*pub use|pub type|type SimpleBlock|SimpleBlock =" rs/moq-mux . || trueRepository: moq-dev/moq
Length of output: 2273
🌐 Web query:
site:github.com/austinleroy/webm-iterable "new_uncheked" OR "new_unchecked" SimpleBlock
💡 Result:
I couldn’t find any authoritative or relevant source for the specific query terms “new_uncheked” / “new_unchecked” together with “SimpleBlock”. A web search returned only an unrelated GitHub repository mentioning “Block” and “Master” tags, but nothing about those identifiers or “SimpleBlock” specifically.[1] If you can share the language/ecosystem (e.g., Rust, C++, Java) and where you saw “SimpleBlock” (which library/project), I can re-run targeted searches for the exact symbol names and their correct spellings.
Citations:
🌐 Web query:
site:docs.rs/webm-iterable "struct SimpleBlock" "new_"
💡 Result:
In the context of the Rust library webm-iterable, the SimpleBlock struct (defined in webm_iterable::matroska_spec::simple_block) represents a Matroska SimpleBlock element [1][2]. There is no public constructor named new_ for the SimpleBlock struct in the current API. Instead, developers typically interact with SimpleBlock through conversion traits [1][3]. Specifically, because MatroskaSpec::SimpleBlock contains raw binary data (Vec), you can convert it to a SimpleBlock struct using the TryFrom trait [3][4]: let simple_block_data: &Vec =...; // From MatroskaSpec::SimpleBlock(ref data) let simple_block: SimpleBlock = simple_block_data.as_slice.try_into.unwrap; The SimpleBlock struct provides an easy way to work with block data parsed from EBML/Matroska streams [1]. If you are looking for a constructor for writing webm files, the WebmWriter (an alias for TagWriter) uses a standard new constructor (WebmWriter::new) to initialize a writer, which then accepts MatroskaSpec variants [5][4].
Citations:
- 1: https://docs.rs/webm-iterable/latest/src/webm_iterable/matroska_spec/mod.rs.html
- 2: https://docs.rs/webm-iterable/latest/webm_iterable/all.html
- 3: https://docs.rs/webm-iterable/latest/webm_iterable/matroska_spec/enum.MatroskaSpec.html
- 4: https://docs.rs/webm-iterable/latest/i686-unknown-linux-gnu/webm_iterable/
- 5: https://docs.rs/webm-iterable/latest/webm_iterable/type.WebmWriter.html
🌐 Web query:
"SimpleBlock" "new_unchecked" "webm-iterable" 0.6.4
💡 Result:
For the crate webm-iterable v0.6.4, SimpleBlock is a Matroska element represented in the crate as a Rust struct named SimpleBlock, and it includes fields like track (u64) and timestamp (i16), along with flags such as invisible, lacing, discardable, and keyframe. The crate also exposes methods on SimpleBlock, including read_frame_data and raw_frame_data. [1][2] I did not find any source-backed evidence that webm-iterable v0.6.4 contains a function/identifier literally named new_unchecked associated with SimpleBlock. The provided search results only show the SimpleBlock definition and its methods; they do not mention new_unchecked. [1][2] If you want, paste the exact Rust snippet (or the full search target such as the crate path/module) where you expect new_unchecked to exist (e.g., the type signature you saw). I can then re-search more precisely for that identifier within v0.6.4.
Citations:
Fix SimpleBlock constructor spelling / API usage
rs/moq-mux/src/export/mkv_test.rs (line 270) calls SimpleBlock::new_uncheked(...), but webm-iterable is pinned to 0.6.4 and SimpleBlock has no public constructor named new_uncheked (and no new_unchecked either); it’s intended to be created via the crate’s supported MatroskaSpec::SimpleBlock conversion/transform paths.
🤖 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-mux/src/export/mkv_test.rs` around lines 269 - 270, The test uses a
non-existent SimpleBlock::new_uncheked constructor; replace that call in the
simple_block closure with the crate-supported path for creating a
SimpleBlock/MatroskaSpec (do not call new_uncheked/new_unchecked). Locate the
simple_block closure and produce a MatroskaSpec SimpleBlock via the public
conversion/variant API (e.g., construct the MatroskaSpec::SimpleBlock variant or
use any provided MatroskaSpec::from/into helper for SimpleBlock) supplying the
same track, rel_ts, keyframe and payload values so the closure returns the
correct MatroskaSpec without using a private/incorrect constructor.
Addresses the qsx0004 / KVS pilot blockers from #1438 review: 1. **Avc3/Hev1 sources now work end-to-end.** The MKV exporter no longer demands a populated catalog `description`. New `export::Avc1` / `export::Hvc1` helpers cache inline parameter sets per track, synthesize `AVCDecoderConfigurationRecord` / `HEVCDecoderConfigurationRecord`, strip SPS/PPS/VPS from sample data, and length-prefix the remaining NALs. `Container::Legacy` sources with avc1/hvc1 shape (description populated) pass through unchanged. Per the close comment on #1437, the codec-shape transcode belongs in the exporter (codec-aware writer), not the importer. These helpers live in `export/` because that's their natural home; they're public so other container exporters (e.g. fmp4) can reuse them later. 2. **Header emission is deferred.** When any video track lacks a codec config (Avc3 with description=None), the file header waits until each transmuxer has observed SPS+PPS (typically the first keyframe). After that, the header is emitted with CodecPrivate populated; sample bytes that arrived during the wait are already transformed and flow normally. 3. **Configurable fragment duration.** `Mkv::with_fragment_duration(d)` batches frames into Clusters of roughly `d` (default 2s) and rolls on video keyframes or i16 timestamp overflow. `Duration::ZERO` keeps the old one-cluster-per-frame behavior. This unblocks KVS PutMedia, which throttles at much lower fragment rates than per-frame. 4. **Graceful close on dropped producer.** Both catalog and track polls now treat `moq_net::Error::Dropped` as EOS. Producers that drop without calling `finish()` no longer surface as exporter errors. Restructuring: - `pub use mkv::*;` tightened to `pub use mkv::Mkv;` (and `Fmp4`, `Avc1`, `Hvc1`); only intended types are re-exported from `export`. - `mkv_test.rs` moved under `export/test/` to match the import-side layout. - `import/annexb` is now `pub(crate)` so the export-side transmuxers can share its NAL iterator. New tests: - `export_avc3_source_synthesizes_avcc_and_length_prefixes` — end-to-end proof that an Avc3 broadcast (inline=true, no description, Annex-B samples) yields valid MKV with avcC in CodecPrivate and length-prefixed blocks. - `export_fragment_duration_batches_blocks` — 5 frames within 100ms batch into a single Cluster when fragment_duration=2s. - `avc1_*` / `hvc1_*` unit tests for the transmuxer state machine (parameter caching, codec config build, passthrough, idempotency). Mid-stream Tracks updates are still a known limitation; out of scope here. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-mux/src/export/mkv.rs (1)
597-602:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject multichannel Opus until the channel mapping is encoded correctly.
build_opus_head()always writes mapping family0, which is only valid for mono/stereo. Forchannel_count > 2this produces an invalidCodecPrivateeven though the rest of the track metadata says the stream is multichannel.Suggested fix
AudioCodec::Opus => ( "A_OPUS", - Some(build_opus_head(config.sample_rate, config.channel_count)), + Some(build_opus_head(config.sample_rate, config.channel_count)?), ), @@ -fn build_opus_head(sample_rate: u32, channels: u32) -> Vec<u8> { +fn build_opus_head(sample_rate: u32, channels: u32) -> anyhow::Result<Vec<u8>> { + anyhow::ensure!(channels <= 2, "MKV export only supports mono/stereo Opus for now"); let mut head = Vec::with_capacity(19); head.extend_from_slice(b"OpusHead"); head.push(1); // version head.push(channels as u8); head.extend_from_slice(&0u16.to_le_bytes()); // pre-skip head.extend_from_slice(&sample_rate.to_le_bytes()); head.extend_from_slice(&0i16.to_le_bytes()); // output gain head.push(0); // channel mapping family (0 = mono/stereo) - head + Ok(head) }Also applies to: 632-640
🤖 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-mux/src/export/mkv.rs` around lines 597 - 602, The code currently always builds an Opus CodecPrivate with build_opus_head(...) which hardcodes mapping family 0 (valid only for 1–2 channels), so in build_audio_track_entry (and the similar block around lines 632–640) add a guard for AudioCodec::Opus that rejects channel_count > 2: check config.channel_count and return an anyhow::Error (or bail!) when > 2, with a clear message like "multichannel Opus not supported — mapping family not encoded"; alternatively, if you prefer to support it now, extend build_opus_head to accept a mapping_family/channel mapping parameter and only call it when you can construct a valid mapping for channel_count > 2. Reference: build_audio_track_entry, build_opus_head, AudioCodec::Opus, config.channel_count.
🤖 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 `@rs/moq-mux/src/export/mkv.rs`:
- Around line 240-255: The loop currently stores slice-only frames into
track.pending (in the poll_read loop inside the tracks iteration) even when
codec_private() is still None for Avc3/Hev1, which can prevent later SPS/PPS/VPS
reads and stall; change the logic in the poll/push path that sets track.pending
(the branch that uses track.video_transmux and assigns Some(f) to track.pending)
to first check the track’s codec type and that track.codec_private() is present
for Avc3/Hev1 before parking a slice-only frame, skipping/continuing the loop
instead if codec_private() is None; apply the same guard to the analogous code
region around the other block mentioned (the second polling site).
- Around line 541-545: In build_video_transmux, normalize
VideoConfig.description by treating Some(empty) as None before calling Avc1::new
/ Hvc1::new: e.g. compute let desc = config.description.clone().filter(|b|
!b.is_empty()); and pass desc to Avc1::new or Hvc1::new instead of
config.description.clone() so empty Bytes don't cause a passthrough and leave an
empty CodecPrivate; update references in build_video_transmux accordingly.
---
Outside diff comments:
In `@rs/moq-mux/src/export/mkv.rs`:
- Around line 597-602: The code currently always builds an Opus CodecPrivate
with build_opus_head(...) which hardcodes mapping family 0 (valid only for 1–2
channels), so in build_audio_track_entry (and the similar block around lines
632–640) add a guard for AudioCodec::Opus that rejects channel_count > 2: check
config.channel_count and return an anyhow::Error (or bail!) when > 2, with a
clear message like "multichannel Opus not supported — mapping family not
encoded"; alternatively, if you prefer to support it now, extend build_opus_head
to accept a mapping_family/channel mapping parameter and only call it when you
can construct a valid mapping for channel_count > 2. Reference:
build_audio_track_entry, build_opus_head, AudioCodec::Opus,
config.channel_count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abafb0f7-13a8-4370-a2bd-d08f40cfc8d6
📒 Files selected for processing (7)
rs/moq-mux/src/export/avc1.rsrs/moq-mux/src/export/hvc1.rsrs/moq-mux/src/export/mkv.rsrs/moq-mux/src/export/mod.rsrs/moq-mux/src/export/test/mkv.rsrs/moq-mux/src/export/test/mod.rsrs/moq-mux/src/import/mod.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-mux/src/export/test/mod.rs
The MKV exporter previously defaulted to 2-second fragments; the fMP4 exporter emitted one fragment per frame. Both are now consistent and GOP-aligned by default: - `Fmp4` gains `with_fragment_duration(d)`, mirroring `Mkv`. Each video fragment now contains one GOP (rolled over on keyframes); the cap applies in addition. `Duration::ZERO` retains the historical one-fragment-per-frame behavior. - `Mkv::with_fragment_duration` now takes `Option<Duration>` semantics internally — default `None` means "roll only on GOP boundaries or i16 overflow", `Some(d)` caps each cluster to roughly `d`, `Some(ZERO)` emits one cluster per frame. - moq-cli `subscribe` learns `--format mkv` and `--fragment-duration` (humantime-parsed). The new flag is plumbed into both exporters; if unset, each exporter uses its per-GOP default. - `Fmp4` also picks up the `is_dropped` helper so a producer drop without finish() doesn't error out the exporter. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
So callers with an Option<Duration> in hand (like moq-cli reading `--fragment-duration`) can pass it through without an `if let Some` unwrap, while callers with a plain Duration still work. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-mux/src/export/fmp4.rs`:
- Around line 169-183: The chosen-branch can never flush for live audio-only
when fragment_duration is None, causing indefinite buffering; update the branch
in the poll path (the block that reads fragment_duration, tracks.get_mut(&name),
track.pending.take(), should_flush, encode_fragment and calls self.poll_next) so
that when should_flush is false but fragment_duration is None and the track
represents audio-only (use the track kind/metadata to detect audio-only), you
force a flush: take track.buffer, call encode_fragment(track, frames) to produce
emit, push the pending frame back into buffer and return
Poll::Ready(Ok(Some(emit))). This ensures audio-only streams emit fragments in
the default None mode instead of recursing forever.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9d5a89e-9bed-41d9-8db8-ef251fe44c13
📒 Files selected for processing (3)
rs/moq-cli/src/subscribe.rsrs/moq-mux/src/export/fmp4.rsrs/moq-mux/src/export/mkv.rs
The Avc1/Hvc1 transmuxers are pure per-frame transforms with no broadcast coupling; they happened to live in export/ only because the MKV exporter was the first caller. Hoist them into their own module so future codec-shape transforms (or other consumers) have an obvious home, and so the public surface reads as composable primitives rather than export internals. - New `moq_mux::transform` module with `Avc1` and `Hvc1`. - `transform(payload: Bytes)` instead of `&[u8]`: passthrough is a refcount clone (zero-copy), Avc3 path slices into the input. - H.264 NAL type literals replaced with named constants (NAL_TYPE_SPS, NAL_TYPE_PPS). - Round-trip assertion added to the Avc3→MKV integration test: the re-imported catalog's `description` must equal the avcC the exporter wrote. Catches structural mistakes that field-by-field checks miss. - Renamed internal Mkv state from `transmux`/`VideoTransmux` to `transform`/`VideoTransform` to match the module name. - Fixed a stale `crate::transmux::Avc1` doc link in hvc1.rs that the rustdoc `-D warnings` check flagged. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
1. import/mkv: dedup Tracks on partial-input replay. drain() retains `last_emitted_tag_offset` so the iterator can restart cleanly on the next chunk. If Tracks happens to be the last fully-emitted tag (no later Cluster yet), that offset points at Tracks's start and the next call re-emits it, which previously created duplicate renditions with fresh unique-track names. Guard with `tracks_seen`. 2. export/mkv: don't park Avc3/Hev1 slice-only frames before the codec config exists. A receiver subscribing mid-GOP would land on a P-slice before any SPS/PPS, the transform produces a valid length-prefixed payload but `codec_private()` stays None. Parking it in `pending` stopped further polling, so the eventual keyframe's parameter sets were never read and the exporter stalled. Drop pre-header slices when the transmuxer hasn't produced its config yet; keep polling. 3. export/fmp4: audio-only streams stalled in default `None` mode. With no video keyframe and no time cap, should_flush always returned false and the buffer grew without bound. Fall back to per-frame fragments for audio in audio-only broadcasts. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
The previous commit's should_flush signature exceeded fmt's multi-line trigger threshold by one parameter; rustfmt collapses it back to a single line. CI was failing on `cargo fmt --all --check`. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-mux/src/import/mkv.rs (1)
648-653:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAV1 bitdepth derivation uses wrong av1C field (
seq_tier_0)In
rs/moq-mux/src/import/mkv.rs(build_av1_config, lines 648-653), bitdepth is computed from(av1c.seq_tier_0, av1c.high_bitdepth), but inav1Cbitdepth is determined byhigh_bitdepthandtwelve_bit(8 if!high_bitdepth, 10 ifhigh_bitdepth && !twelve_bit, 12 ifhigh_bitdepth && twelve_bit).seq_tier_0is the sequence tier indicator, not a bitdepth flag—switch the match to useav1c.twelve_bit.🤖 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-mux/src/import/mkv.rs` around lines 648 - 653, The AV1 bitdepth calculation in build_av1_config is using the wrong av1C field (av1c.seq_tier_0); change the match to use av1c.twelve_bit instead of seq_tier_0 and compute bitdepth as: if !av1c.high_bitdepth => 8, else if av1c.high_bitdepth && !av1c.twelve_bit => 10, else (high_bitdepth && twelve_bit) => 12; update the match arm on the tuple (av1c.twelve_bit, av1c.high_bitdepth) or equivalent boolean logic and ensure the field names (twelve_bit, high_bitdepth) are correctly referenced in build_av1_config.
🧹 Nitpick comments (3)
rs/moq-mux/src/transform/hvc1.rs (1)
171-179: 💤 Low valueConstraint flags may be incomplete for extended profiles.
The
pack_constraint_flagsfunction only populates the first byte of the 6-byte constraint flags array. For Main/Main10 profiles this is typically sufficient, but profiles usingmax_*_constraint_flagsorinbld_flag(bytes 1-5) will have those bits zeroed. This is a low-risk edge case since most H.265 content uses common profiles.🤖 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-mux/src/transform/hvc1.rs` around lines 171 - 179, The pack_constraint_flags function currently only fills flags[0], leaving bytes 1–5 zero which drops extended constraint bits (e.g., max_*_constraint_flags and inbld_flag) for extended profiles; update pack_constraint_flags (and its use of scuffle_h265::Profile) to serialize all six constraint bytes by mapping the Profile's max_*_constraint_flags fields and inbld_flag into flags[1]..flags[5] using the correct bit positions from ITU H.265 §7.3.3 so extended-profile bits are preserved (ensure you read the corresponding Profile fields and OR/shift them into flags[1..=5] rather than leaving them zero).rs/moq-mux/src/export/mkv.rs (1)
640-654: 💤 Low valuePotential panic from
.unwrap()on codec_private.Line 646 calls
codec_private.unwrap()after the match arms have already set it toSome(...). While this is currently safe because both match arms setcodec_privatetoSome(...), a future edit adding a new codec could introduce a panic if it setscodec_private = None. Consider restructuring to avoid the unwrap.♻️ Suggested safer alternative
fn build_audio_track_entry(track_number: u64, config: &AudioConfig) -> anyhow::Result<MatroskaSpec> { - let (codec_id, codec_private) = match &config.codec { + let (codec_id, codec_private): (&str, Vec<u8>) = match &config.codec { AudioCodec::Opus => ( "A_OPUS", - Some(build_opus_head(config.sample_rate, config.channel_count)), + build_opus_head(config.sample_rate, config.channel_count), ), AudioCodec::AAC(_) => ( "A_AAC", - Some( - config - .description - .as_ref() - .context("AAC track missing AudioSpecificConfig (description)")? - .to_vec(), - ), + config + .description + .as_ref() + .context("AAC track missing AudioSpecificConfig (description)")? + .to_vec(), ), other => anyhow::bail!("MKV export does not support audio codec {:?}", other), }; let entry = vec![ MatroskaSpec::TrackNumber(track_number), MatroskaSpec::TrackUID(track_number), MatroskaSpec::TrackType(2), MatroskaSpec::CodecID(codec_id.to_string()), - MatroskaSpec::CodecPrivate(codec_private.unwrap()), + MatroskaSpec::CodecPrivate(codec_private), MatroskaSpec::Audio(Master::Full(vec![🤖 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-mux/src/export/mkv.rs` around lines 640 - 654, The code currently calls codec_private.unwrap() when building the TrackEntry which is fragile; instead, avoid unwrap by conditionally including MatroskaSpec::CodecPrivate only when codec_private is Some. Modify the TrackEntry construction in the function that builds the entry: create the entry Vec, push the fixed fields as before (TrackNumber, TrackUID, TrackType, CodecID, Audio...), then use if let Some(cp) = codec_private { entry.push(MatroskaSpec::CodecPrivate(cp)); } before returning Ok(MatroskaSpec::TrackEntry(Master::Full(entry))). This removes the unwrap and prevents a future panic if codec_private becomes None.rs/moq-mux/src/import/mkv.rs (1)
62-69: 💤 Low value
MkvTrack.groupfield appears to be dead code.The
groupfield is initialized toNone(line 310) and only accessed via.take()(lines 379, 397) but is never assigned a value. The conditionif let Some(mut prev) = track.group.take()will always beNone, making the associatedfinish()calls unreachable.If groups are managed internally by
track.track, consider removing this field and the associated dead branches.🧹 Remove unused field
struct MkvTrack { kind: TrackKind, track: crate::container::Producer<crate::container::Hang>, - group: Option<moq_net::GroupProducer>, /// Highest block timestamp (Matroska ticks: cluster_ts + block_relative) already emitted. /// Used to dedup re-parsed blocks across decode() calls. last_emitted_ticks: Option<i64>, }And remove the corresponding initialization and
.take()calls at lines 310, 379-381, and 397-399.🤖 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-mux/src/import/mkv.rs` around lines 62 - 69, The MkvTrack.group field is unused and its .take() branches are dead; remove the field from struct MkvTrack, remove any initialization that sets it to None (the track creation/initialization site), and delete the code paths that call track.group.take() and subsequently call finish() on the extracted value (the branches in decode()/processing where you do `if let Some(mut prev) = track.group.take() { prev.finish() }`). Ensure no other code references `group` so the struct and its consumers compile cleanly.
🤖 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.
Outside diff comments:
In `@rs/moq-mux/src/import/mkv.rs`:
- Around line 648-653: The AV1 bitdepth calculation in build_av1_config is using
the wrong av1C field (av1c.seq_tier_0); change the match to use av1c.twelve_bit
instead of seq_tier_0 and compute bitdepth as: if !av1c.high_bitdepth => 8, else
if av1c.high_bitdepth && !av1c.twelve_bit => 10, else (high_bitdepth &&
twelve_bit) => 12; update the match arm on the tuple (av1c.twelve_bit,
av1c.high_bitdepth) or equivalent boolean logic and ensure the field names
(twelve_bit, high_bitdepth) are correctly referenced in build_av1_config.
---
Nitpick comments:
In `@rs/moq-mux/src/export/mkv.rs`:
- Around line 640-654: The code currently calls codec_private.unwrap() when
building the TrackEntry which is fragile; instead, avoid unwrap by conditionally
including MatroskaSpec::CodecPrivate only when codec_private is Some. Modify the
TrackEntry construction in the function that builds the entry: create the entry
Vec, push the fixed fields as before (TrackNumber, TrackUID, TrackType, CodecID,
Audio...), then use if let Some(cp) = codec_private {
entry.push(MatroskaSpec::CodecPrivate(cp)); } before returning
Ok(MatroskaSpec::TrackEntry(Master::Full(entry))). This removes the unwrap and
prevents a future panic if codec_private becomes None.
In `@rs/moq-mux/src/import/mkv.rs`:
- Around line 62-69: The MkvTrack.group field is unused and its .take() branches
are dead; remove the field from struct MkvTrack, remove any initialization that
sets it to None (the track creation/initialization site), and delete the code
paths that call track.group.take() and subsequently call finish() on the
extracted value (the branches in decode()/processing where you do `if let
Some(mut prev) = track.group.take() { prev.finish() }`). Ensure no other code
references `group` so the struct and its consumers compile cleanly.
In `@rs/moq-mux/src/transform/hvc1.rs`:
- Around line 171-179: The pack_constraint_flags function currently only fills
flags[0], leaving bytes 1–5 zero which drops extended constraint bits (e.g.,
max_*_constraint_flags and inbld_flag) for extended profiles; update
pack_constraint_flags (and its use of scuffle_h265::Profile) to serialize all
six constraint bytes by mapping the Profile's max_*_constraint_flags fields and
inbld_flag into flags[1]..flags[5] using the correct bit positions from ITU
H.265 §7.3.3 so extended-profile bits are preserved (ensure you read the
corresponding Profile fields and OR/shift them into flags[1..=5] rather than
leaving them zero).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd7ccbb8-a427-48ad-9c20-5e95b602ca24
📒 Files selected for processing (9)
rs/moq-mux/src/export/fmp4.rsrs/moq-mux/src/export/mkv.rsrs/moq-mux/src/export/mod.rsrs/moq-mux/src/export/test/mkv.rsrs/moq-mux/src/import/mkv.rsrs/moq-mux/src/lib.rsrs/moq-mux/src/transform/avc1.rsrs/moq-mux/src/transform/hvc1.rsrs/moq-mux/src/transform/mod.rs
Collapse the `Tracks` idempotency check into a match guard to satisfy clippy::collapsible_match (the CI failure). Treat an empty `description` the same as `None` in `build_video_transform` so Annex-B H.264/H.265 sources don't get stuck in passthrough mode. Reject add/remove of MKV renditions once the `Tracks` header has been emitted since the MKV `Tracks` element is written once and can't be amended. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-mux/src/export/fmp4.rs (1)
352-369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore drained video tracks in the audio-only fallback.
This fallback still breaks once video ends before audio.
has_video_trackis derived from every entry inself.tracks, so a finished/drained video track keeps Line 369 false and live audio can buffer indefinitely again until the backlog happens to drain.💡 Minimal fix
- let has_video_track = self.tracks.values().any(|t| t.is_video); + let has_video_track = self + .tracks + .values() + .any(|t| t.is_video && (!t.finished || t.pending.is_some() || !t.buffer.is_empty()));🤖 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-mux/src/export/fmp4.rs` around lines 352 - 369, The audio-only fallback uses has_video_track derived from all tracks so drained/finished video tracks prevent audio fragments from flushing; update the logic that computes/passes has_video_track into should_flush to only consider active (non-drained) video tracks (e.g. video tracks with buffered frames or a !drained flag) and/or rename the parameter to has_active_video_track, then use that new value in should_flush (the None branch) so audio-only flushing ignores drained video tracks. Ensure callers that compute has_video_track (the site that iterates self.tracks) are changed accordingly and tests updated to reflect the active-video semantics.
🤖 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 `@rs/moq-mux/src/export/mkv.rs`:
- Around line 359-371: The current post-header layout check wrongly compares the
new catalog against the mutable live map self.tracks (which is pruned), causing
false "track added/removed" errors; instead freeze and compare against the
header-declared snapshot. Fix by ensuring when you emit the header you capture
and store the declared rendition set into self.catalog_snapshot (e.g., record
the set of track names at header emission) and then in the block guarded by
header_emitted use that stored snapshot (self.catalog_snapshot) as the canonical
list to compare against active, not self.tracks; bail only if active differs
from that frozen snapshot and do not overwrite catalog_snapshot on subsequent
checks.
---
Outside diff comments:
In `@rs/moq-mux/src/export/fmp4.rs`:
- Around line 352-369: The audio-only fallback uses has_video_track derived from
all tracks so drained/finished video tracks prevent audio fragments from
flushing; update the logic that computes/passes has_video_track into
should_flush to only consider active (non-drained) video tracks (e.g. video
tracks with buffered frames or a !drained flag) and/or rename the parameter to
has_active_video_track, then use that new value in should_flush (the None
branch) so audio-only flushing ignores drained video tracks. Ensure callers that
compute has_video_track (the site that iterates self.tracks) are changed
accordingly and tests updated to reflect the active-video semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e300c41e-1a49-42fb-9811-68b99d775846
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rs/moq-mux/src/export/fmp4.rsrs/moq-mux/src/export/mkv.rsrs/moq-mux/src/import/mkv.rs
| if self.header_emitted { | ||
| for name in active.keys() { | ||
| if !self.tracks.contains_key(name) { | ||
| anyhow::bail!("MKV track layout changed after header was emitted: track '{name}' added"); | ||
| } | ||
| } | ||
| for name in self.tracks.keys() { | ||
| if !active.contains_key(name) { | ||
| anyhow::bail!("MKV track layout changed after header was emitted: track '{name}' removed"); | ||
| } | ||
| } | ||
| self.catalog_snapshot = Some(catalog); | ||
| return Ok(()); |
There was a problem hiding this comment.
Compare post-header layouts against the declared header, not self.tracks.
self.tracks is pruned once a track finishes (Line 343), so this can reject an unchanged later catalog snapshot as a bogus “track added” after a drained track is removed locally. Freeze the header-declared rendition set and compare against that instead of the mutable live-track map here.
Suggested direction
if self.header_emitted {
+ let declared = self
+ .catalog_snapshot
+ .as_ref()
+ .context("missing header catalog snapshot")?;
+ let declared_names = declared
+ .video
+ .renditions
+ .keys()
+ .chain(declared.audio.renditions.keys())
+ .collect::<std::collections::HashSet<_>>();
for name in active.keys() {
- if !self.tracks.contains_key(name) {
+ if !declared_names.contains(&name) {
anyhow::bail!("MKV track layout changed after header was emitted: track '{name}' added");
}
}
- for name in self.tracks.keys() {
+ for name in declared_names {
if !active.contains_key(name) {
anyhow::bail!("MKV track layout changed after header was emitted: track '{name}' removed");
}
}
- self.catalog_snapshot = Some(catalog);
return Ok(());
}📝 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.
| if self.header_emitted { | |
| for name in active.keys() { | |
| if !self.tracks.contains_key(name) { | |
| anyhow::bail!("MKV track layout changed after header was emitted: track '{name}' added"); | |
| } | |
| } | |
| for name in self.tracks.keys() { | |
| if !active.contains_key(name) { | |
| anyhow::bail!("MKV track layout changed after header was emitted: track '{name}' removed"); | |
| } | |
| } | |
| self.catalog_snapshot = Some(catalog); | |
| return Ok(()); | |
| if self.header_emitted { | |
| let declared = self | |
| .catalog_snapshot | |
| .as_ref() | |
| .context("missing header catalog snapshot")?; | |
| let declared_names = declared | |
| .video | |
| .renditions | |
| .keys() | |
| .chain(declared.audio.renditions.keys()) | |
| .collect::<std::collections::HashSet<_>>(); | |
| for name in active.keys() { | |
| if !declared_names.contains(&name) { | |
| anyhow::bail!("MKV track layout changed after header was emitted: track '{name}' added"); | |
| } | |
| } | |
| for name in declared_names { | |
| if !active.contains_key(name) { | |
| anyhow::bail!("MKV track layout changed after header was emitted: track '{name}' removed"); | |
| } | |
| } | |
| return Ok(()); | |
| } |
🤖 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-mux/src/export/mkv.rs` around lines 359 - 371, The current post-header
layout check wrongly compares the new catalog against the mutable live map
self.tracks (which is pruned), causing false "track added/removed" errors;
instead freeze and compare against the header-declared snapshot. Fix by ensuring
when you emit the header you capture and store the declared rendition set into
self.catalog_snapshot (e.g., record the set of track names at header emission)
and then in the block guarded by header_emitted use that stored snapshot
(self.catalog_snapshot) as the canonical list to compare against active, not
self.tracks; bail only if active differs from that frozen snapshot and do not
overwrite catalog_snapshot on subsequent checks.
| /// is already an `AVCDecoderConfigurationRecord` (avc1 source); pass | ||
| /// `None` for an avc3 source whose samples are Annex-B with inline | ||
| /// parameter sets. | ||
| pub fn new(description: Option<Bytes>) -> Self { |
There was a problem hiding this comment.
Remove this description? IDK it seems useless because we're converting Avc3 (no description) -> Avc1 (description).
| pub struct Avc1 { | ||
| /// Pre-supplied avcC (Avc1 source) or one we built ourselves (Avc3 source). | ||
| avcc: Option<Bytes>, | ||
| cached_sps: Option<Bytes>, |
There was a problem hiding this comment.
remove the cached_ prefix thanks.
| /// downstream consumers that throttle by fragment rate (e.g. KVS | ||
| /// PutMedia) typically need. The cap applies in addition to GOP rollover. |
There was a problem hiding this comment.
Remove the comment about KVS PutMedia.
| buffer: Vec<Frame>, | ||
|
|
||
| /// True if this track is video. Video tracks roll fragments on keyframes. | ||
| is_video: bool, |
There was a problem hiding this comment.
We should do this at a higher level? Like we want audio to be muxed with video into a single GoP-sized fragment. If there's no video tracks at all, then we can fragment per frame I guess.
ex. maybe a flush() method called on every track when there's a keyframe? Or immediately with no video tracks? IDK
|
|
||
| /// Frames accumulated for the current fragment. Flushed as a single | ||
| /// moof+mdat on the next keyframe (video) or duration cap. | ||
| buffer: Vec<Frame>, |
There was a problem hiding this comment.
Do we also need pending above? Do they overlap?
| Poll::Ready(Err(ref e)) if is_dropped(e) => { | ||
| self.catalog = None; | ||
| break; | ||
| } |
There was a problem hiding this comment.
No this seems like a bug. We should return Ok(None) if the track is cleanly terminated. Dropped means finish was not called.
| break; | ||
| } | ||
| Poll::Pending => break, | ||
| Poll::Ready(Err(ref e)) if is_dropped(e) => { |
| track.finished = true; | ||
| break; | ||
| } | ||
| Poll::Ready(Err(e)) if is_dropped(&e) => { |
There was a problem hiding this comment.
Same, remove this it's hiding a bug with the publisher.
| match container { | ||
| Container::Legacy => Ok(()), | ||
| Container::Cmaf { .. } => { | ||
| anyhow::bail!("MKV export does not support CMAF {} track '{}'", kind, name); |
There was a problem hiding this comment.
We need a plan for this. If you need a transform middleware, then use/make it.
- Resolve merge conflicts with main; integrate CatalogFormat selection into both Fmp4::new and the new Mkv::new (factored CatalogSource/CatalogFormat into export/mod.rs so both exporters share them). - Stop swallowing moq_net::Error::Dropped as graceful end-of-stream in both Fmp4 and Mkv exporters; Dropped means finish() was not called, so it propagates as an error instead. Tests now explicitly finish catalogs. - Drop the description parameter from Avc1::new / Hvc1::new: these transforms only handle Avc3 -> Avc1 / Hev1 -> Hvc1, and build_video_transform now short-circuits when the source already carries an avcC/hvcC. The avcC/hvcC for those passthrough cases is taken from the catalog description in build_video_track_entry. - Rename cached_sps/cached_pps/cached_vps to sps/pps/vps. - Drop the KVS PutMedia callout from the fragment_duration doc comments and the Mkv top-level doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…0lWOH # Conflicts: # rs/moq-cli/src/subscribe.rs # rs/moq-mux/src/export/fmp4.rs
2 participants
Adds comprehensive Matroska (MKV) and WebM container support to moq-mux, enabling ingestion and re-export of MKV/WebM files as MoQ broadcast streams.
Summary
This change introduces bidirectional Matroska/WebM support:
rs/moq-mux/src/import/mkv.rs): Parses MKV/WebM files and converts them to MoQ broadcast streams, supporting both batch and streaming input with idempotent buffer replayrs/moq-mux/src/export/mkv.rs): Subscribes to MoQ broadcasts and produces valid Matroska/WebM byte streams for consumption by standard media playersKey Changes
webm-iterable, manages per-track state with deduplication across buffer replays, and supports multiple video codecs (H.264, H.265, VP8, VP9, AV1) and audio codecs (AAC, Opus)Mkvvariant to bothFramedFormatandStreamFormatenums to enable CLI supportwebm-iterable = "0.6"for EBML parsing and writingImplementation Details
WebmIteratorwith hierarchy problem tolerance and idempotent tag handling to support streaming input where the buffer may be replayed mid-elementlast_emitted_ticksdeduplication prevents duplicate frame emission across decode() callshangcatalog abstraction for codec metadata and thecontainer::Producer/Consumerfor frame I/Ohttps://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45