Mux import with existing track#1684
Merged
Merged
Conversation
Contributor
WalkthroughThis PR enables publishing media onto dynamically-requested tracks by introducing a TrackProvider abstraction that decouples codec decoders from the broadcast producer. Codec decoders are refactored to use TrackProvider, which abstracts whether tracks are created via broadcast (unique mode) or provided pre-existing (fixed mode). The Framed importer gains new_with_track support with multi-track format validation. A publish_media_on_track API is added to the Rust FFI layer, exposing format parsing and decoder initialization for fixed tracks. The Python binding wraps this functionality with the same method name. Integration tests across the Rust FFI and Python layers demonstrate requesting dynamic tracks, publishing media, and validating catalog metadata and received frames. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
rs/moq-mux/src/codec/h264/import.rs (1)
446-472: 💤 Low valueConsider aligning guard placement with AV1/H265 pattern.
The reconfiguration guard at lines 455-458 is correct but less optimal than the AV1/H265 pattern:
Current H.264 flow:
- Lock catalog (line 453)
- Take track (line 454)
- Check if fixed → restore track and error (lines 455-458)
AV1/H265 flow (see av1/import.rs:102-111):
- Check if fixed → error
- Take track
- Lock catalog and modify
The AV1/H265 pattern avoids taking the track and locking the catalog when the operation will fail, making it slightly more efficient and easier to follow.
♻️ Align with AV1/H265 guard pattern
fn swap_config(&mut self, config: hang::catalog::VideoConfig) -> anyhow::Result<()> { if let Some(old) = &self.config && old == &config { return Ok(()); } - let mut catalog = self.catalog.lock(); + if let Some(_) = &self.track + && self.tracks.is_fixed() + { + anyhow::bail!("fixed track cannot be reconfigured"); + } + + let mut catalog = self.catalog.lock(); if let Some(track) = self.track.take() { - if self.tracks.is_fixed() { - self.track = Some(track); - anyhow::bail!("fixed track cannot be reconfigured"); - } tracing::debug!(name = ?track.name, "reinitializing H.264 track"); catalog.video.renditions.remove(&track.name); } let track = self.tracks.create()?;🤖 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/import.rs` around lines 446 - 472, Move the "fixed track" guard to mirror AV1/H265: in swap_config, check self.tracks.is_fixed() and bail early (using the same error text) before calling self.track.take() or locking self.catalog; only take the track and lock the catalog after passing that guard, then proceed to remove the old rendition, create a new track via self.tracks.create(), insert into catalog.video.renditions, set self.config and self.track (using crate::container::Producer::new(...)) and return Ok(()). Ensure you reassign the error path semantics so the behavior and messages match the current fixed-track handling.rs/moq-mux/src/track_provider.rs (1)
1-34: 💤 Low valueConsider adding doc comments for maintainability.
The
TrackProviderabstraction is clean and well-structured. While the code is fairly self-explanatory, adding module-level and method-level documentation would help future maintainers understand the two modes (unique track creation vs. fixed track binding) and when each is appropriate.📝 Example documentation structure
+/// Abstraction over track creation for codec importers. +/// +/// Supports two modes: +/// - `Unique`: Creates new tracks via broadcast with a suffix (e.g., ".aac") +/// - `Fixed`: Uses a pre-provided track (for dynamically-requested tracks) pub(crate) enum TrackProvider { + /// Create unique tracks via broadcast with the given suffix. Unique { broadcast: moq_net::BroadcastProducer, suffix: &'static str, }, + /// Use a pre-provided fixed track. Fixed(moq_net::TrackProducer), }🤖 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/track_provider.rs` around lines 1 - 34, Add doc comments describing the purpose and modes of TrackProvider and document each public method: annotate the enum TrackProvider with a short module-level summary explaining the two variants (Unique: creates suffix-keyed unique tracks via moq_net::BroadcastProducer; Fixed: returns a cloned moq_net::TrackProducer), then add brief doc comments for the constructors TrackProvider::unique and TrackProvider::fixed, the predicate TrackProvider::is_fixed, the mutator TrackProvider::set_suffix (explain it only applies to Unique), and TrackProvider::create (explain behavior and returned Result). Use /// comments above the enum and each fn to improve maintainability and clarify expected use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rs/moq-mux/src/codec/h264/import.rs`:
- Around line 446-472: Move the "fixed track" guard to mirror AV1/H265: in
swap_config, check self.tracks.is_fixed() and bail early (using the same error
text) before calling self.track.take() or locking self.catalog; only take the
track and lock the catalog after passing that guard, then proceed to remove the
old rendition, create a new track via self.tracks.create(), insert into
catalog.video.renditions, set self.config and self.track (using
crate::container::Producer::new(...)) and return Ok(()). Ensure you reassign the
error path semantics so the behavior and messages match the current fixed-track
handling.
In `@rs/moq-mux/src/track_provider.rs`:
- Around line 1-34: Add doc comments describing the purpose and modes of
TrackProvider and document each public method: annotate the enum TrackProvider
with a short module-level summary explaining the two variants (Unique: creates
suffix-keyed unique tracks via moq_net::BroadcastProducer; Fixed: returns a
cloned moq_net::TrackProducer), then add brief doc comments for the constructors
TrackProvider::unique and TrackProvider::fixed, the predicate
TrackProvider::is_fixed, the mutator TrackProvider::set_suffix (explain it only
applies to Unique), and TrackProvider::create (explain behavior and returned
Result). Use /// comments above the enum and each fn to improve maintainability
and clarify expected use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 746eae3b-7374-4489-ab87-ac1e34b4546d
📒 Files selected for processing (14)
py/moq-rs/moq/publish.pypy/moq-rs/tests/test_local.pyrs/moq-ffi/src/producer.rsrs/moq-ffi/src/test.rsrs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/codec/av1/import.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/codec/opus/import.rsrs/moq-mux/src/codec/vp8/import.rsrs/moq-mux/src/codec/vp9/import.rsrs/moq-mux/src/import.rsrs/moq-mux/src/lib.rsrs/moq-mux/src/track_provider.rs
Add a fixed-track importer path so callers can bind single-track media importers to an existing TrackProducer. Keep generated track allocation behind TrackProvider for existing broadcast-backed publishers.
Expose publish_media_on_track so UniFFI callers can turn requested raw tracks into catalog-backed media producers. Add Python wrapper coverage for the dynamic request flow.
Qizot
force-pushed
the
mux-import-with-track
branch
from
June 12, 2026 09:58
71c24b8 to
e30c44d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
py/moq-rs/tests/test_local.py (1)
277-278: 💤 Low valueConsider asserting a more specific exception type.
The test validates that
track.nameraises an exception after the track's inner producer is consumed bypublish_media_on_track. Usingpytest.raises(Exception)is overly broad—asserting a specific exception type (e.g.,RuntimeErroror a custom FFI exception) would make the test more precise and easier to debug if behavior changes.💡 Example improvement
If the FFI raises a specific exception type (e.g.,
RuntimeError), use:with pytest.raises(RuntimeError): _ = track.name🤖 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 `@py/moq-rs/tests/test_local.py` around lines 277 - 278, The test currently uses a broad pytest.raises(Exception) when accessing track.name after publish_media_on_track consumes the inner producer; change this to assert the specific exception type the FFI or code actually raises (e.g., RuntimeError or the library's custom exception) so the test is precise: replace pytest.raises(Exception) with pytest.raises(<SpecificException>) for the access to track.name (reference symbols: track.name and publish_media_on_track) and update the import if needed to reference the specific exception class used by the FFI.Source: Linters/SAST tools
🤖 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 `@py/moq-rs/moq/publish.py`:
- Around line 193-194: Rename the parameter named "format" in the method
publish_media_on_track to avoid shadowing the built-in format() function; update
the method signature (publish_media_on_track(self, track: TrackProducer, ???:
str, init: bytes)) to a clearer name such as "fmt" or "mime_type", and update
the use of that parameter in the body (the call to
self._inner.publish_media_on_track) and any callers to pass the new parameter
name accordingly, ensuring type hints and docstrings (if any) reflect the new
name.
---
Nitpick comments:
In `@py/moq-rs/tests/test_local.py`:
- Around line 277-278: The test currently uses a broad pytest.raises(Exception)
when accessing track.name after publish_media_on_track consumes the inner
producer; change this to assert the specific exception type the FFI or code
actually raises (e.g., RuntimeError or the library's custom exception) so the
test is precise: replace pytest.raises(Exception) with
pytest.raises(<SpecificException>) for the access to track.name (reference
symbols: track.name and publish_media_on_track) and update the import if needed
to reference the specific exception class used by the FFI.
🪄 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: 6fb0c19b-0fbc-48bd-ac09-9da1fce32272
📒 Files selected for processing (14)
py/moq-rs/moq/publish.pypy/moq-rs/tests/test_local.pyrs/moq-ffi/src/producer.rsrs/moq-ffi/src/test.rsrs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/codec/av1/import.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/codec/opus/import.rsrs/moq-mux/src/codec/vp8/import.rsrs/moq-mux/src/codec/vp9/import.rsrs/moq-mux/src/import.rsrs/moq-mux/src/lib.rsrs/moq-mux/src/track_provider.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-mux/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- rs/moq-mux/src/codec/vp9/import.rs
- rs/moq-mux/src/track_provider.rs
- rs/moq-ffi/src/test.rs
- rs/moq-mux/src/import.rs
- rs/moq-mux/src/codec/av1/import.rs
- rs/moq-mux/src/codec/opus/import.rs
- rs/moq-mux/src/codec/vp8/import.rs
- rs/moq-mux/src/codec/aac/import.rs
- rs/moq-ffi/src/producer.rs
- rs/moq-mux/src/codec/h264/import.rs
Comment on lines
+193
to
+194
| def publish_media_on_track(self, track: TrackProducer, format: str, init: bytes) -> MediaProducer: | ||
| return MediaProducer(self._inner.publish_media_on_track(track._inner, format, init)) |
Contributor
There was a problem hiding this comment.
Rename format parameter to avoid shadowing Python builtin.
The parameter name format shadows Python's built-in format() function. While this doesn't cause runtime issues here, it violates Python naming best practices and can reduce code clarity.
📝 Proposed fix
- def publish_media_on_track(self, track: TrackProducer, format: str, init: bytes) -> MediaProducer:
- return MediaProducer(self._inner.publish_media_on_track(track._inner, format, init))
+ def publish_media_on_track(self, track: TrackProducer, media_format: str, init: bytes) -> MediaProducer:
+ return MediaProducer(self._inner.publish_media_on_track(track._inner, media_format, init))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def publish_media_on_track(self, track: TrackProducer, format: str, init: bytes) -> MediaProducer: | |
| return MediaProducer(self._inner.publish_media_on_track(track._inner, format, init)) | |
| def publish_media_on_track(self, track: TrackProducer, media_format: str, init: bytes) -> MediaProducer: | |
| return MediaProducer(self._inner.publish_media_on_track(track._inner, media_format, init)) |
🧰 Tools
🪛 Ruff (0.15.15)
[error] 193-193: Function argument format is shadowing a Python builtin
(A002)
🤖 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 `@py/moq-rs/moq/publish.py` around lines 193 - 194, Rename the parameter named
"format" in the method publish_media_on_track to avoid shadowing the built-in
format() function; update the method signature (publish_media_on_track(self,
track: TrackProducer, ???: str, init: bytes)) to a clearer name such as "fmt" or
"mime_type", and update the use of that parameter in the body (the call to
self._inner.publish_media_on_track) and any callers to pass the new parameter
name accordingly, ensuring type hints and docstrings (if any) reflect the new
name.
Source: Linters/SAST tools
Collaborator
|
I think this is fine for now to unblock you, but I want to revamp the API on I think each Import needs to have two states: Uninitialized (no TrackProducer) and Initialized (yes TrackProducer). |
kixelated
pushed a commit
that referenced
this pull request
Jun 15, 2026
…w fixes Main's #1684 added Framed::new_with_track, whose match over FramedFormat didn't cover the new Flv variant. FLV is multi-track (video + audio), so add it to the "can publish multiple tracks" arm alongside Fmp4/Mkv/Ts. Also address CodeRabbit review feedback: - import: bound the FLV header data_offset (cap unbounded buffering). - export: reject tag bodies larger than FLV's 24-bit DataSize instead of silently truncating into a corrupt header. - export: fix the doc comment to note LOC tracks are accepted too.
Merged
kixelated
added a commit
that referenced
this pull request
Jun 15, 2026
Reconciles main's new features with dev's reshaped APIs: - moq-mux codec imports: combined main's `TrackProvider` (import onto an existing track, #1684) with dev's per-track timescale. The timescale is baked into `TrackProvider::create()` so unique tracks publish at the native container timescale; fixed tracks keep the caller's info. Added a `FixedTrackReconfigured` error variant per codec to replace the anyhow::bail in the crate::Result paths. - kio: dev's new `Weak::poll_closed` now registers on main's split `waiters_closed` list (#1739) instead of the removed single `waiters`. - js/watch: re-implemented main's distance-based `visible` download control (#1744) on top of dev's input/output signal architecture. The renderer takes `visible: Visible` as an input and emits the boolean `output.visible`; the backend keeps owning the paused/enabled gating. - noq/websocket: bumped web-transport-noq to 0.2 (#1741, #1743) and qmux to 0.1.3, then mapped qmux's new `Error::Http(status)` to `ConnectError::Unauthorized` so the WebSocket fallback surfaces auth rejections like the QUIC backend. - Cargo: took the newer of each shared dep (web-async 0.1.4, iroh 0.6, noq 0.2); Cargo.lock based on dev's to avoid gratuitous transitive bumps. - Brought the test code main introduced (and dev's stale integration tests) up to dev's current API (create_track(name, info), announced().next(), track.name(), with_publisher/with_subscriber, moq_net::Timestamp). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 tasks
Merged
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.
The regular flow when creating mux import is to create a new track. This is fine most of the time but breaks when used together with dynamic tracks (produced by
requested_track), we can't gracefully add the dynamic track to the catalog due to API limitations.This PR adds support to moq-mux for importing from an already existing track.
There are no breaking changes, there is additional API and also the functionality has been added to moq-ffi.