fix(moq-mux): hold TS export PSI until PMT-declared tracks resolve#1980
fix(moq-mux): hold TS export PSI until PMT-declared tracks resolve#1980t0ms wants to merge 3 commits into
Conversation
Fixes moq-dev#1979. The MPEG-TS exporter's PSI (PAT/PMT program tables) are locked synchronously and cannot be re-issued when new tracks arrive. This raced against codec-specific catalog-rendition timing: AAC registers its config on the first ADTS frame, while H.264/H.265 wait for the first keyframe's inline SPS. When audio resolved before video, the exporter locked PSI on an audio-only layout, then crashed with "TS track layout changed after PAT/PMT was emitted: '<name>' added" when video's rendition finally arrived. Solution: Use the PMT's elementary-stream count (known synchronously at parse time, before any codec resolves) as a hint to gate PSI construction. The importer publishes this hint into the `mpegts` catalog extension as `expected_tracks`, and the exporter waits for that many tracks to resolve before building PSI. This decouples readiness from codec-specific timing while preserving backward compatibility: non-TS sources (no hint) export immediately as before. Changes: - `catalog.rs`: Add `Mpegts::expected_tracks: Option<u16>` field - `import.rs`: Set `expected_tracks` when the PMT is parsed - `export.rs`: New `layout_complete()` gate; PSI build waits for declared count - `export_test.rs`: Four integration tests covering audio-first race, video-first order, non-TS sources, and real import→export roundtrip All 340 moq-mux tests pass; no regressions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughThis change adds an Changes
Sequence Diagram(s)sequenceDiagram
participant Import
participant Catalog
participant Export
participant PSIBuilder
Import->>Catalog: set mpegts.expected_tracks from PMT ES count
Catalog->>Export: update_catalog copies expected_tracks
Export->>Export: poll_next checks layout_complete()
alt tracks.len() < expected_tracks
Export-->>PSIBuilder: withhold PSI/program tables
else layout complete
Export->>PSIBuilder: build PSI/program tables
end
Related issues: #1979 Estimated code review effort: Medium-High — the gating logic in Suggested labels: moq-mux, mpeg-ts, bugfix Suggested reviewers: (repository maintainers familiar with moq-mux TS container code) Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
245-282: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPublish
expected_tracksbefore anyensure_*side effects.
expected_tracksis only written after the loop, butensure_sectionand the verbatim-producingensure_streampaths can register tracks during that same PMT pass. That still leaves a window where an exporter can observe a partial layout with no hint and lock PSI on it, so the race remains for immediately-resolved section/verbatim tracks. Compute the count directly frompmt.es_infoand store it before callingensure_section/ensure_stream.🤖 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 245 - 282, The `expected_tracks` update in the PMT handling flow needs to happen before any `ensure_section` or `ensure_stream` side effects can register tracks. In the PMT loop in `import.rs`, move the `expected` computation and the `catalog::mpegts_mut` assignment ahead of the per-ES `ensure_*` calls, using `pmt.es_info` and the existing `sections`/`streams` checks so exporters see the full expected layout during the same pass. Keep the logic anchored around `pmt.es_info`, `ensure_section`, `ensure_stream`, and `expected_tracks` so the PSI state is published first.
🧹 Nitpick comments (1)
rs/moq-mux/src/container/ts/export_test.rs (1)
293-570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffKeep these regressions inline with
export.rs.Adding more cases to
export_test.rscontinues the split from the repo's Rust test convention. Please move them under#[cfg(test)] mod testsinrs/moq-mux/src/container/ts/export.rs. As per coding guidelines, "Keep Rust tests inline as#[cfg(test)] mod testsin the source file".🤖 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_test.rs` around lines 293 - 570, The new regression tests were added in the standalone export_test module, but they should live inline with the production code. Move the cases into the `#[cfg(test)] mod tests` block inside `export.rs`, keeping the existing test names and helpers like `export_holds_psi_until_pmt_declared_tracks_resolve`, `export_holds_psi_until_all_declared_tracks_resolve_video_first`, and `import_export_roundtrip_carries_expected_tracks_hint` so they remain alongside `Export::with_ts` and related TS export logic.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/export_test.rs`:
- Around line 507-510: The test around exporter.next() is too loose because it
only checks that the future completes, which allows Ok(Err(_)) and Ok(Ok(None))
to pass. Update the assertion in export_test::... to explicitly match
Ok(Ok(Some(_))) as the only success case, and make the other Ok(...) branches
fail so the test verifies an actual emitted frame instead of just completion.
- Around line 541-546: The test currently skips validation when expected_tracks
is None, which lets the importer regression pass unnoticed. Update the assertion
in this export test to require expected_tracks to be set and at least 1, so the
test fails immediately if the importer never populates the hint. Keep the check
in the same test block where expected_tracks is examined, and make sure it
enforces both presence and value instead of only validating Some(count).
---
Outside diff comments:
In `@rs/moq-mux/src/container/ts/import.rs`:
- Around line 245-282: The `expected_tracks` update in the PMT handling flow
needs to happen before any `ensure_section` or `ensure_stream` side effects can
register tracks. In the PMT loop in `import.rs`, move the `expected` computation
and the `catalog::mpegts_mut` assignment ahead of the per-ES `ensure_*` calls,
using `pmt.es_info` and the existing `sections`/`streams` checks so exporters
see the full expected layout during the same pass. Keep the logic anchored
around `pmt.es_info`, `ensure_section`, `ensure_stream`, and `expected_tracks`
so the PSI state is published first.
---
Nitpick comments:
In `@rs/moq-mux/src/container/ts/export_test.rs`:
- Around line 293-570: The new regression tests were added in the standalone
export_test module, but they should live inline with the production code. Move
the cases into the `#[cfg(test)] mod tests` block inside `export.rs`, keeping
the existing test names and helpers like
`export_holds_psi_until_pmt_declared_tracks_resolve`,
`export_holds_psi_until_all_declared_tracks_resolve_video_first`, and
`import_export_roundtrip_carries_expected_tracks_hint` so they remain alongside
`Export::with_ts` and related TS export logic.
🪄 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: 6ca2435c-7b2e-4ec6-a4ed-a70488b775b1
📒 Files selected for processing (4)
rs/moq-mux/src/container/ts/catalog.rsrs/moq-mux/src/container/ts/export.rsrs/moq-mux/src/container/ts/export_test.rsrs/moq-mux/src/container/ts/import.rs
| match tokio::time::timeout(std::time::Duration::from_millis(50), exporter.next()).await { | ||
| Ok(_) => {} // got output, as expected (no waiting for missing video) | ||
| Err(_) => panic!("exporter must not wait when expected_tracks is None"), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that a frame was emitted, not just that the future completed.
This accepts Ok(Err(_)) and Ok(Ok(None)), so the test can pass on exporter failure or EOF. Match Ok(Ok(Some(_))) explicitly and fail the other Ok(...) cases.
Suggested fix
- match tokio::time::timeout(std::time::Duration::from_millis(50), exporter.next()).await {
- Ok(_) => {} // got output, as expected (no waiting for missing video)
- Err(_) => panic!("exporter must not wait when expected_tracks is None"),
- }
+ match tokio::time::timeout(std::time::Duration::from_millis(50), exporter.next()).await {
+ Ok(Ok(Some(_))) => {}
+ Ok(other) => panic!("expected an output frame, got {other:?}"),
+ Err(_) => panic!("exporter must not wait when expected_tracks is None"),
+ }📝 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.
| match tokio::time::timeout(std::time::Duration::from_millis(50), exporter.next()).await { | |
| Ok(_) => {} // got output, as expected (no waiting for missing video) | |
| Err(_) => panic!("exporter must not wait when expected_tracks is None"), | |
| } | |
| match tokio::time::timeout(std::time::Duration::from_millis(50), exporter.next()).await { | |
| Ok(Ok(Some(_))) => {} | |
| Ok(other) => panic!("expected an output frame, got {other:?}"), | |
| Err(_) => panic!("exporter must not wait when expected_tracks is 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/export_test.rs` around lines 507 - 510, The test
around exporter.next() is too loose because it only checks that the future
completes, which allows Ok(Err(_)) and Ok(Ok(None)) to pass. Update the
assertion in export_test::... to explicitly match Ok(Ok(Some(_))) as the only
success case, and make the other Ok(...) branches fail so the test verifies an
actual emitted frame instead of just completion.
| if let Some(count) = expected { | ||
| assert!( | ||
| count >= 1, | ||
| "expected_tracks should be at least 1 for this file with audio+video" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail when the importer never sets expected_tracks.
The if let makes this regression succeed when the hint is still None, which is exactly the behavior this test is supposed to catch.
Suggested fix
- if let Some(count) = expected {
- assert!(
- count >= 1,
- "expected_tracks should be at least 1 for this file with audio+video"
- );
- }
+ let count = expected.expect("importer should populate expected_tracks from the PMT");
+ assert!(
+ count >= 1,
+ "expected_tracks should be at least 1 for this file with audio+video"
+ );📝 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.
| if let Some(count) = expected { | |
| assert!( | |
| count >= 1, | |
| "expected_tracks should be at least 1 for this file with audio+video" | |
| ); | |
| } | |
| let count = expected.expect("importer should populate expected_tracks from the PMT"); | |
| assert!( | |
| count >= 1, | |
| "expected_tracks should be at least 1 for this file with audio+video" | |
| ); |
🤖 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_test.rs` around lines 541 - 546, The test
currently skips validation when expected_tracks is None, which lets the importer
regression pass unnoticed. Update the assertion in this export test to require
expected_tracks to be set and at least 1, so the test fails immediately if the
importer never populates the hint. Keep the check in the same test block where
expected_tracks is examined, and make sure it enforces both presence and value
instead of only validating Some(count).
| /// complete one instead of locking PSI on whichever subset has resolved first. | ||
| /// `None` when the catalog wasn't produced from an MPEG-TS source. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub expected_tracks: Option<u16>, |
There was a problem hiding this comment.
I think this is the wrong fix.
I think export should instead wait IDK like 100ms for the catalog to stabilize, since TS doesn't support reinitializing (just like fMP4).
On the import side, we could wait until all tracks are initialized before publishing the catalog too, but it's still racey.
|
superseded by #1991 |
2 participants
Summary
Fixes #1979: MPEG-TS exporter PSI (program tables) race condition. When audio codec resolves before video, the exporter locked PSI on an audio-only layout, then crashed when video's rendition arrived.
Solution: Gate PSI construction on the PMT's elementary-stream count (known at parse time), not on whichever subset of tracks has resolved. The importer publishes this as
expected_tracksin thempegtscatalog extension; the exporter waits for that many tracks before building PSI.Changes
Mpegts::expected_tracks: Option<u16>field carrying PMT-declared elementary stream countexpected_trackswhen PMT is parsed (before any codec resolves)layout_complete()gate; PSI build waits for declared count to resolveTesting
Backward Compatibility
Non-TS sources (no `expected_tracks` hint) export immediately as before, preserving existing behavior.
🤖 Generated with Claude Code