feat(moq-mux, libmoq): add CMSF muxer, demuxer, and C API#1408
Open
ksletmoe-aws wants to merge 1 commit into
Open
feat(moq-mux, libmoq): add CMSF muxer, demuxer, and C API#1408ksletmoe-aws wants to merge 1 commit into
ksletmoe-aws wants to merge 1 commit into
Conversation
Contributor
|
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:
WalkthroughAdds end-to-end CMSF support: extends MSF catalog schema with CMSF fields, adds CMSF parsing utilities, refactors the fMP4 decoder into a parser/sink, implements CMSF broadcast producer and per-track producers with grouping/SAP/jitter logic, provides an fMP4→CMSF importer and CMSF demuxers (including merged-init builder), exposes Rust Publish APIs and C FFI for CMSF, integrates CMSF into moq-cli and demo recipes, and updates JS decoders. The change includes unit and integration tests covering parsing, import, publish, demux, and error cases. 🚥 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
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
rs/moq-mux/src/import/test/mod.rs (1)
131-271: 💤 Low valueConsider extracting magic numbers to named constants.
Several magic numbers appear throughout the new tests without clear semantic names:
90000,48000(timescales)0x0200_0000(appears multiple times, likely a SAP type flag)3003(duration/timestamp)1024(frame size)10,3(test iteration limits)While test code context makes some of these clear, named constants would improve readability and maintainability, especially for the hex constant
0x0200_0000which appears in multiple tests.As per coding guidelines, "Avoid using magic numbers; use named constants instead."
💡 Example improvement
// At module level const VIDEO_TIMESCALE: u32 = 90000; const AUDIO_TIMESCALE: u32 = 48000; const SAP_TYPE_1_FLAG: u32 = 0x0200_0000; // SAP type 1 (closed GOP) const TEST_FRAME_DURATION: u64 = 3003; const TEST_FRAME_SIZE: u64 = 1024; // Then use in tests: let init_data = crate::cmsf::test_helpers::build_init_segment(VIDEO_TIMESCALE); let frame = crate::cmsf::test_helpers::build_moof_mdat(0, TEST_FRAME_DURATION, SAP_TYPE_1_FLAG);🤖 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/test/mod.rs` around lines 131 - 271, Tests use several unexplained magic numbers (timescales, SAP flag, durations, sizes, loop limits); extract these into module-level named constants and replace literals in tests like test_cmsf_producer_consumer_roundtrip, test_cmsf_demuxer_skips_malformed_frames, and test_cmsf_demuxer_end_reason_track_finished. Define constants (e.g., VIDEO_TIMESCALE, AUDIO_TIMESCALE, SAP_TYPE_1_FLAG, TEST_FRAME_DURATION, TEST_FRAME_SIZE, MAX_ITERATIONS) and use them where calling crate::cmsf::test_helpers::build_init_segment, build_moof_mdat, and in loop/assert limits instead of hard-coded numbers to improve readability and reuse.rs/moq-mux/src/import/fmp4.rs (1)
447-453: ⚡ Quick winRemove the em dashes from this doc comment.
This comment uses
—, which the repository prose rules forbid. Please replace them with commas or separate sentences.As per coding guidelines, "No em dashes (—) in code, comments, doc comments, commit messages, or any prose. Use a period and start a new sentence, or use a comma/parenthesis if clauses are tightly bound".
🤖 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/fmp4.rs` around lines 447 - 453, The doc comment describing the AAC AudioSpecificConfig uses em dashes (—); update that comment block (the lines mentioning audioObjectType, samplingFreqIndex, samplingFrequency, and channelConfiguration) to remove all em dash characters and replace them with commas or separate sentences per project prose rules so the entries read like normal comma- or sentence-separated descriptions (e.g., "samplingFreqIndex (4 bits), index into the standard table, or 0xF" or break into two sentences) and ensure the example line remains unchanged except for replacing any em dash occurrences.rs/moq-mux/src/import/cmsf_fmp4.rs (1)
99-101: ⚡ Quick winReplace the em dashes in these doc comments.
The repository prose rules disallow
—in comments and doc comments. Please split these into separate sentences or use commas/parentheses instead.As per coding guidelines, "No em dashes (—) in code, comments, doc comments, commit messages, or any prose. Use a period and start a new sentence, or use a comma/parenthesis if clauses are tightly bound".
Also applies to: 233-235
🤖 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/cmsf_fmp4.rs` around lines 99 - 101, Replace all em dashes in the doc comments with periods or commas and split clauses into separate sentences; specifically update the NOTE comment that currently reads "All video tracks are assigned `alt_group: Some(1)` — this assumes a single ABR ladder where all video renditions are switchable. Multi-angle or independent video tracks are not supported via the fMP4 import path." (and the similar comment at the other occurrence on lines 233-235) so it becomes two sentences or uses a comma/parenthesis (e.g., "All video tracks are assigned `alt_group: Some(1)`. This assumes a single ABR ladder where all video renditions are switchable. Multi-angle or independent video tracks are not supported via the fMP4 import path."). Ensure no em dash characters remain in cmsf_fmp4.rs doc comments.rs/moq-mux/src/import/cmsf_types.rs (1)
40-44: ⚡ Quick winReplace the em dashes in the new comments.
The added doc/test comments on these lines use em dashes, which conflicts with the repo prose rule. Please switch them to commas, parentheses, or separate sentences.
As per coding guidelines,
**/*.{rs,ts,tsx,js,jsx,md}:No em dashes (—) in code, comments, doc comments, commit messages, or any prose. Use a period and start a new sentence, or use a comma/parenthesis if clauses are tightly bound.Also applies to: 51-55, 516-516
🤖 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/cmsf_types.rs` around lines 40 - 44, Replace all em dashes in the doc comments in cmsf_types.rs with commas, parentheses, or separate sentences; specifically update the doc block that begins "A CMSF Object — the moof+mdat payload..." and the sentence that references CmsfBroadcastProducer::write() to remove the em dash and rephrase using a comma or period, and also fix the other doc/test comments in the same file (the subsequent comment block around the same description and the comment near the end of the file that currently uses an em dash) so no em dash remains anywhere in those comment blocks.
🤖 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 `@js/watch/src/audio/decoder.ts`:
- Around line 274-277: The isConfigSupported probe (the function that checks
supported(config)) must mirror runCmafDecoder's fallback: when config.codec !==
"opus" and config.description is absent, use the init segment's description as
the codec description before probing support. Update
isConfigSupported/supported(config) to obtain description = config.description ?
Util.Hex.toBytes(config.description) : init.description (keeping the opus case
returning undefined) and pass that into the support probe so CMAF audio without
catalog description is treated the same way as runCmafDecoder.
In `@js/watch/src/video/decoder.ts`:
- Around line 365-369: The comment above the description assignment contains an
em dash; remove the em dash and split into two sentences per guidelines. Edit
the comment that mentions "MSF/CMSF catalogs don't carry a separate description
field — the codec description..." so it becomes two sentences (e.g., "MSF/CMSF
catalogs don't carry a separate description field. The codec description (avcC,
hvcC, etc.) lives inside the init segment's moov box.") and leave the following
code using this.config.description, Util.Hex.toBytes, and init.description
unchanged.
In `@rs/libmoq/src/publish.rs`:
- Around line 122-123: The conversion currently does an unchecked cast (`tid.0
as u32`) that can silently truncate large TrackId values; replace it with a
checked conversion and checked add: use u32::try_from(tid.0)? (or
TryFrom::try_from(tid.0)?) to convert tid.0 to u32, then use
.checked_add(1).ok_or(...) to detect overflow before calling Id::try_from on the
incremented value; apply the same fix to the other occurrence that uses `tid.0
as u32` (the second instance around lines 151–152) so both use checked
conversion + checked_add before Id::try_from.
In `@rs/moq-cli/src/publish.rs`:
- Line 47: The inline comment containing "CMSF manages its own catalog tracks —
skip catalog::Producer." uses an em dash which violates the prose rules; edit
that comment near the publish.rs code (around the CMSF/catalog::Producer
mention) to remove the em dash and rephrase using a period or comma—for example:
"CMSF manages its own catalog tracks. Skip catalog::Producer."—so the comment no
longer contains an em dash.
In `@rs/moq-mux/src/cmsf.rs`:
- Around line 67-76: The loop currently coercing missing durations to zero will
hide incomplete metadata; change the logic in the fragment-duration calculation
(around traf.tfhd.default_sample_duration, trun.entries and entry.duration) to
treat "no default and no entry.duration" as an error instead of using 0:
validate each entry's duration by computing duration_value =
entry.duration.or(traf.tfhd.default_sample_duration).ok_or(<meaningful error>)?
and propagate/return that error from the surrounding function (or convert to a
Result) so incomplete sample durations are rejected rather than silently counted
as zero.
- Around line 23-48: Add the #[non_exhaustive] attribute to the public error
enum CmsfError so future variants are non-breaking: place #[non_exhaustive]
immediately above the existing #[derive(...)]/pub enum CmsfError declaration and
run cargo check to ensure no local exhaustive matches require wildcard updates
(e.g., update any pattern matches on CmsfError to include a catch-all `_` if
needed).
In `@rs/moq-mux/src/export/cmsf.rs`:
- Around line 226-243: The current loop over self.tracks.values_mut() always
walks the HashMap in the same order so one ready track can monopolize output;
change polling to use a round-robin cursor or ready queue instead. Add a cursor
field (e.g., round_robin_index or last_polled_key) to the demuxer struct and
iterate tracks starting from that cursor (wrap around) when calling
state.demuxer.poll_next(waiter), advancing the cursor to the next track after
each poll and updating it when you return a segment or when a track becomes
inactive, or alternatively maintain a VecDeque of active track keys and pop/push
keys to rotate them after polling; update uses of self.tracks and
state.demuxer.poll_next accordingly so each call continues from the next track
rather than the HashMap's fixed order.
- Around line 113-116: The code in the poll loop currently treats
Poll::Ready(Err(_)) from poll_read_frame() as a normal group boundary by
clearing self.current_group and continuing; instead, preserve and propagate the
error or mark the track ended: when poll_read_frame() returns Err(e) in the
block handling group reads, do not just set self.current_group = None and
continue—instead set the track end reason (e.g. self.end_reason =
Some(EndReason::TransportError(e))) and stop the track (e.g. call the track-stop
logic used elsewhere, or return Poll::Ready(Err(e)) so the caller sees the
transport error). Update the branch handling Poll::Ready(Err(_)) to either
return the error up from poll_read_frame() or to set end_reason and invoke the
existing track shutdown/stop method instead of silently skipping the rest of the
group.
In `@rs/moq-mux/src/import/cmsf_broadcast.rs`:
- Around line 229-235: The code currently calls self.flush_audio_buffer() /
self.start_non_switching_audio_groups() inside the is_video_keyframe branch,
which causes audio rotation to run for every video rendition; instead add a
broadcast-level latch keyed to the shared GOP/boundary (e.g. introduce a field
like last_broadcast_boundary_id or rotated_for_boundary flag on the struct) and
compute a unique boundary id from the shared group transition; then in the
is_video_keyframe branch only perform flush_audio_buffer() or
start_non_switching_audio_groups() when that boundary id differs from the
latched value, and update the latch after rotating so subsequent renditions at
the same broadcast boundary skip rotation (leave references to
first_video_group, flush_audio_buffer(), start_non_switching_audio_groups(), and
is_video_keyframe to locate the change).
- Around line 154-171: The explicit non-switching audio branches return early
after calling track.track_object_sap(...) and track.update_jitter(...), skipping
the shared republish/catalog update logic found in the republish check at Lines
223-227; update these early-return paths (the blocks around the shown diff plus
the similar blocks at 181-190 and 324-337) to invoke the same
republish/catalog-update logic before returning (either by calling the same
function/routine used in the 223-227 republish check or by extracting that logic
into a helper and calling it), so MSF/hang catalogs get their SAP and jitter
metadata refreshed when fast-path audio is processed.
In `@rs/moq-mux/src/import/fmp4_parser.rs`:
- Around line 181-183: The emitted moof currently only carries resolved sample
flags while discarding resolved default_sample_duration and default_sample_size
from the trex resolution; update the code paths that build the emitted fragment
(the logic around default_sample_duration, default_sample_size, and
default_sample_flags in the fmp4 parsing/rewrite) to copy the resolved
default_sample_duration (and default_sample_size where appropriate) into the
emitted moof/sample entries so downstream parsers can recover per-sample timing;
ensure the same fix is applied to the other mirrored block around lines
referenced (the second occurrence that currently handles trex defaults at
291-304) so both places preserve the resolved defaults in the output fragment.
- Around line 227-239: The PTS calculation for each trun entry can produce a
negative value before casting to u64 (using dts and entry.cts), which currently
wraps to a huge timestamp; change the logic in the loop over trun.entries (where
pts is computed) to compute the sum as a signed i64, clamp it to zero if
negative, and then cast to u64 (e.g., compute let adjusted = dts as i64 +
entry.cts.unwrap_or_default() as i64; use 0 when adjusted < 0, otherwise
adjusted as u64) so negative CTS-adjusted PTSs do not wrap and corrupt
ordering/jitter.
---
Nitpick comments:
In `@rs/moq-mux/src/import/cmsf_fmp4.rs`:
- Around line 99-101: Replace all em dashes in the doc comments with periods or
commas and split clauses into separate sentences; specifically update the NOTE
comment that currently reads "All video tracks are assigned `alt_group: Some(1)`
— this assumes a single ABR ladder where all video renditions are switchable.
Multi-angle or independent video tracks are not supported via the fMP4 import
path." (and the similar comment at the other occurrence on lines 233-235) so it
becomes two sentences or uses a comma/parenthesis (e.g., "All video tracks are
assigned `alt_group: Some(1)`. This assumes a single ABR ladder where all video
renditions are switchable. Multi-angle or independent video tracks are not
supported via the fMP4 import path."). Ensure no em dash characters remain in
cmsf_fmp4.rs doc comments.
In `@rs/moq-mux/src/import/cmsf_types.rs`:
- Around line 40-44: Replace all em dashes in the doc comments in cmsf_types.rs
with commas, parentheses, or separate sentences; specifically update the doc
block that begins "A CMSF Object — the moof+mdat payload..." and the sentence
that references CmsfBroadcastProducer::write() to remove the em dash and
rephrase using a comma or period, and also fix the other doc/test comments in
the same file (the subsequent comment block around the same description and the
comment near the end of the file that currently uses an em dash) so no em dash
remains anywhere in those comment blocks.
In `@rs/moq-mux/src/import/fmp4.rs`:
- Around line 447-453: The doc comment describing the AAC AudioSpecificConfig
uses em dashes (—); update that comment block (the lines mentioning
audioObjectType, samplingFreqIndex, samplingFrequency, and channelConfiguration)
to remove all em dash characters and replace them with commas or separate
sentences per project prose rules so the entries read like normal comma- or
sentence-separated descriptions (e.g., "samplingFreqIndex (4 bits), index into
the standard table, or 0xF" or break into two sentences) and ensure the example
line remains unchanged except for replacing any em dash occurrences.
In `@rs/moq-mux/src/import/test/mod.rs`:
- Around line 131-271: Tests use several unexplained magic numbers (timescales,
SAP flag, durations, sizes, loop limits); extract these into module-level named
constants and replace literals in tests like
test_cmsf_producer_consumer_roundtrip, test_cmsf_demuxer_skips_malformed_frames,
and test_cmsf_demuxer_end_reason_track_finished. Define constants (e.g.,
VIDEO_TIMESCALE, AUDIO_TIMESCALE, SAP_TYPE_1_FLAG, TEST_FRAME_DURATION,
TEST_FRAME_SIZE, MAX_ITERATIONS) and use them where calling
crate::cmsf::test_helpers::build_init_segment, build_moof_mdat, and in
loop/assert limits instead of hard-coded numbers to improve readability and
reuse.
🪄 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: 507de74b-a73a-460b-989f-cb7a02b564e2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
demo/pub/justfilejs/watch/src/audio/decoder.tsjs/watch/src/video/decoder.tsrs/libmoq/src/api.rsrs/libmoq/src/publish.rsrs/moq-cli/src/publish.rsrs/moq-msf/src/lib.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog/producer.rsrs/moq-mux/src/cmsf.rsrs/moq-mux/src/export/cmsf.rsrs/moq-mux/src/export/mod.rsrs/moq-mux/src/import/cmsf_broadcast.rsrs/moq-mux/src/import/cmsf_fmp4.rsrs/moq-mux/src/import/cmsf_types.rsrs/moq-mux/src/import/fmp4.rsrs/moq-mux/src/import/fmp4_parser.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/test/mod.rsrs/moq-mux/src/lib.rs
ksletmoe-aws
force-pushed
the
cmsf-muxer
branch
5 times, most recently
from
bbec117 to
c0710d3
Compare
ksletmoe-aws
force-pushed
the
cmsf-muxer
branch
5 times, most recently
from
2a7f431 to
4764893
Compare
Contributor
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/mod.rs (1)
1-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate module doc to reflect both exports and remove em dash.
The module documentation only describes the
Fmp4functionality but the module now also exports thecmsfmodule. Additionally, line 5 contains an em dash (—) which violates the coding guideline. As per coding guidelines, no em dashes in doc comments; use a period or comma instead. As per coding guidelines, comments must reflect the current state of the code.📝 Proposed fix
//! Subscribe to a moq broadcast and decode media frames. //! -//! [`Fmp4`] subscribes to a broadcast, decodes every track via +//! ## Fmp4 +//! +//! [`Fmp4`] subscribes to a broadcast, decodes every track via //! [`Consumer<Hang>`](crate::container::Consumer), and yields a single fMP4 / CMAF byte -//! stream — the merged init segment followed by moof+mdat fragments in +//! stream. The merged init segment is followed by moof+mdat fragments in //! timestamp order across tracks. +//! +//! ## CMSF +//! +//! The [`cmsf`] module provides [`CmsfTrackDemuxer`](cmsf::CmsfTrackDemuxer) and +//! [`CmsfBroadcastDemuxer`](cmsf::CmsfBroadcastDemuxer) for consuming CMSF broadcasts.🤖 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/mod.rs` around lines 1 - 6, Module docs only describe Fmp4 and include an em dash; update the module-level comment in mod.rs to mention both exported items (Fmp4 and the cmsf module) and remove the em dash by replacing it with a period or comma. Edit the top doc comment so it briefly states that the module exports Fmp4 (which subscribes to a broadcast and decodes tracks via Consumer<Hang>) and also exports the cmsf module, and ensure punctuation follows guidelines (no em dash).
🤖 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-cli/src/subscribe.rs`:
- Around line 61-64: The CLI currently constructs CmsfBroadcastDemuxer
unconditionally causing --max-latency to be silently ignored in CmsfFmp4 mode;
update subscribe.rs to detect when the selected mode is CmsfFmp4 (or when
creating moq_mux::export::cmsf::CmsfBroadcastDemuxer via
CmsfBroadcastDemuxer::new(self.broadcast)) and if a non-zero/max-latency value
is provided, return an explicit error (or print a clear warning and exit)
informing the user that --max-latency is not supported for CmsfFmp4 and
suggesting using CmsfTrackDemuxer or omitting the flag; this makes the failure
fast and prevents unbounded segment accumulation until demuxer-side skipping is
implemented.
- Line 67: Replace the explicit map_err stringification that discards error
sources with anyhow::Context on the demuxer calls: locate usages like
demuxer.ready().await.map_err(|e| anyhow::anyhow!("{e}")) and replace them with
demuxer.ready().await.context("failed to wait for demuxer ready") (and do the
same for the other occurrences on lines using .map_err(...) around
demuxer.open(...) and demuxer.subscribe(...)); this preserves the original error
chain while adding a clear contextual message; keep the context strings
descriptive and specific to the failing operation names (e.g., "opening
demuxer", "subscribing to demuxer").
---
Outside diff comments:
In `@rs/moq-mux/src/export/mod.rs`:
- Around line 1-6: Module docs only describe Fmp4 and include an em dash; update
the module-level comment in mod.rs to mention both exported items (Fmp4 and the
cmsf module) and remove the em dash by replacing it with a period or comma. Edit
the top doc comment so it briefly states that the module exports Fmp4 (which
subscribes to a broadcast and decodes tracks via Consumer<Hang>) and also
exports the cmsf module, and ensure punctuation follows guidelines (no em dash).
🪄 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: aa2c7286-23fd-4f2e-9061-3d7d90987c55
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
demo/pub/justfilejs/watch/src/audio/decoder.tsjs/watch/src/video/decoder.tsrs/libmoq/src/api.rsrs/libmoq/src/publish.rsrs/moq-cli/src/publish.rsrs/moq-cli/src/subscribe.rsrs/moq-msf/src/lib.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog/producer.rsrs/moq-mux/src/cmsf.rsrs/moq-mux/src/export/cmsf.rsrs/moq-mux/src/export/mod.rsrs/moq-mux/src/import/cmsf_broadcast.rsrs/moq-mux/src/import/cmsf_fmp4.rsrs/moq-mux/src/import/cmsf_types.rsrs/moq-mux/src/import/fmp4.rsrs/moq-mux/src/import/fmp4_parser.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/test/mod.rsrs/moq-mux/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-mux/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (18)
- rs/moq-mux/src/lib.rs
- js/watch/src/audio/decoder.ts
- rs/moq-mux/src/catalog/producer.rs
- demo/pub/justfile
- rs/moq-mux/src/import/test/mod.rs
- rs/moq-msf/src/lib.rs
- rs/moq-mux/src/import/mod.rs
- js/watch/src/video/decoder.ts
- rs/moq-mux/src/cmsf.rs
- rs/libmoq/src/api.rs
- rs/moq-cli/src/publish.rs
- rs/libmoq/src/publish.rs
- rs/moq-mux/src/import/cmsf_types.rs
- rs/moq-mux/src/import/fmp4_parser.rs
- rs/moq-mux/src/import/cmsf_broadcast.rs
- rs/moq-mux/src/export/cmsf.rs
- rs/moq-mux/src/import/cmsf_fmp4.rs
- rs/moq-mux/src/import/fmp4.rs
ksletmoe-aws
force-pushed
the
cmsf-muxer
branch
2 times, most recently
from
c257441 to
f33c0c2
Compare
Contributor
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/libmoq/src/api.rs`:
- Around line 375-383: The function moq_cmsf_create currently calls
state.publish.cmsf_create before calling state.origin.publish, which can leak a
producer if origin.publish fails; fix this by reordering the operations so you
call state.origin.publish(origin_id, path, /*consumer placeholder?*/ ) first (or
perform whatever checks needed) and only call state.publish.cmsf_create() after
origin.publish returns Ok, or if you must create the producer first then ensure
you immediately rollback/close it (via the publish API) when
state.origin.publish returns Err; update moq_cmsf_create to either (a) move
state.publish.cmsf_create after state.origin.publish, or (b) call the
appropriate state.publish cleanup method with the returned id/consumer on error
from state.origin.publish to avoid leaving an unreachable producer.
🪄 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: d873f067-2e1c-4904-b650-a3fa58375a0f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
demo/pub/justfilejs/watch/src/audio/decoder.tsjs/watch/src/video/decoder.tsrs/libmoq/src/api.rsrs/libmoq/src/publish.rsrs/moq-cli/src/publish.rsrs/moq-cli/src/subscribe.rsrs/moq-msf/src/lib.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog/producer.rsrs/moq-mux/src/cmsf.rsrs/moq-mux/src/export/cmsf.rsrs/moq-mux/src/export/mod.rsrs/moq-mux/src/import/cmsf_broadcast.rsrs/moq-mux/src/import/cmsf_fmp4.rsrs/moq-mux/src/import/cmsf_types.rsrs/moq-mux/src/import/fmp4.rsrs/moq-mux/src/import/fmp4_parser.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/test/mod.rsrs/moq-mux/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- rs/moq-mux/src/lib.rs
- js/watch/src/audio/decoder.ts
- rs/moq-mux/src/export/mod.rs
- rs/moq-mux/src/catalog/producer.rs
- rs/moq-cli/src/subscribe.rs
- rs/moq-cli/src/publish.rs
- rs/moq-mux/src/import/mod.rs
- rs/moq-mux/Cargo.toml
- rs/moq-msf/src/lib.rs
- js/watch/src/video/decoder.ts
- demo/pub/justfile
- rs/moq-mux/src/import/cmsf_fmp4.rs
- rs/moq-mux/src/import/test/mod.rs
- rs/moq-mux/src/cmsf.rs
- rs/moq-mux/src/import/fmp4_parser.rs
- rs/moq-mux/src/export/cmsf.rs
- rs/moq-mux/src/import/cmsf_types.rs
- rs/libmoq/src/publish.rs
- rs/moq-mux/src/import/cmsf_broadcast.rs
- rs/moq-mux/src/import/fmp4.rs
Implements the full CMSF (Common Media Server Format) pipeline: Import (publish): - CmsfFmp4Importer: bridges fMP4 input to CMSF broadcast output - CmsfBroadcastProducer: multi-rendition publisher with keyframe-aligned groups, audio pre-roll buffering, and SAP type derivation - Fmp4Parser: shared fMP4 parsing infrastructure (ftyp/moov/moof+mdat) Export (subscribe): - CmsfBroadcastDemuxer: broadcast-level coordinator managing MSF catalog subscriptions and per-track demuxers - CmsfTrackDemuxer: per-track async wrapper yielding CmsfSegment - build_merged_init: merges per-track init segments into multi-track fMP4 - moq-cli: --output cmsf-fmp4 subscribe format (reads MSF catalog) Also includes moq-msf CMSF fields, libmoq C API updates, and moq-gst sink integration.
Contributor
Author
|
Fixed in the latest push. |
kixelated
reviewed
May 16, 2026
Collaborator
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
@ksletmoe-aws what's the end goal here? it's a lot of changes...
My thought is that we should support both hang/MSF via an intermediate catalog/playlist representation, so we don't copy-paste so much of this shared CMAF code. The hang catalog is the initial intermediate representation but that was just to avoid boilerplate. I really don't want two divergent implementations that could share a huge amount of code.
We definitely need some arg like --catalog msf to download catalog instead of catalog.json. Otherwise, we should be able to share the rest of the pipeline. MSF has some differences (ex. render_group, sap stuff) that could go into the intermediate representation, get ignored entirely, or get stubbed out for now (ex. render_group seems hard-coded?).
And yeah I think you need another library for building CMAF in C. I'm a bit worried about polluting the public API with something so limited. If you need the ability to set more specific fields on a per-track basis, the existing C API could be updated instead? Currently we try to auto-detect a lot of stuff from the raw codec bitstream (ex. width/height).
| const description = this.config.description ? Util.Hex.toBytes(this.config.description) : undefined; | ||
| // MSF/CMSF catalogs don't carry a separate description field — the codec | ||
| // description (avcC, hvcC, etc.) lives inside the init segment's moov box. | ||
| const description = this.config.description ? Util.Hex.toBytes(this.config.description) : init.description; |
Collaborator
There was a problem hiding this comment.
Yeah, we should move the description so it's specific to the hang "legacy" container. Should be a separate PR of course.
| /// # Safety | ||
| /// - All pointer+length pairs must be valid. | ||
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn moq_cmsf_add_video( |
Collaborator
There was a problem hiding this comment.
That's a lot of arguments, this feels like it should be a struct.
And do you need this C API? IDK I feel like you might be better off using a different library to produce CMAF and pass it to the existing media endpoints. I reaaaaallly don't want to modify this function each time someone needs some new box added (ex. DRM).
| bitrate: u64, | ||
| framerate: f64, | ||
| timescale: u64, | ||
| init_segment: &[u8], |
Collaborator
There was a problem hiding this comment.
Some of these fields could be inferred by the codec + init segment. But yeah it's more work.
| &mut self, | ||
| id: Id, | ||
| codec: &str, | ||
| description: Option<&[u8]>, |
Collaborator
There was a problem hiding this comment.
I don't think we want both the description and the init segment. They contain overlapping information.
(the description is a subset of the init segment)
| Id::try_from(tid.0 as u32 + 1) | ||
| } | ||
|
|
||
| #[allow(clippy::too_many_arguments)] |
Collaborator
There was a problem hiding this comment.
lul yeah it should be a struct at least.
| let mut stdout = tokio::io::stdout(); | ||
|
|
||
| if self.args.max_latency != Duration::from_millis(500) { | ||
| anyhow::bail!( |
|
|
||
| let ftyp = ftyp_data.ok_or(CmsfError::NoMoov)?; | ||
|
|
||
| // Renumber track IDs sequentially to avoid collisions when merging |
Collaborator
There was a problem hiding this comment.
You should not be reusing the same init segment for multiple tracks. track_id=1 makes a lot of sense; MoQ requires demuxing.
|
|
||
| // mvhd.timescale is the movie-level timescale (not per-track). Use 1000 (ms) | ||
| // which is the conventional value for fragmented MP4 with no movie-level duration. | ||
| let timescale = 1000; |
Collaborator
There was a problem hiding this comment.
conventional? Definitely not for audio
| pub alt_group: Option<u32>, | ||
|
|
||
| /// Maximum SAP type at the start of any group (CMSF §3.4). | ||
| pub max_grp_sap_starting_type: Option<u32>, |
Collaborator
There was a problem hiding this comment.
Can you explain these to me? I don't really follow the purpose in CMSF.
| mod av01; | ||
| mod avc1; | ||
| mod avc3; | ||
| pub(crate) mod cmsf_broadcast; |
Collaborator
There was a problem hiding this comment.
IMO make a cmsf module.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds full CMSF (CMAF Streaming Format) support to the MoQ stack — a muxer (broadcast producer), demuxer (broadcast consumer), fMP4-to-CMSF bridge, and C FFI bindings.
What is CMSF?
CMSF is a profile of CMAF for live streaming over MoQ. It defines how moof+mdat fragments map to moq-lite groups/objects, how catalogs describe tracks with SAP types and init segments, and how group boundaries align with video keyframes.
Changes
New: CMSF Broadcast Producer (
import/cmsf_broadcast.rs)catalog) and optionally a hang catalog (catalog.json)New: CMSF Demuxer (
export/cmsf.rs)CmsfTrackDemuxer: per-track async consumer yieldingCmsfSegmentCmsfBroadcastDemuxer: broadcast-level coordinator managing catalog subscriptionsbuild_merged_init(): merges per-track init segments into a multi-track fMP4 init with track ID renumberingNew:
moq-cli subscribe --output cmsf-fmp4New: fMP4 → CMSF Bridge (
import/cmsf_fmp4.rs)CmsfFmp4Importer: parses fragmented MP4 stdin, auto-detects codecs, feeds CMSF producerPublishDecoder::finish(): explicit cleanup on stdin EOF to close tracks/catalogs cleanlyNew: Shared CMSF Parser (
cmsf.rs)parse_cmsf_metadata(): extracts PTS (with CTS offset), duration, keyframe from moof+mdatparse_timescale(): extracts timescale from init segmentNew: C FFI (
libmoq/src/api.rs,libmoq/src/publish.rs)moq_cmsf_create,moq_cmsf_add_video,moq_cmsf_add_audio,moq_cmsf_write,moq_cmsf_closeRefactored: fMP4 Parser (
import/fmp4_parser.rs)Fmp4Parser<S: Fmp4Sink>traitFmp4importer and newCmsfFmp4Importershare this parserMSF Catalog Extensions (
moq-msf)generated_at,max_grp_sap_starting_type,max_obj_sap_starting_typefieldsTypeScript (
js/watch)Demo
just bbb-cmsfandjust cmsfcommands for publishing via CMSF formatTesting
bbb.mp4) verifying end-to-end codec detection, init segment generation, and fragment splitting