moq-mux: ingest SCTE-35 from MPEG-TS, and tolerate mid-stream joins#1617
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds SCTE-35 cue track support to the MOQ muxer by introducing a new catalog extension type, generifying codec and TS importers over catalog extensions, and implementing SCTE-35 packet detection, reassembly, and publishing. The TS importer now detects SCTE-35 PIDs via PMT CUEI descriptors, buffers and resynchronizes incoming packets on 188-byte boundaries, routes SCTE-35 data to a dedicated reassembly pipeline instead of the regular mpeg2ts reader, and stamps completed cues with a media clock derived from video PTS. The container producer gains a lenient-start mode to support mid-stream joins without initial keyframes. Extensive tests validate packet resynchronization, mid-stream join robustness, and correct SCTE-35 cue extraction from real captures. 🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ts/import.rs`:
- Around line 120-125: The loop advances off by a full TsPacket::SIZE before
validating the sync byte, so a single-byte misalignment causes permanent desync;
change the logic in the loop in import.rs so you only advance by TsPacket::SIZE
when the packet starts with 0x47 and otherwise advance off by 1 to scan
byte-wise for the next sync. Concretely: in the loop around scratch/off and
TsPacket::SIZE (where pkt is created), don't unconditionally increment off by
TsPacket::SIZE; instead validate pkt[0] == 0x47 first — if valid, then consume
the packet by adding TsPacket::SIZE to off, otherwise increment off by 1 (or set
off = off - TsPacket::SIZE + 1 if you prefer to keep creating pkt before the
check) to perform a byte-wise resync.
🪄 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: 8c5d851e-b99e-41df-b614-3fc8b7e96e78
📒 Files selected for processing (7)
js/hang/src/catalog/data.tsjs/hang/src/catalog/index.tsjs/hang/src/catalog/root.tsrs/hang/src/catalog/data/mod.rsrs/hang/src/catalog/mod.rsrs/hang/src/catalog/root.rsrs/moq-mux/src/container/ts/import.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/hang/src/catalog/scte35.rs (1)
52-60: 💤 Low valueUnnecessary attribute on struct with no
Optionfields.The
#[serde_with::skip_serializing_none]attribute on line 52 has no effect becauseScte35Configcontains noOption<T>fields (only a barecontainer: Container). This attribute only impactsOptiontypes. While harmless, it may be misleading.If this is intentional for consistency with other catalog types or future-proofing, consider adding a comment. Otherwise, the attribute can be removed.
♻️ Proposed fix to remove the unused attribute
-#[serde_with::skip_serializing_none] #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)]🤖 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/scte35.rs` around lines 52 - 60, The #[serde_with::skip_serializing_none] attribute on the Scte35Config struct is redundant because Scte35Config has no Option<T> fields; remove the attribute from the Scte35Config declaration (or if you intentionally want consistency/future-proofing, replace it with a brief comment above Scte35Config explaining why it’s kept). Update the declaration that references Scte35Config and the attribute so only the necessary derives/attributes remain.
🤖 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/hang/src/catalog/scte35.rs`:
- Around line 52-60: The #[serde_with::skip_serializing_none] attribute on the
Scte35Config struct is redundant because Scte35Config has no Option<T> fields;
remove the attribute from the Scte35Config declaration (or if you intentionally
want consistency/future-proofing, replace it with a brief comment above
Scte35Config explaining why it’s kept). Update the declaration that references
Scte35Config and the attribute so only the necessary derives/attributes remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9af485c0-40cf-4263-ba66-3afa7fe89339
📒 Files selected for processing (8)
js/hang/src/catalog/index.tsjs/hang/src/catalog/root.tsjs/hang/src/catalog/scte35.tsrs/hang/src/catalog/mod.rsrs/hang/src/catalog/root.rsrs/hang/src/catalog/scte35.rsrs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/container/ts/import_test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- js/hang/src/catalog/index.ts
- rs/moq-mux/src/container/ts/import_test.rs
- rs/moq-mux/src/container/ts/import.rs
arielmol
changed the title
| export const RootSchema = z.object({ | ||
| video: z.optional(VideoSchema), | ||
| audio: z.optional(AudioSchema), | ||
| scte35: z.optional(Scte35Schema), |
There was a problem hiding this comment.
How about we go back to your meta section instead of scte35 at the root?
| // CUEI signaling are implicit to SCTE-35. | ||
| export const Scte35ConfigSchema = z.object({ | ||
| // The container format, used to decode the timestamp. | ||
| container: ContainerSchema, |
There was a problem hiding this comment.
...do you really want to support CMAF? I would just hard-code legacy for now, we can always negotiate a different container later.
|
|
||
| export const Scte35Schema = z.object({ |
There was a problem hiding this comment.
I think nest scte35 an extra level.
export const MetaSchema = z.object({
scte35: z.optional(z.record(z.string(), Scte35Schema)),
});
There was a problem hiding this comment.
Or maybe we just make it:
export const MetaSchema = z.object({});
And it's up to external applications to determine how to signal "scte35" tracks. hang feels like a weird spot for scte35 especially since TS is explicitly not supported as a container.
There was a problem hiding this comment.
export const MetaSchema = z.object({});
(from Discord)
i went typed "scte35" bcause of your earlier pushback on a generic data bucket but export const MetaSchema = z.object({}); and
it's up to external applications to determine how to signal "scte35" tracks."
signals the other way I'm happy either way, I just don't want to guess wrong. Which is the target? Typed meta.scte35 or empty metaSchema?
|
Also a claude review: Solid PR overall — the byte-level reassembler is carefully written and the test coverage for the reassembler edge cases is genuinely good. The catalog typing is clean and the serde round-trip test pins the wire format properly. My findings cluster around detection robustness and timestamping, plus a few cleanups. Correctness
let scte = pmt.program_info.iter().any(|d| d.tag == 0x05 && ... == b"CUEI");
last_pts is only advanced in the H264 | H265 branch of handle_pes_start. For an audio-only TS with cues, or for any cue arriving before the first video PES, last_pts is None, so every such section is emitted at timestamp 0 and they collapse onto the same timeline point. Worth falling back to audio PTS (or wall clock) when video is absent.
ScteStream::seek only seeks the track; ScteReassembler (acc, last_cc) and media_unwrap/last_pts are untouched. On a loop/restart, a partial section left in acc from before the seek can be completed with post-seek bytes and emitted as a corrupt frame (it can still pass the 0xFC + length checks). Clear the reassembler on seek.
The comment says scanning for 0x47 one byte at a time "recovers instead of desyncing permanently," but once it locks onto a 0x47 it feeds those 188 bytes straight to read_ts_packet()?. I confirmed in the mpeg2ts source that an unknown PID (or "unexpected remaining data") returns Err, which ? propagates and aborts the entire decode(). So a false-sync hit on garbage — or any elementary/SCTE packet that legally precedes the first PMT — kills ingest rather than recovering. (This abort-on-unknown-PID is pre-existing reader strictness, but the new comment claims a robustness the code doesn't deliver.) Worth swallowing the per-packet reader error instead of ?-propagating.
If a CUEI program also carries real DTS-HD lossless audio (stream_type 0x86), that audio PID is also routed to ensure_scte and fed into the section reassembler, dropping the audio track. Rare, but the per-ES check from #1 is the correct fix. Cleanup / efficiency The old path locked the Feed mutex once and drained the whole buffer; the rewrite, for every non-SCTE packet, does feed.lock() + data.clear() + extend_from_slice(&pkt) + a one-iteration read_ts_packet loop. For a 64 KB chunk that's ~340 locks/clears and a second full copy of every media byte (scratch → feed → reader). You could feed contiguous runs of non-SCTE packets in one drain, breaking the run only to divert a SCTE PID.
media_unwrap runs the same video PTS sequence through a second PtsUnwrap that the Stream::H264/H265 variant already unwraps. Two state machines tracking the same wrap can drift. Consider having the video stream expose its latest unwrapped PTS and reading the media clock from there.
if pes.header.pts.is_some() { ... pes.header.pts.map(|t| t.as_u64()) ... } then unwrap_pts re-matches the Option. Cleaner as if let Some(pts) = pes.header.pts { self.last_pts = unwrap_pts(&mut self.media_unwrap, Some(pts.as_u64()))?; }.
It inserts into the raw BTreeMap (silent overwrite) rather than using the Scte35::insert helper that returns Error::Duplicate. Safe today thanks to unique_track, but it skips the invariant the helper exists to enforce. Note (pre-existing, but amplified by this feature) |
A SCTE-35 PID is identified by a program-level CUEI registration descriptor paired with stream_type 0x86, and carries splice_info_sections in private sections, not PES. The mpeg2ts reader PES-parses every PMT elementary PID and aborts on these, so intercept SCTE PIDs before the reader and reassemble their sections onto an application catalog extension (Scte35Ext / Scte35Catalog), keeping the cue schema out of hang. - ScteReassembler: pointer_field alignment, sections split across packets (including a 3-byte header split), continuity gaps, adaptation-field discontinuities, malformed pointer_fields, and stuffing; after a loss it resyncs at the next pointer_field instead of parsing unaligned bytes. - Broadcast hardening: a transport_error_indicator drops the partial and resyncs, an ISO 13818-1 duplicate payload packet is skipped, and PIDs we don't decode never reach the PES reader, where a private section would otherwise be fatal. - Media clock: MPEG-1/2 video advances the cue clock via Stream::Clock, so a feed without H.264/H.265 (MPEG-2 video, or audio-only) stamps real PTS instead of zero. Audio and private PES never drive the clock. - The codec importers (aac/h264/h265) are generic over the catalog extension, so an application can route cues into its own catalog section. - Each whole splice_info_section is published verbatim as a frame on a `.scte35` track, timestamped with the video PTS. - Unit tests cover the reassembly edge cases, the resync, the hardening paths, and the media-clock stamping. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A live capture joins a transport stream already in flight. A netcat grab of an Ateme Kyrion encoder opens on a video continuation packet, with hundreds of media packets before the first PAT/PMT, and the first decodable access unit is a delta. Two importer assumptions aborted the decode on it. Gate what reaches the mpeg2ts reader: feed only the PAT, the PMT PIDs it announces, and the elementary PIDs a PMT registers. Unrouted packets drop until the layout is learned (PSI repeats), then normal demux resumes, so a PES arriving before its PSI no longer aborts the reader. Add an opt-in lenient_start mode to the shared container Producer that drops leading non-keyframes until a keyframe anchors a group. The H.264 and H.265 importers enable it; the strict default is unchanged. survives_midstream_join carves a synthetic mid-GOP join from bbb.ts and exercises both paths. kyrion_dirtystart_extracts_real_cues runs a real Kyrion broadcast captured mid-stream and extracts all six SCTE-35 splice_inserts, which TSDuck decodes CRC32 OK in the golden decode checked in beside the fixture. The fixture is a 1.4 MB excerpt of our own encoder's output, a black feed with no broadcast content. It was captured raw with netcat (no transcode or re-mux), truncated after the last cue with null-packet stuffing dropped, so the video, audio, and SCTE-35 payload stays byte-exact. CONTEXT Rejected alternatives: - Swallow the mpeg2ts reader error to skip the unknown PID. mpeg2ts collapses unknown-PID, genuine corruption, and "unexpected remaining data" into one InvalidInput kind, so matching the error would also hide real faults. The gate avoids reaching that error path at all. - Relax the Producer keyframe invariant globally. It guards against a producer that never marks a keyframe (the first_frame_must_be_keyframe test pins it), so it stays strict and only the video importers opt in. - Duplicate a "wait for the first keyframe" inside each codec importer. It would rot when a codec is added; the shared lenient_start lets a future MPEG-2 transcoder or AV1 inherit it by setting the same flag. Key decisions: - The gate also drops DVB SI PIDs (SDT, EIT, and friends) structurally. Before, low PIDs reached the reader and survived only incidentally as raw payload; a non-routable PID >= 0x20 would have aborted with "unknown PID".
arielmol
changed the title
Heads up!This PR was force-pushed to a clean two-commit history, rebased onto current main (post #1658 / #1667). The earlier discussion and the CodeRabbit walkthrough describe the original approach, a typed scte35 section inside hang plus JS/zod changes which has been dropped. Please disregard the inline comments on that code; they're historical but kept on the same PR to follow the story :) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
rs/moq-mux/src/container/ts/scte35_ext.rs (2)
42-46: ⚡ Quick winAdd
#[non_exhaustive]toScte35Ext.
Scte35Extis a public catalog extension struct that may gain additional sections (e.g.,scte104, metadata). Per coding guidelines, public config structs should have#[non_exhaustive].Suggested fix
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)] +#[non_exhaustive] pub struct Scte35Ext { #[serde(default, skip_serializing_if = "Scte35::is_empty")] pub scte35: Scte35, }🤖 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/scte35_ext.rs` around lines 42 - 46, Add the #[non_exhaustive] attribute to the public struct Scte35Ext so the API can be extended later; locate the Scte35Ext definition (the struct with derives Serialize, Deserialize, Debug, Clone, PartialEq, Default and the field scte35 that uses Scte35::is_empty) and place #[non_exhaustive] immediately above the struct declaration.Source: Coding guidelines
11-15: ⚡ Quick winAdd
#[non_exhaustive]toScte35.
Scte35is a public config-like struct that may gain fields (e.g., metadata, version info). Per coding guidelines, public config structs should have#[non_exhaustive]so external code constructing them via struct literals keeps compiling when fields are added.Suggested fix
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)] #[serde(rename_all = "camelCase")] +#[non_exhaustive] pub struct Scte35 { pub renditions: BTreeMap<String, Scte35Config>, }🤖 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/scte35_ext.rs` around lines 11 - 15, Add the #[non_exhaustive] attribute to the public struct Scte35 so external users constructing Scte35 via struct literals won't break when new fields are added; locate the Scte35 definition (the struct with the renditions: BTreeMap<String, Scte35Config> field) and add #[non_exhaustive] immediately above the #[derive(...)]/struct declaration to mark it non-exhaustive.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-mux/src/container/ts/import.rs`:
- Around line 151-155: The SCTE-35 cues are being stamped with Timestamp::ZERO
when self.last_pts is None; add tracking for audio PTS (e.g., a new field
last_audio_pts updated when audio PES packets are parsed) and change the pts
selection before calling scte.packet(&pkt, pts) to prefer last_pts, then
last_audio_pts, and only then a wall-clock fallback (e.g., current time
converted to a Timestamp) instead of unconditionally using Timestamp::ZERO;
update the struct to hold last_audio_pts, set it where audio PES are processed,
and replace the pts assignment at the scte.packet call to use
last_pts.or(last_audio_pts).or_else(wall_clock) so early SCTE cues preserve
relative ordering.
- Around line 193-207: The CUEI detection only looks at pmt.program_info, so
update the logic to also inspect each ES's descriptors (es.descriptors /
es_info[].descriptors) and compute a per-ES scte flag (falling back to the
program-level detection only if ES-level descriptor absent); inside the for es
in &pmt.es_info loop, determine if that ES has a CUEI registration descriptor
(tag 0x05 and data starts with b"CUEI") and use that per-ES scte decision to
call ensure_scte(es.elementary_pid) versus ensure_stream(es.elementary_pid,
es.stream_type), ensuring genuine StreamType::Dts8ChannelLosslessAudio is only
routed to SCTE handling when its own ES descriptors indicate CUEI.
- Around line 474-477: The seek method (fn seek) must clear the ScteReassembler
state to avoid completing a pre-seek partial SCTE-35 section with post-seek
bytes; update seek(&mut self, sequence: u64) to reset/clear the ScteReassembler
(e.g. call self.scte_reassembler.reset()/clear() or replace it with a fresh
instance) before/after calling self.track.seek(sequence)? to ensure no buffered
bytes survive the discontinuity, then return Ok(()). Locate the ScteReassembler
field on the same struct and invoke its clear/reset method (or add one) inside
the seek function.
---
Nitpick comments:
In `@rs/moq-mux/src/container/ts/scte35_ext.rs`:
- Around line 42-46: Add the #[non_exhaustive] attribute to the public struct
Scte35Ext so the API can be extended later; locate the Scte35Ext definition (the
struct with derives Serialize, Deserialize, Debug, Clone, PartialEq, Default and
the field scte35 that uses Scte35::is_empty) and place #[non_exhaustive]
immediately above the struct declaration.
- Around line 11-15: Add the #[non_exhaustive] attribute to the public struct
Scte35 so external users constructing Scte35 via struct literals won't break
when new fields are added; locate the Scte35 definition (the struct with the
renditions: BTreeMap<String, Scte35Config> field) and add #[non_exhaustive]
immediately above the #[derive(...)]/struct declaration to mark it
non-exhaustive.
🪄 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: 684ca82b-bf9a-4f93-9ffe-0ce6fe927934
📒 Files selected for processing (10)
rs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/container/ts/import_test.rsrs/moq-mux/src/container/ts/mod.rsrs/moq-mux/src/container/ts/scte35_ext.rsrs/moq-mux/src/container/ts/test_data/scte35/kyrion_dirtystart.tsrs/moq-mux/src/container/ts/test_data/scte35/kyrion_dirtystart_tsduck.txt
✅ Files skipped from review due to trivial changes (1)
- rs/moq-mux/src/container/ts/mod.rs
| let pts = self.last_pts.unwrap_or(Timestamp::ZERO); | ||
| if let Some(scte) = self.scte.get_mut(&pid) { | ||
| scte.packet(&pkt, pts)?; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
SCTE-35 sections stamp at Timestamp::ZERO before video arrives.
When last_pts is None (no video PES seen yet), SCTE-35 cues are stamped at Timestamp::ZERO. This collapses all early cues to the same timestamp and loses their relative ordering. Per reviewer feedback, consider falling back to audio PTS or wall clock instead.
A possible approach: track last_audio_pts and use last_pts.or(last_audio_pts).unwrap_or(Timestamp::ZERO), or use a wall-clock fallback when both are 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/ts/import.rs` around lines 151 - 155, The SCTE-35
cues are being stamped with Timestamp::ZERO when self.last_pts is None; add
tracking for audio PTS (e.g., a new field last_audio_pts updated when audio PES
packets are parsed) and change the pts selection before calling
scte.packet(&pkt, pts) to prefer last_pts, then last_audio_pts, and only then a
wall-clock fallback (e.g., current time converted to a Timestamp) instead of
unconditionally using Timestamp::ZERO; update the struct to hold last_audio_pts,
set it where audio PES are processed, and replace the pts assignment at the
scte.packet call to use last_pts.or(last_audio_pts).or_else(wall_clock) so early
SCTE cues preserve relative ordering.
| // SCTE-35 is announced by a program-level registration descriptor with | ||
| // format_identifier 'CUEI' (ITU-T J.181). The stream itself uses | ||
| // stream_type 0x86, which mpeg2ts maps to a DTS audio variant, so | ||
| // detection keys off the CUEI descriptor, not the stream type alone. | ||
| let scte = pmt | ||
| .program_info | ||
| .iter() | ||
| .any(|d| d.tag == 0x05 && d.data.len() >= 4 && &d.data[0..4] == b"CUEI"); | ||
| for es in &pmt.es_info { | ||
| self.ensure_stream(es.elementary_pid, es.stream_type)?; | ||
| if scte && matches!(es.stream_type, StreamType::Dts8ChannelLosslessAudio) { | ||
| self.ensure_scte(es.elementary_pid)?; | ||
| } else { | ||
| self.ensure_stream(es.elementary_pid, es.stream_type)?; | ||
| } | ||
| } |
There was a problem hiding this comment.
CUEI detection only checks program-level descriptors; many streams signal it per-ES.
The current implementation checks only pmt.program_info for the CUEI registration descriptor. Some encoders place CUEI in the ES-level es.descriptors loop instead (or in addition). Additionally, with program-level CUEI, a genuine 0x86 DTS-HD audio ES would be misrouted to SCTE handling.
Consider also checking es.descriptors for each elementary stream to:
- Catch SCTE-35 PIDs signaled only at ES level
- Disambiguate genuine 0x86 audio from SCTE-35 when CUEI appears per-ES rather than program-wide
🤖 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/import.rs` around lines 193 - 207, The CUEI
detection only looks at pmt.program_info, so update the logic to also inspect
each ES's descriptors (es.descriptors / es_info[].descriptors) and compute a
per-ES scte flag (falling back to the program-level detection only if ES-level
descriptor absent); inside the for es in &pmt.es_info loop, determine if that ES
has a CUEI registration descriptor (tag 0x05 and data starts with b"CUEI") and
use that per-ES scte decision to call ensure_scte(es.elementary_pid) versus
ensure_stream(es.elementary_pid, es.stream_type), ensuring genuine
StreamType::Dts8ChannelLosslessAudio is only routed to SCTE handling when its
own ES descriptors indicate CUEI.
| fn seek(&mut self, sequence: u64) -> anyhow::Result<()> { | ||
| self.track.seek(sequence)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
seek() does not reset ScteReassembler state.
A partial SCTE-35 section buffered before seek could be completed with post-seek bytes, producing a corrupt frame. Clear the reassembler on seek to prevent cross-discontinuity corruption.
Suggested fix
fn seek(&mut self, sequence: u64) -> anyhow::Result<()> {
+ self.reassembler = ScteReassembler::default();
self.track.seek(sequence)?;
Ok(())
}📝 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.
| fn seek(&mut self, sequence: u64) -> anyhow::Result<()> { | |
| self.track.seek(sequence)?; | |
| Ok(()) | |
| } | |
| fn seek(&mut self, sequence: u64) -> anyhow::Result<()> { | |
| self.reassembler = ScteReassembler::default(); | |
| self.track.seek(sequence)?; | |
| 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/ts/import.rs` around lines 474 - 477, The seek
method (fn seek) must clear the ScteReassembler state to avoid completing a
pre-seek partial SCTE-35 section with post-seek bytes; update seek(&mut self,
sequence: u64) to reset/clear the ScteReassembler (e.g. call
self.scte_reassembler.reset()/clear() or replace it with a fresh instance)
before/after calling self.track.seek(sequence)? to ensure no buffered bytes
survive the discontinuity, then return Ok(()). Locate the ScteReassembler field
on the same struct and invoke its clear/reset method (or add one) inside the
seek function.
|
I see, rabbit. 1) The media clock is video-only by design: last_pts advances solely on a video PES, and for thread at import.rs:207 (CUEI per-ES / 0x86 disambiguation), ES-level-only CUEI is real and additive. Every encoder in scope, and all the checked-in fixtures (ffmpeg, GStreamer, TSDuck, and the real Ateme CM5000 "Kyrion" capture), signals CUEI at the program level, which is what detection keys off today. The last one is real, ScteStream::seek seeks the track but leaves a half-buffered section in the reassembler, so a partial could splice onto post-seek bytes and emit a corrupt section. The fix is small: a reset() that clears I'll fold that last one it in with the rest of @kixelated reviews rather than force-push the branch again now :) |
Drop the redundant `Scte35` prefix now that the module name supplies it. The module `scte35_ext` becomes `scte35` (public, referenced module-qualified), and the types lose the prefix: `Scte35` -> `Cues`, `Scte35Config` -> `Config`, `Scte35Ext` -> `Ext`, `Scte35Catalog` -> `Catalog`. Call sites now read `scte35::Ext`, `scte35::Config`, etc. The `scte35` field name (and thus the JSON wire key) is unchanged, so the catalog format is byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Two related changes to the MPEG-TS importer in
moq-mux:splice_info_sections into the broadcast verbatim.SCTE-35 ingest
A SCTE-35 PID is announced by a program-level registration descriptor with format_identifier
CUEI, and its elementary stream usesstream_type 0x86.0x86alone is ambiguous (mpeg2ts maps it to a DTS audio variant), so detection keys off theCUEIdescriptor, not the stream type.SCTE-35 rides in private sections (table_id
0xFC), not PES. The mpeg2ts reader PES-parses every PMT elementary PID and would abort on a private section, so the importer intercepts SCTE PIDs ahead of the reader and reassembles their sections (pointer_field alignment, sections split across packets, continuity gaps, adaptation-field discontinuities). Each wholesplice_info_sectionis published verbatim on a.scte35track through a catalog extension, not hang core, so an application routes cues into its own catalog section.Mid-stream join hardening
A live capture joins a stream already in flight: PES arrive before the first PAT/PMT, and the first decodable access unit can be a delta rather than a keyframe. Two assumptions aborted the decode:
Producerrejects a non-keyframe when no group is open. Rather than relax that globally, an opt-inlenient_startmode drops leading non-keyframes until a keyframe anchors a group. The default stays strict; the H.264 and H.265 importers opt in. This touchescontainer/producer.rsand the codec importers, outside thets/tree.Evidence
survives_midstream_joincarves a mid-GOP join frombbb.tsand exercises both paths in one test.kyrion_dirtystart_extracts_real_cuesruns a real Ateme Kyrion broadcast captured mid-stream: the importer survives the join and extracts all six SCTE-35 splice_inserts, which TSDuck decodes CRC32 OK (golden checked in beside the fixture). The fixture is our own encoder's output, a black feed with no broadcast content, byte-exact payload.The Kyrion carries MP2 audio, not yet supported as an audio track, so it is dropped without blocking SCTE-35 extraction. The fixture keeps it on purpose: it is a real industry configuration and exercises dropping an unsupported elementary stream while the cues still come through.
Review focus
lenient_startlives on the sharedcontainer::Producer, outside thets/tree. The strict default is unchanged (first_frame_must_be_keyframepins it); only the video importers enable it. That is the one change worth a careful look.Test plan
cargo test -p moq-mux(all pass)survives_midstream_join) + real Kyrion (kyrion_dirtystart_extracts_real_cues)