moq-mux: harden the scte35_inject fixture generator#1696
Conversation
Address CodeRabbit review on the SCTE-35 fixture generator example: - main returns anyhow::Result and every unwrap/expect/panic becomes a `?` with .context(), so routine failures print a clean error instead of a backtrace. - Allocate the SCTE-35 PID by walking up from 0x0021 to the first PID the input program doesn't already use, instead of hardcoding 0x0021 and risking a collision with an existing ES. Fresh clips that leave 0x0021 free still land on it, so existing fixtures regenerate byte-identically. - Iterate full packets via chunks_exact(188) and reject inputs whose length isn't a multiple of 188, so pid_of never indexes a short trailing slice. - Force the cue insertion positions strictly increasing so two cues can't collapse onto one packet and silently drop via position(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors the SCTE-35 injector to use anyhow for contextual errors and makes main return anyhow::Result. Replaces a fixed SCTE-35 PID with a scan-from-constant allocator that avoids collisions, validates TS packetization (188-byte chunks), decodes cues with context, enforces strictly increasing insertion indices (erroring if out-of-range), updates cue_packet to accept scte_pid and return Result, and writes augmented PMT and output TS with contextual I/O errors. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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/examples/scte35_inject.rs (1)
89-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winContinue scanning until both PAT and PMT are resolved.
Line 94 breaks on the first PMT payload, so PAT-late inputs can fail at Line 99 with a false “no PAT/PMT PID” error even though PMT is present. Capture PMT PID from the packet header when needed and only break once both values are known.
Suggested patch
while let Some(pkt) = reader.read_ts_packet().context("reading TS packet")? { match pkt.payload { Some(TsPayload::Pat(pat)) => pmt_pid = pat.table.first().map(|p| p.program_map_pid), Some(TsPayload::Pmt(pmt)) => { + pmt_pid.get_or_insert(pkt.header.pid); orig = Some(pmt); - break; } _ => {} } + if pmt_pid.is_some() && orig.is_some() { + break; + } }🤖 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 89 - 100, The loop breaks as soon as a PMT is seen, which can cause pmt_pid to still be None for inputs where PAT arrives later; update the loop that iterates reader.read_ts_packet() so it continues scanning until both pmt_pid and orig (the PMT) are known: when encountering TsPayload::Pat set pmt_pid = pat.table.first().map(...), when encountering TsPayload::Pmt set orig = Some(pmt) but do not break immediately — instead also attempt to extract the PMT PID from the packet header if pmt_pid is None (or simply continue looping until pmt_pid.is_some() && orig.is_some()), then after the loop use pmt_pid.context(...) and orig.context(...).
🤖 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 180-188: The current clamping logic using `let last =
packets.len().saturating_sub(1);` and setting `cue_at[i] = (cue_at[i - 1] +
1).min(last);` can produce duplicate `last` entries and cause later cues to be
dropped by `position()`; fix by ensuring all remaining cues get unique
decreasing positions when you hit the end: compute how many cues remain (e.g.
remaining = cue_at.len() - i) and, if `(cue_at[i - 1] + remaining) >= last`,
instead backfill the tail so cues occupy distinct indices ending at `last` (for
example set cue_at[j] = last - (remaining - (j - i)) for j in i..cue_at.len()),
otherwise continue the current strictly-increasing increment logic; update
references to `cue_at`, `last`, and the loop that enforces uniqueness so
`position()` will not drop cues.
---
Outside diff comments:
In `@rs/moq-mux/examples/scte35_inject.rs`:
- Around line 89-100: The loop breaks as soon as a PMT is seen, which can cause
pmt_pid to still be None for inputs where PAT arrives later; update the loop
that iterates reader.read_ts_packet() so it continues scanning until both
pmt_pid and orig (the PMT) are known: when encountering TsPayload::Pat set
pmt_pid = pat.table.first().map(...), when encountering TsPayload::Pmt set orig
= Some(pmt) but do not break immediately — instead also attempt to extract the
PMT PID from the packet header if pmt_pid is None (or simply continue looping
until pmt_pid.is_some() && orig.is_some()), then after the loop use
pmt_pid.context(...) and orig.context(...).
🪄 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: 62903042-61f5-4291-82ae-be80f8513c0d
📒 Files selected for processing (1)
rs/moq-mux/examples/scte35_inject.rs
- Cue positions are now made strictly increasing without clamping to the last packet, and an `ensure!` errors when the input is too short to place all cues after the PMT. The previous clamp could pin several cues to the final packet, which `position()` would then silently collapse to one. - Keep scanning until both the PMT PID and the PMT are known instead of breaking on the first PMT, falling back to the carrying PID for PAT-late streams. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Hardens the
scte35_injectexample (the SCTE-35 fixture generator) against the issues CodeRabbit flagged. The SCTE-35 feature itself merged via #1685; this is a follow-up to the example tool only.mainreturnsanyhow::Result<()>; everyunwrap/expect/panicbecomes a?with.context(), so routine failures print a clean error instead of a backtrace.0x0021to the first PID the input program doesn't already use, instead of hardcoding0x0021and risking a collision with an existing ES. Fresh clips that leave0x0021free still land on it, so existing fixtures regenerate byte-identically.chunks_exact(188)and reject inputs whose length isn't a multiple of 188, sopid_ofnever indexes a short trailing slice.position().Test plan
scte35_injectcompiles.Error:output on bad args / unknown set; picks0x0021for fresh inputs (byte-compatible with prior behavior).moq-muxSCTE-35 tests pass.cargo clippy -D warningsandcargo fmt --checkclean via nix.(Written by Claude)