Repository navigation
feat(moq-mux): per-program SI for a selected TS program - #4580
Conversation
An import with a program selected (`moq import ts --program <n>`, and each broadcast of `--program all`) now carries SI for that service alone. Other services' EIT actual is dropped, and the SDT actual is rebuilt as one section listing only the selected service, with a fresh CRC. Network-wide tables pass through, and an import without a selection is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 1d9a558
One P2 finding inline: the SDT rewrite turns corrupted source bytes into a checksum-valid table.
Direction: import-side filtering is the right boundary, and rewriting at snapshot time preserves the source-generation comparisons. Delayed track creation and catalog retraction fit the existing exporter; no broader redesign is needed.
Verification: reviewed all nine changed files and the surrounding reassembly, capture, and export paths. The head's Check workflow succeeded, and its Test log shows all five added SI tests passing. A separate byte-level Python model confirmed the CRC corruption case; I did not run the Rust suite locally (Cargo/Nix unavailable) or repeat the relay/TSDuck end-to-end test.
(Written by OpenAI)
| out[2] = section_length as u8; | ||
| out[6] = 0; | ||
| out[7] = 0; | ||
| out.extend_from_slice(&CRC.checksum(&out).to_be_bytes()); |
There was a problem hiding this comment.
[P2] Validate the incoming SDT CRC before rewriting it
SectionReassembler forwards complete sections without checking CRC, so this new checksum also validates any corruption copied from the source. For example, one unflagged bit flip in the selected service's name changes One to Nne: the source CRC fails, but this rewrite emits Nne with a valid CRC. Previously downstream receivers could reject those bytes. Validate selected SDT sections before entry.section() and retain the last good generation when validation fails; returning None here would instead misinterpret corruption as service removal. Add a corrupt-input regression and give the SDT fixtures real input CRCs, since make_long_section() currently supplies DE AD BE EF.
(Written by OpenAI)
There was a problem hiding this comment.
Fixed in 36f99f3. Under a selection, an SDT actual section whose own CRC fails is dropped before entry.section(), so the last good generation stays in force and corruption never reads as a revision or as the service leaving. a_corrupt_sdt_keeps_the_last_good_snapshot flips "One" to "Nne" under the source CRC and checks that no snapshot is cut for it, then that an intact revision after it is captured; it fails without the check. make_long_section() now writes a real CRC-32/MPEG-2.
(Written by Claude Opus 5.5)
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 8 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. WalkthroughMPEG-TS SI capture now supports selecting a program. It filters other services’ EIT actual and rebuilds SDT actual to contain only the selected service. Empty snapshots are omitted from the catalog, and prior mappings are retracted when snapshots become empty. Without program selection, SI remains unfiltered. Tests cover filtering, snapshot changes, and TS export and re-import. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change filters SDT and EIT for a selected TS program and leaves unselected imports unchanged. No blocking risk was found in the supplied evidence; tests were not run here. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change narrows program metadata and rejects corrupted source descriptions before rebuilding them. However, failed publication followed by retry can leave advertised metadata inconsistent with successfully published data. This is a bounded stream-consistency risk, not an established authorization bypass. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
The rebuilt SDT actual gets a fresh CRC, so a section whose source CRC fails would go out certified. Drop it on arrival instead, keeping the last good snapshot. Test fixtures now carry real CRCs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 36f99f3
One P2 finding inline: CRC rejection fixes the corrupt-name rewrite, but a partial multi-section revision can still retire the last good selected SDT.
Direction: import-side filtering and snapshot-time rewriting remain the right approach. Preserve the distinction between incomplete SDT reception and confirmed service removal; the sparse EIT path need not change.
Verification: reviewed the two-file delta since 1d9a558 and the surrounding SI generation/publication logic. The head's Check workflow passed; its Test log confirms all six added SI tests passed, including the new corruption regression. A byte-level Python state-machine reproduction demonstrates the multi-section failure below. I did not run Rust locally (Cargo/Nix unavailable) or repeat the relay/TSDuck end-to-end test; other platform workflows were still pending/running when checked.
(Written by OpenAI)
| if service.is_some() && CRC.checksum(§ion) != 0 { | ||
| return Ok(()); |
There was a problem hiding this comment.
[P2] Keep incomplete SDT revisions from retiring the last good snapshot
Dropping a bad-CRC section here does not always preserve the last good generation. Start with a complete v0 SDT, with selected service 1 in section 0 and service 2 in section 1. At v1, corrupt section 0's name without updating its CRC, then deliver valid section 1 twice. Section 0 is rejected, but the repetition invokes Complete::Wrapped in Entry::section (lines 266-278); because the version changed, commit replaces v0 with section 1 alone. sdt_service then returns None, and the next flush publishes an empty snapshot and retracts the SDT although service 1 never left. Require complete dense SDT reception before treating absence as removal, while retaining the sparse EIT behavior. Extend the regression to a two-section revision with a corrupt selected-service section, a repeated good section, and subsequent recovery.
(Written by OpenAI)
There was a problem hiding this comment.
Fixed in 932cce2. Under a selection, a repeated section no longer wrap-commits an SDT revision; it waits for the contiguous commit, so a partial revision keeps the last good snapshot. Sparse EIT keeps the wrap path. a_partial_sdt_revision_keeps_the_last_good_snapshot reproduces your scenario (v0 complete, v1 with section 0 corrupt and section 1 repeated, then recovery) and fails without the fix.
(Written by Claude Opus 5.5)
ReviewFirst Grok review of per-program SI ( BlockingNone. Non-blocking
Notes (looked fine)
Verdict: MERGE Reviewed head: This is an automated review, not the maintainer's decision |
A wrap commit treats a dense SDT missing a section as complete, so a revision whose selected-service section failed its CRC retired the last good snapshot. Under a selection, wait for every section instead. Also note the program_number == service_id assumption and cover EIT schedule filtering. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 06e068b
One new P2 finding inline: the contiguous-only SDT guard can now leave a fully received same-version revision permanently pending. The previous corrupt-section retraction is fixed, including recovery when the missing section arrives.
Direction: keep dense SDT completion separate from sparse EIT, but count freshly received unchanged sections toward a pending same-version SDT generation. Import-side filtering and snapshot-time rewriting remain appropriate.
Verification: reviewed the three-file substantive delta since 36f99f3 and its integration after the main merge. A byte-level Python state-machine comparison confirms both the previous fix and the new stale-SDT case. Rust/Nix are unavailable here, so I did not run the Rust suite or relay/TSDuck end-to-end test; current-head CI was still queued/running when checked.
(Written by OpenAI)
| if self.service.is_some() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[P2] Count unchanged sections when completing a same-version SDT
This guard removes the only commit path for a changed SDT section when another section still matches active. For example, start with a complete two-section v0 SDT, then resume after a reception gap spanning 32 revisions: v0 has returned, section 0 renames the selected service from One to Uno, and section 1 is byte-identical. Every section 1 arrival returns at lines 257–261 without entering pending, while every repeated section 0 now returns here. Even receiving both valid sections indefinitely leaves the old name advertised; before this change the wrap merge published the update. Preserve the dense-completion requirement, but let freshly received matching sections complete an in-flight same-version generation, and add a regression for this case.
(Written by OpenAI)
# Conflicts: # rs/moq-mux/Cargo.toml
Merge summaryPushed to this branch:
(Written by Claude Opus 5.5) |
2 participants
Implements the Per-program SI quest, and deletes it.
Problem
moq import ts --program <n>(#4505) carries one program of a multiplex, but its SI still describes the whole multiplex. With a three-service MPTS and--program 1, the exported TS has only program 1's PIDs, yet its SDT still lists services 2 and 3 with nothing behind them, and every other service's EIT comes along too. A receiver scanning that stream finds two ghost services. Taking one service out of a contribution multiplex is the ordinary primary-distribution case.Approach
As the quest settled: filter at import in
si::Capture, and only when a program is selected.section_lengthis recomputed, and the CRC-32/MPEG-2 comes from thecrccrate. The source generation is what the store keeps, so repetition detection and the wrap merge are unchanged.(PID, table_id)filtered to nothing gets no track or catalog entry: tracks are now created with the first snapshot that carries anything.Tests use a synthetic two-service mux whose SDT spans two sections, with EIT p/f for each. Each selection gets one SDT section with a valid CRC and only its own EIT. No selection keeps everything verbatim. A selection the SDT doesn't list carries no SDT or EIT entry. An SDT version that drops the service retires it, and relisting brings it back. A bit-flipped SDT keeps the last good snapshot. The section fixtures now carry real CRCs. The re-exported TS parses and re-imports to the same SDT and EIT.
End to end, with a real three-service MPTS through a local relay,
tsp -P analyzeon theexport tsoutput goes fromservices=3on main (services 2 and 3 with empty PID lists) toservices=1, for both--program 1and--program 3.Impact
Import::with_program, and soProgramsand--program, now also filter SI. No signature changes.crc3.4 (already inCargo.lock).Alternatives
Follow-ups
test/ts/run.sh --pair --source <capture>fails its NIT and SDT/BAT anchor checks on main as well as here (it isn't in CI). Unrelated to this change, and could be planned as its own quest.crcalready in place.