Repository navigation
fix(mux): reassemble PES headers with bounded staging - #5618
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Outcome: the full PES quest is implemented and removed with its reference. Legal fragmented clear headers assemble correctly; retention is bounded across PIDs, damage cannot concatenate units, and the approved unanchored-section EOF/clock-break policy is enforced. Validation passes: required scoped check and CI gates; 3,130 default native tests, 466 capture, 192 playback, 97/10 minimal media lanes; 180 focused parser/importer cases; all-language interop; generated 20-second fragmented-header round trip and independent FFmpeg/TSDuck checks. All 744 PCR accuracy intervals are within ±500 ns. The two new clock refusal regressions fail when their guards are removed. The final branch merges cleanly with current main and the worktree is clean; all owned validation processes are terminal. Public API/wire: no new export/configuration/catalog field or wire-format change. Input acceptance becomes stricter for exhausted staging, incomplete declared access units, and SCTE-35 sections without an anchor. No follow-up quest was created; the existing separate PES timing semantics work remains independent. Recommended disposition: leave this draft for the normal review/merge workflow; no further maintainer policy decision is pending here. (Written by GPT-6) |
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: 93cc519.
No actionable bugs found. Header staging preserves the complete-header fast path, handles split/special-ID layouts, and clears damaged state across loss, new starts, remaps and EOF. Shared byte/unit reservations release consistently, including rollback; unanchored-section refusal correctly depends on this source's clock rather than a shared offset alone.
Direction: bounded private staging and explicit damaged-input handling are proportionate. Tests cover the important ownership and parser boundaries without weakening the existing quality budgets.
Verification: static GitHub review plus TS job 116694733609 in run 38814226938, whose checkout identifies this head/base merge. Those logs show fragmented-header controls, decoded-content comparison and transport/T-STD/PCR checks succeeding. That fixture covers generated closed-GOP H.264/AAC and permits startup acquisition loss; I did not independently run tests or benchmarks.
|
Merge summary: no code changes from the merge pass.
Enabling auto-merge on this head. (Written by Opus 5.5) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe importer now assembles PES headers across TS packets, applies shared limits to retained PES and section data, and requires media timestamps before releasing held sections. The TS test harness adds a mode that generates fragmented audio and video PES headers and runs transport and content checks. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 62.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 6 files. (4 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main changes: PES header reassembly and bounded staging in the mux. Description check The description directly explains the problem, implementation, impact, validation, and follow-ups for the changeset. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 62.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 6 files. (4 skipped: 4 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 Clippy (1.98.1)
-
Clippy execution timed out
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 help to get the list of available commands.
Problem
A legal clear PES header split across TS packets was discarded as truncated. PES payloads and sections awaiting a clock could also retain input indefinitely, and short declared video payloads could escape as complete access units.
Approach
Assemble only incomplete headers before parsing; preserve the complete-header fast path and special stream-ID layouts. Honor declared payload lengths and reset assembly on loss, discontinuity, remapping, new starts, and EOF. Damaged audio can salvage complete units while incomplete tails are discarded.
PES payloads and untimed sections share a program-wide 16 MiB retention budget, derived from half a MoQ group's byte limit; held sections also share a 4,096-unit ceiling. Ownership accounting releases reservations on flush, damage, remapping, and drop. Following the approved policy, SCTE-35 sections without a source-local media anchor fail at EOF or a clock break, including when a shared timebase already has an offset. Three previous success fixtures now supply anchors.
Regressions cover every legal header split, special IDs, malformed lengths/prefixes, duplicate/lost packets, declared truncation, shared retention and rollback, and missing clocks. An automated fixture moves generated audio/video header splits into following null slots while preserving packet count and PCR position; it runs in the TS workflow. The 18-case PID/payload/fragmentation benchmark is wired nightly. The completed quest and its reference are removed.
Impact
Validation
just checkandjust ci checkpass on current main: 3,130 default native tests, 466 capture tests, 192 playback tests, and the 97/10 minimal media lanes, plus scoped compile/feature, documentation, quest, shell, workflow, and JS checks.just test interop --allpasses.All 180 focused importer/parser tests pass. Removing the EOF and clock-break guards makes both new refusal regressions fail. The 20-second generated fragmented-header round trip passes independent FFmpeg content/decoding and TSDuck continuity, T-STD, scheduling, and PCR checks: 500 video and 842 audio header splits; all 744 PCR accuracy intervals within ±500 ns. Fixture controls and the 18-case benchmark also pass.
Alternatives
Retaining the previous section-only timestamp-zero EOF contract was considered; the maintainer chose explicit refusal. A public resource-limit API was unnecessary because the existing group limits express a bounded private policy.
Follow-ups
No new quest was needed. The separate PES timing/header semantics quest remains out of scope. An initial broader validation invocation was stopped before tests to release compiler-cache locks during resource coordination; the final current-main scoped invocation passed and selects no io_uring test package.
(Written by GPT-6)