moq-mux: export SCTE-35 sections back to MPEG-TS#1685
Merged
Conversation
Mirror the SCTE-35 import: the catalog extension's scte35 cue tracks are written back to the transport stream as private sections. The PMT gains the program-level CUEI registration descriptor and an elementary stream at stream_type 0x86, so a re-import re-detects and reassembles the cue. Sections ride as Section (pointer_field) + Raw continuations, which the writer emits opaquely (no CRC, no reframing), so the splice_info_section round-trips byte-for-byte. A frame that isn't a complete section (table_id 0xFC, length matching section_length) is dropped with a warning rather than emitted malformed. SCTE-35 has no PTS and cannot carry the PCR, so a video or audio track is required; SCTE-35-only broadcasts are rejected. The exporter mirrors the importer's narrow generics, consumer side: Export<E: scte35::Catalog = ()> keeps the existing constructors on the () default (in-tree callers compile unchanged) and adds Export::<scte35::Ext>::with_extension for the extension path. CatalogSource<E> yields the full Catalog<E>; the MSF arm fills an empty extension (MSF is media-only by design), and the fmp4/mkv exporters keep their media-only view via .media(). A round-trip test (import bbb.ts plus a .scte35 track, export, re-import) asserts the cue survives byte-for-byte; a unit test covers section validation.
SCTE-35 annotates a broadcast program, and this bridge clocks each cue from the video PTS at ingest; a splice_info_section carries no PTS of its own. With no video track, the export has no clock the cues belong to (an audio-only ingest stamps them at zero), so a cue program without video is rejected. Programs without cues keep the existing video-or-audio PCR fallback. scte35_without_video_export_is_rejected pins the rule. CONTEXT Rejected alternatives: - Keep the audio PCR fallback for cue programs. The splice times inside the verbatim bytes survive either way, but the cue's delivery timing drives downstream placement, so a silently misplaced cue is worse than a loud rejection. - Justify the rule as a SCTE-35 conformance requirement. SCTE-35 does not mandate video: the splice_info_section is program-level, splice_insert component splicing (program_splice_flag = 0, per component_tag) can target audio components, audio dynamic ad insertion uses SCTE-35 without video, and the PCR may sit on any PID. Requiring video is a scope choice for broadcast, where the spliced program is video, plus the current clock model; it is not a conformance claim, and it reopens when the clock samples audio PIDs. Key decisions: - The cue's frame timestamp is its delivery time, the moment it arrives on the program clock, never the in-band splice_time. That splice_time is the splice instruction and travels verbatim in the payload; conflating the two misplaces the cue. This bridge samples the program clock from the video PTS only, so a program with no video collapses the delivery time to zero, the concrete failure the rule prevents.
Add a fixture corpus that runs the full TS -> MoQ -> TS seam and asserts every splice_info_section survives byte-for-byte. Four synthetic fixtures each cover an axis no other does: ffmpeg (baseline H.264, no audio), GStreamer (a second muxer, 480i, AAC), BigBuckBunny (H.265 + Opus, real frames), and TSDuck-authored splices (section repetition). The existing Kyrion mid-stream capture is reused as a fifth, real-encoder round-trip case. Each fixture pins its total cue count and its distinct cue count, so a clip that lost variety to duplicate sections fails. Every cue's splice_command_type must be a known SCTE-35 command; only the TSDuck-authored fixture, whose cues we control, pins its exact command-type set. TSDuck decodes every checked-in section CRC32 OK, and the decode is checked in beside each clip as <fixture>_tsduck.txt. examples/scte35_inject.rs generates the three injected fixtures by augmenting the PMT with CUEI + 0x86 and placing cues at spread positions. CONTEXT Rejected alternatives: - Assert only byte-exact survival, or pin every external fixture's exact command-type set. Pure survival would miss variety collapsing into duplicates, while pinning external threefive cue types would over-claim fixtures we do not author. The test pins (total, distinct) for every fixture and the exact command-type set only for the TSDuck-authored one. - Add regen or CI freshness machinery for the fixtures (R38b). The clips and their checked-in TSDuck decodes are static evidence; the regen command lives in the test doc comment, and more machinery would be maintenance without a maintainer. - Use threefive's TS-stream parser as the authority. It chokes on the Opus clip, so TSDuck's section decode is the authority; threefive's section-level decoder was used only to cross-check the hand-built custom cues before baking. Key decisions: - TSDuck sends five distinct cues twice, ten on the wire. This is the only coverage of section repetition; the reassembler keeps advancing-CC repeats and drops byte-identical same-CC transport duplicates. - Kyrion is not part of the authored injected corpus. It is reused as a separate real-encoder fixture, captured mid-stream, cues external and unpinned, to prove the round-trip on a real live feed.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds SCTE-35 splice cue support to the MOQ muxer's MPEG-TS output. The catalog infrastructure becomes generic over extension types (e.g., SCTE-35), enabling specialized track handling. The TS exporter gains SCTE-35 as a new track kind, allocates dedicated PIDs, injects CUEI program-level descriptors in the PMT, and packetizes cue payloads as private sections instead of PES. Container adapters update to use media-only catalog snapshots. Tests validate round-trip export/import, PMT structure validation, and fixture byte-for-byte survival. An example utility demonstrates creating SCTE-35 TS fixtures by augmenting input streams. 🚥 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: 1
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)
214-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't surface a chunk for SCTE-35 frames you explicitly drop.
When
write_section()decides a cue is malformed, it returnsOk(()), butwrite_frame()still handspoll_next()a chunk for that frame. On Line 472 that becomes an emptyByteswhen PSI is not due, and a PAT/PMT-only chunk when PSI is due. That is not a real “drop”, and zero-length outputs can make callers spin.💡 One way to keep the drop path truly silent
- let chunk = self.write_frame(&name, frame)?; - return Poll::Ready(Ok(Some(chunk))); + if let Some(chunk) = self.write_frame(&name, frame)? { + return Poll::Ready(Ok(Some(chunk))); + } + continue; } @@ - fn write_frame(&mut self, name: &str, frame: Frame) -> anyhow::Result<Bytes> { + fn write_frame(&mut self, name: &str, frame: Frame) -> anyhow::Result<Option<Bytes>> { @@ - match es_payload { + match es_payload { // SCTE-35 rides in private sections, not PES; carry the bytes verbatim. - None => self.write_section(&mut out, pid, &frame.payload)?, + None => { + if !self.write_section(&mut out, pid, &frame.payload)? { + return Ok(None); + } + } Some(es_payload) => { @@ - Ok(Bytes::from(out)) + Ok(Some(Bytes::from(out))) } @@ - fn write_section(&mut self, out: &mut Vec<u8>, pid: u16, section: &[u8]) -> anyhow::Result<()> { + fn write_section(&mut self, out: &mut Vec<u8>, pid: u16, section: &[u8]) -> anyhow::Result<bool> { @@ if !is_complete_scte35_section(section) { tracing::warn!(pid, len = section.len(), "dropping malformed SCTE-35 section on export"); - return Ok(()); + return Ok(false); } @@ - Ok(()) + Ok(true) }Also applies to: 470-484, 564-571
🤖 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 214 - 219, poll_next is emitting a chunk even for SCTE-35 frames that write_section treats as malformed (write_section returns Ok(())), leading to empty Bytes or PAT/PMT-only outputs; modify write_frame (called from poll_next/pick_next_track) so it returns a Result<Option<Chunk>, Error> (or otherwise signals "dropped") and ensure poll_next only returns a Poll::Ready(Ok(Some(chunk))) when write_frame indicates a real chunk; specifically update write_frame to check the result of write_section and return None when write_section intentionally drops the cue, and update the call sites (in poll_next where pick_next_track and tracks.get_mut(...).pending are used) to skip emitting any chunk if write_frame signals None.
🤖 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/examples/scte35_inject.rs`:
- Around line 69-72: The code uses direct indexing (args[1], args[2]) which
panics with "index out of bounds" on missing args; change to consistent safe
parsing using args.get(1).expect(...), args.get(2).expect(...), and keep the
existing args.get(3).expect(...) usage so all three arguments produce the same
helpful usage message. Update the variables input, out_path, and set assignments
to fetch via .get(...) and then call std::fs::read on the unwrapped input
string, ensuring the error messages mention the expected usage.
---
Outside diff comments:
In `@rs/moq-mux/src/container/ts/export.rs`:
- Around line 214-219: poll_next is emitting a chunk even for SCTE-35 frames
that write_section treats as malformed (write_section returns Ok(())), leading
to empty Bytes or PAT/PMT-only outputs; modify write_frame (called from
poll_next/pick_next_track) so it returns a Result<Option<Chunk>, Error> (or
otherwise signals "dropped") and ensure poll_next only returns a
Poll::Ready(Ok(Some(chunk))) when write_frame indicates a real chunk;
specifically update write_frame to check the result of write_section and return
None when write_section intentionally drops the cue, and update the call sites
(in poll_next where pick_next_track and tracks.get_mut(...).pending are used) to
skip emitting any chunk if write_frame signals None.
🪄 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: 135f485f-0ed5-45c5-a838-7045c9754b16
📒 Files selected for processing (14)
rs/moq-mux/examples/scte35_inject.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/mkv/export.rsrs/moq-mux/src/container/source.rsrs/moq-mux/src/container/ts/export.rsrs/moq-mux/src/container/ts/export_test.rsrs/moq-mux/src/container/ts/test_data/scte35/bbb5s.tsrs/moq-mux/src/container/ts/test_data/scte35/bbb5s_tsduck.txtrs/moq-mux/src/container/ts/test_data/scte35/ffmpeg.tsrs/moq-mux/src/container/ts/test_data/scte35/ffmpeg_tsduck.txtrs/moq-mux/src/container/ts/test_data/scte35/gst480i.tsrs/moq-mux/src/container/ts/test_data/scte35/gst480i_tsduck.txtrs/moq-mux/src/container/ts/test_data/scte35/tsduck.tsrs/moq-mux/src/container/ts/test_data/scte35/tsduck_tsduck.txt
Comment on lines
+69
to
+72
| let args: Vec<String> = std::env::args().collect(); | ||
| let input = std::fs::read(&args[1]).expect("read input TS"); | ||
| let out_path = &args[2]; | ||
| let set = args.get(3).expect("usage: scte35_inject input.ts output.ts <set>"); |
Contributor
There was a problem hiding this comment.
Inconsistent argument parsing produces unhelpful error messages.
Lines 70-71 use unsafe indexing (args[1], args[2]) while line 72 uses safe .get(3). If the example is invoked with missing arguments, the user sees "index out of bounds" instead of the helpful usage message from line 72.
🛠️ Proposed fix for consistent error handling
fn main() {
let args: Vec<String> = std::env::args().collect();
- let input = std::fs::read(&args[1]).expect("read input TS");
- let out_path = &args[2];
- let set = args.get(3).expect("usage: scte35_inject input.ts output.ts <set>");
+ if args.len() < 4 {
+ eprintln!("usage: scte35_inject input.ts output.ts <set>");
+ std::process::exit(1);
+ }
+ let input = std::fs::read(&args[1]).expect("read input TS");
+ let out_path = &args[2];
+ let set = &args[3];
let cues_b64 = &SETS
.iter()
- .find(|(name, _)| name == set)
+ .find(|(name, _)| name == &set)
.unwrap_or_else(|| panic!("unknown set '{set}'; available: gst480i, bbb5s, ffmpeg"))
.1;🤖 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/examples/scte35_inject.rs` around lines 69 - 72, The code uses
direct indexing (args[1], args[2]) which panics with "index out of bounds" on
missing args; change to consistent safe parsing using args.get(1).expect(...),
args.get(2).expect(...), and keep the existing args.get(3).expect(...) usage so
all three arguments produce the same helpful usage message. Update the variables
input, out_path, and set assignments to fetch via .get(...) and then call
std::fs::read on the unwrapped input string, ensuring the error messages mention
the expected usage.
Contributor
Author
|
ok rabbit, last comments find a minor bug accepted, it lives in the examples not in production code, but will fix alonside @kixelated reviews/comments. |
Replace the generic `with_extension` constructor with a concrete one on
`impl Export<scte35::Ext>`. Because the impl pins `E`, callers write
`Export::with_scte35(..)` with no `::<scte35::Ext>` turbofish, mirroring how
`Export::new`/`with_catalog_format` pin `Export<()>`. Both delegate to a private
generic `build`. The `scte35::Catalog` bound only ever resolves to `()` or
`scte35::Ext`, so a single shared generic constructor bought no real generality.
Also fold two `match res { Some => .., None => break }` test loops into
`let ... else { break }`.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a note under PR Reviews to enumerate public API changes (and flag breaking ones per Branch Targeting), distinguishing public surface from pub(crate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merged
This was referenced Jun 12, 2026
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.
Follow-up to #1617 (SCTE-35 ingest, merged). That PR carried SCTE-35 cues from
MPEG-TS into a MoQ catalog extension; this one mirrors it on the way out, writing
the cue tracks back to MPEG-TS. Together they form a byte-exact MoQ <-> MPEG-TS
round-trip for splice information.
Export
A
.scte35cue track is remuxed back to the transport stream as private sections:stream at stream_type 0x86, so a re-import re-detects and reassembles the cue
(the exact signal moq-mux: ingest SCTE-35 from MPEG-TS, and tolerate mid-stream joins #1617 keys off).
Sectionpayload (pointer_field, unit-start bit) plusRawcontinuations, emitted opaquely: no CRC, no reframing. Thesplice_info_sectionround-trips byte-for-byte.section_length) is dropped with a warning rather than emitted malformed. Thisis transport policy, not payload interpretation; the verbatim bytes are never
touched.
The bridge stays media-agnostic at the MoQ layer: cues are carried as opaque
frames and never parsed semantically.
A cue program requires a video track
The bridge clocks each cue from the video PTS at ingest; a
splice_info_sectioncarries no PTS of its own. With no video track the export has no clock the cues
belong to (an audio-only ingest stamps them at zero), so a cue program without
video is rejected. Programs without cues keep the existing video-or-audio PCR
fallback.
This is a scope choice for broadcast plus the current clock model, not a SCTE-35
conformance claim: SCTE-35 is program-level, component splicing can target audio,
and audio dynamic ad insertion uses SCTE-35 without video. This rule can be
revisited if the bridge grows a clock source independent of video PTS.
API
Export<E: scte35::Catalog = ()>. The plain constructors (new,with_catalog_format) live onimpl Export<()>and stay media-only; moq-clidoes not reference the SCTE types, so the default
Export<()>path is unchanged.A concrete
impl Export<scte35::Ext>addsExport::with_scte35, which adds thecue path; pinning the extension on the impl means callers write
Export::with_scte35(..)with no turbofish (same waynewpinsExport<()>).The fmp4/mkv exporters keep their media-only view via
.media().Tests
A round-trip corpus runs the full TS -> MoQ -> TS seam and asserts every
splice_info_sectionsurvives byte-for-byte, pinning each fixture's total anddistinct cue counts so a clip that lost variety to duplicate sections fails:
covering a muxer/codec/audio axis the others don't (H.264 video-only; H.264
480i + AAC; H.265 + Opus).
cues sent twice).
real-encoder case.
TSDuck decodes every checked-in section CRC32-OK; the decode is checked in beside
each clip as
<fixture>_tsduck.txt.examples/scte35_inject.rsgenerates thethree injected fixtures.
The three commits are bisectable (export, force-video, corpus), each green alone.
Test plan
cargo test -p moq-muxgreen (203 tests; 201/202/203 per commit)Export<()>path unchanged)No JS or doc sync: carriage only, no wire or catalog-format change.