Repository navigation
quest(m1): plan PSI pacing within the system buffers - #5304
Conversation
|
Scope decision from the maintainer on #5321: (Written by Claude Opus 5.5) |
|
A data point from a real broadcast recording, for the pacing work this PR waits on. We ran a 125 s cut of a DVB recording (TNT Sports 4, H.264 + AC-3, the video pausing for over a second at a time) through This PR's case occurs in production output. All the overruns fall in three slots, one at 14.3 s and two adjacent at 35.8 s. The two overflowing slots each carry PAT+PMT twice. The video overrun survives it, through a mechanism outside this PR's scope. Each overflowing slot follows one the paused video left empty, so the video's TB starts empty and the video is at its admission cap (358 packets at this rate). The interleave spreads the video over the positions it counts. The program tables go ahead of it outside that count, and so does this PR's end-of-slot padding. Slots with two leading table packets stay within TB: 36 such slots, peaking at 232–459 B. The two slots with four leading table packets peak at 552 and 646 B. With the duplicate dropped and the slot padded at its end, the video still overruns on 65 packets with the same 646 B peak. We also tried charging the tables to the null lane's share instead, which made it worse: 479 packets, peak 746 B, because the null lane starved and the video went out in runs of 21. So whatever paces the tables should keep a capped video lane spread over every position it can drain in. That means counting the table and padding positions in the interleave, or capping the lane by the positions left after them. Otherwise moving or padding table packets can compress the video. The two-table slots already peak within 53 B of the 512 B TB, so we would expect a PMT spanning two packets to overrun the same way without any duplicate, though we have not measured that. The capture is a private recording, so we can't attach it, but we can rerun the replay against any revision. The per-slot event locator we used replays TB on one PID and prints the PID pattern around each overflow. (Written by Claude Opus 5.5) |
|
Status after checking this against #5306 (head
What remains for the system buffers is Bsys (1,536 B drained at Rsys), which nothing models yet. A scratch probe on @t0ms: in #5306 the planner places tables, nulls and media at absolute positions against the video's TB, so the lane compression you found should be gone in padded output. A rerun of your replay against I'm recommending the maintainer close this PR rather than rebase it, and plan the remaining Bsys/ (Written by Claude Opus 5.5) |
|
Maintainer decision (2026-10-13): adjacent-copy suppression is dropped. #5306 supersedes it: its padded planner already paces TBsys (the PAT and PMT share one) and passes this PR's 3/4/8-PID regression as This PR is being repurposed rather than closed. The suppression commits remain in its history (last head @t0ms: your offer to rerun the replay still stands to settle the open point. Running it against #5306's head ( (Written by Claude Opus 5.5) |
Drop adjacent-copy suppression: #5306's padded planner already paces TBsys and passes #5304's regression. Rewrite psi-runs as the remaining work: Bsys beside TBsys in the planner, PID 1-3 passthrough in the same buffers, and a hard system-buffer check in compliance.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1672a5e to
4d057a1
Compare
kixelated
changed the title
|
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 31 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. |
|
@codex review (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 4d057a1 against base 1998d34, including the referenced planner at #5306 head 804196f.
No actionable bugs found in this two-file quest change. Building on the absolute-position planner is the simpler direction: padded layout already bypasses the round-robin stall, while shared transport-buffer admission currently covers PAT/PMT only. The revised scope and maintainer decision (#5304 (comment)) address the earlier suppression/video-compression concerns.
Non-blocking acceptance-test suggestions at quest/m1/tstd/psi-runs.md:40–45: retain explicit changed-version ordering and AAC tune-in checks; make the sustained Bsys burst cross slot boundaries so per-slot occupancy resets cannot pass.
Static review only; no tests or replay run. Bsys byte accounting remains an explicit design question; external standards were not independently validated.
![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: 4d057a11f1
ℹ️ 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".
| - Add Bsys beside TBsys in the planner. Settle first what enters Bsys (section | ||
| bytes, or whole payloads with stuffing): for single-packet tables it decides | ||
| whether Bsys can overflow at all. A probe on #5306 (`a5640abee`) counting | ||
| full payloads peaked near 1.35 KB at 4 aligned video PIDs and 2.3-2.7 KB |
There was a problem hiding this comment.
This is not an open choice between section bytes and payload bytes: H.222.0 §2.4.2.3 specifies that complete system-information TS packets enter TBsys and that bytes removed from TBsys are delivered to Bsys, except duplicate packets. Modeling either section bytes or a 184-byte payload therefore omits TS headers and possibly adaptation fields, so both the planner and compliance.py could accept a stream that crosses the 1,536-byte limit near the boundary. Specify and test all 188 bytes per admitted packet.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly taken in 610f9a7: whole 188 B packets are now listed as a candidate, settled from 2.4.2.3's text. I kept it as the quest's first question rather than deciding it here, since this plan is the wrong place to pin a standards reading without checking the clause against its byte-delivery rules for TBn to Bn. The implementer settles it with a test either way.
(Written by Claude Opus 5.5)
| Decided (2026-10-13), from #5304: adjacent-copy suppression is dropped. It | ||
| would only reach unpadded output, which makes no buffer claim, and #5306's | ||
| padded planner already passes #5304's 3/4/8-PID regression with every tune-in |
There was a problem hiding this comment.
When neither mpegts.muxRate nor --mux-rate is present, the stream still has the piecewise transport rate that T-STD derives from byte positions between PCRs; Schedule continues stamping unpadded slots on its PCR grid, and test/ts/README.md:259-268 explicitly documents grading without a declared mux rate. Consequently, a dense unpadded slot with adjacent PAT/PMT runs can still exceed Rxsys and overflow TBsys. Dropping the only unpadded protection on the premise that this output has no rate leaves that valid VBR case outside the stated T-STD-compliance goal rather than resolving it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining: excluding unpadded output from the system-buffer claim is the maintainer's decision on this PR (decision 1 in the description). Unpadded output has no declared mux rate, so its piecewise rate between PCRs is whatever the media happened to need, and the Goal says to name that exclusion in the profile report rather than grade it. If the maintainer later wants unpadded VBR graded too, that is a separate quest.
(Written by Claude Opus 5.5)
List whole packets as a Bsys accounting candidate, and keep the changed-version, multi-slot Bsys burst and AAC tune-in tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 610f9a7 against previously reviewed 4d057a1; base unchanged at 1998d34.
The suggested acceptance checks are now explicit at quest/m1/tstd/psi-runs.md:40–43: changed-PMT ordering, AAC tune-in, and a multi-slot Bsys burst. No new actionable findings.
The planner-based direction remains appropriate for padded-only scope. Bsys accounting remains an open implementation decision, including whole-packet accounting and consultation of the cited clause; this does not resolve the standards interpretation raised at #5304 (comment).
Static document review only; no tests, replay, or independent standards validation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 610f9a73c4
ℹ️ 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".
| - Open: when Bsys pacing would push a table past its unit's decode deadline, | ||
| fail loud like any missed deadline, or allow dropping a copy identical to one | ||
| still queued. Recommend failing loud unless a real source hits it. |
There was a problem hiding this comment.
When Bsys pacing cannot place a table before its associated unit, dropping an identical queued copy contradicts the goal that distinct tune-in runs are never dropped and reintroduces the suppression behavior abandoned above. Each repeated PAT/PMT establishes a separate mid-stream tune-in point, so retaining one copy does not preserve the later unit's entry point or the stated AAC tune-in invariant. Resolve this case by failing loudly as recommended rather than leaving the dropping option available.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining for now: the maintainer left this open on purpose, with failing loud as the recommendation. The alternative is narrower than the abandoned suppression: it would only drop a copy identical to one still queued ahead of the same unit, so that unit still receives its tables before it. The quest settles it only if a real source hits the deadline; otherwise it fails loud.
(Written by Claude Opus 5.5)
|
Merging. Summary:
(Written by Claude Opus 5.5) |
|
Re-ran the replay against #5306's head ( The video lane compression is gone. TB 111 never overruns (0 of 942,688 video packets, peak One thing is unchanged, in case it matters for the remaining buffer work: a handful of audio units stay in B |
Problem
This PR originally suppressed adjacent identical PAT/PMT runs so aligned keyframes on several video PIDs would not overflow the 512 B TBsys. #5306 rewrites the same layout code: its padded planner places tables, nulls and media at absolute packet positions, paces TBsys (shared by the PAT and PMT), and already passes this PR's 3/4/8-PID regression with every tune-in copy kept. Suppression would only reach unpadded output, which has no mux rate and makes no system-buffer claim. Bsys, PID 1-3 passthrough, and a
compliance.pygrade are still missing.Approach
The suppression commits stay in this PR's history (last head
1672a5e518fc2918535cde5cb39f0da399553823). The branch is reset tomainand now only rewritesquest/m1/tstd/psi-runs.mdin place as the remaining PSI pacing quest, keeping its inbound links:mpegts.sipassthrough on PIDs 1-3 in the same buffers.SystemBufferstall from fix(mux): split PAT and PMT sections across TS packets #5220.test/ts/compliance.py, with negative controls.quest/m1/tstd/README.md's Required line is reworded to match.Decisions
quest/m1/tstd/psi-runs.mdin place, keeping its inbound links (tstd/README.md,ts-reliability/validation.md,m2/tstd-controls.md), and reword the README's Required line.psi-pacing.mdand update the 3 links.Impact
quest checkpasses.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)