Skip to content

quest(m1): plan the TS repeated-tables fix - #5290

Closed
t0ms wants to merge 2 commits into
moq-dev:mainfrom
t0ms:plan/ts-psi-repeat
Closed

t0ms wants to merge 2 commits into
moq-dev:mainfrom
t0ms:plan/ts-psi-repeat

Conversation

@t0ms

@t0ms t0ms commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Plans quest/m1/ts-psi-repeat.md [M], the follow-up kixelated recommended when landing #5207: moq export ts never lays copies of the PAT and PMT back to back where they would overrun those PIDs' 512-byte transport buffers. Every copy that marks its own tune-in point still goes out.

Export carries every video rendition and writes the tables ahead of each video keyframe, so an ABR ladder of three or more renditions with aligned keyframes puts three PATs back to back today (about 526 B at 25 Mb/s), on main before #5207 as after. The quest starts from the narrower diff in the #5207 review thread, which drops only a run laid right behind an identical one. It verifies with a transport-buffer check on the system PIDs, in a unit test and in compliance.py, whose strict T-STD check models only elementary streams today.

Ranked in m1 just after ATSC AC-3, beside the other TS export quests.

Public API: none (planning only). Wire: none. quest check passes.

Decision paper trail

  • Goal: ✅ Never bunch copies past the PAT/PMT buffers, keeping every tune-in copy and changed tables in order, from the endorsed diff (recommended) / Also space out a new version bunched with the old tables
  • Verification: ✅ Unit buffer test on the PAT and PMT, plus a system-buffer check in compliance.py (recommended) / Those plus a three-rendition Interop arm / Unit test only
  • Placement: ✅ m1, just after ATSC AC-3 (recommended) / m2
  • System-buffer check PIDs: ✅ PAT and every PMT PID (recommended) / Those plus DVB SI (SDT, EIT)
  • System-buffer check severity: ✅ Hard, with every existing TS arm still passing (recommended) / Report-only at first

(Written by Claude Opus 5.5)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-10T22:30:17.025091Z 343658a PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kixelated

Copy link
Copy Markdown
Collaborator

Automated review of 343658a (quest-only: quest/m1/ts-psi-repeat.md + README entry)

Checked against main: Slot::layout and Slot::nulls: usize exist in rs/moq-mux/src/container/ts/schedule.rs (L133-188), aac_program_config_follows_each_table exists in export_test.rs, and compliance.py has no system-buffer (TBsys/Rxsys) modeling today, so the "elementary streams only" claim holds. README placement right after ATSC AC-3 matches.

Non-blocking:

  1. The "about 526 B" figure doesn't obviously follow. Three back-to-back PATs are 564 B; at 25 Mb/s the gap between packet arrivals is ~60 µs, so Rxsys = 1 Mb/s drains only ~15 B across the burst, peaking near ~549 B. Please show the arithmetic (or the measured number) so the unit test's expected overflow is grounded; otherwise the test's threshold may be tuned to a wrong figure.
  2. Two-rendition case is unaddressed. Two PATs (376 B) fit, but PAT+PMT share no buffer, while multiple PMT PIDs each have their own. Worth stating explicitly that the check is per-PID and that two renditions are safe, so the test covers 2 (pass) and 3 (fail without fix).
  3. Padding back with nulls "at the end of a padded slot" changes the slot's byte layout; say whether PCR spacing / the existing T-STD strict check is expected to be unchanged, and assert it in the test, since moving packets later in a slot can shift PCR-bearing packets.
  4. The plan leans on a review-thread diff (fix(moq-mux): interleave each TS slot so no PID overruns its transport buffer #5207 discussion) as the starting point; if that diff gets edited or the thread resolved, the quest loses its spec. Consider inlining the key rule (drop a run identical to the run directly before it in the same flush).

Verdict: MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: 343658a

No actionable findings in this planning-only change. Deduplicating identical table runs within the same flush is a focused approach: it preserves distinct tune-in points and changed versions without redesigning the scheduler. The padded/unpadded distinction and buffer-based regression cover the important risks.

Checked both changed files against the linked #5207 discussion, current scheduler/export ordering, AAC tune-in test, and compliance model. No public API or wire changes in this PR. Static review only; tests and quest validation were not run.

(Written by OpenAI)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 343658abc1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread quest/m1/ts-psi-repeat.md
@@ -0,0 +1,40 @@
# [M] TS export never bunches repeated program tables

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Merge this with the existing PSI-runs quest

The parent commit already added quest/m1/tstd/psi-runs.md and linked it from quest/m1/tstd/README.md for this exact #5207 defect: it has the same aligned-keyframe scenario, review-thread/null-padding fix, and system-buffer regression test. Creating a second M1 quest leaves one fix with two statuses ([S] versus [M]), so completing either quest leaves the other falsely open; fold the extra detail into the existing quest instead.

AGENTS.md reference: AGENTS.md:L46-L46

Useful? React with 👍 / 👎.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@t0ms

t0ms commented Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks. Addressed in e6bad36:

  1. The 526 B figure holds, but "three PATs back to back" was loose wording. The flush lays PAT, PMT, PAT, PMT, PAT, PMT, so the third PAT ends five packets in, after about 301 µs at 25 Mb/s. About 38 B drains at 1 Mb/s in that time, leaving 564 − 38 ≈ 526 B. The quest now shows that arithmetic.
  2. The test now runs each system PID through its own buffer and covers two aligned video PIDs, which pass either way (376 B), as well as three, which fail without the fix.
  3. A slot still opens with its clock packet, and padding the dropped packets back keeps it the same length, so no PCR moves. The quest now says so. The existing strict checks in every TS arm keep grading PCR accuracy.
  4. The quest already states the rule in full: a run of tables that the same flush would lay right behind an identical run is dropped, padded back with nulls at the end of a padded slot, and Slot::nulls becomes Option<usize>. The link is only a starting point for the code.

(written by Claude Opus 5.5)

@coderabbitai

coderabbitai Bot commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c475a758-f7b6-4a3f-a3f3-dbe46bf96542

📥 Commits

Reviewing files that changed from the base of the PR and between 7cebbf7 and e6bad36.


📒 Files selected for processing (2)
  • quest/m1/README.md
  • quest/m1/ts-psi-repeat.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.



Walkthrough

The README adds “TS table bursts” to the Required list. A new proposal describes dropping immediately repeated PAT and PMT table runs, restoring padded-slot length with null packets, and checking table bursts against a 512-byte system buffer. It also specifies aligned-keyframe test cases with three and two video PIDs.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to e6bad

This planning-only change introduces no identified runtime risk and is ready to merge subject to normal checks.

Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check Passed The title clearly identifies the quest, its scope, and its purpose: planning the TS repeated-tables fix.
Description check Passed The description directly explains the planned PAT/PMT repeated-table fix, its motivation, verification plan, placement, and planning-only scope.
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches
✨ Simplify code
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kixelated

kixelated commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Thanks @t0ms, this is a careful plan. It turns out to duplicate quest/m1/tstd/psi-runs.md, which landed on main in #5271 just before this opened, and #5304 is already implementing that quest.

Your extra detail is not lost: #5321 folds it into psi-runs.md, including the hard-severity compliance.py system-buffer check, the overflow arithmetic, the rendition-count unit test, and the Slot::nulls notes from the #5207 diff. One correction came out of review there: H.222.0 2.4.2.3 feeds the PAT and the PMT into one shared 512 B TBsys per program, so two back-to-back pairs already overflow (about 729 B at 25 Mb/s) and the two-rendition case is now a negative control. Closing this one in favor of that.

(Written by Claude Opus 5.5)

@kixelated kixelated closed this Oct 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants