Skip to content

feat(moq-mux): generic verbatim MPEG-TS carriage (mpegts catalog section)#1815

Merged
kixelated merged 10 commits into
mainfrom
claude/kind-jennings-485445
Jun 20, 2026
Merged

feat(moq-mux): generic verbatim MPEG-TS carriage (mpegts catalog section)#1815
kixelated merged 10 commits into
mainfrom
claude/kind-jennings-485445

Conversation

@kixelated

@kixelated kixelated commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

Generalizes the SCTE-35-specific MPEG-TS path into a generic per-PID verbatim lane. Any PMT-listed elementary stream the demuxer doesn't decode (SCTE-35, DVB AC-3, teletext/subtitles, private data, ...) is now carried byte-for-byte instead of being dropped, described in a new mpegts catalog section. SCTE-35 becomes one instance of this.

This is the "tier 2" lane from the discussion under #1799: keep full demux for browser/ABR, but stop discarding the ancillary streams a contribution feed carries — and faithfully reconstruct them (PIDs, descriptors, PES stream_ids, program signaling) on export.

Catalog: the mpegts section

One unified per-track map keyed by MoQ track name, plus the program-level descriptors:

pub struct Mpegts {
    pub tracks: BTreeMap<String, Track>,        // media AND verbatim tracks
    pub program_descriptors: Vec<Descriptor>,   // PMT program_info, verbatim
}
pub struct Track {
    pub pid: u16,                               // original MPEG-TS PID
    pub descriptors: Vec<Descriptor>,           // PMT ES descriptors (language, registration, ...)
    pub verbatim: Option<Verbatim>,             // Some ⇒ carried undecoded
}
pub struct Verbatim {
    pub stream_type: u8,
    pub framing: Framing,                       // Pes | Section
    pub stream_id: Option<u8>,                  // original PES stream_id (PES framing)
}
pub struct Descriptor { pub tag: u8, pub data: Bytes /* base64 */ }

Decoded media keeps its codec config in the base video/audio sections; the mpegts entry records only its MPEG-TS identity. Empty section serializes to {}, so a non-TS broadcast is byte-identical. Import/Export are generic over a small Catalog trait so the media-only () catalog and the Ext (mpegts) catalog share one code path.

Behaviour

  • Import routes every undecoded PMT PID to a verbatim track (PES-framed via the normal PES reassembly; section-framed — SCTE-35 and other private sections — intercepted before the reader). It records each track's PID, PMT ES descriptors, the program-level descriptors, and the verbatim PES stream_id.
  • Export rebuilds the PMT from the media renditions + mpegts.tracks: preserves PIDs, re-emits captured ES + program descriptors verbatim (deriving the SCTE-35 'CUEI' registration only when none were captured), re-emits each verbatim PES under its original stream_id (falling back to private_stream_1), and repacketizes per framing. It refreshes track metadata across catalog snapshots until the PMT is built, so PID/descriptors/stream_id filled by the importer across several publishes aren't dropped.

Public API (moq-mux library)

  • Added: container::ts::catalog::{Mpegts, Track, Verbatim, Framing, Descriptor, Ext, Catalog (trait)}.
  • Removed (breaking): container::ts::scte35 module; Export::with_scte35Export::with_ts.

Per [Branch Targeting], a breaking library-API change would normally target dev; on main at the maintainer's request (the SCTE-35 surface is recent and unreleased in this form).

Validated on a real CNN 1080i contribution feed

CNNiEMEA2.ts, 80 MB prefix, import → export → re-import is identical by PID:

  • Media (H.264 1080i + MP2) decoded; PIDs + PMT descriptors captured, including the audio ISO-639 language (0x0a) and video (0x28) descriptors.
  • 3 program-level descriptors round-trip.
  • Five verbatim streams the old demux-only path dropped, all byte-exact with PIDs and stream_ids preserved:
PID stream_type framing stream_id descriptors frames
0x7b 0x06 PES 0xBD lang, AC-3 (0x6a), max-bitrate 233
0x8d / 0x8e / 0x8f 0x86 Section 66/66/67 (3× SCTE-35)
0x83 0x06 PES 0xBD lang, teletext (0x56) 1683

(Video decode for this feed's two-PPS H.264 requires #1812; the two PRs are independent and compose cleanly.)

This is faithful at the elementary-stream + PMT level (content + signaling), not byte-for-byte at the transport level — PCR cadence, continuity counters, stuffing, and SI tables outside the PMT are regenerated. Literal byte fidelity (and CAS, exact PCR for TR 101 290) is the opaque whole-mux lane (Option B in #1799), future work.

Out of scope / follow-ups

  • CLI wiring to carry verbatim streams through moq pub/sub.
  • PCR-PID preservation and MPTS/multi-program (SPTS-scoped today).
  • A mid-session PMT version change that alters ES/program descriptors is recorded once, not refreshed (rare).

Test plan

  • cargo test -p moq-mux (237), incl. private_pes_carried_verbatim, carries_all_sections_verbatim, mpegts catalog (de)serialization, SCTE-35 fixture round-trips
  • cargo clippy -p moq-mux --all-targets -- -D warnings, cargo fmt --check, and cargo doc -D warnings clean
  • moq-cli / moq-relay / hang / libmoq build
  • Real CNN 1080i feed round-trips identical by PID (with fix(moq-mux): carry all distinct SPS/PPS/VPS through transmux, not just the last seen #1812 for video)

Cross-package sync

No js/hang / js/net mirror: the mpegts section is a moq-mux-side extension; browser consumers ignore the unknown section. Base hang catalog and the moq-net wire are unchanged.

(Written by Claude)

Generalize the SCTE-35-specific path into a generic per-PID verbatim lane:
any PMT-listed elementary stream we don't decode is carried byte-for-byte,
described in a new `ts` catalog section (PID map + stream configs). SCTE-35
becomes one instance (stream_type 0x86, section framing).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR removes the dedicated scte35 catalog extension module and replaces it with a new generic ts::catalog module that describes arbitrary undecoded MPEG-TS elementary streams by PID, with Framing (PES or Section), PMT Descriptor data, and optional Verbatim carriage metadata. The Import demuxer is re-parameterized to catalog::Catalog, replacing SCTE-35–specific state with generic SectionStream/SectionReassembler/VerbatimStream infrastructure that emits all private sections verbatim. The Export muxer is similarly re-parameterized, replacing Kind::Scte35 with Kind::Verbatim { stream_type, framing, stream_id }, adding PID-preserving alloc_pid, and updating build_psi/write_frame for section- vs PES-framed dispatch. The public ExportSource::for_scte35 constructor is renamed to for_stream with simplified HangContainer::Legacy subscription. All tests are updated to use the new catalog path and assert cue/section publication via mpegts.tracks instead of SCTE-35–typed renditions.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: introducing generic verbatim MPEG-TS carriage via a new mpegts catalog section, which replaces the previous SCTE-35-specific path.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description comprehensively describes the changeset, documenting the shift from SCTE-35-specific paths to a generic per-PID verbatim lane system, the new mpegts catalog section, import/export behavior, public API changes, validation results, and test coverage.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/kind-jennings-485445

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 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/catalog.rs`:
- Around line 126-130: The Ext struct needs to be marked with the
#[non_exhaustive] attribute to make the extension schema additive and prevent
breaking changes when new optional fields are added in the future. Add
#[non_exhaustive] as an attribute directly above the pub struct Ext definition
on line 126, before the existing derive attribute. Since Ext already derives
Default, the requirement for a constructor is already satisfied, allowing
consumers to construct the struct while keeping it safe for future extension.

In `@rs/moq-mux/src/container/ts/export.rs`:
- Around line 541-546: The Kind::Verbatim variant with framing set to Pes is
currently passing verbatim PES payloads that may have non-audio stream types
(such as teletext or DVB subtitles) to write_pes, which incorrectly applies an
audio stream ID to all non-video tracks. This violates the verbatim elementary
stream contract by rewriting the stream ID. Thread the original PES stream_id
through the import/catalog/export pipeline, or alternatively derive the correct
non-audio stream ID for PMT private-data stream types before calling write_pes.
Update the handling of Kind::Verbatim with framing: catalog::Framing::Pes to use
the correct stream ID instead of defaulting to audio, ensuring the PES header
preserves the original stream type classification.
- Around line 463-465: The condition at line 465 that detects stream_type 0x86
for the CUEI descriptor addition does not account for the framing type. The
issue is that the same Kind::Verbatim with stream_type 0x86 can be packetized as
either sections or PES depending on the framing mode, but the current code only
checks the stream type. When framing is PES, the write_frame function will
packetize the payload as PES, yet the PMT advertises SCTE-35 section signaling,
causing re-import to route the PID incorrectly. Gate the CUEI descriptor
addition by also checking that the framing is not PES (i.e., verify the framing
is section-based) before adding the descriptor alongside the existing
stream_type check.
- Line 443: The call to StreamType::from_u8(*stream_type) at line 443 will fail
when encountering unknown or proprietary stream types (such as teletext 0x10)
that are not defined in the mpeg2ts enum, causing Verbatim track exports to
fail. Either validate and filter unknown stream types before they reach this
point upstream in the export logic, or modify the handling at the
StreamType::from_u8 call site to gracefully handle unknown types by either using
a fallback stream type, wrapping the conversion in a more lenient parser, or
explicitly handling the error case instead of propagating it as a fatal error.

In `@rs/moq-mux/src/container/ts/import_test.rs`:
- Line 298: The SCTE-35 stream selection in the line with
snap.ts.streams.keys().next().expect("scte35 track") is relying on map iteration
order, which is unreliable when new stream types are added to the fixture.
Instead of taking the first stream from the map, filter through the streams to
find the one with stream_type matching 0x86 (the SCTE-35 identifier), and select
that stream by its type rather than by map order position.
🪄 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: 42f89a16-46fc-4fbb-bbb3-1ec221e302f6

📥 Commits

Reviewing files that changed from the base of the PR and between 678a250 and 85b555f.

📒 Files selected for processing (8)
  • rs/moq-mux/src/container/source.rs
  • rs/moq-mux/src/container/ts/catalog.rs
  • rs/moq-mux/src/container/ts/export.rs
  • rs/moq-mux/src/container/ts/export_test.rs
  • rs/moq-mux/src/container/ts/import.rs
  • rs/moq-mux/src/container/ts/import_test.rs
  • rs/moq-mux/src/container/ts/mod.rs
  • rs/moq-mux/src/container/ts/scte35.rs
💤 Files with no reviewable changes (1)
  • rs/moq-mux/src/container/ts/scte35.rs

Comment thread rs/moq-mux/src/container/ts/catalog.rs
Comment thread rs/moq-mux/src/container/ts/export.rs Outdated
Comment thread rs/moq-mux/src/container/ts/export.rs Outdated
Comment thread rs/moq-mux/src/container/ts/export.rs
Comment thread rs/moq-mux/src/container/ts/import_test.rs Outdated
Reshape the ts section into a single per-track map keyed by name: Track{pid,
descriptors, verbatim?}. Captures media-track PIDs and PMT ES descriptors
(audio language, ...) and program-level descriptors, not just verbatim streams.
Rename the catalog key/types to mpegts. SCTE-35/teletext/AC-3 verbatim carriage
unchanged; media language descriptors and program_info now survive the round-trip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kixelated kixelated changed the title feat(moq-mux): generic verbatim MPEG-TS carriage (ts catalog section) feat(moq-mux): generic verbatim MPEG-TS carriage (mpegts catalog section) Jun 19, 2026
kixcord and others added 2 commits June 19, 2026 15:42
Only derive the program-level SCTE-35 CUEI registration for a section-framed
0x86 verbatim stream; a PES-framed 0x86 must not advertise section signaling.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…stream_type

- Mark the mpegts Ext extension #[non_exhaustive] so new sections stay additive.
- On export, skip (with a warning) a verbatim stream whose recorded stream_type
  isn't a known TS stream type rather than aborting the whole export; round-tripped
  streams always parse, this guards a hand-built catalog.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Addressed the review feedback in 54d2cf5/b055063:

  • Gate derived CUEI on section framing — done; only a section-framed 0x86 derives the program-level CUEI registration now.
  • Select SCTE-35 by stream_type, not map order (import_test) — already handled in the unified-model reshape: tests now find the cue track via verbatim.stream_type == 0x86.
  • #[non_exhaustive] on Ext — done.
  • StreamType::from_u8 can Err — export now skips (with a warning) a verbatim stream whose recorded stream_type isn't a known TS type instead of aborting. Round-tripped streams always parse (the type came from the import's PMT); this only guards a hand-built catalog.

Deferred (wants a maintainer call): preserve the original PES stream_id for verbatim PES streams. Today tier-2 carriage regenerates PES headers and preserves the elementary-stream payload byte-for-byte (validated: teletext/AC-3/SCTE all round-trip exactly) plus the PMT signaling; the PES stream_id is not preserved, so a verbatim PES is rewritten with the audio stream_id. Faithful preservation means adding a stream_id field to the mpegts Verbatim record and threading it through import→catalog→export — a catalog-schema change I'd rather confirm before making. PES-header fidelity is otherwise an explicit non-goal of this lane (that's the opaque whole-mux lane in #1799).

(Written by Claude)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/import.rs (1)

208-219: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear stale PID routes before accepting a new PMT mapping.

Line 208 checks sections before streams. If a later PMT remaps a section PID to PES or a codec, the old sections entry keeps intercepting it; in the base-catalog path, the old Stream::Ignored makes ensure_stream return before the new route is installed. Finish and remove stale section routes, and allow Stream::Ignored to be replaced before the early return.

Proposed fix
-		if self.streams.contains_key(&pid) {
-			return Ok(());
-		}
+		if let Some(mut section) = self.sections.remove(&pid.as_u16()) {
+			section.finish()?;
+			self.pending.remove(&pid);
+		}
+		if matches!(self.streams.get(&pid), Some(Stream::Ignored)) {
+			self.streams.remove(&pid);
+		} else if self.streams.contains_key(&pid) {
+			return Ok(());
+		}

Also applies to: 306-307, 400-412

🤖 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 208 - 219, The issue is
that stale PID routes are not being cleared when a PMT remaps a PID from a
section route to a PES or codec route. The current logic at line 208 checks
self.sections.get_mut(&pid) first and continues to use the old section route,
preventing the new route from being installed. Additionally, in the base-catalog
path, the old Stream::Ignored prevents ensure_stream from installing the new
route before the early return. To fix this, before processing the packet with
the current section route, finish and remove the stale section entry if the PID
has been remapped in a new PMT, and ensure that Stream::Ignored entries can be
replaced by finishing the stale route before checking for Stream::Ignored and
making the early continue. Apply the same logic at the other locations mentioned
(lines 306-307 and 400-412).
🤖 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/import.rs`:
- Around line 208-219: The issue is that stale PID routes are not being cleared
when a PMT remaps a PID from a section route to a PES or codec route. The
current logic at line 208 checks self.sections.get_mut(&pid) first and continues
to use the old section route, preventing the new route from being installed.
Additionally, in the base-catalog path, the old Stream::Ignored prevents
ensure_stream from installing the new route before the early return. To fix
this, before processing the packet with the current section route, finish and
remove the stale section entry if the PID has been remapped in a new PMT, and
ensure that Stream::Ignored entries can be replaced by finishing the stale route
before checking for Stream::Ignored and making the early continue. Apply the
same logic at the other locations mentioned (lines 306-307 and 400-412).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1caf070e-4468-4755-9fdf-8a60b92b093c

📥 Commits

Reviewing files that changed from the base of the PR and between 85b555f and b055063.

📒 Files selected for processing (7)
  • rs/moq-mux/src/codec/legacy.rs
  • rs/moq-mux/src/container/source.rs
  • rs/moq-mux/src/container/ts/catalog.rs
  • rs/moq-mux/src/container/ts/export.rs
  • rs/moq-mux/src/container/ts/export_test.rs
  • rs/moq-mux/src/container/ts/import.rs
  • rs/moq-mux/src/container/ts/import_test.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • rs/moq-mux/src/container/source.rs
  • rs/moq-mux/src/container/ts/export_test.rs
  • rs/moq-mux/src/container/ts/export.rs

kixcord and others added 2 commits June 19, 2026 15:58
Capture the original PES stream_id on import and re-emit it on export instead of
relabeling every non-video stream as audio (0xC0). DVB teletext, DVB subtitles,
and DVB AC-3 ride private_stream_1 (0xBD); rewriting them to audio is rejected by
strict broadcast demuxers and TR 101 290 analyzers, and mis-frames stream_ids
whose PES-header rules differ. Recorded as mpegts Verbatim.stream_id; absent (a
non-TS source) falls back to private_stream_1.

Validated on a real CNN feed: AC-3 and teletext both preserve 0xBD across the
round-trip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A later PMT can remap a previously section-framed PID to a PES codec/verbatim
stream. ensure_stream now finishes and drops the stale sections entry first, so
the PID isn't intercepted as a section forever while the new stream starves.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Follow-up in 147cdef / 740249e:

  • Preserve the PES stream_id (the deferred item). The original stream_id is now captured on import into mpegts Verbatim.stream_id and re-emitted on export, instead of relabeling every non-video stream as audio (0xC0). DVB teletext / subtitles / DVB AC-3 ride private_stream_1 (0xBD); rewriting them to audio is rejected by strict broadcast demuxers and TR 101 290 analyzers. Absent (a non-TS source) falls back to private_stream_1. Validated on a real feed: AC-3 and teletext both preserve 0xBD across the round-trip. (No special handling for the no-optional-PES-header stream_ids like private_stream_2/padding — those belong to the opaque whole-mux lane; the realistic verbatim PES all use header-bearing ids.)
  • Clear stale section route on PMT remap (the outside-diff note). ensure_stream now finishes/drops a prior sections entry when a later PMT remaps a section PID to a PES codec/verbatim stream, so it isn't intercepted as a section forever. Skipped the suggested Stream::Ignored-replacement half: codec support is fixed at construction, so an Ignored PID can't later become decodable, and replacing it would just re-warn on every repeated PMT.

(Written by Claude)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

349-372: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Refresh existing verbatim metadata before PSI is built.

update_catalog can see multiple snapshots before build_psi(), but an already-created verbatim track is skipped. Since import first registers Verbatim::new(...) with stream_id: None and fills the original PES id later, export can latch None and then fall back to private_stream_1 at Lines 622-624 instead of preserving the real stream id.

Suggested fix
 			let Some(verbatim) = &track.verbatim else {
 				continue;
 			};
-			if self.tracks.contains_key(name) {
-				continue;
-			}
 			// Skip (don't abort) a verbatim stream whose recorded stream_type isn't a
 			// known TS stream type, so it can't be announced in the PMT. Round-tripped
 			// streams always parse (the type came from the import's PMT); this guards a
 			// hand-built catalog. Mirrors the `active` filter above.
 			if StreamType::from_u8(verbatim.stream_type).is_err() {
@@
 				stream_type: verbatim.stream_type,
 				framing: verbatim.framing,
 				stream_id: verbatim.stream_id,
 			};
+			if let Some(existing) = self.tracks.get_mut(name) {
+				existing.kind = kind;
+				existing.descriptors = track.descriptors.clone();
+				continue;
+			}
 			let source = ExportSource::for_stream(&self.broadcast, name, verbatim, self.latency)?;

Also applies to: 619-632

🤖 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 349 - 372, The issue is
that existing verbatim tracks are being skipped when a track with the same name
already exists in self.tracks. This prevents updating the track's metadata
(particularly stream_id) when a new snapshot is processed. Instead of
unconditionally continuing when self.tracks.contains_key(name) is true, you need
to check if the existing track is a verbatim track and update its metadata with
the new verbatim information from the current snapshot rather than skipping it.
This ensures that when import initially registers a Verbatim with stream_id set
to None and later updates it with the real stream_id, the export will preserve
the updated stream_id instead of falling back to private_stream_1 at the
fallback logic.
🤖 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 349-372: The issue is that existing verbatim tracks are being
skipped when a track with the same name already exists in self.tracks. This
prevents updating the track's metadata (particularly stream_id) when a new
snapshot is processed. Instead of unconditionally continuing when
self.tracks.contains_key(name) is true, you need to check if the existing track
is a verbatim track and update its metadata with the new verbatim information
from the current snapshot rather than skipping it. This ensures that when import
initially registers a Verbatim with stream_id set to None and later updates it
with the real stream_id, the export will preserve the updated stream_id instead
of falling back to private_stream_1 at the fallback logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: edb96715-8dd3-4575-a9e1-16e9a6fab276

📥 Commits

Reviewing files that changed from the base of the PR and between 54d2cf5 and 740249e.

📒 Files selected for processing (3)
  • rs/moq-mux/src/container/ts/catalog.rs
  • rs/moq-mux/src/container/ts/export.rs
  • rs/moq-mux/src/container/ts/import.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • rs/moq-mux/src/container/ts/catalog.rs
  • rs/moq-mux/src/container/ts/import.rs

Comment thread rs/moq-mux/src/codec/legacy.rs Outdated
Comment on lines +82 to +83
/// The underlying MoQ track producer (for its name, etc.).
pub fn track(&self) -> &moq_net::TrackProducer {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not expose the Producer? Just the name() would be fine.

Comment thread rs/moq-mux/src/container/ts/catalog.rs Outdated
use crate::catalog::hang::CatalogExt;

/// Serialize [`Bytes`] as a base64 string in the catalog JSON.
mod base64_bytes {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serde_from or serde_as instead?

/// PMT program-level descriptors (`program_info`), carried verbatim. Export
/// re-emits these; the SCTE-35 'CUEI' registration is derived when absent.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub program_descriptors: Vec<Descriptor>,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

descriptors ?

pub struct Verbatim {
/// PMT `stream_type` to re-announce (0x86 SCTE-35, 0x06 private PES, 0x05
/// private sections, ...).
pub stream_type: u8,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth having an enum (with unknown(u8) or no?

Comment thread rs/moq-mux/src/container/ts/catalog.rs Outdated

/// How the verbatim bytes are timestamp-framed as MoQ frames.
#[serde(default)]
pub container: hang::catalog::Container,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just hard-code hang::legacy::Container? Seems dumb to support CMAF.

/// Keep this stable per catalog: an importer samples support once at
/// construction, so a result that flips between `Some` and `None` mid-stream
/// would disable verbatim carriage or fail.
fn mpegts_mut(&mut self) -> Option<&mut Mpegts>;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why mut? Is this needed?

///
/// Implement this for an application extension to compose MPEG-TS carriage with
/// additional sections.
pub trait Catalog: CatalogExt {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why do we need another trait?

Comment thread rs/moq-mux/src/container/ts/export.rs Outdated
// known TS stream type, so it can't be announced in the PMT. Round-tripped
// streams always parse (the type came from the import's PMT); this guards a
// hand-built catalog. Mirrors the `active` filter above.
if StreamType::from_u8(verbatim.stream_type).is_err() {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to gate based on StreamType?

Comment thread rs/moq-mux/src/container/ts/export.rs Outdated
/// (PAT), [`PMT_PID`], `0x1FFF` (null), and any already-assigned PID are avoided
/// so cross-references survive when possible.
fn alloc_pid(&self, name: &str, tracks: &std::collections::BTreeMap<String, catalog::Track>) -> u16 {
let mut used: std::collections::HashSet<u16> = self.tracks.values().map(|t| t.pid).collect();

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK If we need to construct a HashSet every time, just iterate over the tracks.

- Fix rustdoc broken intra-doc link to `ts` in ts/mod.rs (the CI failure).
- Catalog: use serde_with serde_as/Base64 for Descriptor.data instead of a
  hand-rolled base64 module (matches rs/hang); drop Verbatim.container (always
  Legacy-framed) and hard-code it.
- legacy::Import exposes name() instead of the Producer.
- Export: refresh existing tracks' PID/descriptors/kind on each catalog snapshot
  until the PMT is built, instead of latching the first (partial) snapshot. The
  importer fills PIDs, ES descriptors, and PES stream_ids across several publishes,
  so latching dropped them; this fixes media-PID/descriptor and verbatim stream_id
  preservation (flagged by review + CodeRabbit). PID assignment is a two-pass
  (prefer original, then fill free) with no per-call HashSet.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Thanks — addressed your comments + a self-review in 09a1d505. CI doc-link failure (unresolved link to \ts``) is also fixed.

Changed as suggested:

  • base64 (catalog.rs:19): now #[serde_as] ... #[serde_as(as = "Base64")] via serde_with, matching rs/hang; deleted the hand-rolled module.
  • Verbatim.container (:116): dropped — verbatim frames are always Legacy-framed, hard-coded now.
  • legacy::Import (legacy.rs:83): exposes name() -> &str instead of the Producer.
  • alloc_pid HashSet (export.rs:397): gone — PID assignment is a two-pass over the desired tracks (prefer original, then fill free), no per-call set.
  • from_u8 gate (export.rs:360): de-duplicated into a single verbatim_ok predicate (it was in two places). It's a graceful skip of an unknown stream_type rather than aborting the whole export — only reachable from a hand-built catalog, since round-tripped types always parse. Happy to drop it entirely if you'd rather a bad catalog error loudly.

Correctness fix from the review (you + CodeRabbit both flagged the class): export was latching per-track metadata from the first catalog snapshot. The importer fills a track's PID, ES descriptors, and PES stream_id across several publishes (the codec importer creates the rendition; record_media_track adds PID/descriptors on the first frame; VerbatimStream backfills stream_id). Export now refreshes existing tracks' PID/kind/descriptors on every snapshot until the PMT is built, so none of that is dropped.

Kept (with reasoning — let me know if you disagree):

  • stream_type as u8 (:100) rather than an enum-with-Unknown(u8): it's a raw PMT value we carry verbatim and never interpret (only 0x86→CUEI and from_u8 to re-announce). mpeg2ts already owns the StreamType enum; mirroring it in the catalog is maintenance for no gain. An Unknown(u8) enum would also serialize less cleanly.
  • The Catalog trait (:173): it's what lets Import/Export be generic over () (media-only, zero mpegts overhead — what moq-cli uses today) and Ext with one code path; the () impl returns None. Without it, every TS import/export carries the mpegts machinery or we duplicate code paths. If you'd prefer to drop the trait and always use Ext (section omitted when empty), I can — it simplifies the generics but makes the section always-present in the type.
  • program_descriptors (:53): the PMT program_info descriptors (e.g. the SCTE-35 CUEI registration, program-level registration), carried verbatim so export re-emits them rather than re-deriving.
  • mut (:180): clippy -D warnings runs in CI, so there's no unused mut — the one you spotted is mutated (the test builds the section then inserts into it).

Known minor edges I left (rare, happy to fix if you want): a mid-session PMT version change that alters a track's ES descriptors or adds CUEI to an already-non-empty program_info is recorded once and not refreshed. Unusual in practice; the old SCTE path re-detected CUEI per-PMT.

(Written by Claude)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/export.rs`:
- Around line 269-277: The `verbatim_ok` closure silently skips verbatim tracks
with unsupported stream types without notifying the operator. Add a warning log
statement that triggers when a verbatim track is skipped due to an unsupported
stream_type value (when `StreamType::from_u8(v.stream_type)` returns an error).
The warning should provide context about which track or stream type value is
being dropped and why, ensuring operators have visibility into what catalog
entries are being excluded from the PMT announcement.
🪄 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: 0f450662-4415-4e0d-a57e-6b7245ba88d5

📥 Commits

Reviewing files that changed from the base of the PR and between 740249e and 09a1d50.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • rs/moq-mux/Cargo.toml
  • rs/moq-mux/src/codec/legacy.rs
  • rs/moq-mux/src/container/source.rs
  • rs/moq-mux/src/container/ts/catalog.rs
  • rs/moq-mux/src/container/ts/export.rs
  • rs/moq-mux/src/container/ts/import.rs
  • rs/moq-mux/src/container/ts/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • rs/moq-mux/src/container/ts/mod.rs
  • rs/moq-mux/src/container/ts/import.rs

Comment thread rs/moq-mux/src/container/ts/export.rs Outdated
Comment on lines +269 to +277
// The desired track set: media renditions plus carriable verbatim streams. A
// verbatim stream_type that isn't a known TS stream type can't be announced in
// the PMT, so it's skipped (not aborted); round-tripped streams always parse,
// this only guards a hand-built catalog.
let verbatim_ok = |t: &catalog::Track| {
t.verbatim
.as_ref()
.is_some_and(|v| StreamType::from_u8(v.stream_type).is_ok())
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Warn when skipping unsupported verbatim stream types.

verbatim_ok silently drops verbatim tracks whose stream_type cannot be converted by mpeg2ts. That avoids aborting, but it still loses an explicitly cataloged stream with no operator signal.

Suggested fix
 		for (name, track) in mpegts.tracks.iter() {
 			if verbatim_ok(track) {
 				active.insert(name.clone(), ());
+			} else if let Some(verbatim) = &track.verbatim {
+				tracing::warn!(
+					track = %name,
+					stream_type = verbatim.stream_type,
+					"skipping verbatim TS track with unsupported stream_type"
+				);
 			}
 		}

Also applies to: 285-287

🤖 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 269 - 277, The
`verbatim_ok` closure silently skips verbatim tracks with unsupported stream
types without notifying the operator. Add a warning log statement that triggers
when a verbatim track is skipped due to an unsupported stream_type value (when
`StreamType::from_u8(v.stream_type)` returns an error). The warning should
provide context about which track or stream type value is being dropped and why,
ensuring operators have visibility into what catalog entries are being excluded
from the PMT announcement.

A verbatim stream imported from TS always has a known stream_type (mpeg2ts parsed
it from the PMT), so the from_u8 eligibility gate was unreachable for real data
and only guarded a hand-built catalog. Drop it; build_psi's existing from_u8
conversion is the backstop (a malformed hand-built catalog errors there).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Follow-up (caf90517):

  • from_u8 gate: removed. A verbatim stream imported from TS always has a known stream_type (mpeg2ts parsed it from the PMT), so the gate was unreachable for real data — it only guarded a hand-built catalog. build_psi's existing from_u8 conversion is the backstop. (This moots the "warn on skip" suggestion — there's no skip path anymore.)

  • Dropping the Catalog trait: investigated, recommend keeping it. Tried making Import/Export concrete over Ext, but the trait turns out to be load-bearing: the crate's generic unified import layer (import::Framed::new / import::Import::new) takes one base Producer<()> and feeds it to every format importer including TS; the trait (mpegts_mut() -> Option<_>, None for ()) is what lets TS accept that base catalog with zero overhead for media-only callers. Going concrete-Ext forces Producer<Ext> up through both unified importers and every caller (moq-cli, libmoq/ffi) — a crate-wide cascade. Reverted; happy to do the full cascade if preferred.

(Written by Claude)

@arielmol

arielmol commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Re-read at 09a1d50 round-trip is narrower than carriage. Import carries undecoded stream_type verbatim, but export skips anything StreamType::from_u8() doesn't recognize, so an exotic stream type imports but drops on export, for example KLV crosses the bridge but canot be remuxed.

@arielmol

Copy link
Copy Markdown
Contributor

Oh reviews in cascade, too fast for me :)

@arielmol

Copy link
Copy Markdown
Contributor

bradhards' KLV will work transparently with this

@kixelated kixelated enabled auto-merge (squash) June 20, 2026 00:19
@kixelated kixelated merged commit c602cd3 into main Jun 20, 2026
1 check passed
@kixelated kixelated deleted the claude/kind-jennings-485445 branch June 20, 2026 04:54
@moq-bot moq-bot Bot mentioned this pull request Jun 20, 2026
kixelated pushed a commit that referenced this pull request Jun 21, 2026
dev advanced 3 commits while this PR was open (#1845 PTS-exposing TS Export +
PCR-paced SRT egress, #1847 moq-lite-05 wire sync, #1819 always-on hardware
encoders). Re-merged dev and reconciled two conflicts:

- nix/overlay.nix: kept this PR's cargo+sccache CI direction (dev's tip still
  carries the crane `moqChecks`; the cargo switch lives on main, #1821).
- rs/moq-mux/src/container/ts/export.rs: combined dev's #1845 Frame-returning
  `Export::next` (PTS/keyframe-stamped) with this PR's generic `catalog::Catalog`
  trait (main's #1815 mpegts rename). Updated the moq-cli drain helper to read
  `frame.payload` and refreshed two stale `scte35::Ext` comments.

Verified: moq-mux (279), moq-cli, moq-srt tests pass; workspace clippy -D
warnings and fmt clean (excluding the crates that need libva/gstreamer system
libs unavailable in this sandbox: moq-video/libmoq/moq-boy/moq-gst); JS
type-checks and biome clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BQ9o9paZnRLpYRgbyjUwFV
@moq-bot moq-bot Bot mentioned this pull request Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants