feat(mux): carry EIT present/following through a TS round-trip - #2824
feat(mux): carry EIT present/following through a TS round-trip#2824kixelated wants to merge 4 commits into
Conversation
`moq import ts` dropped EIT at the import gate, so the EPG could not survive a round-trip even though the mechanism to carry it already existed. #2800 measured the cost and found it far cheaper than assumed: the `si` map dedupes by (table_id, table_id_extension, section_number) and compares before taking the catalog's write lock, so cost tracks the revision rate rather than the transmission rate. EIT p/f was 4 distinct sections out of 592 transmitted over ten minutes. The gap was that `SI_PIDS` keyed on PID alone and `si_section` captured every section on an intercepted PID whatever its table_id. That is right for the PIDs it covered, but it made EIT all-or-nothing: p/f (0x4E, 0x4F) and schedule (0x50..=0x6F) share PID 0x0012. `Si::interval` made it concrete, being one value per PID where p/f actual wants 2s and schedule wants 10-30s. So `SI_PIDS` becomes `&[SiPid]` with a `Tables::{All, Only}` filter. Selecting on table_id keeps the sections opaque: it is the first byte under generic section syntax, the same byte `section_key` already reads to dedupe. The filter runs after reassembly because the EIT PID interleaves the two tables, and no surviving section means no entry, so a schedule-only stream leaves no empty PID behind for export to emit. Schedule stays out. The catalog is whole-state and republished on every change, so a full multi-service eight-day EPG would cost every subscriber the whole document at join, ahead of media. It belongs on its own track if it is ever wanted, the way SCTE-35 sections already ride one. TDT/TOT stays out because every section is new content, and an exporter's own clock beats a time relayed from an upstream multiplexer of unknown delay. Both rationales are now recorded next to the entries rather than being silent omissions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 605ee6ae88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // SCTE-35 sections already ride one (see `SectionStream` in the import path). | ||
| SiPid { | ||
| pid: 0x0012, | ||
| tables: Tables::Only(&[(0x4E, 0x4F)]), |
There was a problem hiding this comment.
When PID 0x0012 contains 0x4F sections for services on different transport streams or networks that reuse the same service_id and section number, enabling EIT-other capture here feeds them to Si::upsert, whose key is only (table_id, table_id_extension, section_number). Because an EIT's table_id_extension is just its service_id, the later section silently replaces the earlier one, and the exported EIT p/f omits a service. Either exclude 0x4F or include its transport_stream_id and original_network_id in the section identity. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L98-L98
Useful? React with 👍 / 👎.
Review findings on the parent commit. Capturing EIT p/f other (0x4F) was unsound. `Si::upsert` identifies a section by (table_id, table_id_extension, section_number), and an EIT's table_id_extension is its service_id, which is only unique within a transport stream. The rest of the DVB triplet (transport_stream_id, original_network_id) lives at bytes 8..12, past the generic section header `section_key` reads. EIT other describes services in other multiplexes, which is exactly where service_id reuse happens, so two foreign services sharing one would collide on a single entry and overwrite each other on every repetition: perpetual catalog churn, and only the last arrival exported. Telling them apart means parsing EIT specifically, a far bigger break of section opacity than reading a table_id, so capture narrows to p/f actual (0x4E). That is also what #2800 measured and bounded, and it makes the 2s interval exact rather than the tightest of two. Two of the tests proved nothing. Sync needs the following packet's 0x47 to confirm the 188-byte stride, so a single-packet input routes nothing at all and both "not captured" assertions passed without the import gate ever running. Each now carries a captured SDT as a positive control, so absence is only asserted once the gate is known to have run. The positive test used single-packet sections, which a per-packet prefilter would also have passed; it now uses a multi-packet p/f section, since a continuation packet carries no table_id and only post-reassembly filtering can rebuild it. Verified by mutation: widening the filter to `Tables::All` fails both. Nothing covered the round-trip the parent commit is named for, so add one that imports real EIT packets, exports, and re-imports to assert the section returns on 0x0012 byte-for-byte. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second review round on this branch. `section_key` identifies a section by (table_id, table_id_extension, section_number), which is every part of the generic section header except byte 5. Byte 5 is where the version lives, so the currently-applicable version of a sub-table and the next one, which a source may transmit ahead of a version change with current_next_indicator clear, are indistinguishable to it. Carrying both has them replace each other on every repetition: each swap counts as a change, so it takes the catalog's write lock and republishes, and export emits whichever landed last, which half the time is the version that does not apply yet. So drop sections whose current_next_indicator is clear. Reading the low bit of byte 5 is generic section syntax, the same class of thing as the table_id in byte 0 and the section_number in byte 6 that `section_key` already reads, so this parses no table and keeps the sections opaque. A short-form section has no version field and is always current. This is not EIT-specific and is not new: SDT, NIT and BAT have the same header and have been carried since the SI work landed. EIT is what surfaced it. A conformant multiplexer transmits the applicable table with the indicator set, so a stream that never pre-announces a version change is unaffected. The synthetic sections the tests build had the indicator clear by accident, which is why none of them caught this. `make_section` now sets it, so fixtures are shaped like something a multiplexer would emit, and the new regression clears it again on purpose to build the pending version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third review round pointed out that carrying only the currently-applicable version is a real behavior change for SDT, NIT and BAT, which shipped before it, and that nothing on the public type said so. Document it on `Si`, along with the related consequence that replacement is per-section, so a multi-section table changes version one section at a time and a consumer can briefly observe a mix of the two. Both fall out of identifying a section by the generic header minus the version, so they belong next to the type that stores them rather than in the import path that applies them. Fixing either properly needs a generation-aware store (#2881), and the question of whether carried SI belongs in the whole-state catalog at all is #2882. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes #2800 (partially: p/f and TDT/TOT, not schedule).
moq import tsdropped EIT at the import gate, so the EPG could not survive a TS round-trip even though #2440 had already built the mechanism that would carry it. #2800 measured the cost on a synthesized EPG and found it far cheaper than we assumed: thesimap dedupes by(table_id, table_id_extension, section_number)and compares before taking the catalog's write lock, so cost tracks the revision rate, not the transmission rate. Over ten minutes, EIT present/following was 4 distinct sections out of 592 transmitted.Root cause of the gap
SI_PIDSwas keyed by PID alone, andsi_sectioncaptured every completed section on an intercepted PID whatever itstable_id. That is deliberate and correct for the PIDs it covered (the SDT PID also carries the BAT; an unrecognized table is as worth preserving as a known one), but it made EIT all-or-nothing: p/f (0x4E,0x4F) and schedule (0x50..=0x6F) share PID 0x0012.Si::intervalmade that concrete rather than theoretical. It is one value per PID, but per ETSI TS 101 211 p/f actual is 2 s and schedule is 10-30 s. One PID, two cadences, one slot: there was no entry that was correct for both.The change
SI_PIDSbecomes&[SiPid](pid, tables, interval) with aTables::{All, Only}filter, and 0x0012 joins it asOnly(&[(0x4E, 0x4F)])at 2 s.Allstays the default posture and is what 0x0010 and 0x0011 use, so nothing about their capture changes.Filtering on
table_iddoes not make the sections any less opaque: it is the first byte of every section under generic section syntax, the same bytesection_keyalready reads to dedupe. This narrows what we carry without widening what we claim to parse.The filter runs after reassembly, in
si_section, because the EIT PID interleaves p/f with schedule and a section boundary is only findable by following the whole PID. When nothing survives the filter the entry is never created, so a schedule-only stream leaves no empty 0x0012 behind for export to emit.Export needed no change: it already walks
mpegts.sigenerically, honoring each PID's own interval.What is deliberately left out
EIT schedule. The catalog is whole-state and republished on every change, so cost per change is O(catalog), not O(delta), and every subscriber pays the full document at join, ahead of media. #2800's fixture yields 8 distinct schedule sections because it is one service with twelve events; a full multi-service eight-day EPG is thousands of sections whose window edges churn continuously as it rolls forward. That asymmetry does not depend on the magnitude, only on the catalog being whole-state. If schedule is ever wanted it belongs on its own track, the way SCTE-35 sections already ride one via
SectionStream, and the reasoning is recorded next to the 0x0012 entry.TDT/TOT (0x0014), which is #2800's ask 3. Every section is new content rather than a repetition, so each would be a catalog modification and a republish, and it is the table with the least to gain from being relayed: an exporter's own clock is a better source than a time forwarded from an upstream multiplexer of unknown delay. The omission was previously silent and read as an oversight; it now has the rationale next to it.
Tests
Three, in
export_test.rsalongside the existing SI coverage:eit_carries_present_following_not_schedulefeeds six sections interleaving p/f with schedule at both ends of both ranges, and asserts exactly the two p/f sections survive at a 2 s interval. This is also the regression test for the filter running post-reassembly.eit_schedule_alone_creates_no_entrycovers the empty-entry case.tdt_is_not_capturedpins the 0x0014 policy.The existing
multi_packet_si_section_is_capturedis refactored onto the newimport_sihelper rather than duplicating the setup.Not done here
#2800 offers TSDuck fixture scripts (
make-eit-fixture.sh,eit-roundtrip.sh) that synthesize an EPG onto any clip. Worth taking as a follow-up: no broadcast capture we hold carries EIT, which is why this gap survived #2440 with the mechanism in place and the table unrouted. The tests here are synthetic sections, not a real multiplex.Cross-package sync
doc/bin/cli.mddescribed TDT/TOT and EIT together as "live or bulky rather than static identity" - two correct rationales covering three tables, and the same conflation this PR splits. Updated. No draft change: thempegtscatalog section is not specified indrafts/. No js mirror:js/has no MPEG-TS support.(written by Opus 5)