Add FLV (Flash Video / RTMP) container support to moq-mux - #1745
Merged
Conversation
Add an FLV import (demuxer) and export (muxer) alongside the existing
fMP4, MKV, and MPEG-TS containers. FLV carries H.264 video as
length-prefixed NALU with an out-of-band AVCDecoderConfigurationRecord
and AAC audio raw with an out-of-band AudioSpecificConfig, both of which
map directly onto the Legacy container and the existing Avcc/aac::Config
parsers. Only the trivial tag framing is hand-rolled.
- container::flv::Import demuxes an FLV byte stream into a broadcast.
- container::flv::Export muxes a broadcast back into FLV.
- Wire Flv into the FramedFormat / StreamFormat dispatchers ("flv").
- Add flv to moq-cli publish and subscribe (--format flv).
- Document FLV in the cli and moq-mux crate docs.
Enhanced E-RTMP FourCC payloads (HEVC, AV1, Opus) and the older codecs
(VP6, MP3) are logged and dropped on import and rejected on export.
Contributor
WalkthroughThis pull request adds FLV (Flash Video / RTMP container) support to 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rs/moq-mux/src/container/flv/export_test.rs (1)
128-128: ⚡ Quick winAvoid brittle indexing in assertions and tag filters.
Use
starts_withandget(1)to prevent panic-driven failures in tests and keep failures diagnostic-focused.Suggested patch
- assert_eq!(&exported[0..3], b"FLV"); + assert!(exported.starts_with(b"FLV")); let avc_seq = tags .iter() - .filter(|t| t.tag_type == super::TAG_VIDEO && t.body[1] == super::AVC_SEQUENCE_HEADER) + .filter(|t| { + t.tag_type == super::TAG_VIDEO + && t.body.get(1).is_some_and(|b| *b == super::AVC_SEQUENCE_HEADER) + }) .count(); let aac_seq = tags .iter() - .filter(|t| t.tag_type == super::TAG_AUDIO && t.body[1] == super::AAC_SEQUENCE_HEADER) + .filter(|t| { + t.tag_type == super::TAG_AUDIO + && t.body.get(1).is_some_and(|b| *b == super::AAC_SEQUENCE_HEADER) + }) .count(); let video_frames = tags .iter() - .filter(|t| t.tag_type == super::TAG_VIDEO && t.body[1] == super::AVC_NALU) + .filter(|t| { + t.tag_type == super::TAG_VIDEO + && t.body.get(1).is_some_and(|b| *b == super::AVC_NALU) + }) .count(); let audio_frames = tags .iter() - .filter(|t| t.tag_type == super::TAG_AUDIO && t.body[1] == super::AAC_RAW) + .filter(|t| { + t.tag_type == super::TAG_AUDIO + && t.body.get(1).is_some_and(|b| *b == super::AAC_RAW) + }) .count(); let video_ts: Vec<u32> = tags .iter() - .filter(|t| t.tag_type == super::TAG_VIDEO && t.body[1] == super::AVC_NALU) + .filter(|t| { + t.tag_type == super::TAG_VIDEO + && t.body.get(1).is_some_and(|b| *b == super::AVC_NALU) + }) .map(|t| t.timestamp) .collect();Also applies to: 172-176, 184-188, 243-243
🤖 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` at line 128, Replace brittle indexing patterns throughout the test file to prevent panic-driven failures. In rs/moq-mux/src/container/flv/export_test.rs at line 128, replace the slice indexing `&exported[0..3]` with a `starts_with` check against b"FLV" instead. At lines 172-176 and 184-188 where tag filters use direct indexing like array access to check tag types or fields, replace those with safe `get()` method calls that return an Option, allowing proper diagnostic failure messages instead of panics. At line 243, apply the same `starts_with` pattern instead of slice indexing. These changes ensure tests fail with meaningful assertions rather than index out-of-bounds panics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-mux/src/container/flv/export.rs`:
- Around line 38-39: The documentation at lines 38-39 states that only Legacy
tracks are supported, but the `ensure_legacy` function actually accepts both
`Container::Legacy` and `Container::Loc`. Update the documentation comments at
both the anchor site (lines 38-39) and the sibling site (lines 355-360) to
accurately reflect that the function accepts both Legacy and Loc container
types, ensuring the documented behavior matches what the `ensure_legacy`
function actually supports.
- Around line 344-353: The write_tag function silently truncates body lengths
larger than 0x00FFFFFF (16,777,215 bytes) when converting to 24-bit format,
resulting in corrupted FLV tag headers. Add a validation check at the beginning
of the write_tag function to ensure body.len() does not exceed the maximum
24-bit value (0x00FFFFFF), and reject or return an error if the body is too
large. This prevents silent data corruption and ensures only valid tag sizes are
written to the output stream.
In `@rs/moq-mux/src/container/flv/import.rs`:
- Around line 104-108: The FLV data_offset value extracted from the file header
(bytes 5-8) is not validated against an upper bound, allowing a crafted FLV file
to specify an arbitrarily large offset that forces unbounded buffering and
memory growth. After extracting data_offset at lines 104-105, add an upper bound
constraint check to ensure the offset does not exceed a reasonable maximum value
(such as a sensible FLV header size limit like a few megabytes). Perform this
validation before the existing buffer length check on line 107 to prevent the
unbounded buffering issue.
In `@rs/moq-mux/src/container/mod.rs`:
- Line 22: The public module `flv` exported on line 22 is missing a doc comment.
Add a `///` doc comment above the `pub mod flv;` declaration to document the
module's purpose and functionality, following the coding guideline that requires
documentation for every exported Rust symbol.
---
Nitpick comments:
In `@rs/moq-mux/src/container/flv/export_test.rs`:
- Line 128: Replace brittle indexing patterns throughout the test file to
prevent panic-driven failures. In rs/moq-mux/src/container/flv/export_test.rs at
line 128, replace the slice indexing `&exported[0..3]` with a `starts_with`
check against b"FLV" instead. At lines 172-176 and 184-188 where tag filters use
direct indexing like array access to check tag types or fields, replace those
with safe `get()` method calls that return an Option, allowing proper diagnostic
failure messages instead of panics. At line 243, apply the same `starts_with`
pattern instead of slice indexing. These changes ensure tests fail with
meaningful assertions rather than index out-of-bounds panics.
🪄 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: 28700847-c2af-4e1a-8fe1-7ff47eb777a1
📒 Files selected for processing (11)
doc/bin/cli.mddoc/lib/rs/crate/moq-mux.mdrs/moq-cli/src/publish.rsrs/moq-cli/src/subscribe.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/flv/mod.rsrs/moq-mux/src/container/mod.rsrs/moq-mux/src/import.rs
Comment on lines
+38
to
+39
| /// keyframe). Only [`Legacy`](crate::catalog::hang::Container) tracks are | ||
| /// supported; CMAF tracks are rejected. |
Contributor
There was a problem hiding this comment.
Align docs with behavior for supported source containers.
Lines 38–39 state only Legacy tracks are supported, but ensure_legacy also accepts Container::Loc. Please update one side so behavior and docs match.
Also applies to: 355-360
🤖 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.rs` around lines 38 - 39, The
documentation at lines 38-39 states that only Legacy tracks are supported, but
the `ensure_legacy` function actually accepts both `Container::Legacy` and
`Container::Loc`. Update the documentation comments at both the anchor site
(lines 38-39) and the sibling site (lines 355-360) to accurately reflect that
the function accepts both Legacy and Loc container types, ensuring the
documented behavior matches what the `ensure_legacy` function actually supports.
| mod producer; | ||
| mod source; | ||
|
|
||
| pub mod flv; |
Contributor
There was a problem hiding this comment.
Add an item doc comment for the newly exported module.
Line 22 exports a new public module without /// docs, which leaves this symbol undocumented in-module.
💡 Suggested fix
- pub mod flv;
+ /// FLV (Flash Video / RTMP container) import and export support.
+ pub mod flv;As per coding guidelines, "Document every exported Rust symbol with doc comments (///), including every pub item and module-level docs (//! block at module root)`."
📝 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
| pub mod flv; | |
| /// FLV (Flash Video / RTMP container) import and export support. | |
| pub mod flv; |
🤖 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/mod.rs` at line 22, The public module `flv` exported
on line 22 is missing a doc comment. Add a `///` doc comment above the `pub mod
flv;` declaration to document the module's purpose and functionality, following
the coding guideline that requires documentation for every exported Rust symbol.
Source: Coding guidelines
…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
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.
Summary
Adds an FLV import (demuxer) and export (muxer) to
moq-mux, alongside the existing fMP4, MKV, and MPEG-TS containers.FLV is the classic RTMP container. It carries H.264 video as length-prefixed NALU with an out-of-band
AVCDecoderConfigurationRecord(avcC), and AAC audio raw with an out-of-bandAudioSpecificConfig. Both records map directly onto theLegacycontainer and the existingAvcc/aac::Configparsers, and the sample bytes pass straight through, so no codec transform is needed. Only the trivial tag framing is hand-rolled (wire-level framing, which the repo guidelines reserve bespoke code for).What's included
container::flv::Import— demuxes an FLV byte stream into a broadcast. Supports incremental/streaming input (buffers partial tags acrossdecodecalls), composition-time → PTS, and a sequence-header change rebuilding the track. The headerdata_offsetis bounded to avoid unbounded buffering on crafted input.container::flv::Export— muxes a broadcast back into FLV: file header, AVC/AAC sequence headers, then one tag per frame interleaved by timestamp. Accepts Avc3 (Annex-B) sources via the sharedAvc1transform, deferring the header until the codec config resolves. Rejects tag bodies larger than FLV's 24-bitDataSizeinstead of silently truncating.Flvinto theFramedFormat/StreamFormatdispatchers (string"flv"), including the multi-track guard inFramed::new_with_track.flvintomoq-clipublish(stdin demux) andsubscribe(--format flv).doc/bin/cli.mdand the supported-format list indoc/lib/rs/crate/moq-mux.md.Enhanced E-RTMP FourCC payloads (HEVC, AV1, Opus) and the older codecs (VP6, MP3) are logged and dropped on import, and rejected on export.
Public API changes (all additive)
moq_mux::container::flvwithImportandExport.#[non_exhaustive]enum variants:FramedFormat::Flv,StreamFormat::Flv.moq-clisubcommandpublish flvand--format flv.No wire-protocol or breaking API changes, so this targets
main.Cross-package sync
This is a
rs/moq-muxcontainer addition. The sync table doesn't list a JS counterpart for new containers (FLV is a native ingest/egress format), and themoq-cli+ doc rows are updated here.Test plan
cargo test -p moq-mux— 234 pass, incl. 7 new FLV tests (catalog population, frame decode, split input, non-FLV rejection, export round-trip through import, sequence-header/frame counts, timestamp preservation)cargo clippy -p moq-mux -p moq-cli --all-targetscleanmain;Framed::new_with_track(added by Mux import with existing track #1684) now coversFramedFormat::Flv(Written by Claude)