moq-mux backport + dual-API cleanup#1341
Merged
Merged
Conversation
Ports the dev-branch moq-mux refactor to main, bumping to 0.4.0
(breaking). moq-lite is intentionally kept on main's surface (handled
in a separate PR) and adapter-mapped where necessary.
moq-mux (0.3.9 → 0.4.0, breaking):
- rename `import` module to `producer`; `Decoder` → `Framed`,
`DecoderFormat` → `FramedFormat`
- drop all cargo feature flags (mp4/h264/h265/hls/av1/aac/opus);
codec deps are mandatory
- delete the stats API (`Stats`, `StatsDrift`, `DriftTracker`,
per-decoder `.stats()` methods) including moq-cli's `--stats`
telemetry loop
- add `hang::container::Media` enum replacing the `Container` trait
impl on `hang::catalog::Container`; init_data is parsed upfront
- `msf.rs` reads init_data directly from Cmaf catalog entries
- `CatalogProducer::snapshot()` added; `ordered::Consumer::track`
is now pub
- add fMP4 fixture tests (`bbb.mp4`, `av1.mp4`, `vp9.mp4`)
hang (0.15.8 → 0.16.0, breaking):
- `catalog::Container::Cmaf` variant `{ timescale, track_id }` →
`{ init_data }` (base64 ftyp+moov)
- remove `Error::Duplicate`, `Audio::insert/remove`,
`Video::insert/remove`, and related `#[deprecated]` markers
Intentionally deferred to the moq-lite PR:
- `src/consumer/` module (`container.rs`, `fmp4.rs`, `frame.rs`,
`muxer.rs`, `ordered.rs`): depends on `moq_lite::TrackSubscriber`
and the new subscription/poll model
- `src/cmaf/convert.rs` and `src/hang/convert.rs`: depend on
`moq_lite::Subscription` and `consume_track` / new
`subscribe_track(_, Subscription)`
- moq-cli `subscribe` subcommand
Consumer crates updated for the `import` → `producer` rename: moq-cli,
libmoq, moq-ffi, moq-gst, moq-boy. moq-ffi's `Container` FFI type now
exposes `init_data` and consumer-side media uses
`moq_mux::hang::Media` instead of the removed
`impl Container for hang::catalog::Container`.
https://claude.ai/code/session_01L8DdkRyEijYj81eniqmgcf
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:
WalkthroughThis pull request refactors the media import/export infrastructure across the moq-mux, hang, and client crates. Key changes include: moving ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
![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: 8
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/producer/opus.rs (1)
45-50:⚠️ Potential issue | 🔴 CriticalKeep
Opus::track()compatible or update the caller.
rs/moq-boy/src/audio.rs:115-118still callsself.opus.track(), butOpusno longer exposes that method andtrackis private. That caller will fail to compile unless updated in this PR or a compatibility accessor is restored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/opus.rs` around lines 45 - 50, The Opus struct made its track field private and removed the Opus::track() accessor, breaking callers that still call self.opus.track(); restore compatibility by adding a public accessor (e.g., pub fn track(&self) -> &hang::container::OrderedProducer) on impl Opus that returns &self.track, or alternatively update all callers (like the audio code that calls self.opus.track()) to use the new API you introduced; reference the Opus struct, its private field track, and the removed Opus::track() accessor when making the change.
🧹 Nitpick comments (6)
rs/moq-mux/src/ordered/consumer.rs (1)
17-18: Publictrackfield risks breaking internal invariants.Exposing
track: moq_lite::TrackConsumeraspub(rather thanpub(crate)or a narrow accessor) lets external callers invoketrack.poll_recv_group(...)directly, which would race withConsumer's internal group buffering/ordering state inpoll_read_finishand desynchronizeself.current/self.pending. Sinceinto_inner()andclosed()already exist, consider tightening visibility topub(crate)or adding a read-only accessor for the specific field the callers need (e.g., track info).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/ordered/consumer.rs` around lines 17 - 18, The public field track on struct Consumer risks external callers calling track.poll_recv_group(...) and breaking internal ordering state (self.current / self.pending) used by poll_read_finish; change visibility of Consumer::track from pub to pub(crate) (or private) so only crate code can access it, or remove pub and add a narrow read-only accessor that returns only safe info (e.g., track metadata) used by callers, while preserving existing into_inner() and closed() methods for controlled access; ensure no external code directly calls poll_recv_group after this change.rs/moq-ffi/src/producer.rs (1)
66-66: Nit: rename stale localdecoder→framed.After the rename, the local binding is still called
decoder(also in thewrite_frame/finishmethods on lines 205, 209, 224, 225). Consider renaming toframedormediafor consistency with the new API. No functional issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-ffi/src/producer.rs` at line 66, Local variable named `decoder` should be renamed to `framed` (or `media`) to match the new API: update the binding created from moq_mux::producer::Framed::new (the let decoder = ... line) to use `framed`, and replace all subsequent uses of `decoder` inside the same module — including in the write_frame and finish methods where `decoder` is referenced — with the new name so the identifier is consistent across Framed::new, write_frame, and finish.rs/moq-mux/src/msf.rs (1)
18-24: Conversion logic is consistent with the new Container::Cmaf shape.Passing
init_datathrough verbatim for CMAF (already base64-encoded perhang::catalog::Containerdocs) and base64-encodingdescriptionfor legacy renditions preserves the MSF output contract. The duplication between video and audio blocks is minor; consider extracting a helper if this pattern expands.♻️ Optional helper extraction
+fn container_init_data(container: &hang::catalog::Container, description: Option<&bytes::Bytes>) -> Option<String> { + match container { + hang::catalog::Container::Cmaf { init_data } => Some(init_data.clone()), + _ => description.map(|d| base64::engine::general_purpose::STANDARD.encode(d.as_ref())), + } +}Also applies to: 51-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/msf.rs` around lines 18 - 24, Ensure the conversion keeps the CMAF branch verbatim and base64-encodes legacy descriptions: when matching config.container use hang::catalog::Container::Cmaf to return Some(init_data.clone()) and otherwise base64-encode config.description.as_ref(); apply the identical logic in both places (the block around the init_data binding and the similar block at lines 51-57) and optionally extract into a helper (e.g., encode_init_data(config) or init_data_for_config(config)) to remove duplication between the video and audio blocks.rs/hang/CHANGELOG.md (1)
10-10: Add the compare link for the new release heading.All nearby released versions are linked; keep
0.16.0consistent for release navigation.Proposed changelog heading
-## [0.16.0] - 2026-04-22 +## [0.16.0](https://github.com/moq-dev/moq/compare/hang-v0.15.8...hang-v0.16.0) - 2026-04-22🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/hang/CHANGELOG.md` at line 10, Update the release heading "## [0.16.0] - 2026-04-22" to include the GitHub compare link like the other entries; locate the heading string "## [0.16.0] - 2026-04-22" and replace it with the same bracketed-version-with-compare-link pattern used for other releases (i.e., make the [0.16.0] part a link to the compare URL comparing the previous tag to v0.16.0 so navigation is consistent).rs/moq-mux/src/producer/avc3.rs (1)
292-296: Consider makingfinish()a no-op when the track was never initialized.With the new optional
track, callingfinish()before the first SPS produces anErr("not initialized"), whereas teardown paths typically just want to flush-if-present. This makes graceful shutdown on an empty/failed stream noisier than necessary.♻️ Proposed refactor
/// Finish the track, flushing the current group. pub fn finish(&mut self) -> anyhow::Result<()> { - let track = self.track.as_mut().context("not initialized")?; - track.finish()?; + if let Some(track) = self.track.as_mut() { + track.finish()?; + } Ok(()) }The same consideration likely applies to the mirrored
finish()methods inavc1.rs,av01.rs, andhev1.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/avc3.rs` around lines 292 - 296, The current finish() in avc3.rs returns an error if self.track is None; change it to be a no-op when not initialized: check self.track (e.g., if let Some(track) = self.track.as_mut()) and only call track.finish()? inside that branch, otherwise return Ok(()) so teardown can silently succeed on empty/failed streams; apply the same pattern to the finish() methods in avc1.rs, av01.rs, and hev1.rs to keep behavior consistent.rs/moq-mux/src/producer/test/mod.rs (1)
11-15: Silently ignoringfmp4.decodeerrors may mask real regressions.
let _ = fmp4.decode(&mut buf)drops every error — not just the intended "incomplete trailing fragment" case. A genuine init-segment parsing regression would still proceed to the catalog assertions and surface only as a downstream mismatch (or panic on.unwrap()when accessing renditions), hiding the actual root cause.Consider draining fragments iteratively and only tolerating the final short-read, or at minimum logging the error:
♻️ Proposed refactor
- let mut buf = bytes::BytesMut::from(data); - // Ignore errors from incomplete/malformed trailing fragments in test files. - let _ = fmp4.decode(&mut buf); + let mut buf = bytes::BytesMut::from(data); + // Incomplete/malformed trailing fragments in the fixtures are expected; + // log but don't fail the test so the catalog assertions drive correctness. + if let Err(err) = fmp4.decode(&mut buf) { + eprintln!("fmp4.decode returned (expected for trailing fragments): {err:?}"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/test/mod.rs` around lines 11 - 15, The test currently silences all errors from fmp4.decode(&mut buf) which can mask real parsing regressions; change the call to handle the Result instead of throwing it away: either loop calling fmp4.decode(&mut buf) draining fragments until it returns the expected short-read/end-of-input sentinel (only ignore that specific error), or at minimum match the Err and log/assert that it is the known short-read/incomplete-fragment error before proceeding to catalog.snapshot(); reference fmp4.decode, the mutable buf variable, and the subsequent catalog.snapshot() call to locate and update the test flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/hang/src/catalog/video/mod.rs`:
- Line 68: The public method documentation for remove_track was written with a
single-line comment (//) so it doesn't appear in generated rustdoc; change the
comment before the public fn remove_track in rs/hang/src/catalog/video/mod.rs
from // to a doc comment /// (and do the same for the mirrored audio
remove_track if present) so the description is included in rustdoc, keeping the
text identical and positioned immediately above the pub fn signature.
In `@rs/moq-ffi/src/consumer.rs`:
- Around line 107-113: The current code subscribes via
self.inner.subscribe_track(...) before validating/converting the container,
which can cause side effects if try_into() later fails; reorder to first
convert/validate container into hang::catalog::Container and then try_into() to
produce moq_mux::hang::Media (mapping errors to MoqError::Codec), only after
successful conversion call self.inner.subscribe_track(...) with moq_lite::Track,
then create the consumer with moq_mux::ordered::Consumer::new(track,
media).with_latency(Duration::from_millis(max_latency_ms)).
In `@rs/moq-mux/src/hang/container.rs`:
- Around line 56-81: Clarify intent and improve error semantics: confirm (and
document in comments above TryFrom<&hang::catalog::Container> for Media and the
Media::Cmaf variant) that dropping the Ftyp atom when decoding init_data via
mp4_atom::Any::decode_maybe and returning the first Moov only is intentional for
runtime efficiency (mention that write() and poll_read() consume only Moov), and
add a new InvalidBase64 variant to crate::cmaf::Error; change the
base64::engine::general_purpose::STANDARD.decode(...) error mapping so decode
failures map to crate::cmaf::Error::InvalidBase64 instead of wrapping into
mp4_atom::Error::Io, and update any error conversions or docs/tests that rely on
the previous mapping.
In `@rs/moq-mux/src/producer/aac.rs`:
- Around line 115-134: The new AAC catalog entry must be removed if
broadcast.create_track(track) fails: after calling cat.audio.create_track("aac",
...) and before returning on error from broadcast.create_track(track) inside
Aac::new, catch the error, remove the previously inserted rendition from the
catalog (e.g., call the catalog removal method on the created track such as
cat.audio.remove_track(track.name) or the appropriate delete method on the same
catalog handle used to create it), then propagate the original error; ensure you
use the same locked catalog (catalog.lock()) and the created track identifier to
perform the rollback so no stale catalog metadata remains.
In `@rs/moq-mux/src/producer/fmp4.rs`:
- Around line 44-45: The field moof_raw (declared as moof_raw: Option<Bytes>) is
captured via self.moof_raw.replace(...) in the constructor/extract flow but
never consumed in extract() which reconstructs single_traf_moof from parsed
structures; either remove the dead field and its constructor initialization
(moof_raw: None) and the replace call, or wire moof_raw into the passthrough
path by having extract() prefer using self.moof_raw (and slice/rewrite
data_offset as needed for multi-track cases) instead of rebuilding
single_traf_moof to avoid re-encoding; update all references to
self.moof_raw.replace(...) and the constructor to reflect the chosen approach.
- Around line 578-584: The current fallback that assigns track_mdat_data =
&mdat.data[..] when track_data_start/track_data_end are out of bounds is unsafe
because data_offset for the traf was computed assuming the per-track slice;
instead, replace the fallback with an explicit error return (use anyhow::bail!
or return Err(...)) when track_data_start >= mdat.data.len() or track_data_end >
mdat.data.len() or track_data_start >= track_data_end, so callers won't continue
with mismatched data_offset, and include a clear error mentioning traf,
track_data_start, track_data_end, and mdat.data.len() to aid debugging.
- Around line 573-617: The bug is that new_moof_size is computed before we
mutate traf flags (clearing traf_mut.tfhd.base_data_offset and adding
trun_mut.data_offset), so the first encoded size is stale and all computed
data_offset values are off; to fix, stop encoding adjusted_moof to get
new_moof_size before making structural changes—first iterate adjusted_moof.traf
and for each traf_mut clear tfhd.base_data_offset and set trun_mut.data_offset
(computing cumulative_offset using sample sizes) so flags/field presence are
finalized, then encode adjusted_moof to compute new_moof_size and finally append
per_track_mdat; ensure the same approach covers cases where trun originally had
no data_offset (adding Some changes encoded size).
In `@rs/moq-mux/src/producer/opus.rs`:
- Around line 58-75: The catalog is mutated by cat.audio.create_track(...)
before broadcast.create_track(track)? so if broadcast creation fails the catalog
entry remains; change Opus::new to either defer catalog mutation until after
broadcast.create_track succeeds or, if you must create the catalog entry first,
add a rollback on error: after calling cat.audio.create_track("opus", ...) keep
the track identifier, attempt broadcast.create_track(track) and if that returns
Err re-acquire the catalog lock (or use the same lock scope) and call the
catalog removal API (e.g., cat.audio.remove_track(track.id) or similar method on
the created track name/id) before returning the error; reference the
functions/values cat.audio.create_track, broadcast.create_track, and the
Opus::new construction to locate where to implement the rollback.
---
Outside diff comments:
In `@rs/moq-mux/src/producer/opus.rs`:
- Around line 45-50: The Opus struct made its track field private and removed
the Opus::track() accessor, breaking callers that still call self.opus.track();
restore compatibility by adding a public accessor (e.g., pub fn track(&self) ->
&hang::container::OrderedProducer) on impl Opus that returns &self.track, or
alternatively update all callers (like the audio code that calls
self.opus.track()) to use the new API you introduced; reference the Opus struct,
its private field track, and the removed Opus::track() accessor when making the
change.
---
Nitpick comments:
In `@rs/hang/CHANGELOG.md`:
- Line 10: Update the release heading "## [0.16.0] - 2026-04-22" to include the
GitHub compare link like the other entries; locate the heading string "##
[0.16.0] - 2026-04-22" and replace it with the same
bracketed-version-with-compare-link pattern used for other releases (i.e., make
the [0.16.0] part a link to the compare URL comparing the previous tag to
v0.16.0 so navigation is consistent).
In `@rs/moq-ffi/src/producer.rs`:
- Line 66: Local variable named `decoder` should be renamed to `framed` (or
`media`) to match the new API: update the binding created from
moq_mux::producer::Framed::new (the let decoder = ... line) to use `framed`, and
replace all subsequent uses of `decoder` inside the same module — including in
the write_frame and finish methods where `decoder` is referenced — with the new
name so the identifier is consistent across Framed::new, write_frame, and
finish.
In `@rs/moq-mux/src/msf.rs`:
- Around line 18-24: Ensure the conversion keeps the CMAF branch verbatim and
base64-encodes legacy descriptions: when matching config.container use
hang::catalog::Container::Cmaf to return Some(init_data.clone()) and otherwise
base64-encode config.description.as_ref(); apply the identical logic in both
places (the block around the init_data binding and the similar block at lines
51-57) and optionally extract into a helper (e.g., encode_init_data(config) or
init_data_for_config(config)) to remove duplication between the video and audio
blocks.
In `@rs/moq-mux/src/ordered/consumer.rs`:
- Around line 17-18: The public field track on struct Consumer risks external
callers calling track.poll_recv_group(...) and breaking internal ordering state
(self.current / self.pending) used by poll_read_finish; change visibility of
Consumer::track from pub to pub(crate) (or private) so only crate code can
access it, or remove pub and add a narrow read-only accessor that returns only
safe info (e.g., track metadata) used by callers, while preserving existing
into_inner() and closed() methods for controlled access; ensure no external code
directly calls poll_recv_group after this change.
In `@rs/moq-mux/src/producer/avc3.rs`:
- Around line 292-296: The current finish() in avc3.rs returns an error if
self.track is None; change it to be a no-op when not initialized: check
self.track (e.g., if let Some(track) = self.track.as_mut()) and only call
track.finish()? inside that branch, otherwise return Ok(()) so teardown can
silently succeed on empty/failed streams; apply the same pattern to the finish()
methods in avc1.rs, av01.rs, and hev1.rs to keep behavior consistent.
In `@rs/moq-mux/src/producer/test/mod.rs`:
- Around line 11-15: The test currently silences all errors from
fmp4.decode(&mut buf) which can mask real parsing regressions; change the call
to handle the Result instead of throwing it away: either loop calling
fmp4.decode(&mut buf) draining fragments until it returns the expected
short-read/end-of-input sentinel (only ignore that specific error), or at
minimum match the Err and log/assert that it is the known
short-read/incomplete-fragment error before proceeding to catalog.snapshot();
reference fmp4.decode, the mutable buf variable, and the subsequent
catalog.snapshot() call to locate and update the test flow.
🪄 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: 8caf9e9a-9698-47b6-84b5-5f5aed21b65a
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockrs/moq-mux/src/producer/test/av1.mp4is excluded by!**/*.mp4rs/moq-mux/src/producer/test/bbb.mp4is excluded by!**/*.mp4rs/moq-mux/src/producer/test/vp9.mp4is excluded by!**/*.mp4
📒 Files selected for processing (43)
Cargo.tomlrs/hang/CHANGELOG.mdrs/hang/Cargo.tomlrs/hang/src/catalog/audio/mod.rsrs/hang/src/catalog/container.rsrs/hang/src/catalog/video/mod.rsrs/hang/src/error.rsrs/libmoq/src/publish.rsrs/moq-boy/src/audio.rsrs/moq-boy/src/video.rsrs/moq-cli/src/client.rsrs/moq-cli/src/main.rsrs/moq-cli/src/publish.rsrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/producer.rsrs/moq-gst/src/sink/imp.rsrs/moq-mux/CHANGELOG.mdrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog.rsrs/moq-mux/src/cmaf/container.rsrs/moq-mux/src/cmaf/error.rsrs/moq-mux/src/cmaf/mod.rsrs/moq-mux/src/error.rsrs/moq-mux/src/hang/container.rsrs/moq-mux/src/hang/mod.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/stats.rsrs/moq-mux/src/lib.rsrs/moq-mux/src/msf.rsrs/moq-mux/src/ordered/consumer.rsrs/moq-mux/src/producer/aac.rsrs/moq-mux/src/producer/annexb.rsrs/moq-mux/src/producer/av01.rsrs/moq-mux/src/producer/avc1.rsrs/moq-mux/src/producer/avc3.rsrs/moq-mux/src/producer/decoder.rsrs/moq-mux/src/producer/fmp4.rsrs/moq-mux/src/producer/hev1.rsrs/moq-mux/src/producer/hls.rsrs/moq-mux/src/producer/mod.rsrs/moq-mux/src/producer/opus.rsrs/moq-mux/src/producer/test/mod.rs
💤 Files with no reviewable changes (5)
- rs/hang/src/error.rs
- rs/moq-mux/src/error.rs
- rs/moq-mux/src/cmaf/mod.rs
- rs/moq-mux/src/import/mod.rs
- rs/moq-mux/src/import/stats.rs
Comment on lines
115
to
134
| let track = { | ||
| let mut cat = catalog.lock(); | ||
|
|
||
| let audio_config = hang::catalog::AudioConfig { | ||
| codec: hang::catalog::AAC { | ||
| profile: config.profile, | ||
| } | ||
| .into(), | ||
| sample_rate: config.sample_rate, | ||
| channel_count: config.channel_count, | ||
| bitrate: None, | ||
| description: None, | ||
| container: hang::catalog::Container::Legacy, | ||
| jitter: None, | ||
| }; | ||
| let track = cat.audio.create_track("aac", audio_config.clone()); | ||
| tracing::debug!(name = ?track.name, config = ?audio_config, "starting track"); | ||
|
|
||
| broadcast.create_track(track)? | ||
| }; |
Contributor
There was a problem hiding this comment.
Rollback the catalog entry if broadcast track creation fails.
cat.audio.create_track(...) has already inserted the AAC rendition. If broadcast.create_track(track)? fails, Aac::new exits before Drop, leaving stale catalog metadata.
Proposed rollback
- let track = cat.audio.create_track("aac", audio_config.clone());
- tracing::debug!(name = ?track.name, config = ?audio_config, "starting track");
-
- broadcast.create_track(track)?
+ let track_info = cat.audio.create_track("aac", audio_config.clone());
+ tracing::debug!(name = ?track_info.name, config = ?audio_config, "starting track");
+
+ match broadcast.create_track(track_info.clone()) {
+ Ok(track) => track,
+ Err(err) => {
+ cat.audio.remove_track(&track_info);
+ return Err(err.into());
+ }
+ }📝 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.
Suggested change
| let track = { | |
| let mut cat = catalog.lock(); | |
| let audio_config = hang::catalog::AudioConfig { | |
| codec: hang::catalog::AAC { | |
| profile: config.profile, | |
| } | |
| .into(), | |
| sample_rate: config.sample_rate, | |
| channel_count: config.channel_count, | |
| bitrate: None, | |
| description: None, | |
| container: hang::catalog::Container::Legacy, | |
| jitter: None, | |
| }; | |
| let track = cat.audio.create_track("aac", audio_config.clone()); | |
| tracing::debug!(name = ?track.name, config = ?audio_config, "starting track"); | |
| broadcast.create_track(track)? | |
| }; | |
| let track = { | |
| let mut cat = catalog.lock(); | |
| let audio_config = hang::catalog::AudioConfig { | |
| codec: hang::catalog::AAC { | |
| profile: config.profile, | |
| } | |
| .into(), | |
| sample_rate: config.sample_rate, | |
| channel_count: config.channel_count, | |
| bitrate: None, | |
| description: None, | |
| container: hang::catalog::Container::Legacy, | |
| jitter: None, | |
| }; | |
| let track_info = cat.audio.create_track("aac", audio_config.clone()); | |
| tracing::debug!(name = ?track_info.name, config = ?audio_config, "starting track"); | |
| match broadcast.create_track(track_info.clone()) { | |
| Ok(track) => track, | |
| Err(err) => { | |
| cat.audio.remove_track(&track_info); | |
| return Err(err.into()); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/producer/aac.rs` around lines 115 - 134, The new AAC catalog
entry must be removed if broadcast.create_track(track) fails: after calling
cat.audio.create_track("aac", ...) and before returning on error from
broadcast.create_track(track) inside Aac::new, catch the error, remove the
previously inserted rendition from the catalog (e.g., call the catalog removal
method on the created track such as cat.audio.remove_track(track.name) or the
appropriate delete method on the same catalog handle used to create it), then
propagate the original error; ensure you use the same locked catalog
(catalog.lock()) and the created track identifier to perform the rollback so no
stale catalog metadata remains.
Comment on lines
58
to
75
| let track = { | ||
| let mut cat = catalog.lock(); | ||
|
|
||
| let audio_config = hang::catalog::AudioConfig { | ||
| codec: hang::catalog::AudioCodec::Opus, | ||
| sample_rate: config.sample_rate, | ||
| channel_count: config.channel_count, | ||
| bitrate: None, | ||
| description: None, | ||
| container: hang::catalog::Container::Legacy, | ||
| jitter: None, | ||
| }; | ||
|
|
||
| let track = cat.audio.create_track("opus", audio_config.clone()); | ||
| tracing::debug!(name = ?track.name, config = ?audio_config, "starting track"); | ||
|
|
||
| broadcast.create_track(track)? | ||
| }; |
Contributor
There was a problem hiding this comment.
Rollback the catalog entry if broadcast track creation fails.
cat.audio.create_track(...) mutates the catalog before broadcast.create_track(track)?. On error, Opus::new returns without Drop, leaving a catalog entry for a track that was never created.
Proposed rollback
- let track = cat.audio.create_track("opus", audio_config.clone());
- tracing::debug!(name = ?track.name, config = ?audio_config, "starting track");
-
- broadcast.create_track(track)?
+ let track_info = cat.audio.create_track("opus", audio_config.clone());
+ tracing::debug!(name = ?track_info.name, config = ?audio_config, "starting track");
+
+ match broadcast.create_track(track_info.clone()) {
+ Ok(track) => track,
+ Err(err) => {
+ cat.audio.remove_track(&track_info);
+ return Err(err.into());
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/producer/opus.rs` around lines 58 - 75, The catalog is mutated
by cat.audio.create_track(...) before broadcast.create_track(track)? so if
broadcast creation fails the catalog entry remains; change Opus::new to either
defer catalog mutation until after broadcast.create_track succeeds or, if you
must create the catalog entry first, add a rollback on error: after calling
cat.audio.create_track("opus", ...) keep the track identifier, attempt
broadcast.create_track(track) and if that returns Err re-acquire the catalog
lock (or use the same lock scope) and call the catalog removal API (e.g.,
cat.audio.remove_track(track.id) or similar method on the created track name/id)
before returning the error; reference the functions/values
cat.audio.create_track, broadcast.create_track, and the Opus::new construction
to locate where to implement the rollback.
…ux-main-9euJs # Conflicts: # rs/moq-mux/src/producer/aac.rs # rs/moq-mux/src/producer/av01.rs # rs/moq-mux/src/producer/avc1.rs # rs/moq-mux/src/producer/avc3.rs # rs/moq-mux/src/producer/fmp4.rs # rs/moq-mux/src/producer/hev1.rs # rs/moq-mux/src/producer/hls.rs # rs/moq-mux/src/producer/opus.rs
After merging main, the producer files referenced TrackProducer's now-private
`info` field. Use the existing Deref chain (OrderedProducer → TrackProducer →
Track) to access fields directly, and rely on deref coercion for &Track args.
The PR's lazy track init in Avc3 broke moq-boy, which needs an eager
TrackProducer to monitor used()/unused() before any frames arrive. Restore the
prior pattern: create the track eagerly via unique_track(".avc3"), and only
update the catalog rendition lazily once the SPS provides the codec config.
Add track() accessors on Opus and Avc3 (used by moq-boy), update the
BroadcastProducer::new() test calls to the new arg-taking signature, and
restore /// doc comments on the catalog remove_track methods.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
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 (2)
rs/moq-mux/src/producer/fmp4.rs (1)
446-510:⚠️ Potential issue | 🟠 Major
track_data_startonly honors the firsttrun's explicitdata_offset.
track_data_startis set on the first iteration wheretrack_data_start.is_none()(lines 506–509), but if a latertrunin the same traf carries its owndata_offsetthat jumps to a non-contiguous region of the mdat, that gap won't be reflected intrack_mdat_data(which is&mdat.data[track_data_start..track_data_end]). Any non-contiguous traf will silently produce a wrong slice. Either assert contiguity (bail!if a later trun's resolved offset != currentoffset) or build the per-track mdat by concatenating each trun's range explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/fmp4.rs` around lines 446 - 510, In extract(), track_data_start is only set once but later trun entries may contain their own data_offset that jumps to a non-contiguous mdat region; update extract() (in function extract) to handle this by either (A) asserting contiguity: after resolving each trun's data_offset and computing offset, if track_data_start.is_some() then ensure resolved offset == current offset and bail (anyhow::bail!) on mismatch, or (B) build the per-track mdat by accumulating each trun's slice ranges into a Vec<u8>/Vec<&[u8]> (use resolved offset and sample sizes per trun to compute start/end for each trun and append) and replace uses of track_data_start/track_data_end/track_mdat_data with the concatenated result; refer to symbols traf, trun, tfhd, offset, track_data_start, moof_size and header_size to locate where to add the check or the concatenation logic.rs/moq-mux/src/producer/avc3.rs (1)
87-101:⚠️ Potential issue | 🟠 MajorTrack naming inconsistency:
avc3uses broadcast-generated names instead of catalog-style naming.
avc3inserts renditions underself.track.namefrombroadcast.unique_track(".avc3")(generates "0.avc3", "1.avc3", ...), whereas all other producers (avc1,av01,hev1,aac,opus,fmp4) callcatalog.video.create_track()which produces "video0.{codec}", "video1.{codec}", etc. This divergence means therenditionsmap uses inconsistent key naming across producers. If downstream consumers expect the catalog-style naming convention, lookups foravc3tracks will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/avc3.rs` around lines 87 - 101, The code inserts the avc3 rendition under broadcast-generated self.track.name (e.g. "0.avc3") causing inconsistent keys; change the logic in avc3.rs so you generate/obtain the catalog-style track name (the same way other producers do) and use that as the renditions map key instead of self.track.name — i.e., call or reuse catalog.video.create_track() (or the same naming helper used by avc1/av01/hev1/etc.) to produce "videoN.avc3", insert into catalog.video.renditions with that key, and update self.track.name/self.config accordingly so stored name matches catalog naming.
♻️ Duplicate comments (1)
rs/moq-mux/src/producer/fmp4.rs (1)
569-617:⚠️ Potential issue | 🔴 CriticalCritical:
new_moof_sizeis still measured before mutatingtfhd.base_data_offset/trun.data_offset.This is the same defect previously flagged and not addressed. The flow (lines 573–612) is:
adjusted_moof.encode(&mut moof_buf)?(line 575) encodes the clonedtrafwhosetfhdmay still carrybase_data_offset = Some(_).new_moof_size = moof_buf.len()is captured.- Lines 590–608 then clear
traf_mut.tfhd.base_data_offset(drops 8 bytes when the0x000001flag was set) and assignSome(_)to everytrun.data_offset(which can also flip thetrun0x000001flag bit and change encoded size).- The
data_offsetvalues written at line 595 are computed from the stalenew_moof_size, so after the re-encode at line 612 the moof is smaller, and everydata_offsetnow points 8 bytes (or more) past the correct sample boundary in the newmdat.For any input where
tfhdcarries an explicitbase_data_offset(very common: muxers writing absolute file offsets), the resulting passthrough fragment will mis-address sample data and decoders will pull garbage. Apply the structural mutations before measuringnew_moof_sizeand re-encoding once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/fmp4.rs` around lines 569 - 617, The current code measures new_moof_size by encoding adjusted_moof before clearing traf.tfhd.base_data_offset and setting each trun.data_offset, causing stale size offsets; fix by performing the structural mutations on adjusted_moof first: iterate adjusted_moof.traf and for each traf_mut clear traf_mut.tfhd.base_data_offset and compute/set each trun_mut.data_offset (using mdat_header_size_new and cumulative_offset) before encoding into moof_buf and capturing new_moof_size; then re-encode only if needed and append per_track_mdat — ensure new_moof_size is computed after these mutations so data_offset values reference the final encoded moof size.
🧹 Nitpick comments (7)
rs/moq-mux/src/producer/av01.rs (1)
94-107: Same&self.track.take()style inconsistency asAvc1.Lines 94 and 227 use
if let Some(track) = &self.track.take()whileDropat line 419 usesif let Some(track) = self.track.take(). Prefer the moving form for readability and consistency.♻️ Suggested cleanup
- if let Some(track) = &self.track.take() { - tracing::debug!(name = ?track.name, "reinitializing track"); - self.catalog.lock().video.remove_track(track); - } + if let Some(track) = self.track.take() { + tracing::debug!(name = ?track.name, "reinitializing track"); + self.catalog.lock().video.remove_track(&track); + } @@ - if let Some(track) = &self.track.take() { - self.catalog.lock().video.remove_track(track); - } + if let Some(track) = self.track.take() { + self.catalog.lock().video.remove_track(&track); + }Also applies to: 227-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/av01.rs` around lines 94 - 107, The code inconsistently borrows self.track when calling take() — change occurrences using if let Some(track) = &self.track.take() to the moving form if let Some(track) = self.track.take() so the Option is consumed consistently (as already done in Drop); update the occurrences in the Av01 implementation (the track cleanup/start sequence where self.track.take() is used and the block around the broadcast.create_track call) to move the value instead of borrowing it, ensuring subsequent uses (self.track = Some(...)) remain correct.rs/moq-mux/src/producer/avc1.rs (1)
93-103: Inconsistent&self.track.take()vsself.track.take().Line 95 uses
if let Some(track) = &self.track.take()(bindstrack: &OrderedProduceragainst a reference to a temporary), whileDropat line 198 uses the more idiomaticif let Some(track) = self.track.take(). The&-on-temp form is correct but reads strangely. Also note the catalog is locked across the implicit drop of the oldOrderedProducer(the temporary is dropped at end of theif letblock); fine today since no inner type tries to re-lock the catalog, but worth keeping in mind.♻️ Suggested cleanup
- if let Some(track) = &self.track.take() { - tracing::debug!(name = ?track.name, "reinitializing avc1 track"); - catalog.video.remove_track(track); - } + if let Some(track) = self.track.take() { + tracing::debug!(name = ?track.name, "reinitializing avc1 track"); + catalog.video.remove_track(&track); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/avc1.rs` around lines 93 - 103, Replace the odd reference-to-a-temporary pattern in the avc1 reinit block by taking ownership like the Drop path: use if let Some(track) = self.track.take() instead of if let Some(track) = &self.track.take(), then call tracing::debug! and catalog.video.remove_track(track) using the owned track (clone or borrow fields like track.name as needed) so the code is consistent with the Drop implementation (functions/vars: self.track.take(), catalog.lock(), catalog.video.remove_track, tracing::debug!).rs/moq-mux/src/ordered/consumer.rs (1)
17-34: Unnecessary publictrackfield exposes internal implementation details.Making
track: moq_lite::TrackConsumerpublic is not required—no external code directly accesses this field. The public visibility creates unnecessary coupling tomoq_litetypes and locks in an internal implementation detail. SinceConsumeralready provides accessor methods (closed(), etc.), keepingtrackprivate protects against future accidental misuse.♻️ Suggested change
- pub track: moq_lite::TrackConsumer, + track: moq_lite::TrackConsumer,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/ordered/consumer.rs` around lines 17 - 34, The Consumer struct exposes its internal moq_lite::TrackConsumer via the public field `track`; change `pub track: moq_lite::TrackConsumer` to a private field `track: moq_lite::TrackConsumer` in the `Consumer<F: Container>` definition and update any internal uses to access it directly (or via existing accessor methods like `closed()`), ensuring no external API relied on the field; this removes the unnecessary public coupling to moq_lite while keeping `Consumer`'s behavior the same.rs/moq-mux/src/producer/avc3.rs (1)
36-50: Consider propagatingunique_trackerrors rather than panicking in the constructor.
broadcast.unique_track(".avc3").expect(...)will panic if the broadcast has been aborted (Error::Dropped), which is a valid state error that should be handled, not panic. While Avc1 and Av01 use lazy track creation during initialization (returningResult), Avc3's eager creation is intentional to monitor track usage early. If eager creation is necessary, either ensure the broadcast is guaranteed valid at this point, or changeAvc3::newto returnResult<Self>to propagate errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/avc3.rs` around lines 36 - 50, Avc3::new currently panics on broadcast.unique_track(".avc3") via expect which leaks a valid Drop/Dropped error; change Avc3::new to return Result<Self, E> (propagating the unique_track error) and replace the expect call with ? (or map_err) so the constructor fails gracefully when the broadcast is aborted; adjust the return type and propagate the error from broadcast.unique_track(".avc3") while leaving the rest of the initialization (catalog, track: track.into(), config, current, zero, cached_sps, cached_pps) unchanged so callers can handle the error.rs/moq-gst/src/sink/imp.rs (1)
466-466: Redundantmap_err(|e| anyhow::anyhow!(e)).
Framed::decode_framealready returnsanyhow::Result<()>, so the closure is an identity mapping that drops any chained context. Just propagate with?(optionally adding.context(...)for diagnostics).Suggested simplification
- pad.decoder.decode_frame(&mut data, ts).map_err(|e| anyhow::anyhow!(e)) + pad.decoder.decode_frame(&mut data, ts)As per coding guidelines: "Use
anyhow::Context(.context("msg")) instead of.map_err(|_| anyhow::anyhow!("msg"))for error conversion".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-gst/src/sink/imp.rs` at line 466, The call pad.decoder.decode_frame(&mut data, ts).map_err(|e| anyhow::anyhow!(e)) is a redundant identity map — Framed::decode_frame already returns anyhow::Result, so remove the .map_err(...) and propagate the error with ?; if you want diagnostic context, use .context("decoding frame in pad.decoder.decode_frame") from anyhow::Context on the Framed::decode_frame call instead of mapping the error into a new anyhow.rs/moq-ffi/src/producer.rs (1)
62-63: Preserve the original format-parse error.
FramedFormat::from_stralready returns a typed error (Error::UnknownFormat), butmap_err(|_| ...)discards it and re-emits a string built solely from the input. Forwarding the source error gives clearer FFI diagnostics.Suggested change
- let format = moq_mux::producer::FramedFormat::from_str(&format) - .map_err(|_| MoqError::Codec(format!("unknown format: {format}")))?; + let format = moq_mux::producer::FramedFormat::from_str(&format) + .map_err(|e| MoqError::Codec(format!("unknown format '{format}': {e}")))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-ffi/src/producer.rs` around lines 62 - 63, The current call to moq_mux::producer::FramedFormat::from_str(&format) uses map_err(|_| MoqError::Codec(...)) which discards the original parse error; update the map_err to forward or include the original error (e.g., map_err(|e| MoqError::Codec(format!("unknown format: {format}: {e}"))) or convert the source error into MoqError via From) so the original Error::UnknownFormat is preserved in diagnostics; target the FramedFormat::from_str invocation and the MoqError::Codec construction in producer.rs.rs/moq-mux/src/producer/hev1.rs (1)
86-89: Nit:&self.track.take()is awkward.The pattern takes ownership of the
Option, then immediately re-borrows it just to log and remove. A directif let Some(track) = self.track.take()reads more cleanly and matches theDropimpl style.Suggested simplification
- if let Some(track) = &self.track.take() { + if let Some(track) = self.track.take() { tracing::debug!(name = ?track.name, "reinitializing track"); - catalog.video.remove_track(track); + catalog.video.remove_track(&track); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/hev1.rs` around lines 86 - 89, The current code awkwardly takes a reference to the result of self.track.take(); change the pattern to take ownership directly by using if let Some(track) = self.track.take() so you don't re-borrow the Option result — adjust the tracing::debug!(name = ?track.name, ...) and catalog.video.remove_track(track) calls to work with the owned track value; this matches the Drop impl style and avoids the unnecessary & on self.track.take().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-mux/src/producer/hev1.rs`:
- Around line 84-99: The catalog and broadcast can get out of sync if
self.broadcast.create_track(track)? fails after catalog.video.create_track has
already inserted a new track; either create the broadcast track before mutating
the catalog or roll back the catalog mutation on error. Concretely: obtain the
new catalog track descriptor via catalog.video.create_track("hev1",
config.clone()) but do not remove the old track / replace self.track until after
successfully calling self.broadcast.create_track(track); alternatively, if you
must call catalog.video.create_track first, catch errors from
self.broadcast.create_track and on error call catalog.video.remove_track(...) to
restore the previous state and avoid changing self.config or self.track,
ensuring is_initialized() stays consistent. Make the changes around the
create/remove calls and the assignments to self.config and self.track in
hev1.rs.
---
Outside diff comments:
In `@rs/moq-mux/src/producer/avc3.rs`:
- Around line 87-101: The code inserts the avc3 rendition under
broadcast-generated self.track.name (e.g. "0.avc3") causing inconsistent keys;
change the logic in avc3.rs so you generate/obtain the catalog-style track name
(the same way other producers do) and use that as the renditions map key instead
of self.track.name — i.e., call or reuse catalog.video.create_track() (or the
same naming helper used by avc1/av01/hev1/etc.) to produce "videoN.avc3", insert
into catalog.video.renditions with that key, and update
self.track.name/self.config accordingly so stored name matches catalog naming.
In `@rs/moq-mux/src/producer/fmp4.rs`:
- Around line 446-510: In extract(), track_data_start is only set once but later
trun entries may contain their own data_offset that jumps to a non-contiguous
mdat region; update extract() (in function extract) to handle this by either (A)
asserting contiguity: after resolving each trun's data_offset and computing
offset, if track_data_start.is_some() then ensure resolved offset == current
offset and bail (anyhow::bail!) on mismatch, or (B) build the per-track mdat by
accumulating each trun's slice ranges into a Vec<u8>/Vec<&[u8]> (use resolved
offset and sample sizes per trun to compute start/end for each trun and append)
and replace uses of track_data_start/track_data_end/track_mdat_data with the
concatenated result; refer to symbols traf, trun, tfhd, offset,
track_data_start, moof_size and header_size to locate where to add the check or
the concatenation logic.
---
Duplicate comments:
In `@rs/moq-mux/src/producer/fmp4.rs`:
- Around line 569-617: The current code measures new_moof_size by encoding
adjusted_moof before clearing traf.tfhd.base_data_offset and setting each
trun.data_offset, causing stale size offsets; fix by performing the structural
mutations on adjusted_moof first: iterate adjusted_moof.traf and for each
traf_mut clear traf_mut.tfhd.base_data_offset and compute/set each
trun_mut.data_offset (using mdat_header_size_new and cumulative_offset) before
encoding into moof_buf and capturing new_moof_size; then re-encode only if
needed and append per_track_mdat — ensure new_moof_size is computed after these
mutations so data_offset values reference the final encoded moof size.
---
Nitpick comments:
In `@rs/moq-ffi/src/producer.rs`:
- Around line 62-63: The current call to
moq_mux::producer::FramedFormat::from_str(&format) uses map_err(|_|
MoqError::Codec(...)) which discards the original parse error; update the
map_err to forward or include the original error (e.g., map_err(|e|
MoqError::Codec(format!("unknown format: {format}: {e}"))) or convert the source
error into MoqError via From) so the original Error::UnknownFormat is preserved
in diagnostics; target the FramedFormat::from_str invocation and the
MoqError::Codec construction in producer.rs.
In `@rs/moq-gst/src/sink/imp.rs`:
- Line 466: The call pad.decoder.decode_frame(&mut data, ts).map_err(|e|
anyhow::anyhow!(e)) is a redundant identity map — Framed::decode_frame already
returns anyhow::Result, so remove the .map_err(...) and propagate the error with
?; if you want diagnostic context, use .context("decoding frame in
pad.decoder.decode_frame") from anyhow::Context on the Framed::decode_frame call
instead of mapping the error into a new anyhow.
In `@rs/moq-mux/src/ordered/consumer.rs`:
- Around line 17-34: The Consumer struct exposes its internal
moq_lite::TrackConsumer via the public field `track`; change `pub track:
moq_lite::TrackConsumer` to a private field `track: moq_lite::TrackConsumer` in
the `Consumer<F: Container>` definition and update any internal uses to access
it directly (or via existing accessor methods like `closed()`), ensuring no
external API relied on the field; this removes the unnecessary public coupling
to moq_lite while keeping `Consumer`'s behavior the same.
In `@rs/moq-mux/src/producer/av01.rs`:
- Around line 94-107: The code inconsistently borrows self.track when calling
take() — change occurrences using if let Some(track) = &self.track.take() to the
moving form if let Some(track) = self.track.take() so the Option is consumed
consistently (as already done in Drop); update the occurrences in the Av01
implementation (the track cleanup/start sequence where self.track.take() is used
and the block around the broadcast.create_track call) to move the value instead
of borrowing it, ensuring subsequent uses (self.track = Some(...)) remain
correct.
In `@rs/moq-mux/src/producer/avc1.rs`:
- Around line 93-103: Replace the odd reference-to-a-temporary pattern in the
avc1 reinit block by taking ownership like the Drop path: use if let Some(track)
= self.track.take() instead of if let Some(track) = &self.track.take(), then
call tracing::debug! and catalog.video.remove_track(track) using the owned track
(clone or borrow fields like track.name as needed) so the code is consistent
with the Drop implementation (functions/vars: self.track.take(), catalog.lock(),
catalog.video.remove_track, tracing::debug!).
In `@rs/moq-mux/src/producer/avc3.rs`:
- Around line 36-50: Avc3::new currently panics on
broadcast.unique_track(".avc3") via expect which leaks a valid Drop/Dropped
error; change Avc3::new to return Result<Self, E> (propagating the unique_track
error) and replace the expect call with ? (or map_err) so the constructor fails
gracefully when the broadcast is aborted; adjust the return type and propagate
the error from broadcast.unique_track(".avc3") while leaving the rest of the
initialization (catalog, track: track.into(), config, current, zero, cached_sps,
cached_pps) unchanged so callers can handle the error.
In `@rs/moq-mux/src/producer/hev1.rs`:
- Around line 86-89: The current code awkwardly takes a reference to the result
of self.track.take(); change the pattern to take ownership directly by using if
let Some(track) = self.track.take() so you don't re-borrow the Option result —
adjust the tracing::debug!(name = ?track.name, ...) and
catalog.video.remove_track(track) calls to work with the owned track value; this
matches the Drop impl style and avoids the unnecessary & on self.track.take().
🪄 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: 61d5510f-415a-4ec7-8d92-670aa0f5f1e5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
rs/hang/src/catalog/audio/mod.rsrs/hang/src/catalog/video/mod.rsrs/libmoq/src/publish.rsrs/moq-cli/src/client.rsrs/moq-cli/src/publish.rsrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/producer.rsrs/moq-gst/src/sink/imp.rsrs/moq-mux/src/ordered/consumer.rsrs/moq-mux/src/producer/aac.rsrs/moq-mux/src/producer/av01.rsrs/moq-mux/src/producer/avc1.rsrs/moq-mux/src/producer/avc3.rsrs/moq-mux/src/producer/fmp4.rsrs/moq-mux/src/producer/hev1.rsrs/moq-mux/src/producer/hls.rsrs/moq-mux/src/producer/opus.rsrs/moq-mux/src/producer/test/mod.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rs/moq-cli/src/client.rs
- rs/libmoq/src/publish.rs
- rs/moq-mux/src/producer/opus.rs
- rs/hang/src/catalog/audio/mod.rs
- rs/moq-mux/src/producer/test/mod.rs
kixelated
commented
Apr 26, 2026
| } | ||
|
|
||
| /// Create a new audio track with the given extension and configuration. | ||
| #[deprecated( |
Collaborator
Author
There was a problem hiding this comment.
Revert this? Why are we undeprecating? Use unique_track instead?
|
|
||
| /// Create a new video track with the given extension and configuration. | ||
| #[deprecated( | ||
| note = "use BroadcastProducer::unique_track to create the track, then insert into the catalog when initialized" |
Collaborator
Author
There was a problem hiding this comment.
Same, this should be removed.
In fact we can remove this deprecated functions since htis is a semver change.
| track_id: u32, | ||
| /// Base64-encoded init segment (ftyp+moov) | ||
| #[serde(rename = "initData")] | ||
| init_data: String, |
Collaborator
Author
There was a problem hiding this comment.
just call it init honestly. Also is there some serde_as thing we could use instead of String?
| license = "MIT OR Apache-2.0" | ||
|
|
||
| version = "0.15.8" | ||
| version = "0.16.0" |
Collaborator
Author
There was a problem hiding this comment.
release-plz will bump this; don't manually do it.
|
|
||
| ### Breaking | ||
|
|
||
| - `catalog::Container::Cmaf` variant changed from `{ timescale, track_id }` to `{ init_data }` (base64-encoded ftyp+moov init segment). |
Collaborator
Author
There was a problem hiding this comment.
Same release-plz will do this changelog.
| /// Configure the maximum acceptable delay through the consumer's latency settings. | ||
| pub struct Consumer<F: Container> { | ||
| track: moq_lite::TrackConsumer, | ||
| pub track: moq_lite::TrackConsumer, |
Collaborator
Author
There was a problem hiding this comment.
Could we avoid making this pub? I don't want users to accidentally call the wrong methods?
| @@ -85,16 +66,12 @@ impl fmt::Display for DecoderFormat { | |||
| #[non_exhaustive] | |||
| pub enum StreamFormat { | |||
Collaborator
Author
There was a problem hiding this comment.
Maybe we split this into stream.rs
| _ => Err(Error::UnknownFormat(s.to_string())), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for DecoderFormat { | ||
| impl fmt::Display for FramedFormat { |
Collaborator
Author
There was a problem hiding this comment.
Maybe we split this into framed.rs. decoder.rs doesn't make sense any longe.r
| // Jitter tracking: minimum duration between consecutive frames. | ||
| last_timestamp: Option<hang::container::Timestamp>, | ||
| min_duration: Option<hang::container::Timestamp>, | ||
| jitter: Option<hang::container::Timestamp>, |
Collaborator
Author
There was a problem hiding this comment.
I don't think we should remove these jitter calculations. Maybe make a jitter.rs helper instead of copy-pasting?
jitter isn't a great name, really it should be min_frame_duration or something.
…ux-main-9euJs # Conflicts: # rs/hang/src/catalog/audio/mod.rs # rs/hang/src/catalog/video/mod.rs # rs/moq-mux/src/ordered/consumer.rs
- Drop manual Cargo.toml version bumps and CHANGELOG entries (release-plz handles them) - Rename catalog Container::Cmaf field init_data → init and store as Bytes via serde_with::Base64 - Remove deprecated Audio/Video::create_track and remove_track helpers; switch producers to BroadcastProducer::unique_track + direct renditions map access (semver change) - Make ordered::Consumer::track private (already non-pub via merge with main) - Split producer/decoder.rs into producer/stream.rs and producer/framed.rs - Restore jitter (min frame duration) tracking via shared producer/jitter.rs helper - fmp4: fix critical data_offset miscalculation by applying tfhd/trun flag changes before measuring the moof size; bail when the per-track sample range overflows mdat instead of silently falling back; drop unused moof_raw field - moq-ffi: parse the catalog Container before subscribing so we don't leave a dangling subscription on init failure
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/moq-mux/src/producer/avc3.rs (1)
268-293:⚠️ Potential issue | 🟠 MajorDon’t emit frames before SPS/PPS has initialized the rendition.
new()now creates the track eagerly, butinit()at Lines 98-103 is the only place that inserts the catalog entry.maybe_start_frame()can still write slices beforeself.configis set, so a stream that starts mid-GOP can publish undecodable media on a track that is still absent from the catalog.[suggested fix: drop or buffer pre-init slices until
self.config.is_some()]💡 One straightforward guard
fn maybe_start_frame(&mut self, pts: Option<hang::container::Timestamp>) -> anyhow::Result<()> { // If we haven't seen any slices, we shouldn't flush yet. if !self.current.contains_slice { return Ok(()); } + + if self.config.is_none() { + tracing::debug!(name = ?self.track.name, "dropping frame before SPS/PPS init"); + self.current = Default::default(); + return Ok(()); + } let pts = pts.context("missing timestamp")?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/avc3.rs` around lines 268 - 293, maybe_start_frame is writing frames before the rendition/catalog entry is initialized (new() creates the track eagerly while init() inserts catalog entry), so guard against emitting undecodable pre-init slices by checking self.config.is_some() before proceeding; update maybe_start_frame to return early (or buffer/drop) when self.config.is_none(), ensuring you do not call self.track.write(...) or self.track.keyframe() until init() has set the catalog entry (also keep the existing contains_slice/contains_idr logic intact so only fully-initialized streams are emitted).rs/moq-mux/src/producer/aac.rs (1)
117-127:⚠️ Potential issue | 🟡 MinorKeep publishing AAC jitter metadata.
AAC frame cadence is deterministic from 1024 samples and
sample_rate, so switching this tojitter: Nonedrops a timing hint the catalog can still provide. Downstream readers now have to assume immediate flush, which can make audio buffering too aggressive compared with the real frame cadence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/aac.rs` around lines 117 - 127, The AudioConfig being built for AAC sets jitter: None which drops useful frame timing; update the audio_config construction (hang::catalog::AudioConfig in aac.rs) to populate jitter instead of None — either use the jitter value from the source config (config.jitter) if present or compute the AAC frame cadence as 1024 samples (i.e. frame_duration = 1024 / config.sample_rate) and set jitter to Some(...) using the catalog's Jitter variant so the catalog retains the AAC frame timing hint.
♻️ Duplicate comments (1)
rs/moq-mux/src/producer/hev1.rs (1)
91-101:⚠️ Potential issue | 🟠 MajorCreate the replacement track before dropping the current rendition.
If Line 96 fails, the old rendition is already removed and
self.trackhas been taken, so reinitialization leaves the producer unpublished until another SPS arrives.🔁 Suggested fix
- if let Some(track) = &self.track.take() { - tracing::debug!(name = ?track.name, "reinitializing track"); - catalog.video.renditions.remove(&track.name); - } - - let track = self.broadcast.unique_track(".hev1")?; - tracing::debug!(name = ?track.name, ?config, "starting track"); - catalog.video.renditions.insert(track.name.clone(), config.clone()); + let track = self.broadcast.unique_track(".hev1")?; + if let Some(old_track) = self.track.take() { + tracing::debug!(name = ?old_track.name, "reinitializing track"); + catalog.video.renditions.remove(&old_track.name); + } + tracing::debug!(name = ?track.name, ?config, "starting track"); + catalog.video.renditions.insert(track.name.clone(), config.clone());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/producer/hev1.rs` around lines 91 - 101, Call self.broadcast.unique_track(".hev1") and insert the new rendition into catalog.video.renditions before mutating or taking self.track so failures don't leave the producer unpublished; i.e., obtain let new_track = self.broadcast.unique_track(".hev1")? first, tracing::debug on new_track and insert new_track.name.clone() with config.clone() into catalog.video.renditions, then if let Some(old) = self.track.take() { tracing::debug!(name = ?old.name, "reinitializing track"); catalog.video.renditions.remove(&old.name); } and finally set self.config = Some(config) and self.track = Some(new_track.into()).
🧹 Nitpick comments (1)
rs/moq-mux/src/msf.rs (1)
18-24: Factor theinit_datamapping into one helper.The video and audio paths now duplicate the same
Cmaf { init }vsdescriptionselection. A tiny helper would keep future packaging changes from drifting between the two branches.Also applies to: 51-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/msf.rs` around lines 18 - 24, Extract the duplicated logic that picks Cmaf { init } vs description and base64-encodes it into a small helper (e.g., fn get_init_data(config: &Config) -> Option<String> or encode_init_data(container: &hang::catalog::Container, description: Option<&String>) -> Option<String>), replace the inline match that builds init_data with calls to that helper wherever used (the current occurrence using config.container and the similar logic around lines 51-57), and ensure the helper returns the same Option<String> semantics using base64::engine::general_purpose::STANDARD.encode on the chosen byte slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-mux/Cargo.toml`:
- Around line 25-34: You removed previously optional dependencies/feature flags
in moq-mux's Cargo.toml (h264-parser, mp4-atom, m3u8-rs, scuffle-av1,
scuffle-h265, etc.) while leaving the crate version = "0.3.9", which is a public
Cargo feature-API break; either (A) restore backward-compatible feature flags in
Cargo.toml for moq-mux (redeclare those deps as optional and re-add a features
table mapping feature names to the optional deps, preserving past defaults) so
external consumers using moq-mux = { features = [...] } continue to work, or (B)
if you intend to remove those features permanently, increment the crate's
semantic version to a new major (e.g., 1.0.0) in Cargo.toml to signal the
breaking change; pick one and apply consistently.
In `@rs/moq-mux/src/producer/av01.rs`:
- Around line 100-115: The current code calls self.track.take() (dropping the
live track) before calling self.broadcast.unique_track(".av01") so if
unique_track() fails we lose the published track; change the order in init() and
init_from_av1c(): call unique_track() first and only after it succeeds insert
the new rendition into self.catalog.lock().video.renditions and then replace
self.track (and remove the old rendition if needed). In short, avoid taking
self.track before unique_track() succeeds—create new_track =
self.broadcast.unique_track(".av01")?, update catalog.insert(track.name.clone(),
config.clone()), set self.config and self.track to the new values, and then
remove the old rendition/cleanup using the old track name if present.
In `@rs/moq-mux/src/producer/avc1.rs`:
- Around line 101-111: The code currently calls self.track.take() and removes
the old rendition before allocating a new track, so if
self.broadcast.unique_track(".avc1") fails the stream loses its published track;
to fix, allocate the new track first (call self.broadcast.unique_track(".avc1")
before mutating self.track or catalog.video.renditions), then insert the new
rendition into catalog.video.renditions, set self.config = Some(config) and
self.track = Some(track.into()), and only after successful allocation remove the
old rendition and drop the old track (or avoid taking self.track until
allocation succeeds by using as_ref()/clone of the old name or storing the old
Option in a temp and only consuming it on success). Ensure you update the code
paths referencing self.track, catalog.video.renditions.remove/insert, and
self.broadcast.unique_track(".avc1") accordingly.
In `@rs/moq-mux/src/producer/fmp4.rs`:
- Around line 561-624: The current code builds track_mdat_data as one contiguous
slice (track_data_start..track_data_end) which breaks when a traf contains
multiple truns with gaps or independent offsets; instead, construct per-track
mdat bytes by iterating adjusted_moof.traf and each trun.entries and copying
each sample's exact bytes from the original mdat (using the parsed sample
offsets/sizes you computed earlier) into a new Vec in the same order you will
lay out samples in the rewritten mdat, assign that Vec to per_track_mdat.data
(replacing track_mdat_data.to_vec()), and compute cumulative_offset based on the
same sequence of sample sizes so trun.data_offset assignments remain correct
(refer to adjusted_moof, trun.data_offset, trun.entries,
track_data_start/track_data_end, per_track_mdat, and cumulative_offset to locate
the change).
In `@rs/moq-mux/src/producer/jitter.rs`:
- Around line 25-27: The bug is that observe() updates self.last_timestamp
before checking monotonicity, so a rejected out-of-order ts corrupts future
measurements; modify observe (function observe) to only update
self.last_timestamp after validating ts against the current last value: if
last_timestamp is None initialize it with ts and return None, if Some(last) then
if ts < last return None without changing last_timestamp, otherwise compute
duration = ts.checked_sub(last) and then set last_timestamp = Some(ts) and
proceed; ensure all early returns leave last_timestamp unchanged on rejected
timestamps.
---
Outside diff comments:
In `@rs/moq-mux/src/producer/aac.rs`:
- Around line 117-127: The AudioConfig being built for AAC sets jitter: None
which drops useful frame timing; update the audio_config construction
(hang::catalog::AudioConfig in aac.rs) to populate jitter instead of None —
either use the jitter value from the source config (config.jitter) if present or
compute the AAC frame cadence as 1024 samples (i.e. frame_duration = 1024 /
config.sample_rate) and set jitter to Some(...) using the catalog's Jitter
variant so the catalog retains the AAC frame timing hint.
In `@rs/moq-mux/src/producer/avc3.rs`:
- Around line 268-293: maybe_start_frame is writing frames before the
rendition/catalog entry is initialized (new() creates the track eagerly while
init() inserts catalog entry), so guard against emitting undecodable pre-init
slices by checking self.config.is_some() before proceeding; update
maybe_start_frame to return early (or buffer/drop) when self.config.is_none(),
ensuring you do not call self.track.write(...) or self.track.keyframe() until
init() has set the catalog entry (also keep the existing
contains_slice/contains_idr logic intact so only fully-initialized streams are
emitted).
---
Duplicate comments:
In `@rs/moq-mux/src/producer/hev1.rs`:
- Around line 91-101: Call self.broadcast.unique_track(".hev1") and insert the
new rendition into catalog.video.renditions before mutating or taking self.track
so failures don't leave the producer unpublished; i.e., obtain let new_track =
self.broadcast.unique_track(".hev1")? first, tracing::debug on new_track and
insert new_track.name.clone() with config.clone() into catalog.video.renditions,
then if let Some(old) = self.track.take() { tracing::debug!(name = ?old.name,
"reinitializing track"); catalog.video.renditions.remove(&old.name); } and
finally set self.config = Some(config) and self.track = Some(new_track.into()).
---
Nitpick comments:
In `@rs/moq-mux/src/msf.rs`:
- Around line 18-24: Extract the duplicated logic that picks Cmaf { init } vs
description and base64-encodes it into a small helper (e.g., fn
get_init_data(config: &Config) -> Option<String> or encode_init_data(container:
&hang::catalog::Container, description: Option<&String>) -> Option<String>),
replace the inline match that builds init_data with calls to that helper
wherever used (the current occurrence using config.container and the similar
logic around lines 51-57), and ensure the helper returns the same Option<String>
semantics using base64::engine::general_purpose::STANDARD.encode on the chosen
byte slice.
🪄 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: 1304af4a-4692-4dcf-b436-2d2aaafa2c16
📒 Files selected for processing (24)
rs/hang/Cargo.tomlrs/hang/src/catalog/audio/mod.rsrs/hang/src/catalog/container.rsrs/hang/src/catalog/video/mod.rsrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/producer.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog.rsrs/moq-mux/src/hang/container.rsrs/moq-mux/src/msf.rsrs/moq-mux/src/ordered/consumer.rsrs/moq-mux/src/producer/aac.rsrs/moq-mux/src/producer/av01.rsrs/moq-mux/src/producer/avc1.rsrs/moq-mux/src/producer/avc3.rsrs/moq-mux/src/producer/fmp4.rsrs/moq-mux/src/producer/framed.rsrs/moq-mux/src/producer/hev1.rsrs/moq-mux/src/producer/jitter.rsrs/moq-mux/src/producer/mod.rsrs/moq-mux/src/producer/opus.rsrs/moq-mux/src/producer/stream.rsrs/moq-mux/src/producer/test/mod.rs
✅ Files skipped from review due to trivial changes (2)
- rs/hang/Cargo.toml
- rs/moq-mux/src/producer/mod.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rs/moq-mux/src/catalog.rs
- rs/hang/src/catalog/audio/mod.rs
- rs/moq-mux/src/ordered/consumer.rs
- rs/moq-mux/src/producer/test/mod.rs
- rs/moq-mux/src/producer/opus.rs
Comment on lines
+561
to
+624
| // Compute the data range within the original mdat for this traf's samples. | ||
| let track_data_start = track_data_start.unwrap_or(0); | ||
| let track_data_end = offset; // offset was advanced past all samples above | ||
|
|
||
| // The per-track sample range must be in bounds of the original mdat. | ||
| // If not, the parsed sample sizes/offsets disagree with the actual data | ||
| // and we cannot safely emit a passthrough fragment with rewritten offsets. | ||
| anyhow::ensure!( | ||
| track_data_start <= track_data_end && track_data_end <= mdat.data.len(), | ||
| "track sample range {}..{} is out of bounds of mdat (len {})", | ||
| track_data_start, | ||
| track_data_end, | ||
| mdat.data.len() | ||
| ); | ||
| let track_mdat_data = &mdat.data[track_data_start..track_data_end]; | ||
|
|
||
| let mut adjusted_moof = single_traf_moof; | ||
|
|
||
| // Apply structural (flag-presence) changes BEFORE measuring the encoded | ||
| // size, otherwise the rewritten data_offset values are computed from a | ||
| // stale size and the resulting fragment misaddresses sample data. | ||
| // In particular: clearing tfhd.base_data_offset removes 8 bytes per traf, | ||
| // and ensuring trun.data_offset is Some(...) reserves 4 bytes per trun. | ||
| for traf_mut in &mut adjusted_moof.traf { | ||
| traf_mut.tfhd.base_data_offset = None; | ||
| for trun_mut in &mut traf_mut.trun { | ||
| // Reserve the data_offset field; the real value is filled in below. | ||
| trun_mut.data_offset = Some(0); | ||
| } | ||
| } | ||
|
|
||
| let moof_raw = self.moof_raw.as_ref().context("missing moof box")?; | ||
| let mut moof_buf = Vec::new(); | ||
| adjusted_moof.encode(&mut moof_buf)?; | ||
| let new_moof_size = moof_buf.len(); | ||
|
|
||
| // Re-encode moof with corrected per-trun data_offset for the per-track fragment. | ||
| // Each trun's data_offset points to the start of that run's data within the new mdat. | ||
| let mdat_header_size_new = 8u64; // 4 bytes size + 4 bytes 'mdat' | ||
| let mut cumulative_offset = 0u64; | ||
| for traf_mut in &mut adjusted_moof.traf { | ||
| for trun_mut in &mut traf_mut.trun { | ||
| trun_mut.data_offset = | ||
| Some((new_moof_size as u64 + mdat_header_size_new + cumulative_offset) as i32); | ||
|
|
||
| // Advance past this trun's sample data | ||
| let trun_data_size: u64 = trun_mut | ||
| .entries | ||
| .iter() | ||
| .map(|e| { | ||
| e.size | ||
| .unwrap_or(traf_mut.tfhd.default_sample_size.unwrap_or(default_sample_size)) as u64 | ||
| }) | ||
| .sum(); | ||
| cumulative_offset += trun_data_size; | ||
| } | ||
| } | ||
|
|
||
| // To avoid an extra allocation, we use the chunked API to write the moof and mdat atoms separately. | ||
| let mut frame = g.create_frame(moq_lite::Frame { | ||
| size: moof_raw.len() as u64 + mdat_raw.len() as u64, | ||
| })?; | ||
| moof_buf.clear(); | ||
| adjusted_moof.encode(&mut moof_buf)?; | ||
|
|
||
| frame.write(moof_raw.clone())?; | ||
| frame.write(Bytes::copy_from_slice(mdat_raw))?; | ||
| frame.finish()?; | ||
| let per_track_mdat = Mdat { | ||
| data: track_mdat_data.to_vec(), | ||
| }; | ||
| per_track_mdat.encode(&mut moof_buf)?; |
Contributor
There was a problem hiding this comment.
track_mdat_data is only correct for contiguous trun payloads.
Lines 561-575 copy one continuous span from the first run start to the final end, but Lines 598-614 rewrite each trun.data_offset as if the new mdat were tightly packed from sample sizes only. For valid trafs with multiple truns separated by gaps or independent offsets, the second and later runs will point at the wrong bytes in the rewritten fragment.
🔧 Suggested direction
- // Compute the data range within the original mdat for this traf's samples.
- let track_data_start = track_data_start.unwrap_or(0);
- let track_data_end = offset; // offset was advanced past all samples above
- ...
- let track_mdat_data = &mdat.data[track_data_start..track_data_end];
+ // Record each run's exact byte range while walking the truns, then pack
+ // only those ranges into the new per-track mdat in the same order.
+ let mut run_ranges = Vec::new();
+ ...
+ let run_start = offset;
+ ...
+ let run_end = offset;
+ run_ranges.push((run_start, run_end));
+ ...
+ let mut track_mdat_data = Vec::new();
+ for (start, end) in &run_ranges {
+ track_mdat_data.extend_from_slice(&mdat.data[*start..*end]);
+ }
...
- let per_track_mdat = Mdat {
- data: track_mdat_data.to_vec(),
- };
+ let per_track_mdat = Mdat { data: track_mdat_data };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/producer/fmp4.rs` around lines 561 - 624, The current code
builds track_mdat_data as one contiguous slice
(track_data_start..track_data_end) which breaks when a traf contains multiple
truns with gaps or independent offsets; instead, construct per-track mdat bytes
by iterating adjusted_moof.traf and each trun.entries and copying each sample's
exact bytes from the original mdat (using the parsed sample offsets/sizes you
computed earlier) into a new Vec in the same order you will lay out samples in
the rewritten mdat, assign that Vec to per_track_mdat.data (replacing
track_mdat_data.to_vec()), and compute cumulative_offset based on the same
sequence of sample sizes so trun.data_offset assignments remain correct (refer
to adjusted_moof, trun.data_offset, trun.entries,
track_data_start/track_data_end, per_track_mdat, and cumulative_offset to locate
the change).
Comment on lines
+25
to
+27
| pub fn observe(&mut self, ts: Timestamp) -> Option<moq_lite::Time> { | ||
| let last = self.last_timestamp.replace(ts)?; | ||
| let duration = ts.checked_sub(last).ok()?; |
Contributor
There was a problem hiding this comment.
Don't advance last_timestamp on rejected timestamps.
Line 26 stores ts before the monotonicity check. After one out-of-order timestamp, the next valid frame is measured against the bad value, so the recorded minimum duration can drift away from the real stream.
🛠️ Suggested fix
pub fn observe(&mut self, ts: Timestamp) -> Option<moq_lite::Time> {
- let last = self.last_timestamp.replace(ts)?;
- let duration = ts.checked_sub(last).ok()?;
+ let Some(last) = self.last_timestamp else {
+ self.last_timestamp = Some(ts);
+ return None;
+ };
+ let duration = ts.checked_sub(last).ok()?;
+ self.last_timestamp = Some(ts);
if duration >= self.min_duration.unwrap_or(Timestamp::MAX) {
return None;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/producer/jitter.rs` around lines 25 - 27, The bug is that
observe() updates self.last_timestamp before checking monotonicity, so a
rejected out-of-order ts corrupts future measurements; modify observe (function
observe) to only update self.last_timestamp after validating ts against the
current last value: if last_timestamp is None initialize it with ts and return
None, if Some(last) then if ts < last return None without changing
last_timestamp, otherwise compute duration = ts.checked_sub(last) and then set
last_timestamp = Some(ts) and proceed; ensure all early returns leave
last_timestamp unchanged on rejected timestamps.
Three-part change folded into one commit:
1. Backport the export side from origin/dev
- Adds moq_mux::consumer::* (now ::export::*) — the catalog-aware
OrderedConsumer engine and its support files (container, frame, fmp4,
muxer, ordered).
- Adds cmaf::Convert and hang::Convert (now ::convert::cmaf::Convert /
::convert::hang::Convert) — broadcast format converters for moq-cli's
new `subscribe` subcommand.
- Migrates hang::container::OrderedConsumer to a 49-line deprecation
stub. The implementation moved to moq_mux. All in-tree callers
(libmoq, moq-gst, moq-ffi, hang::examples::subscribe) now use
moq_mux::export::OrderedConsumer with an explicit ContainerFormat.
2. Rename producer/ -> import/ and consumer/ -> export/
The directional verbs match what these modules actually do (ingest
external formats / emit external formats), instead of overloading the
moq-lite producer/consumer naming.
3. Restructure
- CatalogProducer / CatalogGuard moved into import/, since every
importer instantiates one.
- ordered/ split: Producer<C: Container> -> import/producer.rs;
Consumer<F: Container> -> export/consumer.rs. Top-level ordered/ gone.
- cmaf/ and hang/ collapsed into:
* container/{cmaf,hang}.rs for format definitions (Container impls,
Error, parse_timescale/decode/encode helpers, Legacy/Media)
* convert/{cmaf,hang}.rs for broadcast format converters
- Dropped unused cmaf::Producer / cmaf::Consumer / hang::Producer /
hang::Consumer type aliases.
Public path changes for downstream callers:
moq_mux::producer::* -> moq_mux::import::*
moq_mux::consumer::* -> moq_mux::export::*
moq_mux::CatalogProducer -> moq_mux::import::CatalogProducer
moq_mux::cmaf::Convert -> moq_mux::convert::cmaf::Convert
moq_mux::hang::Media -> moq_mux::container::hang::Media
hang::container::OrderedConsumer -> moq_mux::export::OrderedConsumer
(now generic over ContainerFormat)
just check passes (rust + js + biome + shear); 110 moq-mux + 12 hang
lib tests green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Delete hang::container::OrderedConsumer / OrderedFrame deprecation
stubs. They only panicked; all in-tree callers were migrated to
moq_mux::export::OrderedConsumer in the previous backport.
- Inline rs/moq-mux/src/msf.rs (to_msf + tests) into import/catalog.rs.
CatalogGuard::drop is the only caller; to_msf was pub but unused
outside the crate. Removes the bare top-level msf module.
- Drop deprecated type aliases in import/{framed,stream}.rs:
DecoderFormat, Decoder, StreamDecoder. None used in-tree.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promote the per-format types out of the cmaf/ and hang/ submodules so
the public API reads as a flat set of Container implementations:
container::Hang enum { Legacy, Cmaf(Cmaf) } — was hang::Media
container::Cmaf struct wrapping mp4_atom::Trak — new
container::CmafError was container::cmaf::Error
The standalone hang::Legacy unit struct is folded into Hang::Legacy.
hang::Media's Cmaf variant now carries our own Cmaf type instead of a
Box<mp4_atom::Moov>, so the catalog parsing path lives entirely on Cmaf
(via Cmaf::from_init). The mp4_atom::Trak inside Cmaf is heap-allocated
to keep the Hang enum compact.
The container::cmaf module is now pub(crate); only the helpers
parse_timescale/decode/encode remain inside it as crate-private utilities
for convert::hang.
Caller updates:
moq_mux::container::hang::Media -> moq_mux::container::Hang
crate::container::hang::Legacy -> crate::container::Hang::Legacy
crate::container::cmaf::Error -> crate::container::CmafError
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rack to remove Reverts the Audio/Video helper API to its pre-backport shape, then a bit further: - Add Audio::insert(name, config) and Video::insert(name, config) that return Err(Error::Duplicate(name)) if the rendition name is already taken. Adds the `Duplicate` variant to hang::Error. - Drop Audio::create_track and Video::create_track. The "extension + auto-numbered name" pattern belongs at the call site, not in the catalog API. - Rename Audio::remove_track / Video::remove_track to ::remove and take a &str instead of &moq_lite::Track. The method is acting on the rendition map, not on a moq track handle. No in-workspace callers of these methods, so the only breakage is for downstream users of the hang crate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
rs/hang/examples/subscribe.rs (1)
60-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't force
Legacyfor every subscribed track.
config.containeris available here, but Lines 85-86 always build aLegacyconsumer. That makes the example fail on CMAF catalog entries instead of either decoding the right container or rejecting it explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/hang/examples/subscribe.rs` around lines 60 - 86, The example currently always constructs an OrderedConsumer with moq_mux::export::Legacy which forces legacy handling; instead inspect the selected rendition's container via info (the variable config and config.container) and choose the appropriate consumer flavor when calling moq_mux::export::OrderedConsumer::new (e.g., use Legacy for legacy containers, use the CMAF/appropriate enum variant for CMAF entries, or return an explicit error if the container is unsupported); update the construction site where OrderedConsumer::new is invoked to switch on config.container and pass the matching moq_mux::export variant rather than unconditionally using Legacy.rs/moq-gst/src/source/imp.rs (1)
444-469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject or route CMAF tracks instead of forcing
Legacy.Both loops build
OrderedConsumer<Legacy>without checkingconfig.container, and the fixedLegacypump signatures lock that in. If the catalog advertises CMAF, this source will subscribe successfully and then parse fragments as legacy frames, which breaks playback. Please gate track setup onconfig.containerand either dispatch to the matching path or fail early with a clear unsupported-container error.Also applies to: 499-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-gst/src/source/imp.rs` around lines 444 - 469, The loops that create tracks (over catalog.video.renditions and catalog.audio.renditions) always construct moq_mux::export::OrderedConsumer::new(..., moq_mux::export::Legacy, ...) regardless of config.container; change track setup to inspect config.container and either construct the appropriate consumer/export mode for CMAF vs Legacy (or return an explicit unsupported-container error) before calling spawn_track_pump; update the branches that call request_pad, broadcast.subscribe_track, and spawn_track_pump (the code around TrackDescriptor, video_caps/audio_caps, request_pad, moq_lite::Track::new, broadcast.subscribe_track, and moq_mux::export::OrderedConsumer::new) to route based on config.container, and apply the same fix to the other occurrence mentioned (lines ~499-510).rs/moq-cli/src/publish.rs (1)
23-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFlush the decoder before returning on EOF.
The
n == 0branch exits the stdin path without callingfinish(), so any buffered state and the final track/group closure forAvc3/Fmp4never get emitted.🔧 Suggested fix
impl PublishDecoder { /// Decode a chunk of bytes from stdin (Avc3 or Fmp4 only). fn decode_buf(&mut self, buffer: &mut bytes::BytesMut) -> anyhow::Result<()> { match self { Self::Avc3(d) => d.decode_stream(buffer, None), Self::Fmp4(d) => d.decode(buffer), Self::Hls(_) => unreachable!(), } } + + fn finish(&mut self) -> anyhow::Result<()> { + match self { + Self::Avc3(d) => d.finish(), + Self::Fmp4(d) => d.finish(), + Self::Hls(_) => unreachable!(), + } + } } @@ loop { let n = tokio::io::AsyncReadExt::read_buf(&mut stdin, &mut buffer).await?; if n == 0 { - return Ok(()); + self.decoder.finish()?; + return Ok(()); } self.decoder.decode_buf(&mut buffer)?; }Also applies to: 70-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-cli/src/publish.rs` around lines 23 - 31, When encountering EOF (n == 0) ensure the decoder is flushed by calling its finish() before returning so buffered state and final track/group closures are emitted; in the stdin read path invoke finish() on the PublishDecoder variants (call d.finish() for both Self::Avc3 and Self::Fmp4, ensuring Avc3's decode_stream path also calls finish()) instead of returning immediately, and mirror the same change in the other stdin handling block referenced (the second occurrence around lines 70-83) so both code paths flush before exit.rs/moq-mux/src/import/avc1.rs (1)
93-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume the init buffer before the unchanged-config early return.
This branch returns
Ok(())without advancingbuf, even thoughinitialize()documents full consumption. Callers that reuse the buffer will keep seeing the stale AVCC payload.🔧 Suggested fix
if let Some(old) = &self.config && old == &config { + buf.advance(buf.remaining()); return Ok(()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/import/avc1.rs` around lines 93 - 97, The early-return when self.config == config skips consuming the init buffer; ensure the init payload is consumed before returning from initialize(): before the Ok(()) return in the branch that compares &self.config and &config, call the same buffer-consumption logic used by initialize() (i.e., consume/drain the incoming buf or invoke the helper that parses/advances the AVCC payload) so that buf is fully advanced even when the config is unchanged; update the branch in avc1.rs (the initialize() path that checks self.config/config and buf) to perform that consumption prior to returning.
♻️ Duplicate comments (1)
rs/moq-mux/src/import/avc1.rs (1)
101-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllocate the replacement track before dropping the current one.
If
unique_track(".avc1")fails here, the current code has already removed the active rendition and droppedself.track, so a transient allocation error tears down the published stream.🔁 Suggested fix
- if let Some(track) = &self.track.take() { - tracing::debug!(name = ?track.name, "reinitializing avc1 track"); - catalog.video.renditions.remove(&track.name); - } - let track = self.broadcast.unique_track(".avc1")?; + if let Some(old_track) = self.track.take() { + tracing::debug!(name = ?old_track.name, "reinitializing avc1 track"); + catalog.video.renditions.remove(&old_track.name); + } tracing::debug!(name = ?track.name, ?config, "starting avc1 track"); catalog.video.renditions.insert(track.name.clone(), config.clone());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/import/avc1.rs` around lines 101 - 111, Call broadcast.unique_track(".avc1") and obtain the new track before taking or removing the existing one so failures don't tear down the live rendition; specifically, invoke self.broadcast.unique_track(".avc1")? first (and log starting when successful), then if let Some(old) = self.track.take() { tracing::debug!(name = ?old.name, "reinitializing avc1 track"); catalog.video.renditions.remove(&old.name); } then insert the newly allocated track.name into catalog.video.renditions, set self.config = Some(config) and self.track = Some(track.into()). Ensure references to unique_track, self.track, catalog.video.renditions, and self.config are used to locate and reorder the operations.
🧹 Nitpick comments (5)
rs/moq-mux/src/export/frame.rs (1)
24-28: 💤 Low valueConsider renaming
is_keyframeto better reflect semantics.The method returns
index == 0, but the doc comment (lines 19-21) notes that for duration-based grouping (e.g., audio), the first frame is not necessarily a keyframe. The nameis_keyframecould mislead callers for audio tracks.Consider
is_group_start()or keepis_keyframewith the understanding that callers must check the track type. Since the behavior is documented, this is not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/export/frame.rs` around lines 24 - 28, The method OrderedFrame::is_keyframe returns whether index == 0 but the name is misleading for duration-based groups; rename is_keyframe to is_group_start (update the doc comment to state it returns true for the first frame in the group regardless of track type), then update all call sites, tests, and any trait impls to use OrderedFrame::is_group_start; optionally add a short deprecated wrapper named is_keyframe that calls is_group_start to preserve compatibility while updating docs and references.rs/moq-cli/src/subscribe.rs (1)
123-138: ⚡ Quick winDuplicate
parse_timescale_from_initfunction.This function is duplicated verbatim from
rs/moq-mux/src/export/fmp4.rs. Consider extracting it to a shared location (e.g.,moq_mux::exportor a common utility module) to avoid maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-cli/src/subscribe.rs` around lines 123 - 138, The parse_timescale_from_init function is duplicated; extract it into a shared utility module (e.g., create a new function in moq-mux::export or a common crate/module like moq_util::mp4) and replace the copy in rs/moq-cli/src/subscribe.rs with a use/import and call to that single implementation; update rs/moq-mux/src/export/fmp4.rs to call or re-export the shared parse_timescale_from_init and ensure the signature (fn parse_timescale_from_init(init_data_b64: &str) -> anyhow::Result<u64>) and dependencies (base64::Engine, mp4_atom::DecodeMaybe) are preserved and publicly accessible for both crates.rs/moq-mux/src/export/muxer.rs (2)
64-67: 💤 Low valueTrack errors are logged but not propagated to caller.
When
poll_readreturns an error, the track is marked finished and a warning is logged, but the error is not returned to the caller. This means track failures are silently swallowed and the muxer continues with remaining tracks. If this is intentional graceful degradation, consider documenting it. If errors should surface, returnPoll::Ready(Err(e))instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/export/muxer.rs` around lines 64 - 67, The current branch in the muxer's poll_read handling swallows errors by setting track.finished = true and logging via tracing::warn!, but does not propagate the error; change the logic in the Poll::Ready(Err(e)) arm of poll_read to return the error to the caller (i.e., return Poll::Ready(Err(e))) instead of only marking track.finished, or if you still want to mark the track finished, mark it then immediately return Poll::Ready(Err(e)) so the caller sees the failure; update the Poll::Ready(Err(e)) arm accordingly (refer to poll_read, track.finished, tracing::warn!, and the existing Poll::Ready(Err(e)) match arm).
73-85: 💤 Low valueConsider using iterator methods for minimum selection.
The manual tracking of
min_idx/min_tscould be simplified with iterator methods.♻️ Suggested simplification
- let mut min_idx = None; - let mut min_ts = None; - - for (i, track) in self.tracks.iter().enumerate() { - if let Some(frame) = &track.pending { - let ts: std::time::Duration = frame.timestamp.into(); - if min_ts.is_none() || ts < min_ts.unwrap() { - min_ts = Some(ts); - min_idx = Some(i); - } - } - } + let min_idx = self + .tracks + .iter() + .enumerate() + .filter_map(|(i, t)| t.pending.as_ref().map(|f| (i, f))) + .min_by_key(|(_, f)| Into::<std::time::Duration>::into(f.timestamp)) + .map(|(i, _)| i);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/export/muxer.rs` around lines 73 - 85, Replace the manual loop that tracks min_idx/min_ts by using iterator combinators on self.tracks: use self.tracks.iter().enumerate().filter_map(|(i, track)| track.pending.as_ref().map(|f| (i, f.timestamp.into()))).min_by_key(|(_, ts)| *ts) to find the minimum (index, timestamp) pair and then set min_idx and min_ts from that result (e.g., map the found pair to Some(i) and Some(ts)); this removes the explicit mutable min_idx/min_ts and simplifies the selection logic.rs/moq-mux/src/export/fmp4.rs (1)
149-149: 💤 Low valuePotential integer overflow in DTS calculation for long-running streams.
The expression
frame.timestamp.as_micros() as u64 * track.timescale / 1_000_000can overflow for streams longer than ~31 years at 90kHz timescale. While unlikely in practice, consider using checked/saturating arithmetic or computing via u128 intermediates.🛡️ Safer calculation
- let dts = frame.timestamp.as_micros() as u64 * track.timescale / 1_000_000; + let dts = (frame.timestamp.as_micros() * track.timescale as u128 / 1_000_000) as u64;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/export/fmp4.rs` at line 149, The DTS calculation can overflow: replace the direct u64 multiply in the dts assignment with a wider intermediate and saturate to u64; e.g. compute using u128 like let dts = ((frame.timestamp.as_micros() as u128 * track.timescale as u128) / 1_000_000u128).min(u64::MAX as u128) as u64 so the multiplication uses a 128-bit intermediate and the result is clamped to u64; update the code around the dts variable (the expression using frame.timestamp.as_micros() and track.timescale in fmp4.rs) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/hang/src/catalog/consumer.rs`:
- Around line 39-46: The code currently calls self.group.take() in both the
ready and pending branches, which drops the group on Poll::Pending and loses the
ability to resume reading; only remove the group when you actually consume a
frame. Fix by removing the self.group.take() call from the else/Poll::Pending
branch and keep it only inside the Poll::Ready(Some(frame)) branch (where
group.poll_read_frame(...) returns a frame) so Catalog::from_slice(&frame) is
called after you take ownership and do not drop the group on Pending; also
ensure any other branches that represent terminal states (e.g., Ready(None) or
Err) correctly take/drop the group as appropriate.
In `@rs/moq-mux/src/export/container.rs`:
- Around line 50-66: The public error enum CmafError should be marked
non-exhaustive like the other Error enum; add the #[non_exhaustive] attribute
immediately above the #[derive(Debug, thiserror::Error)] for CmafError so
external consumers can’t exhaustively match it and future variants can be added
without breaking changes.
- Around line 20-31: The public error enum Error in container.rs should be
marked non-exhaustive to allow future variants without breaking changes; add the
#[non_exhaustive] attribute immediately above the pub enum Error declaration
(the enum that derives Debug and thiserror::Error and contains variants Moq,
Mp4, and Other) so consumers cannot exhaustively match on it.
In `@rs/moq-mux/src/import/catalog.rs`:
- Around line 28-41: with_catalog currently stores the provided catalog in
current but never publishes it, leaving hang_track and msf_track empty; after
creating hang_track and msf_track, call the BroadcastProducer publish/seed API
to publish a seeded snapshot to both tracks: publish the given hang::Catalog to
hang_track and publish the corresponding MSF snapshot (derived from the catalog
or constructed from moq_msf::DEFAULT_NAME) to msf_track so subscribers created
immediately after construction see the initial state; update with_catalog to
perform these publish calls before returning and keep current as-is.
In `@rs/moq-mux/src/import/stream.rs`:
- Around line 64-143: The public methods Stream::initialize,
Stream::decode_stream, and Stream::finish must not return anyhow::Result; create
a public error enum (e.g., StreamError) using thiserror::Error with #[from]
variants for each inner codec error type (map the Avc3, Fmp4, Hev1, Av01 decoder
error types into variants) plus any local errors (e.g., BufferNotConsumed) and
change the signatures to Result<(), StreamError>; update all match arms in
initialize/decode_stream/finish/is_initialized to propagate codec errors (they
will convert via #[from]) and replace the anyhow::ensure! call with an explicit
check that returns Err(StreamError::BufferNotConsumed) when buf.has_remaining()
is true so the API exposes a typed error for callers.
---
Outside diff comments:
In `@rs/hang/examples/subscribe.rs`:
- Around line 60-86: The example currently always constructs an OrderedConsumer
with moq_mux::export::Legacy which forces legacy handling; instead inspect the
selected rendition's container via info (the variable config and
config.container) and choose the appropriate consumer flavor when calling
moq_mux::export::OrderedConsumer::new (e.g., use Legacy for legacy containers,
use the CMAF/appropriate enum variant for CMAF entries, or return an explicit
error if the container is unsupported); update the construction site where
OrderedConsumer::new is invoked to switch on config.container and pass the
matching moq_mux::export variant rather than unconditionally using Legacy.
In `@rs/moq-cli/src/publish.rs`:
- Around line 23-31: When encountering EOF (n == 0) ensure the decoder is
flushed by calling its finish() before returning so buffered state and final
track/group closures are emitted; in the stdin read path invoke finish() on the
PublishDecoder variants (call d.finish() for both Self::Avc3 and Self::Fmp4,
ensuring Avc3's decode_stream path also calls finish()) instead of returning
immediately, and mirror the same change in the other stdin handling block
referenced (the second occurrence around lines 70-83) so both code paths flush
before exit.
In `@rs/moq-gst/src/source/imp.rs`:
- Around line 444-469: The loops that create tracks (over
catalog.video.renditions and catalog.audio.renditions) always construct
moq_mux::export::OrderedConsumer::new(..., moq_mux::export::Legacy, ...)
regardless of config.container; change track setup to inspect config.container
and either construct the appropriate consumer/export mode for CMAF vs Legacy (or
return an explicit unsupported-container error) before calling spawn_track_pump;
update the branches that call request_pad, broadcast.subscribe_track, and
spawn_track_pump (the code around TrackDescriptor, video_caps/audio_caps,
request_pad, moq_lite::Track::new, broadcast.subscribe_track, and
moq_mux::export::OrderedConsumer::new) to route based on config.container, and
apply the same fix to the other occurrence mentioned (lines ~499-510).
In `@rs/moq-mux/src/import/avc1.rs`:
- Around line 93-97: The early-return when self.config == config skips consuming
the init buffer; ensure the init payload is consumed before returning from
initialize(): before the Ok(()) return in the branch that compares &self.config
and &config, call the same buffer-consumption logic used by initialize() (i.e.,
consume/drain the incoming buf or invoke the helper that parses/advances the
AVCC payload) so that buf is fully advanced even when the config is unchanged;
update the branch in avc1.rs (the initialize() path that checks
self.config/config and buf) to perform that consumption prior to returning.
---
Duplicate comments:
In `@rs/moq-mux/src/import/avc1.rs`:
- Around line 101-111: Call broadcast.unique_track(".avc1") and obtain the new
track before taking or removing the existing one so failures don't tear down the
live rendition; specifically, invoke self.broadcast.unique_track(".avc1")? first
(and log starting when successful), then if let Some(old) = self.track.take() {
tracing::debug!(name = ?old.name, "reinitializing avc1 track");
catalog.video.renditions.remove(&old.name); } then insert the newly allocated
track.name into catalog.video.renditions, set self.config = Some(config) and
self.track = Some(track.into()). Ensure references to unique_track, self.track,
catalog.video.renditions, and self.config are used to locate and reorder the
operations.
---
Nitpick comments:
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 123-138: The parse_timescale_from_init function is duplicated;
extract it into a shared utility module (e.g., create a new function in
moq-mux::export or a common crate/module like moq_util::mp4) and replace the
copy in rs/moq-cli/src/subscribe.rs with a use/import and call to that single
implementation; update rs/moq-mux/src/export/fmp4.rs to call or re-export the
shared parse_timescale_from_init and ensure the signature (fn
parse_timescale_from_init(init_data_b64: &str) -> anyhow::Result<u64>) and
dependencies (base64::Engine, mp4_atom::DecodeMaybe) are preserved and publicly
accessible for both crates.
In `@rs/moq-mux/src/export/fmp4.rs`:
- Line 149: The DTS calculation can overflow: replace the direct u64 multiply in
the dts assignment with a wider intermediate and saturate to u64; e.g. compute
using u128 like let dts = ((frame.timestamp.as_micros() as u128 *
track.timescale as u128) / 1_000_000u128).min(u64::MAX as u128) as u64 so the
multiplication uses a 128-bit intermediate and the result is clamped to u64;
update the code around the dts variable (the expression using
frame.timestamp.as_micros() and track.timescale in fmp4.rs) accordingly.
In `@rs/moq-mux/src/export/frame.rs`:
- Around line 24-28: The method OrderedFrame::is_keyframe returns whether index
== 0 but the name is misleading for duration-based groups; rename is_keyframe to
is_group_start (update the doc comment to state it returns true for the first
frame in the group regardless of track type), then update all call sites, tests,
and any trait impls to use OrderedFrame::is_group_start; optionally add a short
deprecated wrapper named is_keyframe that calls is_group_start to preserve
compatibility while updating docs and references.
In `@rs/moq-mux/src/export/muxer.rs`:
- Around line 64-67: The current branch in the muxer's poll_read handling
swallows errors by setting track.finished = true and logging via tracing::warn!,
but does not propagate the error; change the logic in the Poll::Ready(Err(e))
arm of poll_read to return the error to the caller (i.e., return
Poll::Ready(Err(e))) instead of only marking track.finished, or if you still
want to mark the track finished, mark it then immediately return
Poll::Ready(Err(e)) so the caller sees the failure; update the
Poll::Ready(Err(e)) arm accordingly (refer to poll_read, track.finished,
tracing::warn!, and the existing Poll::Ready(Err(e)) match arm).
- Around line 73-85: Replace the manual loop that tracks min_idx/min_ts by using
iterator combinators on self.tracks: use
self.tracks.iter().enumerate().filter_map(|(i, track)|
track.pending.as_ref().map(|f| (i, f.timestamp.into()))).min_by_key(|(_, ts)|
*ts) to find the minimum (index, timestamp) pair and then set min_idx and min_ts
from that result (e.g., map the found pair to Some(i) and Some(ts)); this
removes the explicit mutable min_idx/min_ts and simplifies the selection logic.
🪄 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: 81d4e8da-64b5-4e87-8ea0-35115c075ba7
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockrs/moq-mux/src/import/test/av1.mp4is excluded by!**/*.mp4rs/moq-mux/src/import/test/bbb.mp4is excluded by!**/*.mp4rs/moq-mux/src/import/test/vp9.mp4is excluded by!**/*.mp4
📒 Files selected for processing (61)
rs/hang/Cargo.tomlrs/hang/examples/subscribe.rsrs/hang/src/catalog/audio/mod.rsrs/hang/src/catalog/consumer.rsrs/hang/src/catalog/root.rsrs/hang/src/catalog/video/mod.rsrs/hang/src/container/mod.rsrs/hang/src/container/producer.rsrs/libmoq/src/consume.rsrs/libmoq/src/error.rsrs/libmoq/src/publish.rsrs/moq-boy/src/audio.rsrs/moq-boy/src/main.rsrs/moq-boy/src/video.rsrs/moq-cli/Cargo.tomlrs/moq-cli/src/main.rsrs/moq-cli/src/publish.rsrs/moq-cli/src/subscribe.rsrs/moq-ffi/Cargo.tomlrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/producer.rsrs/moq-gst/src/sink/imp.rsrs/moq-gst/src/source/imp.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog.rsrs/moq-mux/src/cmaf/error.rsrs/moq-mux/src/cmaf/mod.rsrs/moq-mux/src/container/cmaf.rsrs/moq-mux/src/container/hang.rsrs/moq-mux/src/container/mod.rsrs/moq-mux/src/convert/cmaf.rsrs/moq-mux/src/convert/hang.rsrs/moq-mux/src/convert/mod.rsrs/moq-mux/src/error.rsrs/moq-mux/src/export/consumer.rsrs/moq-mux/src/export/container.rsrs/moq-mux/src/export/fmp4.rsrs/moq-mux/src/export/frame.rsrs/moq-mux/src/export/mod.rsrs/moq-mux/src/export/muxer.rsrs/moq-mux/src/export/ordered.rsrs/moq-mux/src/hang/container.rsrs/moq-mux/src/hang/mod.rsrs/moq-mux/src/import/aac.rsrs/moq-mux/src/import/av01.rsrs/moq-mux/src/import/avc1.rsrs/moq-mux/src/import/avc3.rsrs/moq-mux/src/import/catalog.rsrs/moq-mux/src/import/fmp4.rsrs/moq-mux/src/import/framed.rsrs/moq-mux/src/import/hev1.rsrs/moq-mux/src/import/hls.rsrs/moq-mux/src/import/jitter.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/opus.rsrs/moq-mux/src/import/producer.rsrs/moq-mux/src/import/stream.rsrs/moq-mux/src/import/test/mod.rsrs/moq-mux/src/lib.rsrs/moq-mux/src/ordered/mod.rs
💤 Files with no reviewable changes (8)
- rs/hang/src/container/mod.rs
- rs/hang/src/catalog/root.rs
- rs/moq-mux/src/ordered/mod.rs
- rs/moq-mux/src/hang/container.rs
- rs/moq-mux/src/cmaf/error.rs
- rs/moq-mux/src/catalog.rs
- rs/moq-mux/src/cmaf/mod.rs
- rs/moq-mux/src/hang/mod.rs
✅ Files skipped from review due to trivial changes (6)
- rs/moq-ffi/Cargo.toml
- rs/moq-cli/Cargo.toml
- rs/moq-boy/src/main.rs
- rs/moq-mux/src/export/mod.rs
- rs/moq-ffi/src/media.rs
- rs/moq-gst/src/sink/imp.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- rs/hang/Cargo.toml
- rs/moq-mux/src/error.rs
- rs/moq-boy/src/audio.rs
- rs/moq-boy/src/video.rs
- rs/moq-mux/Cargo.toml
- rs/moq-ffi/src/producer.rs
Comment on lines
+20
to
+31
| /// Errors from container format operations. | ||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { | ||
| #[error("moq: {0}")] | ||
| Moq(#[from] moq_lite::Error), | ||
|
|
||
| #[error("mp4: {0}")] | ||
| Mp4(#[from] mp4_atom::Error), | ||
|
|
||
| #[error("{0}")] | ||
| Other(String), | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add #[non_exhaustive] to public error enums.
Per coding guidelines, public thiserror enums in library crates should have #[non_exhaustive] to allow adding variants without breaking changes.
♻️ Proposed fix
/// Errors from container format operations.
#[derive(Debug, thiserror::Error)]
+#[non_exhaustive]
pub enum Error {
#[error("moq: {0}")]
Moq(#[from] moq_lite::Error),As per coding guidelines: "Always add #[non_exhaustive] to public thiserror enums".
📝 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.
Suggested change
| /// Errors from container format operations. | |
| #[derive(Debug, thiserror::Error)] | |
| pub enum Error { | |
| #[error("moq: {0}")] | |
| Moq(#[from] moq_lite::Error), | |
| #[error("mp4: {0}")] | |
| Mp4(#[from] mp4_atom::Error), | |
| #[error("{0}")] | |
| Other(String), | |
| } | |
| /// Errors from container format operations. | |
| #[derive(Debug, thiserror::Error)] | |
| #[non_exhaustive] | |
| pub enum Error { | |
| #[error("moq: {0}")] | |
| Moq(#[from] moq_lite::Error), | |
| #[error("mp4: {0}")] | |
| Mp4(#[from] mp4_atom::Error), | |
| #[error("{0}")] | |
| Other(String), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/export/container.rs` around lines 20 - 31, The public error
enum Error in container.rs should be marked non-exhaustive to allow future
variants without breaking changes; add the #[non_exhaustive] attribute
immediately above the pub enum Error declaration (the enum that derives Debug
and thiserror::Error and contains variants Moq, Mp4, and Other) so consumers
cannot exhaustively match on it.
Comment on lines
+50
to
+66
| #[derive(Debug, thiserror::Error)] | ||
| pub enum CmafError { | ||
| #[error("mp4: {0}")] | ||
| Mp4(#[from] mp4_atom::Error), | ||
|
|
||
| #[error("timestamp overflow")] | ||
| TimestampOverflow(#[from] moq_lite::TimeOverflow), | ||
|
|
||
| #[error("no traf in moof")] | ||
| NoTraf, | ||
|
|
||
| #[error("no tfdt in traf")] | ||
| NoTfdt, | ||
|
|
||
| #[error("no moof found in CMAF frame data")] | ||
| NoMoof, | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add #[non_exhaustive] to CmafError.
Same rationale as Error above.
♻️ Proposed fix
#[derive(Debug, thiserror::Error)]
+#[non_exhaustive]
pub enum CmafError {
#[error("mp4: {0}")]
Mp4(#[from] mp4_atom::Error),As per coding guidelines: "Always add #[non_exhaustive] to public thiserror enums".
📝 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.
Suggested change
| #[derive(Debug, thiserror::Error)] | |
| pub enum CmafError { | |
| #[error("mp4: {0}")] | |
| Mp4(#[from] mp4_atom::Error), | |
| #[error("timestamp overflow")] | |
| TimestampOverflow(#[from] moq_lite::TimeOverflow), | |
| #[error("no traf in moof")] | |
| NoTraf, | |
| #[error("no tfdt in traf")] | |
| NoTfdt, | |
| #[error("no moof found in CMAF frame data")] | |
| NoMoof, | |
| } | |
| #[derive(Debug, thiserror::Error)] | |
| #[non_exhaustive] | |
| pub enum CmafError { | |
| #[error("mp4: {0}")] | |
| Mp4(#[from] mp4_atom::Error), | |
| #[error("timestamp overflow")] | |
| TimestampOverflow(#[from] moq_lite::TimeOverflow), | |
| #[error("no traf in moof")] | |
| NoTraf, | |
| #[error("no tfdt in traf")] | |
| NoTfdt, | |
| #[error("no moof found in CMAF frame data")] | |
| NoMoof, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/export/container.rs` around lines 50 - 66, The public error
enum CmafError should be marked non-exhaustive like the other Error enum; add
the #[non_exhaustive] attribute immediately above the #[derive(Debug,
thiserror::Error)] for CmafError so external consumers can’t exhaustively match
it and future variants can be added without breaking changes.
Comment on lines
+28
to
+41
| /// Create a new catalog producer with the given initial catalog. | ||
| pub fn with_catalog( | ||
| broadcast: &mut moq_lite::BroadcastProducer, | ||
| catalog: hang::Catalog, | ||
| ) -> Result<Self, moq_lite::Error> { | ||
| let hang_track = broadcast.create_track(hang::Catalog::default_track())?; | ||
| let msf_track = broadcast.create_track(moq_lite::Track::new(moq_msf::DEFAULT_NAME))?; | ||
|
|
||
| Ok(Self { | ||
| hang_track, | ||
| msf_track, | ||
| current: Arc::new(Mutex::new(catalog)), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Publish the seeded snapshot from with_catalog().
with_catalog() records the supplied catalog in memory, but both catalog tracks stay empty until a later mutation. Since there is no public publish() path, callers cannot actually expose a non-default initial catalog to subscribers created right after construction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/import/catalog.rs` around lines 28 - 41, with_catalog
currently stores the provided catalog in current but never publishes it, leaving
hang_track and msf_track empty; after creating hang_track and msf_track, call
the BroadcastProducer publish/seed API to publish a seeded snapshot to both
tracks: publish the given hang::Catalog to hang_track and publish the
corresponding MSF snapshot (derived from the catalog or constructed from
moq_msf::DEFAULT_NAME) to msf_track so subscribers created immediately after
construction see the initial state; update with_catalog to perform these publish
calls before returning and keep current as-is.
Comment on lines
+64
to
+143
| pub struct Stream { | ||
| decoder: StreamKind, | ||
| } | ||
|
|
||
| impl Stream { | ||
| /// Create a new stream importer with the given format. | ||
| pub fn new( | ||
| broadcast: moq_lite::BroadcastProducer, | ||
| catalog: crate::import::CatalogProducer, | ||
| format: StreamFormat, | ||
| ) -> Self { | ||
| let decoder = match format { | ||
| StreamFormat::Avc3 => super::Avc3::new(broadcast, catalog).into(), | ||
| StreamFormat::Fmp4 => Box::new(super::Fmp4::new(broadcast, catalog)).into(), | ||
| StreamFormat::Hev1 => super::Hev1::new(broadcast, catalog).into(), | ||
| StreamFormat::Av01 => super::Av01::new(broadcast, catalog).into(), | ||
| }; | ||
|
|
||
| Self { decoder } | ||
| } | ||
|
|
||
| /// Initialize the decoder with the given buffer and populate the broadcast. | ||
| /// | ||
| /// This is not required for self-describing formats like fMP4 or AVC3. | ||
| /// | ||
| /// The buffer will be fully consumed, or an error will be returned. | ||
| pub fn initialize<T: Buf + AsRef<[u8]>>(&mut self, buf: &mut T) -> anyhow::Result<()> { | ||
| match self.decoder { | ||
| StreamKind::Avc3(ref mut decoder) => decoder.initialize(buf)?, | ||
| StreamKind::Fmp4(ref mut decoder) => decoder.decode(buf)?, | ||
| StreamKind::Hev1(ref mut decoder) => decoder.initialize(buf)?, | ||
| StreamKind::Av01(ref mut decoder) => decoder.initialize(buf)?, | ||
| } | ||
|
|
||
| anyhow::ensure!(!buf.has_remaining(), "buffer was not fully consumed"); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Decode a stream of data from the given buffer. | ||
| /// | ||
| /// This method should be used when the caller does not know the frame boundaries. | ||
| /// For example, reading a fMP4 file from disk or receiving annex.b over the network. | ||
| /// | ||
| /// A timestamp cannot be provided because you don't even know if the buffer contains a frame. | ||
| /// The wall clock time will be used if the format does not contain its own timestamps. | ||
| /// | ||
| /// If the buffer is not fully consumed, more data is needed. | ||
| pub fn decode_stream<T: Buf + AsRef<[u8]>>(&mut self, buf: &mut T) -> anyhow::Result<()> { | ||
| match self.decoder { | ||
| StreamKind::Avc3(ref mut decoder) => decoder.decode_stream(buf, None), | ||
| StreamKind::Fmp4(ref mut decoder) => decoder.decode(buf), | ||
| StreamKind::Hev1(ref mut decoder) => decoder.decode_stream(buf, None), | ||
| StreamKind::Av01(ref mut decoder) => decoder.decode_stream(buf, None), | ||
| } | ||
| } | ||
|
|
||
| /// Finish the decoder, flushing any buffered data. | ||
| /// | ||
| /// This should be called when the input stream ends to ensure the last | ||
| /// group is properly finalized. | ||
| pub fn finish(&mut self) -> anyhow::Result<()> { | ||
| match self.decoder { | ||
| StreamKind::Avc3(ref mut decoder) => decoder.finish(), | ||
| StreamKind::Fmp4(ref mut decoder) => decoder.finish(), | ||
| StreamKind::Hev1(ref mut decoder) => decoder.finish(), | ||
| StreamKind::Av01(ref mut decoder) => decoder.finish(), | ||
| } | ||
| } | ||
|
|
||
| /// Check if the decoder has read enough data to be initialized. | ||
| pub fn is_initialized(&self) -> bool { | ||
| match self.decoder { | ||
| StreamKind::Avc3(ref decoder) => decoder.is_initialized(), | ||
| StreamKind::Fmp4(ref decoder) => decoder.is_initialized(), | ||
| StreamKind::Hev1(ref decoder) => decoder.is_initialized(), | ||
| StreamKind::Av01(ref decoder) => decoder.is_initialized(), | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type rust 'pub fn (initialize|decode_stream|finish)|-> anyhow::Result' rs/moq-mux/src/import/stream.rsRepository: moq-dev/moq
Length of output: 297
Use a typed error enum for this public library API instead of anyhow::Result.
The Stream::initialize, decode_stream, and finish methods are new public entrypoints in the moq-mux library crate but currently return anyhow::Result. Per the coding guidelines, library crates should use thiserror with #[from] for error types, not anyhow. This allows downstream users to handle errors in a typed, refactor-safe way. Create a public thiserror error enum and derive #[from] on the inner codec failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-mux/src/import/stream.rs` around lines 64 - 143, The public methods
Stream::initialize, Stream::decode_stream, and Stream::finish must not return
anyhow::Result; create a public error enum (e.g., StreamError) using
thiserror::Error with #[from] variants for each inner codec error type (map the
Avc3, Fmp4, Hev1, Av01 decoder error types into variants) plus any local errors
(e.g., BufferNotConsumed) and change the signatures to Result<(), StreamError>;
update all match arms in initialize/decode_stream/finish/is_initialized to
propagate codec errors (they will convert via #[from]) and replace the
anyhow::ensure! call with an explicit check that returns
Err(StreamError::BufferNotConsumed) when buf.has_remaining() is true so the API
exposes a typed error for callers.
Address PR feedback on rs/hang/src/catalog/container.rs: the field was called init_data and stored as a base64 String. Rename to init, type as bytes::Bytes, and use serde_with::Base64 to handle the wire encoding so callers don't have to base64-decode/encode themselves. Removes the unused pub on OrderedConsumer<F>::track in moq-mux/src/export/ordered.rs (PR comment: avoid making this pub). Drops dead deprecated aliases that were noted but never cleaned up: import::DecoderFormat, import::Decoder, import::StreamDecoder. Backports the corresponding JS-side CMAF init-segment changes from dev: - ContainerSchema cmaf variant carries `init: z.base64()` (was timescale + trackId). - createVideoInitSegment / createAudioInitSegment no longer read timescale/trackId from the container; they hard-code the legacy values, since on the CMAF path the catalog now ships the canonical init segment. - video/audio decoder + mse paths decode the catalog init bytes once and read the timescale out of them via Cmaf.decodeInitSegment, then feed the raw bytes straight into the SourceBuffer. - A small base64ToBytes helper lives at js/watch/src/base64.ts and is shared across watch's mse/decoder/msf paths. - watch/msf.ts forwards the MSF initData straight through as the cmaf container's `init` field; the timescale/trackId derivation lives in hang's CMAF decode now. Drop unused base64 deps from moq-cli and moq-ffi (cargo shear). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
rs/moq-mux/src/import/catalog.rs (1)
100-120: 💤 Low valueConsider logging errors in
CatalogGuard::drop.The
Dropimplementation silently discards errors fromappend_group(),write_frame(), andfinish(). WhileDropcannot return errors, logging failures would help with debugging catalog publishing issues.♻️ Suggested improvement
impl Drop for CatalogGuard<'_> { fn drop(&mut self) { if !self.updated { return; } // Publish hang catalog - if let Ok(mut group) = self.hang_track.append_group() { - let frame = self.catalog.to_string().expect("invalid catalog"); - let _ = group.write_frame(frame); - let _ = group.finish(); + match self.hang_track.append_group() { + Ok(mut group) => { + let frame = self.catalog.to_string().expect("invalid catalog"); + if let Err(e) = group.write_frame(frame) { + tracing::warn!(%e, "failed to write hang catalog frame"); + } + if let Err(e) = group.finish() { + tracing::warn!(%e, "failed to finish hang catalog group"); + } + } + Err(e) => tracing::warn!(%e, "failed to append hang catalog group"), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/import/catalog.rs` around lines 100 - 120, The Drop impl for CatalogGuard currently ignores errors from self.hang_track.append_group(), group.write_frame(), group.finish(), and the msf path (to_msf/calling write_frame/finish); update CatalogGuard::drop to record/log any failures instead of silently discarding them: check the Result from append_group(), map Err to a call to the project's logger (or eprintln! if no logger exists) with context ("hang_track.append_group" / "msf_track.append_group"), and for each group operation check write_frame() and finish() and log any Err with context including the catalog via self.catalog.to_string() (or the msf string), ensuring all calls still proceed safely within drop (no panics) and avoid returning errors.rs/moq-mux/src/import/fmp4.rs (1)
629-640: 💤 Low valueVerify first frame of each track is always a keyframe.
The code assumes the first frame is a keyframe (
context("no keyframe at start")). This is valid for well-formed CMAF streams but will error on malformed input where a track starts mid-GOP.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/import/fmp4.rs` around lines 629 - 640, The code currently panics on tracks that start mid‑GOP by calling .context("no keyframe at start") when contains_keyframe is false; change this to tolerate non‑key start frames by not erroring out: when contains_keyframe is false and track.group is None, skip or buffer the incoming fragment_bytes instead of failing (i.e., do not call .context to unwrap), and only create a group (via track.track.append_group()) and call write_frame() once a keyframe is seen; keep using track.group.take(), track.track.append_group(), track.group = Some(g), finish(), and write_frame() to manage groups but handle the missing-keyframe case by dropping/buffering the frame and returning early so malformed mid‑GOP starts do not error.rs/moq-mux/src/export/ordered.rs (1)
343-343: 💤 Low valueConsider using a typed error variant instead of
Error::Other.
Error::Other("empty group".into())loses type safety compared to a dedicatedError::EmptyGroupvariant. If callers need to distinguish empty groups from other errors, matching on string content is fragile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/export/ordered.rs` at line 343, Replace the string-based error return Error::Other("empty group".into()) with a typed variant by adding Error::EmptyGroup to the Error enum and using Error::EmptyGroup at the call site; update any Display/From/FromStr/serialization impls and match arms that previously inspected the "empty group" string to handle Error::EmptyGroup so callers can pattern-match on the variant instead of brittle string comparisons, and run/update tests that assert this error case accordingly.rs/moq-mux/src/export/fmp4.rs (2)
145-145: 💤 Low valuePayload collection allocates unnecessarily.
frame.payload.clone().into_iter().flat_map(|c| c.into_iter()).collect()creates an intermediate allocation. Consider usingbytes::Buftrait methods or iterating directly when encoding tomdat.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/export/fmp4.rs` at line 145, The current payload assembly (payload: Vec<u8> = frame.payload.clone().into_iter().flat_map(|c| c.into_iter()).collect()) does an unnecessary clone+collect; instead iterate the frame.payload segments directly when writing the mdat so you avoid the intermediate Vec. In the function that encodes to mdat, replace the collect pattern by draining/iterating frame.payload (or using the bytes::Buf API on each chunk) and write/extend each slice into the mdat buffer/writer (e.g., call writer.write_all(chunk) or extend_from_slice(chunk) for each segment) so you eliminate the clone and extra allocation; look for symbols frame.payload and the mdat encoding block in fmp4.rs to apply the change.
144-144: 💤 Low valuePotential precision loss in timestamp conversion.
The expression
frame.timestamp.as_micros() as u64 * track.timescale / 1_000_000may lose precision due to integer division. For example, ifas_micros()returns 16666 (≈16.666ms) and timescale is 90000, the result is16666 * 90000 / 1_000_000 = 1499instead of the more precise 1500.Consider reordering or using 128-bit intermediate to reduce truncation:
-let dts = frame.timestamp.as_micros() as u64 * track.timescale / 1_000_000; +let dts = (frame.timestamp.as_micros() as u128 * track.timescale as u128 / 1_000_000) as u64;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/export/fmp4.rs` at line 144, The calculation for dts (using frame.timestamp.as_micros() as u64 * track.timescale / 1_000_000) can lose precision due to integer truncation and might overflow for large values; change the computation to use a wider intermediate (u128) and perform the multiply before dividing to preserve precision, e.g., cast frame.timestamp.as_micros() and track.timescale to u128, compute the product, then divide by 1_000_000 (optionally add half the divisor for rounding) and finally cast back to u64; update the dts assignment where dts is computed from frame.timestamp.as_micros() and track.timescale.rs/moq-cli/src/subscribe.rs (1)
123-134: 💤 Low valueDuplicated
parse_timescale_from_initfunction.This function is identical to
rs/moq-mux/src/export/fmp4.rs:196-205. Consider extracting it to a shared location to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-cli/src/subscribe.rs` around lines 123 - 134, The function parse_timescale_from_init is duplicated; extract it into a shared utility module (e.g., a common crate or rs/common module) and replace the local copies with a single imported function; ensure the extracted function keeps the signature parse_timescale_from_init(init: &[u8]) -> anyhow::Result<u64> and uses the same mp4_atom::Any::decode_maybe logic and trak.mdia.mdhd.timescale access, make it public (pub or pub(crate) as needed) and update callers in rs/moq-cli/src/subscribe.rs and rs/moq-mux/src/export/fmp4.rs to call the shared implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/hang/src/catalog/container.ts`:
- Around line 15-20: The schema for the "cmaf" container uses z.base64(), which
doesn't exist in zod/mini; replace z.base64() on the init property in the object
schema for the "cmaf" variant with a string-based validator such as
z.string().regex(/^[A-Za-z0-9+/]*={0,2}$/) or
z.string().refine(customBase64Validator) to validate base64, or alternatively
switch the import to full zod if you intentionally want z.base64(); update the
init field in the container schema accordingly.
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 68-74: The Fmp4 exporter (moq_mux::export::Fmp4) is instantiated
as exporter and only used for init() but never for exporter.frame(), while later
code writes raw CMAF payloads directly; either remove the unused exporter and
its init() call if you intend pure CMAF passthrough, or switch to using
exporter.frame() to convert incoming payloads into fMP4 frames before writing
them out—update the code paths around init() and the loop that writes payloads
so they consistently use exporter.frame(...) (or remove exporter and adjust the
comment to indicate passthrough).
In `@rs/moq-mux/src/convert/cmaf.rs`:
- Around line 359-361: The non-keyframe sample flags in build_moof_mdat are
incorrect: replace the non-keyframe flag value currently set to 0x0001_0000 with
the standard pattern 0x0101_0000 so that both sample_depends_on and
sample_is_non_sync_sample are set; update the flags assignment in the
build_moof_mdat function (the local variable flags) to use 0x0101_0000 for
non-keyframes to match the JS encode.ts behavior.
- Around line 607-613: The sample_rate is being truncated by casting to u16 in
build_audio_codec (mp4_atom::Audio) — instead of casting config.sample_rate to
u16, pass the full 32-bit 16.16 fixed-point sample rate into
mp4_atom::FixedPoint so values like 88200/96000 are preserved; update the
FixedPoint construction in build_audio_codec (or add a small helper) to produce
a 16.16 value from config.sample_rate (e.g., use the crate's FixedPoint
constructor that accepts a u32/raw 16.16 value such as
FixedPoint::new_raw/FixedPoint::from_u32 if available, or split
config.sample_rate into integer and fractional parts and call
FixedPoint::new(high: u16, low: u16)), and remove the u16 cast of
config.sample_rate. Ensure the change is applied where
mp4_atom::Audio.sample_rate is set so codec matching remains correct.
---
Nitpick comments:
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 123-134: The function parse_timescale_from_init is duplicated;
extract it into a shared utility module (e.g., a common crate or rs/common
module) and replace the local copies with a single imported function; ensure the
extracted function keeps the signature parse_timescale_from_init(init: &[u8]) ->
anyhow::Result<u64> and uses the same mp4_atom::Any::decode_maybe logic and
trak.mdia.mdhd.timescale access, make it public (pub or pub(crate) as needed)
and update callers in rs/moq-cli/src/subscribe.rs and
rs/moq-mux/src/export/fmp4.rs to call the shared implementation.
In `@rs/moq-mux/src/export/fmp4.rs`:
- Line 145: The current payload assembly (payload: Vec<u8> =
frame.payload.clone().into_iter().flat_map(|c| c.into_iter()).collect()) does an
unnecessary clone+collect; instead iterate the frame.payload segments directly
when writing the mdat so you avoid the intermediate Vec. In the function that
encodes to mdat, replace the collect pattern by draining/iterating frame.payload
(or using the bytes::Buf API on each chunk) and write/extend each slice into the
mdat buffer/writer (e.g., call writer.write_all(chunk) or
extend_from_slice(chunk) for each segment) so you eliminate the clone and extra
allocation; look for symbols frame.payload and the mdat encoding block in
fmp4.rs to apply the change.
- Line 144: The calculation for dts (using frame.timestamp.as_micros() as u64 *
track.timescale / 1_000_000) can lose precision due to integer truncation and
might overflow for large values; change the computation to use a wider
intermediate (u128) and perform the multiply before dividing to preserve
precision, e.g., cast frame.timestamp.as_micros() and track.timescale to u128,
compute the product, then divide by 1_000_000 (optionally add half the divisor
for rounding) and finally cast back to u64; update the dts assignment where dts
is computed from frame.timestamp.as_micros() and track.timescale.
In `@rs/moq-mux/src/export/ordered.rs`:
- Line 343: Replace the string-based error return Error::Other("empty
group".into()) with a typed variant by adding Error::EmptyGroup to the Error
enum and using Error::EmptyGroup at the call site; update any
Display/From/FromStr/serialization impls and match arms that previously
inspected the "empty group" string to handle Error::EmptyGroup so callers can
pattern-match on the variant instead of brittle string comparisons, and
run/update tests that assert this error case accordingly.
In `@rs/moq-mux/src/import/catalog.rs`:
- Around line 100-120: The Drop impl for CatalogGuard currently ignores errors
from self.hang_track.append_group(), group.write_frame(), group.finish(), and
the msf path (to_msf/calling write_frame/finish); update CatalogGuard::drop to
record/log any failures instead of silently discarding them: check the Result
from append_group(), map Err to a call to the project's logger (or eprintln! if
no logger exists) with context ("hang_track.append_group" /
"msf_track.append_group"), and for each group operation check write_frame() and
finish() and log any Err with context including the catalog via
self.catalog.to_string() (or the msf string), ensuring all calls still proceed
safely within drop (no panics) and avoid returning errors.
In `@rs/moq-mux/src/import/fmp4.rs`:
- Around line 629-640: The code currently panics on tracks that start mid‑GOP by
calling .context("no keyframe at start") when contains_keyframe is false; change
this to tolerate non‑key start frames by not erroring out: when
contains_keyframe is false and track.group is None, skip or buffer the incoming
fragment_bytes instead of failing (i.e., do not call .context to unwrap), and
only create a group (via track.track.append_group()) and call write_frame() once
a keyframe is seen; keep using track.group.take(), track.track.append_group(),
track.group = Some(g), finish(), and write_frame() to manage groups but handle
the missing-keyframe case by dropping/buffering the frame and returning early so
malformed mid‑GOP starts do not error.
🪄 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: 672032e1-6642-470c-bc85-66be154b9f26
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
js/hang/src/catalog/container.tsjs/hang/src/container/cmaf/decode.tsjs/hang/src/container/cmaf/encode.tsjs/watch/src/audio/decoder.tsjs/watch/src/audio/mse.tsjs/watch/src/base64.tsjs/watch/src/msf.tsjs/watch/src/video/decoder.tsjs/watch/src/video/mse.tsrs/hang/src/catalog/container.rsrs/moq-cli/Cargo.tomlrs/moq-cli/src/subscribe.rsrs/moq-ffi/src/media.rsrs/moq-mux/src/container/hang.rsrs/moq-mux/src/convert/cmaf.rsrs/moq-mux/src/convert/hang.rsrs/moq-mux/src/export/fmp4.rsrs/moq-mux/src/export/ordered.rsrs/moq-mux/src/import/catalog.rsrs/moq-mux/src/import/fmp4.rsrs/moq-mux/src/import/test/mod.rs
💤 Files with no reviewable changes (1)
- js/hang/src/container/cmaf/decode.ts
✅ Files skipped from review due to trivial changes (3)
- rs/moq-cli/Cargo.toml
- rs/moq-mux/src/import/test/mod.rs
- rs/moq-mux/src/container/hang.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/hang/src/catalog/container.rs
Comment on lines
+15
to
20
| // CMAF container with base64-encoded init segment (ftyp+moov) | ||
| z.object({ | ||
| kind: z.literal("cmaf"), | ||
| // Time units per second | ||
| timescale: u53Schema, | ||
| // Track ID used in the moof/mdat fragments | ||
| trackId: u53Schema, | ||
| // Base64-encoded init segment (ftyp+moov) | ||
| init: z.base64(), | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does zod/mini support z.base64() schema type?
💡 Result:
No, zod/mini does not support a direct z.base64 schema type. In zod/mini (Zod 4's tree-shakable variant imported as "zod/mini"), string format validations like z.base64 are not available as top-level functions or methods on z.string. The official documentation lists available .check functions for strings (e.g., z.regex, z.minLength, z.includes), but z.base64 is absent. Regular Zod supports z.base64 and z.string.base64, but zod/mini uses a functional API without these specific format checks. Base64 validation can be implemented manually using z.regex with the base64 regex pattern or z.refine with a custom validator.
Citations:
- 1: https://v4.zod.dev/packages/mini?id=zodminitype
- 2: https://zod.dev/api
- 3: https://v4.zod.dev/packages/mini?id=check
- 4: https://v4.zod.dev/api?id=strings
- 5: https://v4.zod.dev/v4?id=introducing-zod-mini
- 6: https://v4.zod.dev/?id=strings
🏁 Script executed:
cat -n js/hang/src/catalog/container.ts | head -30Repository: moq-dev/moq
Length of output: 1066
🏁 Script executed:
head -10 js/hang/src/catalog/container.tsRepository: moq-dev/moq
Length of output: 475
Remove z.base64() or replace with manual base64 validation.
The code uses z.base64() at line 19, but this method does not exist in zod/mini. The zod/mini package (Zod 4's tree-shakable variant) does not include format-specific schema methods like base64.
Use z.string().regex(/^[A-Za-z0-9+/]*={0,2}$/) or z.string().refine() with a custom base64 validator instead.
Alternatively, switch the import to regular zod if z.base64() is preferred, but this defeats the purpose of using the lightweight zod/mini variant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/hang/src/catalog/container.ts` around lines 15 - 20, The schema for the
"cmaf" container uses z.base64(), which doesn't exist in zod/mini; replace
z.base64() on the init property in the object schema for the "cmaf" variant with
a string-based validator such as z.string().regex(/^[A-Za-z0-9+/]*={0,2}$/) or
z.string().refine(customBase64Validator) to validate base64, or alternatively
switch the import to full zod if you intentionally want z.base64(); update the
init field in the container schema accordingly.
kixelated
commented
May 1, 2026
| container: { kind: "cmaf", init: track.initData }, | ||
| // hang's CMAF path reads codec metadata from the init segment, so we | ||
| // don't need to surface a separate description here. | ||
| description: undefined, |
Collaborator
Author
There was a problem hiding this comment.
Can you verify that we correctly decode the description from the init segment when determining if the track is supported for playback?
Also, maybe we move description to a "legacy" container field? If it's not being used by CMAF at all.
|
|
||
| /// Error from the moq-mux consumer layer. | ||
| #[error("mux error: {0}")] | ||
| Mux(Arc<moq_mux::export::Error>), |
Collaborator
Author
There was a problem hiding this comment.
Why is this Arc? Can we make it Clone?
| let broadcast = consumer | ||
| .announced_broadcast(&name) | ||
| .await | ||
| .ok_or_else(|| anyhow::anyhow!("origin closed before broadcast was announced"))?; |
| hang = { workspace = true } | ||
| moq-mux = { workspace = true } | ||
| moq-native = { workspace = true, default-features = false, features = ["aws-lc-rs"] } | ||
| mp4-atom = "0.10.0" |
Collaborator
Author
There was a problem hiding this comment.
Can you make this a workspace dep.
…ecode decodeDataSegment only read defaults from tfhd, falling back to 0 when absent. Fragments produced by passthrough CMAF importers (gstreamer/ffmpeg via moq-mux) often store default sample flags only in moov/mvex/trex, so every sample was decoded as a sync sample. Parse trex during init segment decode and use it as the bottom of the trun -> tfhd -> trex fallback chain per ISO/IEC 14496-12 §8.8.7. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves merge conflicts with main, including: - Drop deprecated Audio/Video::create_track + remove_track helpers (kept the PR's intent of removing them as a semver break). - Remove the hang::container::OrderedConsumer convenience method, since the consumer now lives in moq_mux::export. - Migrate all subscribe_track / OrderedConsumer call sites to use the reverted moq-lite API (no Subscription parameter, TrackConsumer instead of TrackSubscriber). - Replace deprecated poll_next_group with poll_next_group_ordered.
Resolves the player-side conflicts from #1359 (unified Consumer across container formats) against this PR's catalog change (Container::Cmaf stores the base64 init segment instead of a raw timescale field). - Container.Cmaf.Format now takes a parsed InitSegment instead of a bare timescale, since the catalog no longer carries timescale separately. - audio/video decoders decode the init from config.container.init and pass the parsed init to Container.Cmaf.Format. - Kept main's Opus-CMAF special case (description omitted because raw Opus packets aren't OGG-wrapped) and the sample-duration validation in decodeDataSegment. - Updated consumer.test.ts to construct a synthetic InitSegment.
Delete the parallel export-side API (OrderedConsumer, ContainerFormat,
OrderedFrame, OrderedMuxer, export::Cmaf, export::Legacy,
export::container::{Error, CmafError}) and migrate every caller onto the
unified types in container/ and the renamed Consumer/Muxed in export/.
- export::Muxed (new): catalog-driven multi-track merger built on
BroadcastConsumer + CatalogConsumer, replacing the static
Vec<(name, OrderedConsumer)> shape so renditions can come and go.
- export::Fmp4::frame now takes &container::Frame and re-encodes as
moof+mdat. Subscribe.rs no longer pre-converts to CMAF — Consumer<Hang>
decodes both Legacy and CMAF, then Fmp4 produces the wire form.
- libmoq C ABI: moq_consume_frame_chunk(id, idx, dst) →
moq_consume_frame(id, dst). Single-shot read of the whole payload.
- BufList → Bytes everywhere (hang::container::Frame::payload, all import
codec producers). buf-list dependency dropped from hang and moq-mux.
- Doc-strings on lib.rs, container/, import/, export/, error variants,
Consumer (latency ordering), Hang (catalog-driven dispatch), Fmp4,
Muxed/MuxedFrame, both convert::Convert types, Aac/Opus importers.
Net -1414 lines. All ~770 workspace tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kixelated
changed the title
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/libmoq/src/consume.rs (1)
210-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the catalog container here instead of hardcoding
Hang::Legacy.Both ordered-consume helpers ignore
config.containerand always buildexport::Consumerwithmoq_mux::container::Hang::Legacy. After this PR, catalog tracks can be CMAF; those subscriptions will be decoded with the wrong container and will either fail or surface invalid frames. Pull the selected rendition's container from the catalog andtry_into()the actualmoq_mux::container::Hang, like the UniFFI path already does.Suggested shape
- let rendition = consume + let (rendition, config) = consume .catalog .video .renditions - .keys() + .iter() .nth(index) .ok_or(Error::NoIndex)?; + let media: moq_mux::container::Hang = (&config.container).try_into()?; let track = consume.broadcast.subscribe_track(&moq_lite::Track { - name: rendition.clone(), + name: rendition.clone(), priority: 1, // TODO: Remove priority })?; - let track = moq_mux::export::Consumer::new(track, moq_mux::container::Hang::Legacy).with_latency(latency); + let track = moq_mux::export::Consumer::new(track, media).with_latency(latency);Apply the same change to
audio_ordered.Also applies to: 254-267
🤖 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/libmoq/src/consume.rs` around lines 210 - 223, The code currently hardcodes moq_mux::container::Hang::Legacy when building export::Consumer (see the track creation using moq_mux::export::Consumer::new(..., moq_mux::container::Hang::Legacy)); instead, pull the selected rendition's container from the catalog entry (use the same catalog/consume/rendition values already computed), convert it to the proper moq_mux::container::Hang via try_into(), handle conversion errors, and pass that Hang value into Consumer::new; apply the identical change to the audio_ordered path (the analogous block around the other occurrence mentioned in the comment).rs/moq-gst/src/source/imp.rs (1)
450-465:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHonor the catalog container when constructing the consumer.
Both loops now force
Hang::Legacy, and the pump signatures bake that choice in. That breaks any rendition announced asContainer::Cmaf { .. }by the new import/export path, because the source will decode it with the wrong container instead of the one the catalog advertised.Please branch on
config.containerhere, or fail fast for unsupported containers before spawning the pump. Thespawn_track_pump/run_track_pumpsignatures will need to stop hardcodingConsumer<Hang>as part of that.Also applies to: 497-507
🤖 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/source/imp.rs` around lines 450 - 465, The code currently constructs moq_mux::export::Consumer::new(..., moq_mux::container::Hang::Legacy) unconditionally which overrides the catalog-declared container; update the consumer construction to branch on config.container (e.g., match config.container and use the matching moq_mux::container::Hang variant or return an error for unsupported containers) and propagate that choice into the pump by making spawn_track_pump and run_track_pump signatures generic over the consumer/container type (stop hardcoding Consumer<Hang>), or accept a Consumer without assuming Hang; apply the same change to the other occurrence where Hang::Legacy is used so the pump uses the catalog-advertised container or fails fast.
♻️ Duplicate comments (2)
rs/moq-mux/src/import/hev1.rs (1)
88-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelay tearing down the old track until
unique_track()succeeds.This reintroduces the earlier catalog/track sync bug in a new shape:
self.track.take()andcatalog.video.renditions.remove(...)happen before the fallibleself.broadcast.unique_track(".hev1")?. If that call errors, the old rendition is gone,self.trackstaysNone, and the next identical SPS can hit the early-return at Lines 82-85, so the decoder never recovers. Create the new track first, then swap/remove the old state only after success.🤖 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/hev1.rs` around lines 88 - 100, Do not remove the old track before ensuring a new track is successfully created: call self.broadcast.unique_track(".hev1") first and hold its result, and only after that succeeds remove the old rendition from catalog.video.renditions and replace self.track/self.config; avoid calling self.track.take() prior to unique_track so the old track remains intact if unique_track returns Err. Use the new track variable (from unique_track) to insert into catalog.video.renditions and then swap self.track = Some(new_track.into()) and self.config = Some(config).rs/moq-mux/src/import/fmp4.rs (1)
561-623:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
track_mdat_datastill assumes a contiguous sample layout.If a
trafcontains multipletruns with a gap or a freshdata_offset, this copies the entire span from the first run start to the last run end, whilecumulative_offsetis packed from sample sizes only. The rewrittentrun.data_offsets for the later runs will then point into the wrong bytes in the newmdat.Suggested direction
- let track_mdat_data = &mdat.data[track_data_start..track_data_end]; + let mut run_ranges = Vec::new(); + // Record each run's exact [start, end) while walking the truns. + // ... + + let mut track_mdat_data = Vec::new(); + for (start, end) in &run_ranges { + track_mdat_data.extend_from_slice(&mdat.data[*start..*end]); + } @@ - let per_track_mdat = Mdat { - data: track_mdat_data.to_vec(), - }; + let per_track_mdat = Mdat { data: track_mdat_data };🤖 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 561 - 623, The current code builds track_mdat_data as a single contiguous slice track_data_start..track_data_end which breaks when truns have gaps or explicit data_offset; instead, for each traf/trun use the parsed per-sample byte offsets and sizes (from trun.entries and whatever parsed sample-offset fields the parser stores) to extract each run's exact bytes from mdat.data and append them in trun order into a new Vec<u8> (update cumulative_offset as you append), then set per_track_mdat.data to that Vec; update the loops that compute trun_mut.data_offset to use the growing cumulative_offset that matches the concatenated bytes, and remove reliance on track_data_start..track_data_end/track_mdat_data to ensure trun.data_offset points to the correct bytes.
🧹 Nitpick comments (2)
js/hang/src/container/consumer.test.ts (1)
12-18: ⚡ Quick winAdd a regression test for
trexdefaults.All of the updated CMAF cases still decode segments with explicit sample metadata, and
TEST_INITsets everydefaultSample*field to0. That means the newtrun → tfhd → trexfallback path never runs here, so the bug this PR fixes can regress without tripping these tests.Also applies to: 74-124, 451-455
🤖 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/container/consumer.test.ts` around lines 12 - 18, The tests use TEST_INIT (type InitSegment) with defaultSampleDuration/defaultSampleSize/defaultSampleFlags all set to 0 which prevents exercising the trun → tfhd → trex fallback; update TEST_INIT (or add a new test fixture) so those defaultSample* fields are omitted or set to undefined (or to values that indicate "no default" per InitSegment) so the code will fall back to trex, and add a specific regression test that decodes a segment lacking per-sample metadata to assert the trex-path behavior; reference TEST_INIT, InitSegment, defaultSampleDuration, defaultSampleSize, defaultSampleFlags, and the trun/tfhd/trex fallback in the new test.rs/moq-mux/src/import/aac.rs (1)
149-168: 💤 Low valueConsider using
copy_to_bytesfor consistency withframe.rs.The manual chunk-draining loop in
decode(lines 153–158) is equivalent tobuf.copy_to_bytes(buf.remaining()), which is already used inhang/src/container/frame.rs. The same duplication appears inopus.rs.♻️ Proposed simplification (aac.rs and opus.rs)
- // Collect the input into a contiguous Bytes payload. - let mut payload = BytesMut::with_capacity(buf.remaining()); - while buf.has_remaining() { - let chunk = buf.chunk(); - payload.extend_from_slice(chunk); - let len = chunk.len(); - buf.advance(len); - } - - let frame = hang::container::Frame { - timestamp: pts, - payload: payload.freeze(), - }; + let frame = hang::container::Frame { + timestamp: pts, + payload: buf.copy_to_bytes(buf.remaining()), + };The import at line 2 would then revert to
use bytes::Buf;(dropBytesMut).🤖 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/aac.rs` around lines 149 - 168, In decode (fn decode<T: Buf>) replace the manual chunk-draining loop that builds a BytesMut with a single call to buf.copy_to_bytes(buf.remaining()) to produce the payload Bytes directly, update the imports to use bytes::Buf (remove BytesMut import), and mirror the same change in opus.rs so both modules use buf.copy_to_bytes(buf.remaining()) to create the hang::container::Frame.payload before calling self.track.write(frame)?.
🤖 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/hang/src/container/cmaf/decode.ts`:
- Around line 156-170: The code currently falls back to a default trackId (1)
and then uses that to find a trex, which can silently bind defaults from the
wrong track; update the logic in the function that computes trackId (and where
findBox is called) to validate that a real trackId was extracted and, if not,
fail fast by throwing an error (or returning an explicit failure) instead of
continuing; specifically, check the computed trackId before calling findBox/trex
and if it is the fallback sentinel (previously 1) or otherwise absent, throw an
Error indicating a missing/invalid track ID so
trex/defaultSampleDuration/defaultSampleSize/defaultSampleFlags cannot be
misapplied.
In `@js/watch/src/audio/mse.ts`:
- Around line 110-111: The CMAF init parsing (base64ToBytes and
Container.Cmaf.decodeInitSegment) can throw and must be guarded so a bad remote
init doesn't leave the SourceBuffer/subscription open; wrap the calls that
produce initSegment and init in a try/catch inside the MSE setup function,
catching errors from base64ToBytes and Container.Cmaf.decodeInitSegment, and on
error call the existing track-rejection/cleanup path (reject this track,
remove/unsubscribe the created SourceBuffer and subscription, and return/abort
further setup) instead of letting the exception bubble; locate the lines using
base64ToBytes and Container.Cmaf.decodeInitSegment to implement this safe-parse
and cleanup flow.
In `@rs/libmoq/src/api.rs`:
- Around line 529-535: Update the doc summary to reflect the new single-shot
API: replace "Get a chunk of a frame's payload" with a line stating that
moq_consume_frame() returns the entire contiguous payload in one call (e.g.,
"Get a frame's entire payload in a single call"). Mention the pointer lifetime
is valid until moq_consume_frame_close is called and ensure the rest of the doc
text (references to chunking / chunked contract) is removed or reworded to avoid
implying multiple chunks; update the function-level docs for moq_consume_frame()
and the related moq_consume_frame_close reference accordingly.
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 48-63: The exporter (moq_mux::export::Fmp4 instance named
exporter) is built once from the initial catalog via exporter.init() and is not
updated when the catalog changes, so exporter.frame() can operate on stale moov
metadata; fix by detecting catalog updates and rebuilding/re-emitting the init
segment: observe the catalog snapshot/version from the same source used by the
muxer (the hang::CatalogConsumer / muxer_catalog or the broadcast subscription),
and when it changes recreate the exporter with
moq_mux::export::Fmp4::new(&new_catalog), call exporter.init() and write/flush
the new init to stdout before calling exporter.frame() for subsequent
muxer.read() fragments; alternatively, if you prefer freezing behavior,
subscribe to the initial catalog snapshot only (use
broadcast.subscribe_track(&hang::Catalog::default_track()) to capture a frozen
catalog) and ensure muxer only yields frames for tracks present in that frozen
snapshot.
In `@rs/moq-mux/src/export/muxed.rs`:
- Around line 93-124: The selection logic currently picks the smallest timestamp
among tracks with pending frames but can emit out-of-order if any live track is
still pending; after the polling loop over self.tracks.values_mut() (which calls
track.consumer.poll_read(waiter) and sets track.pending or track.finished), add
a guard that if any track has track.pending.is_none() && !track.finished then
return Poll::Pending (do not proceed to the minimum timestamp selection). This
ensures every non-finished track is either buffered or known finished before
computing chosen (the chosen/chosen_ts logic) and emitting a MuxedFrame.
In `@rs/moq-mux/src/import/test/mod.rs`:
- Around line 10-12: The helper currently swallows all failures from
fmp4.decode(&mut buf) (letting every test pass); change this so decode errors
are handled explicitly: replace the blind ignore with a match on the Result from
fmp4.decode and either panic on unexpected errors (use expect/unwrap or return
Err) or assert the specific expected error variant for fixtures that
intentionally include a bad trailing fragment (e.g., compare to the parser's
Incomplete/TrailingFragment error variant). Alternatively, if the fixtures
shouldn't include invalid trailing data, trim buf to the valid prefix before
calling fmp4.decode. Refer to the fmp4.decode call and the test helper that
constructs buf (bytes::BytesMut::from(data)) to locate where to implement the
change.
---
Outside diff comments:
In `@rs/libmoq/src/consume.rs`:
- Around line 210-223: The code currently hardcodes
moq_mux::container::Hang::Legacy when building export::Consumer (see the track
creation using moq_mux::export::Consumer::new(...,
moq_mux::container::Hang::Legacy)); instead, pull the selected rendition's
container from the catalog entry (use the same catalog/consume/rendition values
already computed), convert it to the proper moq_mux::container::Hang via
try_into(), handle conversion errors, and pass that Hang value into
Consumer::new; apply the identical change to the audio_ordered path (the
analogous block around the other occurrence mentioned in the comment).
In `@rs/moq-gst/src/source/imp.rs`:
- Around line 450-465: The code currently constructs
moq_mux::export::Consumer::new(..., moq_mux::container::Hang::Legacy)
unconditionally which overrides the catalog-declared container; update the
consumer construction to branch on config.container (e.g., match
config.container and use the matching moq_mux::container::Hang variant or return
an error for unsupported containers) and propagate that choice into the pump by
making spawn_track_pump and run_track_pump signatures generic over the
consumer/container type (stop hardcoding Consumer<Hang>), or accept a Consumer
without assuming Hang; apply the same change to the other occurrence where
Hang::Legacy is used so the pump uses the catalog-advertised container or fails
fast.
---
Duplicate comments:
In `@rs/moq-mux/src/import/fmp4.rs`:
- Around line 561-623: The current code builds track_mdat_data as a single
contiguous slice track_data_start..track_data_end which breaks when truns have
gaps or explicit data_offset; instead, for each traf/trun use the parsed
per-sample byte offsets and sizes (from trun.entries and whatever parsed
sample-offset fields the parser stores) to extract each run's exact bytes from
mdat.data and append them in trun order into a new Vec<u8> (update
cumulative_offset as you append), then set per_track_mdat.data to that Vec;
update the loops that compute trun_mut.data_offset to use the growing
cumulative_offset that matches the concatenated bytes, and remove reliance on
track_data_start..track_data_end/track_mdat_data to ensure trun.data_offset
points to the correct bytes.
In `@rs/moq-mux/src/import/hev1.rs`:
- Around line 88-100: Do not remove the old track before ensuring a new track is
successfully created: call self.broadcast.unique_track(".hev1") first and hold
its result, and only after that succeeds remove the old rendition from
catalog.video.renditions and replace self.track/self.config; avoid calling
self.track.take() prior to unique_track so the old track remains intact if
unique_track returns Err. Use the new track variable (from unique_track) to
insert into catalog.video.renditions and then swap self.track =
Some(new_track.into()) and self.config = Some(config).
---
Nitpick comments:
In `@js/hang/src/container/consumer.test.ts`:
- Around line 12-18: The tests use TEST_INIT (type InitSegment) with
defaultSampleDuration/defaultSampleSize/defaultSampleFlags all set to 0 which
prevents exercising the trun → tfhd → trex fallback; update TEST_INIT (or add a
new test fixture) so those defaultSample* fields are omitted or set to undefined
(or to values that indicate "no default" per InitSegment) so the code will fall
back to trex, and add a specific regression test that decodes a segment lacking
per-sample metadata to assert the trex-path behavior; reference TEST_INIT,
InitSegment, defaultSampleDuration, defaultSampleSize, defaultSampleFlags, and
the trun/tfhd/trex fallback in the new test.
In `@rs/moq-mux/src/import/aac.rs`:
- Around line 149-168: In decode (fn decode<T: Buf>) replace the manual
chunk-draining loop that builds a BytesMut with a single call to
buf.copy_to_bytes(buf.remaining()) to produce the payload Bytes directly, update
the imports to use bytes::Buf (remove BytesMut import), and mirror the same
change in opus.rs so both modules use buf.copy_to_bytes(buf.remaining()) to
create the hang::container::Frame.payload before calling
self.track.write(frame)?.
🪄 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: 19c35f96-0d0c-40bc-a1c7-46057f12d40a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
js/hang/src/catalog/container.tsjs/hang/src/container/cmaf/decode.tsjs/hang/src/container/cmaf/encode.tsjs/hang/src/container/cmaf/format.tsjs/hang/src/container/consumer.test.tsjs/watch/src/audio/decoder.tsjs/watch/src/audio/mse.tsjs/watch/src/base64.tsjs/watch/src/msf.tsjs/watch/src/video/decoder.tsjs/watch/src/video/mse.tsrs/hang/Cargo.tomlrs/hang/examples/subscribe.rsrs/hang/examples/video.rsrs/hang/src/catalog/consumer.rsrs/hang/src/catalog/container.rsrs/hang/src/container/frame.rsrs/libmoq/src/api.rsrs/libmoq/src/consume.rsrs/libmoq/src/error.rsrs/libmoq/src/test.rsrs/moq-boy/src/main.rsrs/moq-cli/src/subscribe.rsrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/producer.rsrs/moq-gst/src/sink/imp.rsrs/moq-gst/src/source/imp.rsrs/moq-mux/Cargo.tomlrs/moq-mux/src/container/hang.rsrs/moq-mux/src/container/mod.rsrs/moq-mux/src/convert/cmaf.rsrs/moq-mux/src/convert/hang.rsrs/moq-mux/src/error.rsrs/moq-mux/src/export/consumer.rsrs/moq-mux/src/export/fmp4.rsrs/moq-mux/src/export/mod.rsrs/moq-mux/src/export/muxed.rsrs/moq-mux/src/import/aac.rsrs/moq-mux/src/import/av01.rsrs/moq-mux/src/import/avc3.rsrs/moq-mux/src/import/catalog.rsrs/moq-mux/src/import/fmp4.rsrs/moq-mux/src/import/hev1.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/opus.rsrs/moq-mux/src/import/test/mod.rsrs/moq-mux/src/lib.rs
✅ Files skipped from review due to trivial changes (4)
- js/watch/src/base64.ts
- rs/moq-mux/src/container/hang.rs
- js/watch/src/video/decoder.ts
- rs/moq-mux/src/import/catalog.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- js/watch/src/audio/decoder.ts
- rs/hang/src/catalog/consumer.rs
- js/watch/src/video/mse.ts
- rs/moq-boy/src/main.rs
- rs/hang/src/catalog/container.rs
- js/hang/src/catalog/container.ts
- rs/moq-gst/src/sink/imp.rs
- rs/libmoq/src/error.rs
- rs/moq-mux/src/export/consumer.rs
- js/hang/src/container/cmaf/encode.ts
- rs/moq-ffi/src/producer.rs
- rs/hang/Cargo.toml
- rs/moq-mux/src/lib.rs
- rs/moq-mux/src/export/fmp4.rs
- rs/moq-mux/src/convert/hang.rs
- rs/moq-mux/src/import/av01.rs
- rs/moq-mux/src/import/avc3.rs
- rs/moq-mux/src/convert/cmaf.rs
Comment on lines
+156
to
+170
| // Find moov > mvex > trex for this track to extract default sample values. | ||
| // These are the bottom of the fallback chain when tfhd/trun don't specify them. | ||
| const trex = findBox( | ||
| boxes, | ||
| (box): box is TrackExtendsBox & ParsedIsoBox => | ||
| box.type === "trex" && (box as TrackExtendsBox).trackId === trackId, | ||
| ); | ||
|
|
||
| return { | ||
| description, | ||
| timescale: mdhd.timescale, | ||
| trackId, | ||
| defaultSampleDuration: trex?.defaultSampleDuration ?? 0, | ||
| defaultSampleSize: trex?.defaultSampleSize ?? 0, | ||
| defaultSampleFlags: trex?.defaultSampleFlags ?? 0, |
Contributor
There was a problem hiding this comment.
Fail fast if the init segment has no real track ID.
This trex lookup is now correctness-critical, but trackId still falls back to 1 earlier in the function. On a malformed init segment that can silently bind another track’s defaults and mis-decode keyframes/timestamps instead of rejecting the segment.
Suggested fix
- const tkhd = findBox(boxes, isBoxType<any>("tkhd"));
- const trackId = tkhd?.trackId ?? 1;
+ const tkhd = findBox(boxes, isBoxType<any>("tkhd"));
+ if (tkhd?.trackId == null) {
+ throw new Error("No tkhd box found in init segment");
+ }
+ const trackId = tkhd.trackId;🤖 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/container/cmaf/decode.ts` around lines 156 - 170, The code
currently falls back to a default trackId (1) and then uses that to find a trex,
which can silently bind defaults from the wrong track; update the logic in the
function that computes trackId (and where findBox is called) to validate that a
real trackId was extracted and, if not, fail fast by throwing an error (or
returning an explicit failure) instead of continuing; specifically, check the
computed trackId before calling findBox/trex and if it is the fallback sentinel
(previously 1) or otherwise absent, throw an Error indicating a missing/invalid
track ID so trex/defaultSampleDuration/defaultSampleSize/defaultSampleFlags
cannot be misapplied.
Comment on lines
+110
to
+111
| const initSegment = base64ToBytes(config.container.init); | ||
| const init = Container.Cmaf.decodeInitSegment(initSegment); |
Contributor
There was a problem hiding this comment.
Guard invalid CMAF init data before continuing setup.
config.container.init is remote/catalog input, and both base64ToBytes(...) and decodeInitSegment(...) can throw. Right now that fails the whole MSE path after the SourceBuffer and subscription are already created, instead of just rejecting this track cleanly.
Suggested fix
- const initSegment = base64ToBytes(config.container.init);
- const init = Container.Cmaf.decodeInitSegment(initSegment);
+ let initSegment: Uint8Array;
+ let init: ReturnType<typeof Container.Cmaf.decodeInitSegment>;
+ try {
+ initSegment = base64ToBytes(config.container.init);
+ init = Container.Cmaf.decodeInitSegment(initSegment);
+ } catch (error) {
+ console.error("[MSE] Invalid CMAF init segment:", error);
+ return;
+ }📝 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.
Suggested change
| const initSegment = base64ToBytes(config.container.init); | |
| const init = Container.Cmaf.decodeInitSegment(initSegment); | |
| let initSegment: Uint8Array; | |
| let init: ReturnType<typeof Container.Cmaf.decodeInitSegment>; | |
| try { | |
| initSegment = base64ToBytes(config.container.init); | |
| init = Container.Cmaf.decodeInitSegment(initSegment); | |
| } catch (error) { | |
| console.error("[MSE] Invalid CMAF init segment:", error); | |
| return; | |
| } |
🤖 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/audio/mse.ts` around lines 110 - 111, The CMAF init parsing
(base64ToBytes and Container.Cmaf.decodeInitSegment) can throw and must be
guarded so a bad remote init doesn't leave the SourceBuffer/subscription open;
wrap the calls that produce initSegment and init in a try/catch inside the MSE
setup function, catching errors from base64ToBytes and
Container.Cmaf.decodeInitSegment, and on error call the existing
track-rejection/cleanup path (reject this track, remove/unsubscribe the created
SourceBuffer and subscription, and return/abort further setup) instead of
letting the exception bubble; locate the lines using base64ToBytes and
Container.Cmaf.decodeInitSegment to implement this safe-parse and cleanup flow.
Comment on lines
529
to
+535
| /// Get a chunk of a frame's payload. | ||
| /// | ||
| /// Frames may be split into multiple chunks. Call this multiple times with increasing | ||
| /// index values to get all chunks. The destination is filled with the frame chunk information. | ||
| /// Read the payload of a frame as a single contiguous slice. | ||
| /// | ||
| /// Frames are not chunked; the entire payload is delivered through `dst.payload` / | ||
| /// `dst.payload_size` in one call. The pointer is valid until [`moq_consume_frame_close`] | ||
| /// is called for this frame. |
Contributor
There was a problem hiding this comment.
Update the summary line to match the new single-shot API.
The first doc line still says "Get a chunk of a frame's payload", but moq_consume_frame() now returns the whole contiguous payload. Since this is public C ABI documentation, leaving the old wording behind is likely to send callers to the removed chunked contract.
Suggested doc fix
-/// Get a chunk of a frame's payload.
+/// Read a frame's payload as a single contiguous slice.🤖 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/libmoq/src/api.rs` around lines 529 - 535, Update the doc summary to
reflect the new single-shot API: replace "Get a chunk of a frame's payload" with
a line stating that moq_consume_frame() returns the entire contiguous payload in
one call (e.g., "Get a frame's entire payload in a single call"). Mention the
pointer lifetime is valid until moq_consume_frame_close is called and ensure the
rest of the doc text (references to chunking / chunked contract) is removed or
reworded to avoid implying multiple chunks; update the function-level docs for
moq_consume_frame() and the related moq_consume_frame_close reference
accordingly.
Comment on lines
+10
to
+12
| let mut buf = bytes::BytesMut::from(data); | ||
| // Ignore errors from incomplete/malformed trailing fragments in test files. | ||
| let _ = fmp4.decode(&mut buf); |
Contributor
There was a problem hiding this comment.
Don't swallow all decode() failures in the shared helper.
Right now every test in this module passes as soon as catalog/init extraction succeeds, even if fragment rewriting later regresses. If these fixtures intentionally end with a bad trailing fragment, please assert that specific error or trim the input to the valid prefix instead of discarding every decode() failure.
🤖 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 10 - 12, The helper currently
swallows all failures from fmp4.decode(&mut buf) (letting every test pass);
change this so decode errors are handled explicitly: replace the blind ignore
with a match on the Result from fmp4.decode and either panic on unexpected
errors (use expect/unwrap or return Err) or assert the specific expected error
variant for fixtures that intentionally include a bad trailing fragment (e.g.,
compare to the parser's Incomplete/TrailingFragment error variant).
Alternatively, if the fixtures shouldn't include invalid trailing data, trim buf
to the valid prefix before calling fmp4.decode. Refer to the fmp4.decode call
and the test helper that constructs buf (bytes::BytesMut::from(data)) to locate
where to implement the change.
Move CatalogConsumer/CatalogProducer/OrderedProducer out of the hang crate into moq-mux so hang is reduced to wire-format/struct definitions: - hang::CatalogConsumer -> moq_mux::catalog::Consumer - moq_mux::import::CatalogProducer -> moq_mux::catalog::Producer - hang::container::OrderedProducer -> folded into moq_mux::container::Producer Also drop the dead generic Producer<C: Container> shape and rebuild container::Producer around what import codecs actually need: keyframe-driven group boundaries, with a new public finish_group() so callers can flush a group early without waiting for the next keyframe. Audio codecs (AAC, Opus) mark every Nth frame as a keyframe and call finish_group() to release groups to the relay immediately. Hang is now effectively just struct/wire-format definitions; conducer and tracing dependencies are no longer needed there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Subscribers no longer have to subscribe the catalog twice or thread a CatalogConsumer through Muxed. Fmp4::new(BroadcastConsumer) subscribes the catalog internally, builds the merged init segment from the first snapshot, and next().await yields the init bytes (once) followed by moof+mdat fragments in timestamp order across tracks. Also delete the convert/ module: it had no production callers (only its own roundtrip test) and shipping 1442 lines of half-baked Legacy↔CMAF conversion is the kind of dead code we don't want to maintain. Same behavior is reachable by piping import → export. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per PR review: the canonical user-facing API is read(). poll_read is only used internally (by Fmp4 to poll multiple consumers from a single waiter), so hide it from external callers to avoid mistakenly calling the wrong method. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Matches the pattern used elsewhere in moq-mux (catalog::Consumer::poll_next, Fmp4::poll_next) and moq-lite (poll_recv_group, poll_read_frame, etc.). Public poll_xxx APIs are how the conducer crate exposes non-async integration; keep this one consistent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The deprecated poll_next_group_ordered method was removed from TrackConsumer in moq-lite #1378, replaced by the renamed poll_next_group. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Backports the moq-mux structural refactor from
devtomain, then collapses the dual API surface that grew during the merge into a single canonical one.Backport (earlier commits in this PR)
moq_mux::import/moq_mux::export/moq_mux::container/moq_mux::convert.Container::Cmaf { init: Bytes }(init segment in the catalog) replacing the oldtimescale/track_idshape.Decoder→Framed,DecoderFormat→FramedFormat.convert::cmaf::Convertandconvert::hang::Convertfor in-process container rewriting.hangcatalog API:Audio::insert/Video::insert/remove,OrderedProducer,Container::Cmaf { init }schema.Cleanup (final commit,
5e6d5a3)The merge left two parallel APIs in
moq_mux::export. Collapse onto the unified ones and remove the old surface entirely:OrderedConsumer<F: ContainerFormat>Consumer<F: Container>ContainerFormattraitcontainer::ContainertraitOrderedFrame(BufListpayload)container::Frame(Bytespayload)OrderedMuxer<F>(staticVec<(name, OrderedConsumer)>)Muxed(catalog-driven, handles track changes)export::Cmaf { timescale }container::Cmaf { trak }export::Legacycontainer::Hang::Legacyexport::container::{Error, CmafError}crate::Error,container::cmaf::ErrorOther breaking changes:
export::Fmp4::framenow takes&container::Frameand re-encodes as moof+mdat. With this,subscribe.rsno longer needsconvert::cmaf::Convertfor fMP4 output —Consumer<Hang>decodes both Legacy and CMAF, thenFmp4writes the wire form.moq_consume_frame_chunk(id, idx, dst)→moq_consume_frame(id, dst). Single-shot read of the whole payload (frames are no longer chunked).hang::container::Frame::payloadis nowBytes; all import codec producers (aac,avc1,avc3,av01,hev1,opus) build payloads asBytesMut→freeze(). Thebuf-listdependency is dropped fromhangandmoq-mux.lib.rs, module-levelcontainer/,import/,export/,Containertrait +Frame(esp. keyframe semantics per format),Hang(catalog-driven dispatch),Consumer(latency-bounded presentation order),Fmp4,Muxed/MuxedFrame, bothconvert::Converttypes,Errorvariants, and theAac/Opusimporters.Cleanup is net −1414 lines.
Test plan
cargo test --workspace— all ~770 tests passjust check— clean (clippy, cargo-shear, formatting, sort)cargo build --workspaceconvert::cmaf::test::*andconvert::hang::test::*Legacy↔CMAF round-trip tests still cover both directionslibmoqtest suite (14 tests) updated for newmoq_consume_frameABImoq-cli subscribe --output fmp4end-to-end (Legacy and CMAF inputs)🤖 Generated with Claude Code