Repository navigation
fix(ts): carry a full-rate CBR feed at its own multiplex rate - #5516
Conversation
The schedule charged video whole 188-byte packets at the lesser of Rx and Rbx, plus the clock packet, so a source filling its level's leak, as a full-rate feed with no HRD does, fell behind and missed a decode deadline. The transport buffer now drains whole packets at Rx, the clock packet among them, while MB passes only elementary bytes on into EB at Rbx. Unpadded, a slot carries the lesser of the two, fractions carried from slot to slot, without the one-packet slack. `just test ts --leak` round-trips 1080p video with no HRD at level 4.0's 24 Mb/s leak. With the schedule this replaces, its export stops about 9 s into the 30 s clip. Retires quest/m1/tstd/video-leak.md. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Held to a transport buffer's worth ahead of the leak, MB emptied whenever audio due sooner took a position or two between video packets, and the leak idled. On a feed whose video fills Rbx, the export carried about 1 % less video than the source and fell behind. H.222.0 2.14.3.1 gives MB BSmux for this, 4 ms of the larger of Rbx and 2 Mb/s, and BSoh for PES headers. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
A padded export spent a whole clock packet on every 25 ms slot and a PES on every audio frame. A source multiplex with no null packets has no room for either, so an export at the source's own rate fell behind it until its lead ran out and it missed a deadline (moq-dev#5362). - Each slot's PCR rides the adaptation field of the clock PID's first packet in the slot when one goes out there, reflowing that PES's payload, and falls back to a clock packet of its own otherwise. - Consecutive frames of every audio codec (AAC, MP2, AC-3, E-AC-3 and AC-3 carried in a private stream) share a PES while they run on without a gap and fit B whole with their header. Each frame leaves B at its own decode time. PESs start on a grid of the media timeline, so exporters that joined at different frames cut the same PESs. - Frames and grid slots follow each other in decode order however often the exporter is polled, and tables muxed while frames are still packed go out with the next unit, in the order they were written. `just test ts --full` exports a generated 30 s clip at its own rate with no nulls: an x264 video at its level's leak beside MP2 and DVB AC-3, with the PCR in the video PID and several audio frames per PES. The new `overhead.py` fails the export if it spends more packets than the source on clocks and audio framing; CI runs it in the TS workflow. Retires quest/m1/tstd/mux-overhead.md. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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 4c34f29.
One actionable issue: the new embedded-PCR path can underflow its null-packet counter in one-packet slots (inline). Separating transport Rx from elementary-payload Rbx and retaining per-frame audio removal while packing PES are sensible directions; no additional concrete regression found in the inspected packing, reflow and cleanup paths.
Add a one-packet-slot regression. Packing-specific coverage would also benefit from variable-size MP2/E-AC-3 frames and short-delay cases beyond the new AAC/DVB AC-3 tests.
Verification: static inspection of the changed scheduler/exporter, surrounding code, tests and CI harness. No tests or contribution-feed replay executed; reported runtime results were not independently verified.
| } | ||
| out.positions.as_mut().unwrap().push(position); | ||
| out.packets.extend_from_slice(packet); | ||
| out.nulls -= 1; |
There was a problem hiding this comment.
[P2] Account for the embedded clock before decrementing nulls
padded() initializes out.nulls to count - 1, reserving a standalone clock. When count == 1 and position zero can carry a PCR-bearing media packet, this subtracts from zero before the opening branch refunds that reservation at line 1037. Debug/test builds and other overflow-checked builds therefore panic. For example, the accepted 100,000 b/s mux rate produces one- and two-packet slots, so an eligible small PES on an audio-only clock PID can hit this path. Default release wrapping happens to recover, but checked builds fail. Apply the opening-clock adjustment before subtraction (or only decrement for non-opening media) and add a one-packet-slot regression.
There was a problem hiding this comment.
Agreed, reproduced: at 90 kb/s a one-packet slot whose only position carries the PCR panicked with "attempt to subtract with overflow" in a debug test. Fixed in 3b69dad: a PCR-bearing packet takes the clock packet's position without spending a null, and any other packet takes a null's. Regression: the_pcr_rides_the_only_packet_of_a_one_packet_slot.
(Written by Claude Opus 5.5)
ℹ️ Recent review info
WalkthroughThe TS scheduler now models transport drain and video payload leak separately, tracks frame-level decoder-buffer occupancy, and can place an opening PCR in a media packet. The TS export combines eligible consecutive audio frames into PES packets while retaining frame timing and byte counts. New leak and full-multiplex test profiles, an overhead analyzer, CI steps, and documentation exercise these changes. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 91.43% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 105 functions across 6 files. |
| 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. |
| Title check | |
The title clearly summarizes the primary change: enabling TS exports to carry full-rate CBR feeds at their own multiplex rate. |
| Description check | |
The description is directly related to the changeset and explains the problem, implementation approach, tests, impact, alternatives, and follow-up considerations. |
✨ Finishing Touches: 💡 1
-
✨ Simplify code
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Review coverage is incomplete: 3 files could not be fully reviewed. Findings from completed review steps are included; see review info for 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 @coderabbitai help to get the list of available commands.
![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: 4c34f2953f
ℹ️ 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".
| /// Each frame opens with its slot's PCR: at a multiplex rate, in the adaptation field of | ||
| /// the clock PID's packet when one can go out there, else in a clock packet of its own. |
There was a problem hiding this comment.
This new contract correctly allows the PCR to ride a clock-PID media packet, but the module-level documentation at export.rs:4-7 still promises that the PCR always rides its own adaptation-field-only packet and that every output frame contains a separate clock packet. That now misstates the exported wire shape and packet overhead, so update the module header alongside this behavior change.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The module header in 3b69dad now says each frame opens with its PCR, which rides the clock PID's packet when one can open the slot and otherwise takes an adaptation-field-only packet of its own.
(Written by Claude Opus 5.5)
| if let Some(rest) = opening { | ||
| unit.packets.truncate(unit.sent * TsPacket::SIZE); | ||
| unit.packets.extend_from_slice(&rest); | ||
| out.clocked = true; | ||
| out.nulls += 1; |
There was a problem hiding this comment.
At padded rates whose cumulative budget produces a one-packet slot, such as 90,000 b/s, the new position-zero path can emit a PCR-bearing media packet while out.nulls is initialized to zero; the unconditional out.nulls -= 1 then runs before this compensating increment, so debug or overflow-checked exports panic instead of handling or refusing the rate. Compensate without the transient decrement, or reject such a rate before scheduling.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, same underflow as the thread above. Fixed in 3b69dad without the transient decrement, with a 90 kb/s one-packet-slot regression.
(Written by Claude Opus 5.5)
| let joins = !place.is_multiple_of(fit) | ||
| && track.pack.as_ref().is_some_and(|pack| { | ||
| pack.rate == rate | ||
| && pack.payload.len() + payload.len() <= size | ||
| && decode.as_nanos().saturating_sub(pack.decode.as_nanos()) <= budget | ||
| && pack.end().abs_diff(unit.timestamp.as_micros()) <= 12 |
There was a problem hiding this comment.
When a continuous audio frame generates PAT/PMT or SI while an earlier pack is open, tables can be nonempty while joins remains true, after which flush prepends those tables to a PES that starts with older frames. For channel_config == 0 AAC, the PCE is inserted only into the current frame, so a late tuner receives PAT/PMT followed by earlier raw blocks before the required PCE, contrary to the tune-in contract exercised at export_test.rs:2027-2029; make a table-bearing frame start a new pack.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, reproduced: with continuous channel_config 0 AAC under a delay, the PES following the tables started with an older raw block, not the PCE. Fixed in 3b69dad: a frame bearing the tables starts a PES of its own. Regression: aac_program_config_leads_a_packed_pes. Tables follow the shared PSI grid, so exporters still cut the same PESs. The extra cuts did not measurably change --full's overhead (8,061 packets against the source's 8,222).
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
The OpenAI re-review of 3b69dad was right that cutting only the table-bearing frame's own PES missed the other AAC PIDs. 8223fb3 replaces that rule: when PSI fires, the exporter sends on the open PES of every AAC PID that carries a program config element, ahead of the tables. Each PID's PCE frame then starts a PES, whichever PID's frame the tables ride. MP2, AC-3 and AAC with a channel configuration have no element to repeat, so their PESs are no longer cut at the tables. The regression aac_program_config_leads_a_packed_pes now runs two offset AAC PIDs and fails on 3b69dad (a PCE mid-PES on the PID the tables did not ride).
Out of scope: on the wire, Slot::layout puts tables ahead of the first packet muxed after them. So with B-sized packs, the tables can still precede an older pack that shares their slot. That comes from the layout, not from packing, so I've left it for a follow-up.
(Written by Claude Opus 5.5)
- A packet carrying the slot's PCR takes the clock packet's position without first spending a null, so a one-packet slot no longer underflows `nulls` in overflow-checked builds. - A frame bearing the tables starts its own audio PES, so a receiver tuning in at a PAT/PMT reads AAC's program config element before any older raw block. - The module header says the PCR rides the clock PID's packet when it can, rather than always a packet of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3b69dad.
The one-packet PCR underflow is fixed by spending a null only for non-opening media, with a targeted 90 kb/s regression. The module contract is corrected too.
The existing AAC/PCE finding is only partially fixed: #5516 (comment). At export.rs:2062–2069, tables.is_empty() checks only the current frame's tables, although PSI marks every AAC PID for PCE repetition (1886–1891). With two AAC PIDs, A can generate PSI and leave it in self.tables while its new pack stays open. B's next PCE-bearing frame has an empty local tables vector and can join B's older raw frames. When B fills first, push_unit (2142–2147) prefixes A's pending PAT/PMT to B's PES, whose first frame lacks PCE. A late tuner encounters the same missing-config failure before reaching the later PCE. The new single-AAC regression cannot cover this.
Force the PCE-bearing frame to start a PES independently of which PID generated PSI, and ensure pending tables cannot be attached ahead of an older unconfigured audio pack. Add a two-PID regression with staggered sample rates/pack sizes (for example 48 kHz tiny frames and offset 44.1 kHz three-frame packs), checking the first audio block after PAT/PMT. Referencing the existing thread rather than duplicating an inline finding.
Direction: the clock accounting fix is focused; the packing fix needs cross-PID table/config coordination. Verification was static GitHub inspection of the update and a constructive multi-track control-flow trace. No tests, media replay or benchmarks executed.
A frame bearing the tables started its own PES, but the tables also mark every other AAC PID to repeat its program config element on its next frame, and that frame could still join an open PES of older raw blocks. When PSI fires, send on the open PES of every AAC PID that carries the element instead, so each PID's repeating frame starts a PES whichever PID's frame the tables ride. MP2, AC-3 and AAC with a channel configuration have no element to repeat, so their PESs are no longer cut at the tables. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 8223fb3.
The cross-PID pack-boundary case from the previous review is fixed: PSI flushes all PCE-carrying AAC packs before arming repetition, so another PID's repeated PCE cannot join its older raw blocks. The two-PID regression checks this boundary. The one-packet-slot underflow fix remains intact. No new independent actionable bug found in this update.
The stronger late-tune wire-order guarantee is still limited by the existing scheduler issue acknowledged in #5516 (comment). Enqueuing old packs before tables is not necessarily their wire order across PID lanes. The revised test verifies PCE position within each PES but does not verify each PID's first PES after PAT/PMT; referencing the existing thread rather than duplicating it. The unpadded ordering implementation is unchanged from base.
Direction: cutting only PCE-dependent AAC packs is a targeted fix and preserves packing for the other codecs. Verification: static inspection of the delta, scheduler and tests only; no tests, media replay or benchmarks executed.
|
Review findings addressed in 3b69dad and 8223fb3. Each has a regression test that fails without its fix.
Verified on 8223fb3: Follow-up, not in this PR: (Written by Claude Opus 5.5) |
Re-lands #5414, which closed unmerged when #5306's branch was deleted. It's rebased onto
main; thedifferences from #5414 are listed under Since #5414.
Problem
A padded
export tscould not carry a CBR contribution feed at the feed's own multiplex rate whenthe feed fills that rate with no null packets (#5362). The export spent packets the source does not:
packets at the lesser of Rx and Rbx, plus the clock packet, and MB held only a transport buffer's
worth ahead of the leak.
video's adaptation field.
At the source's rate the export fell behind until its lead ran out, and it stopped on missed a
decode deadline on the video PID.
Approach
One commit per step, each retiring its quest:
fix(ts): admit leak-bound video at its full Rbx(quest/m1/tstd/video-leak.md). TB drainswhole packets at Rx, the clock packet among them, and MB passes elementary bytes on into EB at
Rbx. An unpadded slot carries the lesser of the two, with fractions carried from slot to slot and
no one-packet slack.
fix(ts): let video's MB run BSmux ahead of its leak. MB may hold BSmux ahead of the leak(2.14.3.1), so audio taking a position between video packets no longer idles it.
feat(ts): carry the PCR in the clock PID and pack audio frames per PES(
quest/m1/tstd/mux-overhead.md).that PES's payload. A slot without one keeps its clock packet.
--delay, consecutive frames of every audio codec share a PES while they run on withouta gap and fit B with their header. The codecs are AAC, MP2, AC-3, E-AC-3, and AC-3 in a private
stream. Each frame leaves B at its own decode time.
holds, so exporters that joined at different frames cut the same PESs.
Since #5414
mainnow counts thewhole payload in B for every PID. The first commit keeps video's MB passing only elementary bytes
on into EB, since MB alone discards the PES header (2.14.3.1).
main's per-PID transport buffer (512 bytes, 480 for teletext) and itsemptying rule.
audio_between_video_packets_leaves_the_leak_runningmoves to PIDs 7, 8 and 9, becausemainmaps PIDs up to 3 onto the system transport buffer.
--leakand--fullCI steps are ints.yml. Both modes skipdecode.py, which covers onlythe one-video, one-AAC profile, as
--hrdand--headroomalready do.Tested with:
just test ts --leak: 1080p with no HRD at level 4.0's 24 Mb/s leak covers the clip (durationcheck passes), and the strict T-STD check and PCR accuracy pass. With
main'smoq-muxthe samerun stops about 9 s in on the video PID.
just test ts --full: the--leakvideo beside MP2 and DVB AC-3, muxed with its PCR in the videoPID and several audio frames per PES, with the nulls dropped, exports at its own 24.79 Mb/s. It
covers the whole 30 s, the strict T-STD check passes, and every PCR is within 500 ns.
overhead.pycounts 8,022 packets spent on clocks and audio framing, against the source's 8,147.
mainat0d9e9ff78theexport stops 27 s in on the video PID. With this PR it runs through at the source's own 25 Mb/s.
The strict T-STD check,
compliance.pyand PCR accuracy all pass (11,108 PCRs, none off), andevery frame of every track is the source's.
cargo test -p moq-mux: the TS unit tests pass.just fixchanges nothing in this PR.just checkon Ubuntu x86_64 passes every Rust suite;its shell module flags only
test/interop/interop.sh, which this PR does not touch, becauseUbuntu's shfmt 3.12.0 formats it differently from the pinned one. The shell, TOML, justfile and
workflow modules pass on macOS. The Nix modules were skipped (no Nix on either host).
Impact
Scheduleis private tomoq-mux.export tsoutput carries each slot's PCR in the PCR PID's adaptation fieldwhenever that PID has a packet in the slot.
--delay, an exported audio PES carries several frames, up to its B. With nodelay, it carries one frame, as before.
just test ts --leakand--full, their CI steps, andtest/ts/overhead.py.Alternatives
packet. Reflowing a PES the slot already carries costs at most one more packet, and only when its
last packet has no room left.
so the PES size follows B instead.
Follow-ups
--delay, plus a slot and its drain ahead of its decode.So at
--delay 1sor more, the first audio PES after startup can sit in B for longer than theT-STD's 1 s.
--fullruns at 900 ms for this reason. This belongs with the send-ahead quest,which has to leave packed audio time to fill its PES.
arrive. On the 25 Mb/s feed above, the source packs about 9.3 AC-3 frames per PES, while B caps
the export at seven. So the export spends 2.5 % more packets than the source on AC-3, and 1.0 %
more on MP2. It still fits, since it repeats PAT and PMT less often than the source.
Review fixes
Maintainer decision: fix the open review findings with regression tests, then merge. In 3b69dad and 8223fb3:
out.nulls -= 1underflowed and panicked in overflow-checked builds. Regression:the_pcr_rides_the_only_packet_of_a_one_packet_slot, which panicked before the fix.aac_program_config_leads_a_packed_pes, with two offset AAC PIDs. It failed both before any fix and on 3b69dad, whose rule (a table-bearing frame starts its own PES) covered only the PID the tables rode.export.rsmodule header no longer says the PCR always rides its own packet.Re-verified on 8223fb3:
just check,cargo test -p moq-mux,just test ts,--leakand--full(8,015 packets against the source's 8,220). On 3b69dad, the default round failed once onaudio-timingat load 55 (the known flake inquest/m1/test-flakes-2/ts-video-content.md, same 24,531,556-byte capture) and passed on rerun.Follow-up:
Slot::layoutputs the tables just ahead of the first packet muxed after them. So when B-sized packs queue, the tables can precede an older pack sharing their slot on the wire. A tuner then reads that pack's raw blocks before the element. This comes from the layout, not from this PR's mux order.(Written by Claude Opus 5.5)
Need help on this PR? Tag
@codesmith-botwith what you need.