Backport moq-mux to main (adapted to main's moq-net, no wire/API breaks)#1918
Merged
Conversation
moq-mux is a library crate; per the workspace convention (thiserror for
libraries, anyhow for binaries) it shouldn't be exporting anyhow types
in its public API.
Each codec, container, and catalog format module now exports its own
Error enum (codec::{annexb,aac,opus,h264,h265,av1}::Error,
container::{fmp4,mkv,hls}::Error, catalog::msf::Error), flattened into
crate::Error via #[from]. Pure parsing functions (Config::parse,
Sps::parse, Avcc::parse, Avc1/Hvc1::transform, etc.) return their
module-local Result; importer/exporter structs return crate::Result so
they can mix codec errors with moq_net::Error and the other leaf
errors that flow through track operations.
Downstream:
- libmoq::Error::{InitFailed,DecodeFailed} now wrap Arc<moq_mux::Error>
instead of Arc<anyhow::Error>; added BufferNotConsumed for the one
ad-hoc anyhow!() site that didn't fit either bucket.
- moq-cli's PublishDecoder bridges moq_mux::Result into anyhow::Result
with explicit `?` since the binary still uses anyhow.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a small composable layer for narrowing a broadcast's catalog before handing it to an exporter, plus single-rendition H.264 / H.265 Annex-B exporters that emit raw elementary streams for piping into ffmpeg or similar tools. The catalog crate gains a public `Stream` trait, a unified `Consumer` (promoted from the internal `CatalogSource`), and two wrappers: - `Filter<S>` hard-matches on rendition name and codec family. - `Target<S>` soft-matches on max width/height/pixels/bitrate and reduces each axis to one rendition (Rust port of js/watch's selection algorithm). Both wrappers cache the last input snapshot and re-emit on `set_*`, which is the seam future bandwidth-driven ABR will drive. `fmp4::Export` and `mkv::Export` are now generic over `S: catalog::Stream`; the legacy `with_catalog_format` constructor is gone since callers can build the stream themselves. The new `codec::h264::Export` / `codec::h265::Export` subscribe to a single rendition via `ExportSource::for_video_raw` (no avc1/hvc1 shape transform) and emit Annex-B bytes. avc3/hev1 sources pass through; avc1 / hvc1 sources are converted via `annexb::from_length_prefixed` with VPS/SPS/PPS injected at every keyframe from the avcC/hvcC. Timestamps are dropped (Annex-B has no container framing). The CLI gains `H264` / `H265` formats plus video/audio filter and target flags, wired through the `Filter` then `Target` chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#1834) Adds draft-ietf-moq-msf-01 support while staying backwards-compatible with draft-00. The wire format is an internal detail: `moq_msf::Catalog` is a version-agnostic snapshot of tracks, and (de)serialization hides the catalog `version` (number in draft-00, "draft-XX" string in draft-01) and the init-data indirection (draft-01's root `initDataList` + per-track `initRef`). - Decode accepts draft-00 and draft-01, resolves `initRef` into inline `Track::init_data`, and defaults a missing `isLive` to false. - Encode always emits draft-01, hoisting inline init data into a deduplicated `initDataList` with `initRef` pointers. - moq-mux producer/consumer keep using inline init data; @moq/msf mirrors the same encode/decode abstraction. Full delta-update application remains out of scope; the snapshot API can absorb it later without a breaking change.
… per-codec splitters, and make importers pure frame publishers (#1749) Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Luke Curley <luke.curley@discordapp.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The merged Jitter mixed a 90 kHz reorder Timestamp with Timestamp::ZERO (seconds scale). Timestamp::max asserts equal scales, so combined() panicked with "mismatched timestamp scales" on the second frame of any stream (it runs on every observe via report), surfacing as libmoq's video_raw_decode test where moq_publish_media_frame returned -16. Keep the min-frame-duration and max-reorder accumulators as std::time::Duration (the catalog field type), converting each Timestamp at the boundary so the comparisons are scale-free. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVjsvr8bQTua4FPymRwWEL
Contributor
WalkthroughThe PR updates MSF catalog handling to draft-01 wire semantics and makes the catalog snapshot shape version-agnostic. It adds catalog stream, filter, target, and track abstractions, updates producer and consumer types to use generic catalog extensions and demand handles, and propagates those types into downstream wrappers. It also adds typed codec parsing, splitters, and codec importers, then rewires fMP4, MKV, FLV, HLS, and TS container paths plus CLI and GStreamer entry points to the new frame, timestamp, and catalog APIs. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
rs/libmoq/src/error.rs (1)
208-218: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a unique FFI return code for
BufferNotConsumed.Line 208 assigns
-30, butError::Audio(_)already returns-30on Line 218. C/FFI callers cannot distinguish these failures.Proposed fix
- Error::BufferNotConsumed => -30, + Error::BufferNotConsumed => -12,🤖 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/error.rs` around lines 208 - 218, The FFI error-code mapping in Error::to_i32 has a duplicate return value for Error::BufferNotConsumed and Error::Audio(_), so update the match in error.rs to give BufferNotConsumed its own unique negative code and keep Audio(_) on a different value. Make sure the surrounding enum-to-code mapping remains one-to-one for all Error variants so C/FFI callers can distinguish failures reliably.rs/moq-mux/src/container/fmp4/export.rs (1)
215-224: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not emit the init segment before at least one track is subscribed.
With an empty catalog snapshot,
init_ready()is vacuously true becauseself.tracks.values().all(...)succeeds on an empty map. That emits a trackless init segment and setsinit_emitted, so later catalog updates can subscribe media without a matching init. Add the same non-empty guard used by the MKV exporter.🐛 Proposed fix
fn init_ready(&self) -> bool { - self.catalog_snapshot.is_some() && self.tracks.values().all(|t| t.source.header_ready()) + self.catalog_snapshot.is_some() + && !self.tracks.is_empty() + && self.tracks.values().all(|t| t.source.header_ready()) }🤖 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/container/fmp4/export.rs` around lines 215 - 224, The init-segment emission path in `FragmentExporter::poll_next` can fire with an empty track catalog because `init_ready()` is true on an empty `self.tracks` map, causing a trackless init to be marked emitted too early. Update the guard around the `build_init()` call to require at least one subscribed track before emitting, matching the non-empty check used by the MKV exporter, and keep the `init_emitted` flag set only after that condition is satisfied.rs/moq-mux/src/codec/h265/mod.rs (1)
285-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject one-byte HEVC NAL units before classifying them.
Line 289 reads only
nal[0], so a one-byte malformed NAL can be accepted as a VPS/SPS/PPS or emitted with a length prefix even though the HEVC NAL header is two bytes. ReturnNalTooShortfornal.len() < 2.Proposed fix
if nal.is_empty() { return Ok(false); } +if nal.len() < 2 { + return Err(Error::NalTooShort); +} // HEVC NAL header is 2 bytes; type is bits 1..=6 of byte 0. match NALUnitType::from((nal[0] >> 1) & 0x3f) {🤖 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/codec/h265/mod.rs` around lines 285 - 303, The HEVC NAL parsing in the NAL classification helper should reject malformed one-byte inputs before reading the header. In the function that matches on NALUnitType and writes VPS/SPS/PPS or length-prefixed data, add an early length check for nal.len() < 2 and return NalTooShort so nal[0] is never accessed on invalid HEVC NAL units. Keep the existing classification and push_distinct behavior unchanged for valid two-byte-or-longer NALs.rs/moq-mux/src/container/flv/import.rs (1)
210-218: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winActually drop malformed VP9 keyframes after logging.
On
config_from_keyframeerror, the code logs but still falls through towrite_video, so an already-initialized VP9 stream can publish the malformed keyframe. Return after the warning.Proposed fix
- Err(err) => tracing::warn!(%err, "dropping malformed VP9 key frame"), + Err(err) => { + tracing::warn!(%err, "dropping malformed VP9 key frame"); + return Ok(()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/container/flv/import.rs` around lines 210 - 218, The VP9 keyframe handling in import.rs logs malformed keyframe errors in the config_from_keyframe match but still falls through to write_video, which can publish bad data. In the vp9/keyframe branch inside the import flow, make the Err(err) path return immediately after tracing::warn!, so malformed VP9 keyframes are dropped after logging while preserving the existing Ok(Some(config)) and Ok(None) behavior.rs/moq-mux/src/container/fmp4/import.rs (1)
528-543: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve non-contiguous sample runs when rewriting
data_offset.
track_mdat_datacopies one contiguous[track_data_start..track_data_end]span, but the rewrittentrun.data_offsetvalues pack runs back-to-back usingcumulative_offset. If originaltrun.data_offsets are non-contiguous or interleaved with other track data, the second run points into gap/foreign bytes instead of its samples.Also applies to: 564-583
🤖 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/container/fmp4/import.rs` around lines 528 - 543, The `import.rs` fragment rewriting in the `track_mdat_data`/`cumulative_offset` flow is assuming each track’s samples live in one contiguous span, but `trun.data_offset` rewriting can represent multiple non-contiguous runs. Update the logic around the sample range collection and offset rewrite so `track_mdat_data` preserves each run in order instead of copying a single `[track_data_start..track_data_end]` slice; use the existing `track_data_start`, `offset`, and `trun.data_offset` handling to build per-run data and rewrite offsets relative to each preserved run. Apply the same fix in the later reuse of this logic in the 564-583 section so interleaved or gapped sample layouts remain valid.
🟡 Minor comments (7)
rs/moq-mux/src/catalog/msf/mod.rs-72-72 (1)
72-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the public
Resultalias.
Result<T>is part of the public MSF catalog API but has no rustdoc.Proposed fix
+/// A result type for MSF catalog decoding operations. pub type Result<T> = std::result::Result<T, Error>;As per coding guidelines,
**/*.{rs,js,ts,tsx}: Document every exported/public symbol in Rust and JS/TS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/catalog/msf/mod.rs` at line 72, The public `Result<T>` alias in `msf::catalog::mod` is missing rustdoc and should be documented to satisfy the public API guidelines. Add a concise doc comment directly above the `Result` type alias in `mod.rs` describing that it is the MSF catalog result type and what `Error` it uses, keeping the wording consistent with nearby public items in this module.Source: Coding guidelines
rs/moq-mux/src/codec/h264/export.rs-154-156 (1)
154-156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t classify avc1 solely by
descriptionpresence.If an H.264 catalog marks the stream as out-of-band (
inline: false) but the avcCdescriptionis missing/empty, this branch treats the length-prefixed payload as Annex-B passthrough. Use the H.264 inline flag to distinguish avc3 from avc1, and return the existing missing-parameter-set error when avc1 lacks avcC.🤖 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/codec/h264/export.rs` around lines 154 - 156, The H.264 export path in export_h264 currently decides avc1 vs avc3 only from config.description, which misclassifies out-of-band streams when description is empty. Update the match around config.description to also use the H.264 inline flag from the codec/config object so avc3 is treated as Annex-B passthrough only when inline is true. For avc1, keep using the existing missing-parameter-set error path when avcC/description is absent or empty, and ensure the convert logic in export_h264 preserves the current avcc parsing behavior otherwise.rs/moq-ffi/src/session.rs-276-287 (1)
276-287: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the side-specific origin docs.
publisher()only returns theset_publishorigin or a publish-side auto-origin;consumer()returns theset_consumeorigin’s consumer view or a consume-side auto-origin. The current text mentionsset_consumefor the publisher and says “if neither was set,” which can mislead FFI users. As per coding guidelines, “Document public APIs with clear docstrings or comments.”Suggested doc fix
- /// The publish-side origin: where local broadcasts get advertised - /// to the remote. Either the producer the caller wired via - /// `set_publish` / `set_consume` before connect/accept, or one - /// auto-created if neither was set. + /// The publish-side origin: where local broadcasts get advertised + /// to the remote. Either the producer the caller wired via + /// `set_publish` before connect/accept, or one auto-created if + /// `set_publish` was not set. ... - /// origin the caller wired via `set_consume`, or auto-created if - /// neither was set. + /// origin the caller wired via `set_consume`, or auto-created if + /// `set_consume` was not set.🤖 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-ffi/src/session.rs` around lines 276 - 287, Update the public docs on session-side origin accessors in the `Session` impl so they match the actual behavior: `publisher()` should describe only the `set_publish` origin or a publish-side auto-created origin, and `consumer()` should describe the `set_consume` origin’s consumer view or a consume-side auto-created origin. Fix the misleading cross-reference in the `publisher()` comment and replace the vague “if neither was set” wording with side-specific language, keeping the docs aligned with the `publisher` and `consumer` methods for FFI users.Source: Coding guidelines
rs/hang/src/catalog/video/codec.rs-35-42 (1)
35-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the public enum variants.
VideoCodecKindis public and each variant is part of the exported API; add brief rustdoc for the codec-family variants to match the surroundingVideoCodecstyle. As per coding guidelines, “Document every exported/public symbol in Rust and JS/TS.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/hang/src/catalog/video/codec.rs` around lines 35 - 42, `VideoCodecKind` is a public exported enum, but its variants are undocumented; add brief rustdoc comments for each codec-family variant so it matches the surrounding `VideoCodec` style and satisfies the public API documentation requirement. Update the enum definition itself and document the variants `H264`, `H265`, `VP8`, `VP9`, `AV1`, and `Unknown` with concise descriptions.Source: Coding guidelines
rs/moq-mux/src/codec/vp8/mod.rs-17-29 (1)
17-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the public VP8 error variants.
These variants are part of the public parsing error surface; add short rustdoc comments for each one. As per coding guidelines, “Document every exported/public symbol in Rust and JS/TS.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/codec/vp8/mod.rs` around lines 17 - 29, The public VP8 parsing error enum is missing documentation on each exported variant. Add short rustdoc comments to every variant in Error so the public error surface is documented, using the existing Error enum and its FrameTooShort, KeyframeHeaderTooShort, StartCodeMismatch, and EmptyFrame variants as the targets.Source: Coding guidelines
rs/moq-mux/src/codec/av1/mod.rs-188-196 (1)
188-196: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRound-trip the AV1 tier field too.
av1c_from_av1writesseq_tier_0, but this test does not assertback.tier; anHtier catalog can silently come back as the default tier. Add the assertion and mapseq_tier_0inav1_from_av1c.Proposed fix
pub(crate) fn av1_from_av1c(av1c: &mp4_atom::Av1c) -> AV1 { AV1 { profile: av1c.seq_profile, level: av1c.seq_level_idx_0, + tier: if av1c.seq_tier_0 { 'H' } else { 'M' }, bitdepth: bitdepth(av1c.twelve_bit, av1c.high_bitdepth), mono_chrome: av1c.monochrome, chroma_subsampling_x: av1c.chroma_subsampling_x, chroma_subsampling_y: av1c.chroma_subsampling_y, @@ assert_eq!(back.profile, av1.profile); assert_eq!(back.level, av1.level); + assert_eq!(back.tier, av1.tier); assert_eq!(back.bitdepth, av1.bitdepth);🤖 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/codec/av1/mod.rs` around lines 188 - 196, The AV1 round-trip test in av1_from_av1c/av1c_from_av1 is missing coverage for the tier field, so an H-tier value can be lost and fall back to the default. Update av1_from_av1c to read and map seq_tier_0 back into the AV1 tier field, and extend the existing round-trip assertion block to verify back.tier matches av1.tier alongside the other fields.rs/moq-mux/src/container/flv/export_test.rs-253-256 (1)
253-256: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the sequence-header check in the enhanced Opus assertion.
The coded Opus frame also has
AUDIO_FORMAT_EXand"Opus", so this can pass even if the exported sequence header is missing.Proposed fix
assert!( tags.iter().any(|t| t.tag_type == super::TAG_AUDIO + && t.body.len() >= 5 && (t.body[0] >> 4) == super::AUDIO_FORMAT_EX + && (t.body[0] & 0x0f) == super::AUDIO_PACKET_SEQUENCE_START && &t.body[1..5] == b"Opus"), "expected an enhanced Opus audio tag" );🤖 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/container/flv/export_test.rs` around lines 253 - 256, The enhanced Opus assertion in export_test needs to continue checking for the sequence header, because matching TAG_AUDIO, AUDIO_FORMAT_EX, and "Opus" alone can also match coded Opus frames. Update the assertion in the relevant test to include the existing sequence-header condition used elsewhere in the FLV export tests, so it explicitly verifies the exported Opus sequence header is present rather than only a coded audio frame.
🧹 Nitpick comments (11)
rs/moq-mux/src/codec/h264/export.rs (1)
76-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public polling APIs.
nextandpoll_nextare exported methods but currently have no rustdoc. As per coding guidelines, “Document every exported/public symbol in Rust and JS/TS.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/codec/h264/export.rs` around lines 76 - 80, Add rustdoc to the exported H264 export polling APIs: document both `next` and `poll_next` in `codec/h264/export.rs` so their behavior, return values, and waiter/polling contract are clear. Place the docs directly above the `next` and `poll_next` methods and keep the wording aligned with the public `Export`-style async polling flow used in this module.Source: Coding guidelines
rs/moq-ffi/src/test.rs (1)
658-663: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the auto publisher reaches the remote session.
publisher.announce(...)only proves the local origin accepted the broadcast; it doesn’t prove the auto-created publish side was wired into the session. Subscribe onserver_session.consumer()and assert the"local-only"announcement arrives. As per coding guidelines, “Write unit tests for critical functionality.”Suggested test strengthening
- // The auto publisher is wired too: dropping it should not break anything, - // and a local announce() on it should succeed (though the server - // isn't consuming, so we only verify the call doesn't error). + // The auto publisher is wired too: verify the server session receives + // a client-side announcement through its subscribe-side origin. + let server_consumer = server_session.consumer(); + let server_announced = server_consumer.announced("".into()).unwrap(); let local_broadcast = MoqBroadcastProducer::new().unwrap(); publisher.announce("local-only".into(), &local_broadcast).unwrap(); + let local = tokio::time::timeout(TIMEOUT, server_announced.next()) + .await + .expect("timed out waiting for local auto-publish announcement") + .unwrap() + .expect("expected local auto-publish announcement"); + assert_eq!(local.path(), "local-only"); local_broadcast.finish().unwrap();🤖 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-ffi/src/test.rs` around lines 658 - 663, The current test around MoqBroadcastProducer::new and publisher.announce only checks the local call path, not that the auto publisher is actually connected to the remote session. Strengthen the test by subscribing through server_session.consumer(), then announce "local-only" from the publisher and assert the announcement is observed on the remote side before finishing the local_broadcast. Keep the existing local sanity check, but add the session-level verification in the same test so it covers the wired publish path.Source: Coding guidelines
rs/moq-mux/src/container/hls/mod.rs (1)
45-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse transparent wrappers for foreign errors.
These
#[from]variants wrap foreign errors, so they should use#[error(transparent)]instead of adding a prefixed display string. As per coding guidelines, “wrap foreign errors with#[error(transparent)]+#[from].”♻️ Proposed fix
- #[error("url parse: {0}")] + #[error(transparent)] UrlParse(#[from] url::ParseError), - #[error("reqwest: {0}")] + #[error(transparent)] Reqwest(#[from] reqwest::Error), - #[error("io: {0}")] + #[error(transparent)] Io(#[from] std::io::Error),#!/bin/bash # Verify foreign-error wrappers in the HLS error enum use transparent thiserror wrappers. rg -n -C2 '#\[error\("(url parse|reqwest|io): \{0\}"\)\]|#\[error\(transparent\)\]' rs/moq-mux/src/container/hls/mod.rs🤖 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/container/hls/mod.rs` around lines 45 - 52, Update the HLS error enum in the `Error` type so the foreign-error wrapper variants `UrlParse`, `Reqwest`, and `Io` use `#[error(transparent)]` together with `#[from]` instead of prefixed display strings. Keep the variant names and `From` conversions intact, and apply the change consistently to each of these `thiserror` variants in `container::hls::mod` so they follow the foreign-error wrapping convention.rs/moq-mux/src/catalog/stream.rs (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for
poll_next.This is a public trait method, but only the surrounding trait prose documents it. Add item-level docs so generated API docs stay complete. As per coding guidelines, “Document every exported/public symbol in Rust and JS/TS.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/catalog/stream.rs` at line 30, Add item-level rustdoc for the public trait method poll_next on the Stream trait so the API docs include it directly, not just via the surrounding trait prose. Update the method’s documentation in stream.rs with a brief description of what poll_next does and what the waiter parameter and Poll<crate::Result<Option<Catalog<Self::Ext>>>> return represent, keeping the docs attached to the poll_next symbol.Source: Coding guidelines
rs/moq-cli/src/subscribe.rs (1)
9-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new public CLI enums and variants.
SubscribeFormatlacks type-level docs, and several public enum variants are undocumented. Either make these adapters crate-private or add rustdoc so the public API surface is complete. As per coding guidelines, “Document every exported/public symbol in Rust and JS/TS.”Also applies to: 36-44, 58-63
🤖 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-cli/src/subscribe.rs` around lines 9 - 17, The public CLI enums in SubscribeFormat (and the other exported enums/variants in this module) are missing rustdoc, leaving the public API incomplete. Add type-level documentation for each public enum and short docs for every public variant, or make these adapters crate-private if they are not meant to be part of the API. Ensure the docs are added on the enum definitions and variant declarations so the exported surface is fully documented.Source: Coding guidelines
rs/moq-mux/src/codec/h264/mod.rs (1)
276-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public
spsandppsfields.
AvccParamSets::spsandAvccParamSets::ppsare public API but have no rustdoc. As per coding guidelines,**/*.{rs,js,ts,tsx}: Document every exported/public symbol in Rust and JS/TS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/codec/h264/mod.rs` around lines 276 - 283, The public fields AvccParamSets::sps and AvccParamSets::pps are exported API but lack rustdoc, so add concise documentation comments describing what each field contains. Update the AvccParamSets struct in h264::mod so all public symbols, including length_size, sps, and pps, are documented consistently.Source: Coding guidelines
rs/moq-mux/src/container/loc/mod.rs (1)
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the timestamp comment with the actual frame metadata.
FrameInfois constructed with onlysize, so this does not carry the timestamp on the net frame for relay-side ordering. Either attach timestamp metadata if the API supports it, or remove/reword the comment.🤖 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/container/loc/mod.rs` around lines 31 - 35, The comment near moq_mux::container::loc::create_frame is out of sync with the actual FrameInfo metadata, since only size is being set and no timestamp is attached for relay-side ordering. Update the code in the LOC frame creation path to either pass timestamp metadata through moq_net::FrameInfo if that API supports it, or remove/reword the comment so it matches what create_frame actually does.rs/moq-mux/src/catalog/tracks.rs (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd module-level docs.
This new Rust module has public track handle types but no
//!module documentation.As per coding guidelines, "
**/*.{rs,js,ts,tsx}: Document every exported/public symbol in Rust and JS/TS, and add module-level docs to each Rust module root".Proposed fix
+//! Catalog track handles that publish and retire media renditions. + use super::Producer;🤖 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/catalog/tracks.rs` around lines 1 - 2, Add module-level documentation to the `tracks` module by inserting a `//!` doc comment at the top of the file, before the `use` statements. Make sure the docs briefly describe what `tracks.rs` provides and reference the public track handle types/exported symbols in this module so the module root is documented per the Rust guidelines.Source: Coding guidelines
rs/moq-mux/src/catalog/filter.rs (1)
63-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public constructor.
Filter::newis part of the public API but lacks rustdoc.As per coding guidelines, "
**/*.{rs,js,ts,tsx}: Document every exported/public symbol in Rust and JS/TS".Proposed fix
impl<S: Stream> Filter<S> { + /// Wrap `inner` with a hard-match rendition filter. pub fn new(inner: S) -> Self {🤖 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/catalog/filter.rs` around lines 63 - 75, `Filter::new` is a public constructor in `Filter<S>` but is missing rustdoc. Add a concise doc comment directly above `pub fn new(inner: S) -> Self` describing what the constructor does and any relevant expectations for `inner`, matching the documentation style used for other public symbols in `filter.rs`.Source: Coding guidelines
rs/moq-mux/src/codec/h265/export.rs (1)
67-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for the public polling methods.
nextandpoll_nextare public API methods but are undocumented.As per coding guidelines, "
**/*.{rs,js,ts,tsx}: Document every exported/public symbol in Rust and JS/TS".Proposed fix
+ /// Wait for and return the next Annex-B chunk, or `None` at end of stream. pub async fn next(&mut self) -> crate::Result<Option<Bytes>> { kio::wait(|waiter| self.poll_next(waiter)).await } + /// Poll for the next Annex-B chunk. pub fn poll_next(&mut self, waiter: &kio::Waiter) -> Poll<crate::Result<Option<Bytes>>> {🤖 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/codec/h265/export.rs` around lines 67 - 71, The public API methods next and poll_next in the H265 export codec are missing rustdoc. Add concise documentation comments for both exported methods, describing what each returns and how they should be used, following the existing Rust docs style used in this module so these public symbols are documented per the coding guidelines.Source: Coding guidelines
rs/moq-mux/src/catalog/target.rs (1)
23-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public target fields and constructor.
TargetVideo,TargetAudio, andTarget::newexpose public API surface that currently lacks item-level rustdoc.As per coding guidelines, "
**/*.{rs,js,ts,tsx}: Document every exported/public symbol in Rust and JS/TS".Proposed fix
pub struct TargetVideo { + /// Maximum preferred coded width. pub width: Option<u32>, + /// Maximum preferred coded height. pub height: Option<u32>, + /// Maximum preferred coded pixel area. pub pixels: Option<u32>, + /// Maximum preferred video bitrate. pub bitrate: Option<u64>, } /// Soft-match constraints for the audio rendition. #[derive(Debug, Default, Clone)] pub struct TargetAudio { + /// Maximum preferred audio bitrate. pub bitrate: Option<u64>, } @@ impl<S: Stream> Target<S> { + /// Wrap `inner` with a soft-match rendition target. pub fn new(inner: S) -> Self {Also applies to: 68-80
🤖 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/catalog/target.rs` around lines 23 - 34, Add item-level rustdoc for the public API in TargetVideo, TargetAudio, and Target::new: document each exported struct and each public field on TargetVideo/TargetAudio, plus the constructor’s purpose and parameters. Keep the docs concise but explicit about what the soft-match target fields mean so the public surface in target.rs is fully documented.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/msf/src/catalog.ts`:
- Around line 129-131: The decode path in catalog.ts is silently accepting
unresolved initRef values by assigning undefined to track.initData in decode(),
which lets an invalid catalog pass through with missing initialization data.
Update the decode logic around track.initData and initRef to validate that the
referenced entry exists in initDataList and resolves to an inline value before
assigning it; if it is missing or non-inline, throw during decoding instead of
returning a partially populated catalog.
In `@rs/moq-gst/src/sink/imp.rs`:
- Around line 83-86: The PTS anchoring is currently kept per-pad in
PadState::reference_pts, which causes each track to reset independently and lose
the initial offset between audio and video. Move the timeline reference to
RuntimeState, or otherwise derive timestamps from a shared GStreamer
running-time base in Sink::handle_buffer/track processing so all pads use the
same origin. Update the PadState and the code that initializes and consumes
reference_pts so the first observed timestamp is shared across pads instead of
tracked separately.
- Around line 287-325: The sink session currently uses an unbounded mpsc channel
in create/start logic and forwards buffers immediately in forward_buffer, which
can let queued media grow without limit if the worker slows down. Update the
SessionHandle/control-path around run_session, stop_session, and forward_buffer
to use a bounded queue or explicit drop/backpressure behavior, and make
send/reporting reflect queue pressure instead of always returning success.
In `@rs/moq-mux/src/catalog/consumer.rs`:
- Line 25: `Consumer::new` in the consumer catalog module is unnecessarily async
even though it does not await anything, which makes `Subscribe::stream` and
other synchronous call sites incompatible. Remove `async` from `Consumer::new`,
keep it as a plain constructor returning `Result<Self, crate::Error>`, and
update any call sites that were changed to use `.await` so they invoke it
synchronously (notably the `Subscribe::stream` path).
In `@rs/moq-mux/src/catalog/filter.rs`:
- Around line 137-142: The Poll::Pending branch in the stream handling logic is
not terminating after the retained final snapshot has been emitted because
inner_eof remains true while last_input is still Some. Update the state
management around the filter stream (in the code that checks inner_eof and
self.last_input) so that once the final snapshot has been yielded, last_input is
cleared or otherwise marked consumed, allowing the next poll to return EOF
instead of staying pending forever.
In `@rs/moq-mux/src/catalog/target.rs`:
- Around line 152-157: The EOF handling in the target polling logic leaves
`last_input` set after emitting the retained final snapshot, so `Poll::Pending`
can repeat forever once upstream ends. Update the `Poll::Pending` branch in
`Target::poll` to detect that the final retained snapshot has already been
emitted after `inner_eof`, and then clear or otherwise mark `last_input` so the
next poll returns `Ready(Ok(None))` instead of staying pending; use the existing
`inner_eof` and `last_input` state in `Target` to gate this transition.
In `@rs/moq-mux/src/codec/aac/mod.rs`:
- Around line 169-180: The explicit sample-rate path in sample_rate_from_index
is reading three full bytes even though the 24-bit rate begins in the remaining
bits already consumed from b1/b_ext, so it must be changed to preserve and read
across the bit boundary. Update this parsing flow to use a bit-aware reader or
pass the leftover bits into the helper, and make sure the extracted sample_rate
stays aligned so Config::parse matches Config::encode for non-standard rates.
Verify the fix against the sample_rate_from_index and Config round-trip path,
including channel_count parsing.
In `@rs/moq-mux/src/codec/av1/import.rs`:
- Around line 63-66: The av1C handling in import::CodecConfig::parse is gated
too tightly by the current length check before calling init_from_av1c, which
causes valid short av1C records to skip configuration parsing and later fail
with MissingSequenceHeader. Relax the size gate so any buffer that has the av1C
marker byte can be passed into init_from_av1c, and let that helper validate the
actual fields it needs; keep the raw-OBU fallback only for non-av1C data.
In `@rs/moq-mux/src/codec/av1/split.rs`:
- Around line 161-167: The reset path in split::reset only clears current and
leaves the buffered tail bytes behind, which can contaminate the next temporal
unit after a partial OBU. Update reset to clear both current and tail in the
Split state so any pre-reset buffered bytes are discarded along with the
in-flight unit.
In `@rs/moq-mux/src/codec/h264/import.rs`:
- Around line 74-76: In initialize_avc1, the avc1 mode flag is being set before
Avcc::parse validates the input, which can leave the importer in the wrong state
if parsing fails. Move the self.avc1 = true mutation to after
super::Avcc::parse(avcc_bytes)? succeeds, so the importer only switches modes
once avcC parsing has completed successfully.
In `@rs/moq-mux/src/codec/h264/mod.rs`:
- Around line 190-201: The Avcc parsing path is silently accepting truncated SPS
data and returning missing dimensions instead of failing. Update the Avcc::parse
logic in the h264 codec module to reuse parse_avcc_param_sets for the
parameter-set parsing, so when num_sps > 0 and the declared SPS length exceeds
the available avcc buffer the parse returns an error rather than Ok with None
dimensions. Keep the fix localized around the Avcc::parse / parameter-set
extraction flow and ensure malformed avcC records are rejected consistently.
In `@rs/moq-mux/src/codec/h264/split.rs`:
- Around line 140-155: The IDR handling in split.rs currently reconciles SPS/PPS
and marks the current frame as containing an IDR without first ending any
already-open slice-bearing access unit. Update the Avc3NalType::IdrSlice branch
in the split logic to close the existing frame when a bare IDR begins a new
access unit, then start a fresh frame before applying reconcile_keyframe_params
and setting contains_idr/contains_slice so consecutive IDRs cannot be merged
into one frame.
In `@rs/moq-mux/src/codec/h265/split.rs`:
- Around line 147-174: The keyframe handling in the H265 splitter does not start
a new access unit, so a bare IDR/BLA/CRA slice can be appended to the previous
frame in the streaming decode path. Update the keyframe arm in `decode_nal` (the
`NALUnitType::IdrWRadl` / `IdrNLp` / `Bla*` / `CraNut` match) to gate the same
boundary logic used by the non-keyframe slice path: honor
`first_slice_segment_in_pic_flag`, call `maybe_start_frame` when beginning a new
picture, and only run `reconcile_keyframe_params` for the first slice of the
keyframe. Also check the H264 splitter’s analogous keyframe branch for the
expected pattern and mirror that behavior here.
In `@rs/moq-mux/src/container/consumer.rs`:
- Around line 227-236: The `poll_read` error branch in `Consumer` is treating
every `F::Error` as an evicted group and skipping forward, which can hide
malformed payload/decode failures. Update the `Poll::Ready(Err(e))` handling to
only advance past known group-abort/old-cache cases (the eviction-style errors),
and for any other error from `poll_read` propagate it instead of popping
`pending` and bumping `current`. Use the `Consumer` state fields (`pending`,
`current`) and the existing `tracing::warn!` path to keep the skip behavior
limited to actual eviction conditions.
In `@rs/moq-mux/src/container/fmp4/export.rs`:
- Line 327: The `default_frame` calculation in `export.rs` can panic when
`config.framerate` is zero, NaN, or infinite because `Duration::from_secs_f64`
receives an invalid value. Update the `default_frame` logic to validate the
framerate first, using a finite positive value only, and fall back to the
default frame duration when `config.framerate` is missing or invalid. Use the
`config.framerate` and `Duration::from_secs_f64` code path to locate the fix.
In `@rs/moq-mux/src/container/fmp4/import.rs`:
- Around line 480-486: The PTS and sample range math in import handling uses
unchecked arithmetic and can wrap on malformed fragments. In the fmp4 import
flow around the `pts`/`timestamp` calculation and the sample offset/duration
updates, switch to checked/saturating operations so negative composition offsets
do not become huge `u64` timestamps and `offset + size` / `dts += duration`
cannot overflow. Use the existing import logic in `import.rs` (around the `pts`
computation and the loop that advances `dts`) to return an error when arithmetic
fails instead of producing corrupted sample ranges.
In `@rs/moq-mux/src/container/fmp4/mod.rs`:
- Around line 320-330: The timestamp handling in the fMP4 sample builder drops
durations for frames when Frame.duration is None, which causes decode() to keep
DTS unchanged across multiple samples. Update the TrunEntry construction in the
frames mapping so non-last samples can infer a duration from the next frame’s
timestamp (or otherwise derive the missing interval), and only leave duration
absent for cases that can truly remain byte-identical; if a multi-sample
fragment cannot determine durations, reject it explicitly rather than encoding
zero-duration entries.
---
Outside diff comments:
In `@rs/libmoq/src/error.rs`:
- Around line 208-218: The FFI error-code mapping in Error::to_i32 has a
duplicate return value for Error::BufferNotConsumed and Error::Audio(_), so
update the match in error.rs to give BufferNotConsumed its own unique negative
code and keep Audio(_) on a different value. Make sure the surrounding
enum-to-code mapping remains one-to-one for all Error variants so C/FFI callers
can distinguish failures reliably.
In `@rs/moq-mux/src/codec/h265/mod.rs`:
- Around line 285-303: The HEVC NAL parsing in the NAL classification helper
should reject malformed one-byte inputs before reading the header. In the
function that matches on NALUnitType and writes VPS/SPS/PPS or length-prefixed
data, add an early length check for nal.len() < 2 and return NalTooShort so
nal[0] is never accessed on invalid HEVC NAL units. Keep the existing
classification and push_distinct behavior unchanged for valid two-byte-or-longer
NALs.
In `@rs/moq-mux/src/container/flv/import.rs`:
- Around line 210-218: The VP9 keyframe handling in import.rs logs malformed
keyframe errors in the config_from_keyframe match but still falls through to
write_video, which can publish bad data. In the vp9/keyframe branch inside the
import flow, make the Err(err) path return immediately after tracing::warn!, so
malformed VP9 keyframes are dropped after logging while preserving the existing
Ok(Some(config)) and Ok(None) behavior.
In `@rs/moq-mux/src/container/fmp4/export.rs`:
- Around line 215-224: The init-segment emission path in
`FragmentExporter::poll_next` can fire with an empty track catalog because
`init_ready()` is true on an empty `self.tracks` map, causing a trackless init
to be marked emitted too early. Update the guard around the `build_init()` call
to require at least one subscribed track before emitting, matching the non-empty
check used by the MKV exporter, and keep the `init_emitted` flag set only after
that condition is satisfied.
In `@rs/moq-mux/src/container/fmp4/import.rs`:
- Around line 528-543: The `import.rs` fragment rewriting in the
`track_mdat_data`/`cumulative_offset` flow is assuming each track’s samples live
in one contiguous span, but `trun.data_offset` rewriting can represent multiple
non-contiguous runs. Update the logic around the sample range collection and
offset rewrite so `track_mdat_data` preserves each run in order instead of
copying a single `[track_data_start..track_data_end]` slice; use the existing
`track_data_start`, `offset`, and `trun.data_offset` handling to build per-run
data and rewrite offsets relative to each preserved run. Apply the same fix in
the later reuse of this logic in the 564-583 section so interleaved or gapped
sample layouts remain valid.
---
Minor comments:
In `@rs/hang/src/catalog/video/codec.rs`:
- Around line 35-42: `VideoCodecKind` is a public exported enum, but its
variants are undocumented; add brief rustdoc comments for each codec-family
variant so it matches the surrounding `VideoCodec` style and satisfies the
public API documentation requirement. Update the enum definition itself and
document the variants `H264`, `H265`, `VP8`, `VP9`, `AV1`, and `Unknown` with
concise descriptions.
In `@rs/moq-ffi/src/session.rs`:
- Around line 276-287: Update the public docs on session-side origin accessors
in the `Session` impl so they match the actual behavior: `publisher()` should
describe only the `set_publish` origin or a publish-side auto-created origin,
and `consumer()` should describe the `set_consume` origin’s consumer view or a
consume-side auto-created origin. Fix the misleading cross-reference in the
`publisher()` comment and replace the vague “if neither was set” wording with
side-specific language, keeping the docs aligned with the `publisher` and
`consumer` methods for FFI users.
In `@rs/moq-mux/src/catalog/msf/mod.rs`:
- Line 72: The public `Result<T>` alias in `msf::catalog::mod` is missing
rustdoc and should be documented to satisfy the public API guidelines. Add a
concise doc comment directly above the `Result` type alias in `mod.rs`
describing that it is the MSF catalog result type and what `Error` it uses,
keeping the wording consistent with nearby public items in this module.
In `@rs/moq-mux/src/codec/av1/mod.rs`:
- Around line 188-196: The AV1 round-trip test in av1_from_av1c/av1c_from_av1 is
missing coverage for the tier field, so an H-tier value can be lost and fall
back to the default. Update av1_from_av1c to read and map seq_tier_0 back into
the AV1 tier field, and extend the existing round-trip assertion block to verify
back.tier matches av1.tier alongside the other fields.
In `@rs/moq-mux/src/codec/h264/export.rs`:
- Around line 154-156: The H.264 export path in export_h264 currently decides
avc1 vs avc3 only from config.description, which misclassifies out-of-band
streams when description is empty. Update the match around config.description to
also use the H.264 inline flag from the codec/config object so avc3 is treated
as Annex-B passthrough only when inline is true. For avc1, keep using the
existing missing-parameter-set error path when avcC/description is absent or
empty, and ensure the convert logic in export_h264 preserves the current avcc
parsing behavior otherwise.
In `@rs/moq-mux/src/codec/vp8/mod.rs`:
- Around line 17-29: The public VP8 parsing error enum is missing documentation
on each exported variant. Add short rustdoc comments to every variant in Error
so the public error surface is documented, using the existing Error enum and its
FrameTooShort, KeyframeHeaderTooShort, StartCodeMismatch, and EmptyFrame
variants as the targets.
In `@rs/moq-mux/src/container/flv/export_test.rs`:
- Around line 253-256: The enhanced Opus assertion in export_test needs to
continue checking for the sequence header, because matching TAG_AUDIO,
AUDIO_FORMAT_EX, and "Opus" alone can also match coded Opus frames. Update the
assertion in the relevant test to include the existing sequence-header condition
used elsewhere in the FLV export tests, so it explicitly verifies the exported
Opus sequence header is present rather than only a coded audio frame.
---
Nitpick comments:
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 9-17: The public CLI enums in SubscribeFormat (and the other
exported enums/variants in this module) are missing rustdoc, leaving the public
API incomplete. Add type-level documentation for each public enum and short docs
for every public variant, or make these adapters crate-private if they are not
meant to be part of the API. Ensure the docs are added on the enum definitions
and variant declarations so the exported surface is fully documented.
In `@rs/moq-ffi/src/test.rs`:
- Around line 658-663: The current test around MoqBroadcastProducer::new and
publisher.announce only checks the local call path, not that the auto publisher
is actually connected to the remote session. Strengthen the test by subscribing
through server_session.consumer(), then announce "local-only" from the publisher
and assert the announcement is observed on the remote side before finishing the
local_broadcast. Keep the existing local sanity check, but add the session-level
verification in the same test so it covers the wired publish path.
In `@rs/moq-mux/src/catalog/filter.rs`:
- Around line 63-75: `Filter::new` is a public constructor in `Filter<S>` but is
missing rustdoc. Add a concise doc comment directly above `pub fn new(inner: S)
-> Self` describing what the constructor does and any relevant expectations for
`inner`, matching the documentation style used for other public symbols in
`filter.rs`.
In `@rs/moq-mux/src/catalog/stream.rs`:
- Line 30: Add item-level rustdoc for the public trait method poll_next on the
Stream trait so the API docs include it directly, not just via the surrounding
trait prose. Update the method’s documentation in stream.rs with a brief
description of what poll_next does and what the waiter parameter and
Poll<crate::Result<Option<Catalog<Self::Ext>>>> return represent, keeping the
docs attached to the poll_next symbol.
In `@rs/moq-mux/src/catalog/target.rs`:
- Around line 23-34: Add item-level rustdoc for the public API in TargetVideo,
TargetAudio, and Target::new: document each exported struct and each public
field on TargetVideo/TargetAudio, plus the constructor’s purpose and parameters.
Keep the docs concise but explicit about what the soft-match target fields mean
so the public surface in target.rs is fully documented.
In `@rs/moq-mux/src/catalog/tracks.rs`:
- Around line 1-2: Add module-level documentation to the `tracks` module by
inserting a `//!` doc comment at the top of the file, before the `use`
statements. Make sure the docs briefly describe what `tracks.rs` provides and
reference the public track handle types/exported symbols in this module so the
module root is documented per the Rust guidelines.
In `@rs/moq-mux/src/codec/h264/export.rs`:
- Around line 76-80: Add rustdoc to the exported H264 export polling APIs:
document both `next` and `poll_next` in `codec/h264/export.rs` so their
behavior, return values, and waiter/polling contract are clear. Place the docs
directly above the `next` and `poll_next` methods and keep the wording aligned
with the public `Export`-style async polling flow used in this module.
In `@rs/moq-mux/src/codec/h264/mod.rs`:
- Around line 276-283: The public fields AvccParamSets::sps and
AvccParamSets::pps are exported API but lack rustdoc, so add concise
documentation comments describing what each field contains. Update the
AvccParamSets struct in h264::mod so all public symbols, including length_size,
sps, and pps, are documented consistently.
In `@rs/moq-mux/src/codec/h265/export.rs`:
- Around line 67-71: The public API methods next and poll_next in the H265
export codec are missing rustdoc. Add concise documentation comments for both
exported methods, describing what each returns and how they should be used,
following the existing Rust docs style used in this module so these public
symbols are documented per the coding guidelines.
In `@rs/moq-mux/src/container/hls/mod.rs`:
- Around line 45-52: Update the HLS error enum in the `Error` type so the
foreign-error wrapper variants `UrlParse`, `Reqwest`, and `Io` use
`#[error(transparent)]` together with `#[from]` instead of prefixed display
strings. Keep the variant names and `From` conversions intact, and apply the
change consistently to each of these `thiserror` variants in
`container::hls::mod` so they follow the foreign-error wrapping convention.
In `@rs/moq-mux/src/container/loc/mod.rs`:
- Around line 31-35: The comment near moq_mux::container::loc::create_frame is
out of sync with the actual FrameInfo metadata, since only size is being set and
no timestamp is attached for relay-side ordering. Update the code in the LOC
frame creation path to either pass timestamp metadata through moq_net::FrameInfo
if that API supports it, or remove/reword the comment so it matches what
create_frame actually does.
🪄 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: ca11f818-815d-4db7-b1de-627be4ba7a19
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (109)
doc/concept/standard/msf.mddoc/lib/rs/crate/moq-mux.mdjs/msf/src/catalog.test.tsjs/msf/src/catalog.tsjs/msf/tsconfig.jsonkt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/Flows.ktpy/moq-rs/moq/__init__.pypy/moq-rs/moq/publish.pypy/moq-rs/tests/test_local.pyrs/hang/src/catalog/audio/codec.rsrs/hang/src/catalog/video/codec.rsrs/libmoq/src/api.rsrs/libmoq/src/error.rsrs/libmoq/src/publish.rsrs/libmoq/src/session.rsrs/moq-audio/src/producer.rsrs/moq-boy/src/main.rsrs/moq-boy/src/video.rsrs/moq-cli/src/publish.rsrs/moq-cli/src/subscribe.rsrs/moq-ffi/src/producer.rsrs/moq-ffi/src/session.rsrs/moq-ffi/src/test.rsrs/moq-gst/src/sink/imp.rsrs/moq-msf/CHANGELOG.mdrs/moq-msf/README.mdrs/moq-msf/src/lib.rsrs/moq-mux/CHANGELOG.mdrs/moq-mux/Cargo.tomlrs/moq-mux/src/catalog/consumer.rsrs/moq-mux/src/catalog/filter.rsrs/moq-mux/src/catalog/format.rsrs/moq-mux/src/catalog/hang/consumer.rsrs/moq-mux/src/catalog/hang/ext.rsrs/moq-mux/src/catalog/mod.rsrs/moq-mux/src/catalog/msf/consumer.rsrs/moq-mux/src/catalog/msf/mod.rsrs/moq-mux/src/catalog/producer.rsrs/moq-mux/src/catalog/stream.rsrs/moq-mux/src/catalog/target.rsrs/moq-mux/src/catalog/tracks.rsrs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/codec/aac/mod.rsrs/moq-mux/src/codec/ac3.rsrs/moq-mux/src/codec/annexb.rsrs/moq-mux/src/codec/av1/import.rsrs/moq-mux/src/codec/av1/mod.rsrs/moq-mux/src/codec/av1/split.rsrs/moq-mux/src/codec/eac3.rsrs/moq-mux/src/codec/h264/export.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h264/mod.rsrs/moq-mux/src/codec/h264/split.rsrs/moq-mux/src/codec/h265/export.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/codec/h265/mod.rsrs/moq-mux/src/codec/h265/split.rsrs/moq-mux/src/codec/legacy.rsrs/moq-mux/src/codec/mp2.rsrs/moq-mux/src/codec/opus/import.rsrs/moq-mux/src/codec/opus/mod.rsrs/moq-mux/src/codec/vp8/import.rsrs/moq-mux/src/codec/vp8/mod.rsrs/moq-mux/src/codec/vp9/import.rsrs/moq-mux/src/codec/vp9/mod.rsrs/moq-mux/src/container/consumer.rsrs/moq-mux/src/container/flv/export.rsrs/moq-mux/src/container/flv/export_test.rsrs/moq-mux/src/container/flv/import.rsrs/moq-mux/src/container/flv/import_test.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/fmp4/export_test.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/fmp4/import_test.rsrs/moq-mux/src/container/fmp4/mod.rsrs/moq-mux/src/container/hls/import.rsrs/moq-mux/src/container/hls/mod.rsrs/moq-mux/src/container/jitter.rsrs/moq-mux/src/container/legacy/mod.rsrs/moq-mux/src/container/loc/mod.rsrs/moq-mux/src/container/mkv/export.rsrs/moq-mux/src/container/mkv/export_test.rsrs/moq-mux/src/container/mkv/import.rsrs/moq-mux/src/container/mkv/import_test.rsrs/moq-mux/src/container/mkv/mod.rsrs/moq-mux/src/container/mod.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/container/source.rsrs/moq-mux/src/container/ts/catalog.rsrs/moq-mux/src/container/ts/export.rsrs/moq-mux/src/container/ts/export_test.rsrs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/container/ts/import_test.rsrs/moq-mux/src/error.rsrs/moq-mux/src/import.rsrs/moq-mux/src/import/container.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/track.rsrs/moq-mux/src/lib.rsrs/moq-mux/src/track_provider.rsrs/moq-native/src/reconnect.rsrs/moq-net/src/model/broadcast.rsrs/moq-net/src/model/frame.rsrs/moq-net/src/model/group.rsrs/moq-net/src/model/track.rsrs/moq-rtc/src/codec/h264.rsrs/moq-rtc/src/codec/opus.rsrs/moq-srt/src/ts.rsrs/moq-video/src/encode/producer.rs
…tore typed Avcc/Hvcc, fix moq-video
…, Frame.duration, container::Timestamp)
…re codex moq-boy
…am/container importers) to new moq-mux; drop extracted HLS
…make moq-audio generic over catalog ext to preserve #1886 FFI sections
…dev-ffi-surface backport to main
kixelated
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/container/producer.rs (1)
110-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude explicit frame durations in the latency span.
The flush trigger only compares timestamp extrema and ignores
Frame.duration, so a single frame or batch whosetimestamp + durationexceedslatencycan remain buffered until another timestamp/keyframe/finish arrives.Proposed fix
- if self.buffer.len() >= 2 { - let mut iter = self.buffer.iter().map(|f| std::time::Duration::from(f.timestamp)); - let first = iter.next().unwrap(); - let (min, max) = iter.fold((first, first), |(min, max), d| (min.min(d), max.max(d))); - if max.saturating_sub(min) >= self.latency { + if !self.buffer.is_empty() { + let mut iter = self.buffer.iter().map(|f| { + let start = std::time::Duration::from(f.timestamp); + let end = f + .duration + .map(std::time::Duration::from) + .and_then(|duration| start.checked_add(duration)) + .unwrap_or(start); + (start, end) + }); + let (first_start, first_end) = iter.next().unwrap(); + let (min_start, max_end) = iter.fold((first_start, first_end), |(min, max), (start, end)| { + (min.min(start), max.max(end)) + }); + if max_end.saturating_sub(min_start) >= self.latency { self.flush(None)?; } }🤖 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/container/producer.rs` around lines 110 - 115, The flush condition in producer logic only considers timestamp extrema and misses each frame’s duration, so update the buffering check in Producer::flush-trigger path to account for `Frame.duration` alongside `timestamp`. In the `self.buffer` span calculation, derive the latest end time as `timestamp + duration` for each frame and compare that against the earliest start time, then trigger `self.flush(None)?` when that full span meets or exceeds `self.latency`.rs/moq-mux/src/container/flv/export_test.rs (1)
252-257: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the Opus packet-type check.
This assertion now passes for any enhanced Opus tag, so the test no longer fails if the exporter omits the required sequence-start header and emits only coded frames.
Suggested fix
assert!( tags.iter().any(|t| t.tag_type == super::TAG_AUDIO && (t.body[0] >> 4) == super::AUDIO_FORMAT_EX + && (t.body[0] & 0x0f) == super::AUDIO_PACKET_SEQUENCE_START && &t.body[1..5] == b"Opus"), "expected an enhanced Opus audio tag" );🤖 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/container/flv/export_test.rs` around lines 252 - 257, Restore the Opus packet-type validation in the enhanced audio assertion inside export_test’s Opus tag check, so the test specifically requires the sequence-start header rather than any enhanced Opus frame. Update the existing tags.iter().any predicate to include the packet type field used by the exporter’s Opus payload layout, keeping the checks tied to TAG_AUDIO, AUDIO_FORMAT_EX, and the "Opus" signature while rejecting coded-frame-only tags.
🧹 Nitpick comments (5)
rs/moq-mux/src/codec/h264/mod.rs (1)
168-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public
Avccprofile fields.These public fields are now part of the exported API surface but lack field-level rustdoc. As per coding guidelines, “Document every exported/public symbol in Rust.”
🤖 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/codec/h264/mod.rs` around lines 168 - 170, The public Avcc profile fields currently lack rustdoc, so add field-level documentation for profile, constraints, and level in the Avcc struct to satisfy the exported API documentation guideline. Update the Avcc definition in the h264 codec module with concise comments describing each field’s meaning and keep the docs aligned with the existing public API symbols.Source: Coding guidelines
rs/moq-mux/src/codec/h265/mod.rs (1)
81-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd field docs for the public parameter-set vectors.
vps,sps, andppsare public API fields; add concise rustdoc describing that each vector contains complete out-of-band HEVC NAL units of that type. As per coding guidelines, “Document every exported/public symbol in Rust.”🤖 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/codec/h265/mod.rs` around lines 81 - 83, The public fields vps, sps, and pps in the H265 parameter-set struct are undocumented exported API and need rustdoc. Add concise documentation on each field explaining that the vector holds complete out-of-band HEVC NAL units of that type, and keep the comments attached to the struct fields so the public API is properly documented.Source: Coding guidelines
rs/moq-cli/src/subscribe.rs (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for
SubscribeFormat.This enum is public and changed in this PR, but it still has no rustdoc. As per coding guidelines, "Document every exported/public symbol in Rust".
🤖 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-cli/src/subscribe.rs` around lines 8 - 14, Add rustdoc for the public SubscribeFormat enum and its variants so every exported symbol is documented. Update the enum definition in SubscribeFormat with a concise summary that explains what subscription formats it represents, and add brief variant-level docs for Fmp4, Mkv, Ts, and Flv so the public API follows the Rust documentation guideline.Source: Coding guidelines
rs/moq-ffi/src/producer.rs (1)
427-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for
MoqTrackProducer::finish.Line 427 exposes a public method without a doc comment, so this one falls outside the repo’s required Rust API documentation coverage. As per coding guidelines, "Document every exported/public symbol in Rust and JS/TS".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-ffi/src/producer.rs` around lines 427 - 432, Add rustdoc for the public M oqTrackProducer::finish method in producer.rs to satisfy the repo’s API documentation rule for exported Rust symbols. Document what finish does, that it closes/completes the track producer, and mention the possible MoqError::Closed outcome if the producer was already taken or finished; keep the comment directly above finish so it is picked up by rustdoc.Source: Coding guidelines
rs/libmoq/src/publish.rs (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the changed
PublishAPI surface.
Publishand several changed public entrypoints here (create,get,close,media_ordered,media_frame,media_close) still ship without rustdoc, which makes this FFI-facing surface harder to consume and breaks the repo’s public-API doc rule. As per coding guidelines, "Document every exported/public symbol in Rust and JS/TS".Also applies to: 31-61, 63-105
🤖 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/publish.rs` around lines 16 - 18, The public Publish API is missing rustdoc, including the struct itself and the exported entrypoints create, get, close, media_ordered, media_frame, and media_close, so add concise documentation for each symbol. Update the Publish type and its public methods in publish.rs with rustdoc that explains their purpose and usage so this FFI-facing surface complies with the repo’s public-API documentation rule.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-cli/src/publish.rs`:
- Around line 94-100: The EOF handling in the stdin read loop currently returns
immediately in `decoder.decode_chunk` flow, which skips the importer’s final
flush/close step. Update the loop in `publish.rs` so that when
`tokio::io::AsyncReadExt::read_buf` returns `n == 0`, the importer is finalized
before returning, matching the explicit `finish()` behavior used elsewhere after
`decode()`. Use the existing `decoder`/importer finalization path to ensure
buffered trailing data is emitted and tracks are closed cleanly.
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 136-146: The TS export path in run_ts is using the media-only
moq_mux::container::ts::Export::new constructor, which skips catalog handling
and TS extension tracks. Switch this path to the TS-aware exporter setup that
includes self.catalog and re-emits the mpegts extension tracks via with_ts so
verbatim streams and --catalog=msf|hangz continue to work.
- Around line 163-165: The FLV subscription path is ignoring the user-selected
catalog format because `moq_mux::container::flv::Export::new(...)` always
rebuilds with `CatalogFormat::default()`. Update the `subscribe.rs` flow around
`flv::Export::new` and the `flv` export setup so it preserves `self.catalog` the
same way the other exporters do, ensuring `--catalog=msf` and `--catalog=hangz`
are honored instead of being silently overwritten.
In `@rs/moq-gst/src/sink/pad.rs`:
- Around line 117-126: The Opus caps handling in the audio/x-opus branch of
pad.rs accepts any positive channel count, but opus_head() currently always
emits channel-mapping family 0, which is only valid for mono/stereo. Update the
logic around structure.get("channels"), ensure!, and opus_head() so multichannel
caps are not misrepresented: either reject channels > 2 with a clear error in
this branch, or replace the helper call with a proper multistream OpusHead built
from the caps.
---
Outside diff comments:
In `@rs/moq-mux/src/container/flv/export_test.rs`:
- Around line 252-257: Restore the Opus packet-type validation in the enhanced
audio assertion inside export_test’s Opus tag check, so the test specifically
requires the sequence-start header rather than any enhanced Opus frame. Update
the existing tags.iter().any predicate to include the packet type field used by
the exporter’s Opus payload layout, keeping the checks tied to TAG_AUDIO,
AUDIO_FORMAT_EX, and the "Opus" signature while rejecting coded-frame-only tags.
In `@rs/moq-mux/src/container/producer.rs`:
- Around line 110-115: The flush condition in producer logic only considers
timestamp extrema and misses each frame’s duration, so update the buffering
check in Producer::flush-trigger path to account for `Frame.duration` alongside
`timestamp`. In the `self.buffer` span calculation, derive the latest end time
as `timestamp + duration` for each frame and compare that against the earliest
start time, then trigger `self.flush(None)?` when that full span meets or
exceeds `self.latency`.
---
Nitpick comments:
In `@rs/libmoq/src/publish.rs`:
- Around line 16-18: The public Publish API is missing rustdoc, including the
struct itself and the exported entrypoints create, get, close, media_ordered,
media_frame, and media_close, so add concise documentation for each symbol.
Update the Publish type and its public methods in publish.rs with rustdoc that
explains their purpose and usage so this FFI-facing surface complies with the
repo’s public-API documentation rule.
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 8-14: Add rustdoc for the public SubscribeFormat enum and its
variants so every exported symbol is documented. Update the enum definition in
SubscribeFormat with a concise summary that explains what subscription formats
it represents, and add brief variant-level docs for Fmp4, Mkv, Ts, and Flv so
the public API follows the Rust documentation guideline.
In `@rs/moq-ffi/src/producer.rs`:
- Around line 427-432: Add rustdoc for the public M oqTrackProducer::finish
method in producer.rs to satisfy the repo’s API documentation rule for exported
Rust symbols. Document what finish does, that it closes/completes the track
producer, and mention the possible MoqError::Closed outcome if the producer was
already taken or finished; keep the comment directly above finish so it is
picked up by rustdoc.
In `@rs/moq-mux/src/codec/h264/mod.rs`:
- Around line 168-170: The public Avcc profile fields currently lack rustdoc, so
add field-level documentation for profile, constraints, and level in the Avcc
struct to satisfy the exported API documentation guideline. Update the Avcc
definition in the h264 codec module with concise comments describing each
field’s meaning and keep the docs aligned with the existing public API symbols.
In `@rs/moq-mux/src/codec/h265/mod.rs`:
- Around line 81-83: The public fields vps, sps, and pps in the H265
parameter-set struct are undocumented exported API and need rustdoc. Add concise
documentation on each field explaining that the vector holds complete
out-of-band HEVC NAL units of that type, and keep the comments attached to the
struct fields so the public API is properly documented.
🪄 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: 0c7b010b-766e-4abc-aaed-b17a274fb518
📒 Files selected for processing (56)
js/msf/src/catalog.tsrs/hang/examples/video.rsrs/libmoq/src/audio.rsrs/libmoq/src/consume.rsrs/libmoq/src/publish.rsrs/moq-audio/src/producer.rsrs/moq-cli/src/publish.rsrs/moq-cli/src/subscribe.rsrs/moq-ffi/src/audio.rsrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/producer.rsrs/moq-gst/src/sink/pad.rsrs/moq-mux/src/catalog/consumer.rsrs/moq-mux/src/catalog/format.rsrs/moq-mux/src/catalog/hang/consumer.rsrs/moq-mux/src/catalog/hang/ext.rsrs/moq-mux/src/catalog/msf/consumer.rsrs/moq-mux/src/catalog/producer.rsrs/moq-mux/src/codec/av1/split.rsrs/moq-mux/src/codec/h264/export.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h264/mod.rsrs/moq-mux/src/codec/h264/split.rsrs/moq-mux/src/codec/h265/export.rsrs/moq-mux/src/codec/h265/mod.rsrs/moq-mux/src/codec/h265/split.rsrs/moq-mux/src/codec/legacy.rsrs/moq-mux/src/codec/vp8/import.rsrs/moq-mux/src/codec/vp9/import.rsrs/moq-mux/src/container/consumer.rsrs/moq-mux/src/container/flv/export_test.rsrs/moq-mux/src/container/flv/import_test.rsrs/moq-mux/src/container/fmp4/export_test.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/fmp4/import_test.rsrs/moq-mux/src/container/fmp4/mod.rsrs/moq-mux/src/container/hls/import.rsrs/moq-mux/src/container/loc/mod.rsrs/moq-mux/src/container/mkv/export_test.rsrs/moq-mux/src/container/mkv/import_test.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/container/ts/export_test.rsrs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/container/ts/import_test.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/track.rsrs/moq-net/src/model/track.rsrs/moq-rtc/src/codec/h264.rsrs/moq-rtc/src/codec/opus.rsrs/moq-rtc/src/codec/vp8.rsrs/moq-rtc/src/codec/vp9.rsrs/moq-rtmp/src/server.rsrs/moq-srt/src/ts.rsrs/moq-video/src/encode/producer.rsrs/moq-video/src/error.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-mux/src/catalog/consumer.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- rs/moq-mux/src/catalog/format.rs
- rs/moq-mux/src/codec/h264/export.rs
- rs/moq-mux/src/container/loc/mod.rs
- rs/moq-mux/src/codec/h265/split.rs
- rs/moq-mux/src/codec/h265/export.rs
- rs/moq-mux/src/codec/h264/import.rs
- rs/moq-mux/src/container/fmp4/export_test.rs
- rs/moq-mux/src/codec/h264/split.rs
- rs/moq-mux/src/catalog/hang/ext.rs
- rs/moq-mux/src/container/hls/import.rs
- rs/moq-mux/src/codec/av1/split.rs
- rs/moq-mux/src/container/fmp4/mod.rs
- rs/moq-mux/src/codec/vp9/import.rs
- rs/moq-mux/src/codec/legacy.rs
- rs/moq-mux/src/codec/vp8/import.rs
- rs/moq-mux/src/container/fmp4/import.rs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
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/container/producer.rs (1)
110-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude explicit frame durations in the latency span.
The flush trigger only compares timestamp extrema and ignores
Frame.duration, so a single frame or batch whosetimestamp + durationexceedslatencycan remain buffered until another timestamp/keyframe/finish arrives.Proposed fix
- if self.buffer.len() >= 2 { - let mut iter = self.buffer.iter().map(|f| std::time::Duration::from(f.timestamp)); - let first = iter.next().unwrap(); - let (min, max) = iter.fold((first, first), |(min, max), d| (min.min(d), max.max(d))); - if max.saturating_sub(min) >= self.latency { + if !self.buffer.is_empty() { + let mut iter = self.buffer.iter().map(|f| { + let start = std::time::Duration::from(f.timestamp); + let end = f + .duration + .map(std::time::Duration::from) + .and_then(|duration| start.checked_add(duration)) + .unwrap_or(start); + (start, end) + }); + let (first_start, first_end) = iter.next().unwrap(); + let (min_start, max_end) = iter.fold((first_start, first_end), |(min, max), (start, end)| { + (min.min(start), max.max(end)) + }); + if max_end.saturating_sub(min_start) >= self.latency { self.flush(None)?; } }🤖 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/container/producer.rs` around lines 110 - 115, The flush condition in producer logic only considers timestamp extrema and misses each frame’s duration, so update the buffering check in Producer::flush-trigger path to account for `Frame.duration` alongside `timestamp`. In the `self.buffer` span calculation, derive the latest end time as `timestamp + duration` for each frame and compare that against the earliest start time, then trigger `self.flush(None)?` when that full span meets or exceeds `self.latency`.rs/moq-mux/src/container/flv/export_test.rs (1)
252-257: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the Opus packet-type check.
This assertion now passes for any enhanced Opus tag, so the test no longer fails if the exporter omits the required sequence-start header and emits only coded frames.
Suggested fix
assert!( tags.iter().any(|t| t.tag_type == super::TAG_AUDIO && (t.body[0] >> 4) == super::AUDIO_FORMAT_EX + && (t.body[0] & 0x0f) == super::AUDIO_PACKET_SEQUENCE_START && &t.body[1..5] == b"Opus"), "expected an enhanced Opus audio tag" );🤖 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/container/flv/export_test.rs` around lines 252 - 257, Restore the Opus packet-type validation in the enhanced audio assertion inside export_test’s Opus tag check, so the test specifically requires the sequence-start header rather than any enhanced Opus frame. Update the existing tags.iter().any predicate to include the packet type field used by the exporter’s Opus payload layout, keeping the checks tied to TAG_AUDIO, AUDIO_FORMAT_EX, and the "Opus" signature while rejecting coded-frame-only tags.
🧹 Nitpick comments (5)
rs/moq-mux/src/codec/h264/mod.rs (1)
168-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public
Avccprofile fields.These public fields are now part of the exported API surface but lack field-level rustdoc. As per coding guidelines, “Document every exported/public symbol in Rust.”
🤖 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/codec/h264/mod.rs` around lines 168 - 170, The public Avcc profile fields currently lack rustdoc, so add field-level documentation for profile, constraints, and level in the Avcc struct to satisfy the exported API documentation guideline. Update the Avcc definition in the h264 codec module with concise comments describing each field’s meaning and keep the docs aligned with the existing public API symbols.Source: Coding guidelines
rs/moq-mux/src/codec/h265/mod.rs (1)
81-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd field docs for the public parameter-set vectors.
vps,sps, andppsare public API fields; add concise rustdoc describing that each vector contains complete out-of-band HEVC NAL units of that type. As per coding guidelines, “Document every exported/public symbol in Rust.”🤖 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/codec/h265/mod.rs` around lines 81 - 83, The public fields vps, sps, and pps in the H265 parameter-set struct are undocumented exported API and need rustdoc. Add concise documentation on each field explaining that the vector holds complete out-of-band HEVC NAL units of that type, and keep the comments attached to the struct fields so the public API is properly documented.Source: Coding guidelines
rs/moq-cli/src/subscribe.rs (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for
SubscribeFormat.This enum is public and changed in this PR, but it still has no rustdoc. As per coding guidelines, "Document every exported/public symbol in Rust".
🤖 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-cli/src/subscribe.rs` around lines 8 - 14, Add rustdoc for the public SubscribeFormat enum and its variants so every exported symbol is documented. Update the enum definition in SubscribeFormat with a concise summary that explains what subscription formats it represents, and add brief variant-level docs for Fmp4, Mkv, Ts, and Flv so the public API follows the Rust documentation guideline.Source: Coding guidelines
rs/moq-ffi/src/producer.rs (1)
427-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rustdoc for
MoqTrackProducer::finish.Line 427 exposes a public method without a doc comment, so this one falls outside the repo’s required Rust API documentation coverage. As per coding guidelines, "Document every exported/public symbol in Rust and JS/TS".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-ffi/src/producer.rs` around lines 427 - 432, Add rustdoc for the public M oqTrackProducer::finish method in producer.rs to satisfy the repo’s API documentation rule for exported Rust symbols. Document what finish does, that it closes/completes the track producer, and mention the possible MoqError::Closed outcome if the producer was already taken or finished; keep the comment directly above finish so it is picked up by rustdoc.Source: Coding guidelines
rs/libmoq/src/publish.rs (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the changed
PublishAPI surface.
Publishand several changed public entrypoints here (create,get,close,media_ordered,media_frame,media_close) still ship without rustdoc, which makes this FFI-facing surface harder to consume and breaks the repo’s public-API doc rule. As per coding guidelines, "Document every exported/public symbol in Rust and JS/TS".Also applies to: 31-61, 63-105
🤖 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/publish.rs` around lines 16 - 18, The public Publish API is missing rustdoc, including the struct itself and the exported entrypoints create, get, close, media_ordered, media_frame, and media_close, so add concise documentation for each symbol. Update the Publish type and its public methods in publish.rs with rustdoc that explains their purpose and usage so this FFI-facing surface complies with the repo’s public-API documentation rule.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-cli/src/publish.rs`:
- Around line 94-100: The EOF handling in the stdin read loop currently returns
immediately in `decoder.decode_chunk` flow, which skips the importer’s final
flush/close step. Update the loop in `publish.rs` so that when
`tokio::io::AsyncReadExt::read_buf` returns `n == 0`, the importer is finalized
before returning, matching the explicit `finish()` behavior used elsewhere after
`decode()`. Use the existing `decoder`/importer finalization path to ensure
buffered trailing data is emitted and tracks are closed cleanly.
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 136-146: The TS export path in run_ts is using the media-only
moq_mux::container::ts::Export::new constructor, which skips catalog handling
and TS extension tracks. Switch this path to the TS-aware exporter setup that
includes self.catalog and re-emits the mpegts extension tracks via with_ts so
verbatim streams and --catalog=msf|hangz continue to work.
- Around line 163-165: The FLV subscription path is ignoring the user-selected
catalog format because `moq_mux::container::flv::Export::new(...)` always
rebuilds with `CatalogFormat::default()`. Update the `subscribe.rs` flow around
`flv::Export::new` and the `flv` export setup so it preserves `self.catalog` the
same way the other exporters do, ensuring `--catalog=msf` and `--catalog=hangz`
are honored instead of being silently overwritten.
In `@rs/moq-gst/src/sink/pad.rs`:
- Around line 117-126: The Opus caps handling in the audio/x-opus branch of
pad.rs accepts any positive channel count, but opus_head() currently always
emits channel-mapping family 0, which is only valid for mono/stereo. Update the
logic around structure.get("channels"), ensure!, and opus_head() so multichannel
caps are not misrepresented: either reject channels > 2 with a clear error in
this branch, or replace the helper call with a proper multistream OpusHead built
from the caps.
---
Outside diff comments:
In `@rs/moq-mux/src/container/flv/export_test.rs`:
- Around line 252-257: Restore the Opus packet-type validation in the enhanced
audio assertion inside export_test’s Opus tag check, so the test specifically
requires the sequence-start header rather than any enhanced Opus frame. Update
the existing tags.iter().any predicate to include the packet type field used by
the exporter’s Opus payload layout, keeping the checks tied to TAG_AUDIO,
AUDIO_FORMAT_EX, and the "Opus" signature while rejecting coded-frame-only tags.
In `@rs/moq-mux/src/container/producer.rs`:
- Around line 110-115: The flush condition in producer logic only considers
timestamp extrema and misses each frame’s duration, so update the buffering
check in Producer::flush-trigger path to account for `Frame.duration` alongside
`timestamp`. In the `self.buffer` span calculation, derive the latest end time
as `timestamp + duration` for each frame and compare that against the earliest
start time, then trigger `self.flush(None)?` when that full span meets or
exceeds `self.latency`.
---
Nitpick comments:
In `@rs/libmoq/src/publish.rs`:
- Around line 16-18: The public Publish API is missing rustdoc, including the
struct itself and the exported entrypoints create, get, close, media_ordered,
media_frame, and media_close, so add concise documentation for each symbol.
Update the Publish type and its public methods in publish.rs with rustdoc that
explains their purpose and usage so this FFI-facing surface complies with the
repo’s public-API documentation rule.
In `@rs/moq-cli/src/subscribe.rs`:
- Around line 8-14: Add rustdoc for the public SubscribeFormat enum and its
variants so every exported symbol is documented. Update the enum definition in
SubscribeFormat with a concise summary that explains what subscription formats
it represents, and add brief variant-level docs for Fmp4, Mkv, Ts, and Flv so
the public API follows the Rust documentation guideline.
In `@rs/moq-ffi/src/producer.rs`:
- Around line 427-432: Add rustdoc for the public M oqTrackProducer::finish
method in producer.rs to satisfy the repo’s API documentation rule for exported
Rust symbols. Document what finish does, that it closes/completes the track
producer, and mention the possible MoqError::Closed outcome if the producer was
already taken or finished; keep the comment directly above finish so it is
picked up by rustdoc.
In `@rs/moq-mux/src/codec/h264/mod.rs`:
- Around line 168-170: The public Avcc profile fields currently lack rustdoc, so
add field-level documentation for profile, constraints, and level in the Avcc
struct to satisfy the exported API documentation guideline. Update the Avcc
definition in the h264 codec module with concise comments describing each
field’s meaning and keep the docs aligned with the existing public API symbols.
In `@rs/moq-mux/src/codec/h265/mod.rs`:
- Around line 81-83: The public fields vps, sps, and pps in the H265
parameter-set struct are undocumented exported API and need rustdoc. Add concise
documentation on each field explaining that the vector holds complete
out-of-band HEVC NAL units of that type, and keep the comments attached to the
struct fields so the public API is properly documented.
🪄 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: 0c7b010b-766e-4abc-aaed-b17a274fb518
📒 Files selected for processing (56)
js/msf/src/catalog.tsrs/hang/examples/video.rsrs/libmoq/src/audio.rsrs/libmoq/src/consume.rsrs/libmoq/src/publish.rsrs/moq-audio/src/producer.rsrs/moq-cli/src/publish.rsrs/moq-cli/src/subscribe.rsrs/moq-ffi/src/audio.rsrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/producer.rsrs/moq-gst/src/sink/pad.rsrs/moq-mux/src/catalog/consumer.rsrs/moq-mux/src/catalog/format.rsrs/moq-mux/src/catalog/hang/consumer.rsrs/moq-mux/src/catalog/hang/ext.rsrs/moq-mux/src/catalog/msf/consumer.rsrs/moq-mux/src/catalog/producer.rsrs/moq-mux/src/codec/av1/split.rsrs/moq-mux/src/codec/h264/export.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h264/mod.rsrs/moq-mux/src/codec/h264/split.rsrs/moq-mux/src/codec/h265/export.rsrs/moq-mux/src/codec/h265/mod.rsrs/moq-mux/src/codec/h265/split.rsrs/moq-mux/src/codec/legacy.rsrs/moq-mux/src/codec/vp8/import.rsrs/moq-mux/src/codec/vp9/import.rsrs/moq-mux/src/container/consumer.rsrs/moq-mux/src/container/flv/export_test.rsrs/moq-mux/src/container/flv/import_test.rsrs/moq-mux/src/container/fmp4/export_test.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/fmp4/import_test.rsrs/moq-mux/src/container/fmp4/mod.rsrs/moq-mux/src/container/hls/import.rsrs/moq-mux/src/container/loc/mod.rsrs/moq-mux/src/container/mkv/export_test.rsrs/moq-mux/src/container/mkv/import_test.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/container/ts/export_test.rsrs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/container/ts/import_test.rsrs/moq-mux/src/import/mod.rsrs/moq-mux/src/import/track.rsrs/moq-net/src/model/track.rsrs/moq-rtc/src/codec/h264.rsrs/moq-rtc/src/codec/opus.rsrs/moq-rtc/src/codec/vp8.rsrs/moq-rtc/src/codec/vp9.rsrs/moq-rtmp/src/server.rsrs/moq-srt/src/ts.rsrs/moq-video/src/encode/producer.rsrs/moq-video/src/error.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-mux/src/catalog/consumer.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- rs/moq-mux/src/catalog/format.rs
- rs/moq-mux/src/codec/h264/export.rs
- rs/moq-mux/src/container/loc/mod.rs
- rs/moq-mux/src/codec/h265/split.rs
- rs/moq-mux/src/codec/h265/export.rs
- rs/moq-mux/src/codec/h264/import.rs
- rs/moq-mux/src/container/fmp4/export_test.rs
- rs/moq-mux/src/codec/h264/split.rs
- rs/moq-mux/src/catalog/hang/ext.rs
- rs/moq-mux/src/container/hls/import.rs
- rs/moq-mux/src/codec/av1/split.rs
- rs/moq-mux/src/container/fmp4/mod.rs
- rs/moq-mux/src/codec/vp9/import.rs
- rs/moq-mux/src/codec/legacy.rs
- rs/moq-mux/src/codec/vp8/import.rs
- rs/moq-mux/src/container/fmp4/import.rs
🛑 Comments failed to post (4)
rs/moq-cli/src/publish.rs (1)
94-100: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Finalize the importer on EOF.
Returning on
n == 0skips the importer’s end-of-stream flush/close path. That can drop buffered trailing state and leave the published tracks open. The TS importer is explicitlyfinish()ed afterdecode()elsewhere, so this loop needs the same EOF finalization step.Suggested fix
impl PublishDecoder { /// Decode a chunk of stdin bytes. Each importer buffers any partial trailing /// frame internally, so the caller feeds fresh chunks rather than an /// accumulating buffer. fn decode_chunk(&mut self, chunk: &[u8]) -> anyhow::Result<()> { match self { Self::Avc3(d) => d.decode(chunk)?, Self::Fmp4(d) => d.decode(chunk)?, Self::Ts(d) => d.decode(chunk)?, Self::Flv(d) => d.decode(chunk)?, } Ok(()) } + + fn finish(&mut self) -> anyhow::Result<()> { + match self { + Self::Avc3(d) => d.finish()?, + Self::Fmp4(d) => d.finish()?, + Self::Ts(d) => d.finish()?, + Self::Flv(d) => d.finish()?, + } + Ok(()) + } } @@ loop { buffer.clear(); let n = tokio::io::AsyncReadExt::read_buf(&mut stdin, &mut buffer).await?; if n == 0 { + decoder.finish()?; return Ok(()); } decoder.decode_chunk(&buffer)?; } }📝 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.impl PublishDecoder { /// Decode a chunk of stdin bytes. Each importer buffers any partial trailing /// frame internally, so the caller feeds fresh chunks rather than an /// accumulating buffer. fn decode_chunk(&mut self, chunk: &[u8]) -> anyhow::Result<()> { match self { Self::Avc3(d) => d.decode(chunk)?, Self::Fmp4(d) => d.decode(chunk)?, Self::Ts(d) => d.decode(chunk)?, Self::Flv(d) => d.decode(chunk)?, } Ok(()) } fn finish(&mut self) -> anyhow::Result<()> { match self { Self::Avc3(d) => d.finish()?, Self::Fmp4(d) => d.finish()?, Self::Ts(d) => d.finish()?, Self::Flv(d) => d.finish()?, } Ok(()) } } loop { buffer.clear(); let n = tokio::io::AsyncReadExt::read_buf(&mut stdin, &mut buffer).await?; if n == 0 { decoder.finish()?; return Ok(()); } decoder.decode_chunk(&buffer)?; }🤖 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-cli/src/publish.rs` around lines 94 - 100, The EOF handling in the stdin read loop currently returns immediately in `decoder.decode_chunk` flow, which skips the importer’s final flush/close step. Update the loop in `publish.rs` so that when `tokio::io::AsyncReadExt::read_buf` returns `n == 0`, the importer is finalized before returning, matching the explicit `finish()` behavior used elsewhere after `decode()`. Use the existing `decoder`/importer finalization path to ensure buffered trailing data is emitted and tracks are closed cleanly.rs/moq-cli/src/subscribe.rs (2)
136-146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the TS-aware exporter here.
moq_mux::container::ts::Export::new(...)is the media-only constructor. It ignoresself.catalogand drops thempegtsextension tracks thatwith_ts(...)re-emits, so TS output loses verbatim streams like SCTE-35/private data and--catalog=msf|hangzstops working on this path.Suggested fix
- let mut ts = moq_mux::container::ts::Export::new(self.broadcast) - .await? + let mut ts = moq_mux::container::ts::Export::with_ts(self.broadcast, self.catalog) + .await? .with_latency(self.args.max_latency);📝 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.async fn run_ts(self) -> anyhow::Result<()> { let mut stdout = tokio::io::stdout(); // TS emits PAT/PMT then a continuous PES stream (re-emitting PAT/PMT at // keyframes for tune-in). Avc3/Hev1 sources pass through as Annex-B; AAC // is re-framed as ADTS. `fragment_duration` does not apply to TS. Undecoded // elementary streams (SCTE-35, teletext, DVB AC-3, ...) are re-emitted // verbatim on their PIDs. let mut ts = moq_mux::container::ts::Export::with_ts(self.broadcast, self.catalog) .await? .with_latency(self.args.max_latency);🤖 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-cli/src/subscribe.rs` around lines 136 - 146, The TS export path in run_ts is using the media-only moq_mux::container::ts::Export::new constructor, which skips catalog handling and TS extension tracks. Switch this path to the TS-aware exporter setup that includes self.catalog and re-emits the mpegts extension tracks via with_ts so verbatim streams and --catalog=msf|hangz continue to work.
163-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the requested catalog format for FLV export.
flv::Export::new(...)always rebuilds withCatalogFormat::default(), so this path silently ignores--catalog=msfand--catalog=hangzeven though the other exporters honorself.catalog.Suggested fix
- let mut flv = moq_mux::container::flv::Export::new(self.broadcast) - .await? + let mut flv = moq_mux::container::flv::Export::with_catalog_format(self.broadcast, self.catalog) + .await? .with_latency(self.args.max_latency);📝 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.let mut flv = moq_mux::container::flv::Export::with_catalog_format(self.broadcast, self.catalog) .await? .with_latency(self.args.max_latency);🤖 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-cli/src/subscribe.rs` around lines 163 - 165, The FLV subscription path is ignoring the user-selected catalog format because `moq_mux::container::flv::Export::new(...)` always rebuilds with `CatalogFormat::default()`. Update the `subscribe.rs` flow around `flv::Export::new` and the `flv` export setup so it preserves `self.catalog` the same way the other exporters do, ensuring `--catalog=msf` and `--catalog=hangz` are honored instead of being silently overwritten.rs/moq-gst/src/sink/pad.rs (1)
117-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject multichannel Opus until the header is encoded correctly.
This accepts any positive
channels, butopus_head()always emits channel-mapping family0. That header form is only valid for mono/stereo, so 3+ channel Opus caps will produce an invalid or misdescribed stream. Either rejectchannels > 2here or synthesize the full multistream OpusHead from caps.Suggested stop-gap
let channels: i32 = structure.get("channels").context("Opus caps missing channels")?; let rate: i32 = structure.get("rate").context("Opus caps missing rate")?; ensure!(channels > 0, "Opus caps has non-positive channel count {channels}"); + ensure!(channels <= 2, "multichannel Opus is not supported yet (channels={channels})"); ensure!(rate > 0, "Opus caps has non-positive sample rate {rate}"); let head = opus_head(channels as u8, rate as u32);Also applies to: 254-266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-gst/src/sink/pad.rs` around lines 117 - 126, The Opus caps handling in the audio/x-opus branch of pad.rs accepts any positive channel count, but opus_head() currently always emits channel-mapping family 0, which is only valid for mono/stereo. Update the logic around structure.get("channels"), ensure!, and opus_head() so multichannel caps are not misrepresented: either reject channels > 2 with a clear error in this branch, or replace the helper call with a proper multistream OpusHead built from the caps.
…nsumer::new, guard fmp4 framerate, avc1-after-parse, av1 reset clears tail
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-mux/src/container/ts/export.rs (1)
152-183: 🎯 Functional Correctness | 🟠 MajorKeep the constructor path synchronous to avoid a public API break and maintain consistency.
buildcontains noawaitcalls becauseConsumer::newis synchronous. Markingnew,with_catalog_format, andwith_tsasasyncforces downstream callers to use.awaitunnecessarily and breaks API parity withfmp4::Export::newandmkv::Export::new, which remain synchronous for identical patterns.Reverting to synchronous constructors aligns with the "no-API-break" backport goal and removes the artificial future boundary.
Proposed fix
- pub async fn new(broadcast: moq_net::BroadcastConsumer) -> Result<Self, crate::Error> { - Self::with_catalog_format(broadcast, CatalogFormat::default()).await + pub fn new(broadcast: moq_net::BroadcastConsumer) -> Result<Self, crate::Error> { + Self::with_catalog_format(broadcast, CatalogFormat::default()) } - pub async fn with_catalog_format( + pub fn with_catalog_format( broadcast: moq_net::BroadcastConsumer, catalog_format: CatalogFormat, ) -> Result<Self, crate::Error> { - Self::build(broadcast, catalog_format).await + Self::build(broadcast, catalog_format) } - pub async fn with_ts( + pub fn with_ts( broadcast: moq_net::BroadcastConsumer, catalog_format: CatalogFormat, ) -> Result<Self, crate::Error> { - Self::build(broadcast, catalog_format).await + Self::build(broadcast, catalog_format) } } impl<E: catalog::Catalog> Export<E> { @@ - async fn build(broadcast: moq_net::BroadcastConsumer, catalog_format: CatalogFormat) -> Result<Self, crate::Error> { + fn build(broadcast: moq_net::BroadcastConsumer, catalog_format: CatalogFormat) -> Result<Self, crate::Error> { let catalog = crate::catalog::Consumer::<E>::new(&broadcast, catalog_format)?;🤖 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/container/ts/export.rs` around lines 152 - 183, The export constructors are incorrectly marked async even though Export::build only calls the synchronous Consumer::new, which creates an unnecessary await boundary and changes the public API. Make Export::new, Export::with_catalog_format, and Export::with_ts synchronous again, and keep the shared Export<E>::build constructor synchronous as well so the API stays consistent with the other export types and preserves the no-breaking-change behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rs/moq-mux/src/container/ts/export.rs`:
- Around line 152-183: The export constructors are incorrectly marked async even
though Export::build only calls the synchronous Consumer::new, which creates an
unnecessary await boundary and changes the public API. Make Export::new,
Export::with_catalog_format, and Export::with_ts synchronous again, and keep the
shared Export<E>::build constructor synchronous as well so the API stays
consistent with the other export types and preserves the no-breaking-change
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02bee97e-077e-4d77-aa84-9893f4d3a3df
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
js/msf/src/catalog.tsrs/moq-cli/src/subscribe.rsrs/moq-mux/src/catalog/consumer.rsrs/moq-mux/src/codec/av1/split.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/container/flv/export.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/ts/export.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- rs/moq-mux/src/container/flv/export.rs
- rs/moq-cli/src/subscribe.rs
- js/msf/src/catalog.ts
- rs/moq-mux/src/codec/av1/split.rs
- rs/moq-mux/src/container/fmp4/export.rs
- rs/moq-mux/src/codec/h264/import.rs
…rs catalog format for ts(with_ts)/flv; finish importer on stdin EOF; reject multichannel Opus in moq-gst; doc Avcc/Hvcc fields
…hmetic in fmp4 import
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
Brings the
devmoq-mux improvements tomain(per-codec splitters + decoupled importers #1749, AV1 fMP4/CMAF init synthesis #1804, moq-msf draft-01 #1834, thiserror errors, catalog filter/target + Annex-B exporters, scale-free Jitter), adapted tomain's oldermoq-netrather than dragging indev's breaking wire changes.The original backport pulled in far more than moq-mux and was untested (lib compiled, but 327 moq-mux test errors + every downstream consumer/gateway broken). This pass reconciles it so the whole workspace builds, tests, lints, and docs cleanly.
moq-net: additive only (no semver break)
TrackProducer::demand()+TrackDemand,TrackProducer/Consumer::name(),BroadcastProducer::unique_name(). No existing signatures changed/removed.FrameInfo/GroupInfo/TrackSubscriber); moq-mux uses the realFrame/Group/TrackConsumernames.Removed (unrelated dev features the backport had dragged in)
dev-onlymoq_net::ConnectionStats/Session::stats.ClientConfigbuilder rename (with_publisher/with_subscriber) — reverted towith_publish/with_consume.devmoq-ffi surface (MoqTrackInfo/MoqSubscription/MoqTrackRequest); py/kt reverted to match main's moq-ffi.Preserved main-only features the backport would have regressed
#1916typedh264::Avcc{sps,pps}+h265::Hvcc::parse(folded the redundant*ParamSetsback in; the gateways keep their typed API).#1904compressed catalog (CatalogFormat::HangZ,catalog.json.z) — re-ported onto the refactored catalog producer/consumer.#1886untyped FFI catalog sections — kept by makingmoq-audio'sAudioProducergeneric over the catalog ext so theExtracatalog flows through.Adapted downstream to the new moq-mux API
moq-net/hang(Broadcast::new/create_track/consume/container::Timestamp, etc.).Export::new,container::Frame.duration,container::Timestamp.import::{Track,Container,TrackStream,ContainerStream}; moq-cli subscribe builds acatalog::Consumerstream for the Export.Scoped regression
moq-cli publish hlsdropped: HLS ingest was extracted out of moq-mux ondev(noreqwest/m3u8 deps remain) and themoq-hlscrate isn't in this PR. Bring it back via themoq-hlscrate in a follow-up.Testing
cargo build --workspace,cargo clippy --workspace --all-targets,cargo fmt --check,cargo doc -p moq-muxall clean.Moq.Track).(Written by Claude)
Review handling
CodeRabbit review addressed across the iterations. Low-risk findings fixed here (catalog
Consumer::newsync, fmp4 framerate guard, h264 avc1-after-parse, av1reset()clears tail, av1C size gate, fmp4-import checked arithmetic, moq-cli ts/flv honor catalog format, publish EOF finalize, multichannel-Opus reject). The remaining Major findings are pre-existing dev moq-mux behavior, not regressions from this backport, and are tracked in #1923 for a focused follow-up with tests (h264/h265 splitter AU boundaries, AAC explicit sample-rate, container error handling, fmp4 duration-less samples, filter/target EOF).(Written by Claude)