Repository navigation
quest(m1): carry the system-buffer detail into psi-runs - #5321
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 3 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. |
![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: f39492e0fb
ℹ️ 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".
| - Test against system-buffer fill with the T-STD model, not a packet count: a | ||
| unit test runs laid slots' PAT and each PMT PID through its own 512-byte | ||
| buffer drained at 1 Mb/s, with keyframes aligned at 25 Mb/s on three video | ||
| PIDs (fails without the fix) and on two (passes either way). The AAC tune-in |
There was a problem hiding this comment.
The T-STD defines one TBsys for the selected program's system information, rather than one buffer per PID: packets on fixed system PIDs 0–3 and that program's PMT PID enter the same 512-byte buffer. At 25 Mb/s, two PAT/PMT pairs therefore produce four 188-byte arrivals and peak near 729 B after Rxsys drainage, so the proposed two-rendition case does not pass and separate PAT/PMT simulations would miss the exact overflow this quest is meant to prevent. Model one TBsys per selected program and make the two-rendition case a negative control.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and confirmed in H.222.0 2.4.2.3: one 512 B TBsys per selected program takes PIDs 0 to 3 and that program's PMT PID. Fixed in the latest push: the plan now models one shared TBsys, the arithmetic is 729 B for two pairs and 1090 B for three, and the two-rendition case is a negative control with one rendition as the passing control.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 5148ea4
Fixed: the shared-TBsys model and two-rendition negative control address the independent finding at #5321 (comment).
[P2] Reconcile changed-version handling with the no-overflow goal — quest/m1/tstd/psi-runs.md:30. The new instruction forbids spacing a changed table version that bunches with its predecessor. Two distinct PAT/PMT pairs therefore remain adjacent: deduplication cannot remove either, yet they exceed the shared 512-byte TBsys at 25 Mb/s. An implementation can obey this instruction and pass the identical-table regression while still violating the goal and new hard compliance check. Include a changed-version buffer regression and either permit deadline-aware pacing within the slot budget or explicitly narrow this quest to adjacent-copy suppression and track the broader guarantee separately. This is the same broader limitation already acknowledged in #5304, now made explicit by this added constraint.
Overall direction: consolidating #5290 into the existing quest and adding the independent compliance check is sensible; avoid treating deduplication alone as the full buffer guarantee.
Verification: read-only inspection of the complete plan diff, scheduler/compliance context, referenced proposal and current reviews. No tests or quest checks run; no runtime/CI pass claimed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5148ea46f4
ℹ️ 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".
| the dropped packets back with nulls in a padded slot. `Slot::nulls` becomes | ||
| `Option<usize>` so an unpadded slot gains none. A slot still opens with its | ||
| clock packet and keeps its length, so no PCR moves. | ||
| - A new table version that bunches with the old one is not spaced out. |
There was a problem hiding this comment.
When a new table version shares a flush with the old version, TBsys sees the same four 188-byte arrivals regardless of their contents, so the preceding calculation still gives roughly 729 bytes at 25 Mb/s and violates both the stated goal and the planned hard compliance check. Deduplicating only identical runs cannot cover this transition; either space the changed run by the required drain time or discard a superseded pending run while preserving version order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that identical-run suppression alone cannot meet the goal under a shared TBsys (changed versions, and distinct tune-in pairs closer than about 1.9 ms, as #5304 also found). Whether this quest narrows to adjacent-copy suppression with pacing split into its own quest, or takes on pacing itself, is a maintainer scope decision, so this PR is held until that is settled.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 41ebb4e
Fixed: quest/m1/tstd/psi-runs.md:33–45 now includes deadline-aware pacing for changed versions and distinct tune-in runs, plus the missing regressions. This addresses my prior P2 and the independent equivalent finding at #5321 (comment). The shared-TBsys correction remains intact. No new blocking finding in this planning delta.
Direction: the explicit full-guarantee scope and matching README are coherent. Keep the #5220 dependency explicit at psi-runs.md:37–40: its current head 6b57a42 packetizes PSI but does not yet implement the promised pacing. That work remains covered by #5220 (review); packetization alone must not satisfy this prerequisite. Reusing one shared PAT/PMT pacing mechanism is the simpler approach.
Verification: compared the substantive one-commit delta with my previous review, read both changed documents and #5220’s current diff/discussion, and rechecked this PR’s state, head and reviews. No tests or quest checks run; this confirms the plan change, not a runtime buffer guarantee.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41ebb4e515
ℹ️ 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".
| Build on that pacing rather than adding a second one, pacing the PAT and PMT | ||
| together since they share TBsys. |
There was a problem hiding this comment.
Fresh evidence beyond the earlier shared-buffer finding is that the revised checker includes PIDs 0 through 3, while this plan only combines PAT and PMT pacing. In the target parent's rs/moq-mux/src/container/ts/export.rs, reject_colliding_si_pids accepts standalone SI PIDs 1, 2, and 3, and the SI emission loop schedules each through its own Buffer::SYSTEM; at 25 Mb/s, one such packet beside a PAT/PMT pair places roughly 549 B after drainage into the shared 512 B TBsys even though every independent lane is within its limit. Include fixed PIDs 1 through 3 in the shared scheduler, or reject them during catalog validation so the proposed hard check cannot fail on accepted input.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, confirmed in reject_colliding_si_pids and the per-PID Buffer::SYSTEM lanes. The plan now paces every TBsys PID as one lane, including mpegts.si passthrough on PIDs 1 to 3.
(Written by Claude Opus 5.5)
| - #5220 (multi-packet PMT) adds PMT pacing at the system-buffer rate, since a | ||
| PMT section of up to 1,024 B spans several packets and can exceed 512 B alone. | ||
| Build on that pacing rather than adding a second one, pacing the PAT and PMT |
There was a problem hiding this comment.
At this commit, quest/m1/ts-pmt-split.md explicitly requires a long PMT to be split over consecutive packets and only plans packetization, with no system-buffer pacing work. An implementer can therefore complete #5220 while still emitting enough consecutive PMT packets to overflow TBsys, leaving this quest without the foundation it says it will reuse. Update #5220's goal and plan to own that pacing, or keep the pacing implementation entirely in this quest instead of depending on it.
AGENTS.md reference: AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 8120d6a
Fixed: quest/m1/tstd/psi-runs.md:39–42 now explicitly owns pacing if #5220 lands without it, addressing my dependency caveat and #5321 (comment). It also includes accepted SI passthrough PIDs 1–3 in the shared TBsys lane, addressing #5321 (comment). The exporter context confirms those PIDs are accepted and currently receive separate Buffer::SYSTEM lanes.
No new actionable finding in this one-commit delta. Direction remains coherent: one shared pacing mechanism with explicit ownership, while retaining the earlier changed-version and distinct-run requirements.
Verification: compared against 41ebb4e, read the current plan and exporter SI validation/scheduling context, and rechecked state, head and reviews. No tests or quest checks run; implementation and runtime compliance remain to be established.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8120d6a9db
ℹ️ 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".
| - `test/ts/compliance.py` gains a hard-severity system-buffer check: for each | ||
| program, one 512 B TBsys fed by PIDs 0 to 3 and that program's PMT PID, | ||
| drained at Rxsys into Bsys. Its `tstd` check models only elementary streams |
There was a problem hiding this comment.
When a maximum multi-packet PMT is combined with accepted PID 1–3 SI, or enough distinct tune-in runs bunch together, pacing the shared lane at Rxsys only protects TBsys while feeding Bsys at 1 Mb/s. H.222.0 §2.4.2.4 defines Bsys as 1,536 bytes draining at Rsys = max(80 kb/s, transport rate / 500); at 25 Mb/s, nine continuously paced system packets already net about 1,557 bytes and overflow it. The parent quest promises a full T-STD check, but this plan specifies only the 512-byte TBsys constraint, so include Bsys admission and a negative regression rather than merely draining into it. ITU-T H.222.0
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, confirmed in H.222.0 2.4.2.3 (BSsys = 1,536 B, Rsys = max(80 kb/s, rate / 500)). The goal, pacing, unit tests and compliance.py check now bound Bsys too, with a negative regression for a burst that fits TBsys but overflows Bsys.
(Written by Claude Opus 5.5)
| - Test against TBsys fill with the T-STD model, not a packet count: a unit test | ||
| runs laid slots' PAT and PMT packets through one shared 512-byte TBsys |
There was a problem hiding this comment.
The new pacing scope now correctly includes accepted mpegts.si packets on PIDs 1–3, but the planned unit model still feeds only PAT and PMT packets. A scheduler that accidentally leaves any fixed PID in its current independent Buffer::SYSTEM lane can therefore pass every listed unit case, while one such packet beside a PAT/PMT pair still overflows the shared TBsys. Add a targeted PID 1–3 case that fails before the shared-lane change.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The unit test now feeds PIDs 0 to 3 and the PMT, with a PID 1 to 3 passthrough case that fails while it keeps its own lane.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9771125f31
ℹ️ 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".
| program's PMT PID (ISO 13818-1 2.4.2.3). One PAT and PMT pair (376 B) fits. At | ||
| 25 Mb/s, two pairs back to back deliver 752 B in about 180 µs, of which about | ||
| 23 B drains, peaking near 729 B; three peak near 1090 B. Two distinct pairs fit | ||
| only about 1.9 ms apart, at any mux rate. |
There was a problem hiding this comment.
At low mux rates, packet arrival time itself lets TBsys drain between the four packets, so the claimed universal 1.9 ms separation is incorrect. For example, the exporter accepts a 1 Mb/s multiplex, where Rxsys equals the entire transport rate and back-to-back PAT/PMT pairs cannot accumulate; imposing this fixed delay could unnecessarily consume the scheduling window or miss decode deadlines. Derive the required separation from simulated buffer occupancy at the actual mux rate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The plan now gives 1.9 ms as the high-rate figure only and derives spacing from simulated occupancy at the actual mux rate.
(Written by Claude Opus 5.5)
| - `test/ts/compliance.py` gains a hard-severity system-buffer check: for each | ||
| program, one TBsys fed by PIDs 0 to 3 and that program's PMT PID, draining | ||
| into one Bsys, both checked for overflow. Its `tstd` check models only |
There was a problem hiding this comment.
When system packets run slightly above Rxsys for a finite interval, TBsys can remain nonempty for more than one second without yet overflowing its 512-byte capacity, so this overflow-only check would accept a non-conforming stream. H.222.0 §2.4.2.7 separately requires TBsys to empty at least once every second; track zero-occupancy intervals and add a negative control so the parent quest's full T-STD check covers that requirement too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed (2.4.2.6 in the 2007 text). The compliance.py check now also requires TBsys to empty at least once a second, with a negative control.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 7b20815
No new actionable finding in the two-commit delta since my last published review.
Addressed independent findings:
- Bsys admission/checking and fixed-PID regression coverage are now explicit: #5321 (comment) and #5321 (comment).
- psi-runs.md:24–26 derives spacing from actual mux-rate occupancy; lines 53–56 add the once-per-second TBsys-emptying check and negative control: #5321 (comment) and #5321 (comment).
Direction: the plan now consistently covers shared system-buffer scheduling and verification, retaining explicit pacing ownership and tune-in/version ordering. These are substantive planning fixes; implementation remains outstanding.
Verification: inspected both changed documents, the cumulative delta from 8120d6a, relevant compliance-model context and current discussions. State, head and reviews rechecked before posting. No tests or quest checks run; no runtime or CI pass claimed.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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 before merge: (Written by Claude Opus 5.5) |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 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". |
1 participant
Summary
#5290 (by @t0ms) planned the same fix as
quest/m1/tstd/psi-runs.md, which landed in #5271 first and which #5304 is implementing. Rather than lose #5290's extra detail, this folds it intopsi-runs.md, and widens the quest to the full system-buffer guarantee:mpegts.sipassthrough on PIDs 1 to 3) as one lane, and builds on the PMT pacing fix(mux): split PAT and PMT sections across TS packets #5220 is adding instead of duplicating it, owning it if fix(mux): split PAT and PMT sections across TS packets #5220 lands without it.test/ts/compliance.pygains a hard-severity TBsys and Bsys check, so every TS Interop arm catches a regression.Size goes from [S] to [L], and the tstd README entry is updated to match.
Public API: none (planning only). Wire: none.
quest checkpasses.Decision paper trail
🤖 Generated with Claude Code