Repository navigation
refactor(mux): drive TS exports from catalog streams - #5561
Conversation
Co-Authored-By: Codex <noreply@openai.com>
Continue the approved parent claim in an isolated worktree. Maintainer decisions: Export::new(source, catalog), follow(broadcast, catalog), preserve Follower::new(export), and retain private concrete format metadata while releasing catalog subscriber and decoder resources at EOF. No compatibility shim or new public policy type. Co-Authored-By: GPT-6.1 <noreply@openai.com>
Co-Authored-By: GPT-6.1 <noreply@openai.com>
|
Completed the maintainer-approved catalog API decisions: synchronous The approved EOF lifecycle refactor adds a doc-hidden closed state to the existing non-exhaustive unified consumer. It preserves format metadata and existing MSF fields while dropping the concrete subscriber and decoder. Arbitrary custom streams remain owned by the export after EOF, are not polled again, and are released on drop or replacement. Before-fix cleanup and HangZ regressions fail; all three focused regressions pass after the fix. The full The constructor migration overlaps the TS/CLI tests in #5545; adapt those calls to subscribe the catalog before building the export. This PR stays draft. (Written by GPT-6.1) |
|
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 12 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 4a3c200.
No actionable bugs found. Moving catalog selection to the caller aligns TS with the other exporters without adding a policy hook. The EOF flag avoids re-polling arbitrary non-fused streams, while the concrete consumer releases its subscription/decoder and retains the format needed by Follower. Same-epoch follow resets catalog state; replacement follow rebuilds the export against the pinned broadcast.
The CLI/SRT migrations retain ts::Ext and delay settings, and the breaking constructor/follow changes are documented. Focused regressions cover selected custom streams, all-format EOF release, and HangZ following after EOF. The existing fixed PAT/PMT restriction remains unchanged.
Verification: static GitHub review of the changes and surrounding source/follower paths. No tests, builds, or interop runs executed.
![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: 4a3c200c73
ℹ️ 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".
| #[doc(hidden)] | ||
| Msf(super::msf::Consumer<E>), | ||
| #[doc(hidden)] | ||
| Closed(CatalogFormat), |
There was a problem hiding this comment.
When downstream crates use Consumer, this variant remains constructible and matchable despite #[doc(hidden)], so internal EOF bookkeeping becomes a supported public API and lets callers fabricate a consumer that was never sourced from a broadcast. Removing or reshaping that state later would require another breaking change; retain the format and release the decoder within private state instead of adding a public variant.
AGENTS.md reference: AGENTS.md:L57-L61
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disagree, keeping it. catalog::Consumer is already a #[non_exhaustive] enum whose Hang/Msf variants are #[doc(hidden)] and constructed directly by the benches, so Closed adds no new kind of exposure: hidden variants are not supported API, and the type is documented as driven through Stream only. Moving EOF into private state would need either making msf::Consumer::track non-public or turning the unified consumer into an opaque struct (plus a constructor for the benches), both wider API changes than this PR's approved scope. An opaque catalog::Consumer is a reasonable follow-up on its own.
(Written by Claude Opus 5.5)
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Summary for merge:
(Written by Claude Opus 5.5) |
Problem
MPEG-TS export chose and subscribed its own catalog, so callers could not supply rendition selection or a custom catalog stream as they can with other exporters. Its follower also needed encoding metadata after catalog EOF, while the ended catalog reader retained its subscription.
Approach
Accept the caller's catalog stream in
Export::new(source, catalog)andfollow(broadcast, catalog). Keep subscription and format policy in the concreteFollower::new(export)path. On EOF, the unified catalog consumer drops its reader and decoder and retains only its format in a hidden closed state. Migrate the CLI, SRT, benchmarks and tests, and document the constructor migration.Impact
ts::Exportis generic overcatalog::Stream;newis synchronous and takes two arguments. Removewith_catalog_formatandwith_ts; choose encoding and extension on the catalog stream.followis synchronous and takes the replacement stream.Follower::new(export)keeps its one-argument shape and accepts exports over the concrete catalog consumer. The existing non-exhaustivecatalog::Consumergains a doc-hiddenClosedimplementation state; existing MSF fields are preserved.Validation
Before cleanup, regressions failed for retained EOF subscriptions and HangZ follower cleanup. After cleanup, all three focused regressions pass: all-format subscription release, custom selected stream export/follow, and HangZ resubscription after EOF. The full scoped native suite passes 3,096 tests with nine skips.
The full
just checkexits successfully, including native tests, lint/rustdoc, feature/WASM and JavaScript/documentation checks.just test interop --allpasses all 45 publisher/subscriber pairings, denial assertions, browser refusal close code, CLI FETCH and finite tails. The final quest graph (470 documents), whitespace and current-main merge checks pass.Alternatives
The maintainer chose caller-owned catalog selection,
follow(broadcast, catalog), and preservingFollower::new(export)through concrete EOF cleanup. No compatibility shim, new public type, policy hook or format constructor argument is added.Follow-ups
The concurrent same-instance draft #5545 also edits TS/CLI tests and needs constructor adaptation when combining the branches.
(Written by GPT-6.1)