fix(moq-mux): anchor the TS table cadence to the media timeline - #2825
fix(moq-mux): anchor the TS table cadence to the media timeline#2825kixelated wants to merge 1 commit into
Conversation
The MPEG-TS exporter decided when to re-emit a table by measuring forward from its own last emission, so the cadence carried the phase of whenever that process started. Two exporters of one broadcast that did not start together then put PAT/PMT (and SDT/NIT) on different frames, forever, which breaks a redundant pair that compares its two legs packet by packet. `due` now asks whether the timestamp crossed into a later floor(timestamp / interval) slot, which is a property of the broadcast alone. A program with video hid this for PAT/PMT, since the tables are also re-emitted at every video keyframe and that re-anchors both legs to a boundary they share; an audio-only program has no such boundary, and the standalone SI PIDs are never re-anchored in any program. The continuity counter is still numbered from process state. Making it a function of the broadcast forces each group's per-PID packet count to be a multiple of 16, which is a much larger change; the new tests assert that it is the *only* remaining difference across a late join. Refs #2779 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f821c6b245
ℹ️ 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".
| Duration::from(timestamp) | ||
| .checked_sub(Duration::from(last)) | ||
| .is_some_and(|elapsed| elapsed >= interval) | ||
| slot(timestamp, interval) != slot(last, interval) |
There was a problem hiding this comment.
Ignore older slots unless the whole timeline rewinds
When PTS moves backward across a slot boundary, as can happen with B-frames in decode order or when one live track temporarily lags another, != treats the older slot as newly due and stores that older timestamp. The next forward frame then triggers another emission, so PAT/PMT and every SI PID can repeatedly bounce between slots; different inter-track arrival timing also makes redundant exporters produce different packet layouts. Compare against a monotonic high-water slot, or distinguish an actual stream rewind from ordinary timestamp reordering. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| /// | ||
| /// A zero interval degenerates to one slot per microsecond, which makes every frame due. | ||
| fn slot(timestamp: Timestamp, interval: Duration) -> u128 { | ||
| Duration::from(timestamp).as_micros() / interval.as_micros().max(1) |
There was a problem hiding this comment.
Preserve zero-interval emission for every frame
When a custom SI entry specifies Duration::ZERO, mapping the interval to one microsecond does not make every frame due as documented: two tracks can legitimately emit frames at the same timestamp, and the second call receives the same slot and returns false. The previous elapsed-time implementation treated every non-backward frame as due for a zero interval, so this silently skips requested SI repetitions; special-case zero before calculating slots. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
|
Reviewed and tested. The diagnosis is right and the fix does what it says — on a source that Setup: one publisher feeding a real DVB contribution stream (CNN International EMEA HD, 9.95 Mbps 1. It worksByte-comparing the two legs doesn't measure this (§4), so the measurement is where each table lands
The standalone SI PIDs go from never agreeing to almost always. That is exactly the mechanism you 2. B-frames re-trigger the cadence, ~25× on this clip
So every oscillation that straddles a slot boundary re-fires the tables: cross into the new slot, Share of the stream spent on PAT/PMT/SDT/NIT, same clip, same window:
Not a conformance failure — TR 101 290 P1 bounds the maximum PSI interval, not the minimum — but it The tests can't catch this because 3.
|
| PAT/PMT emissions (A) | PAT/PMT frames in disagreement | SDT emissions (A) | SDT frames in disagreement | |
|---|---|---|---|---|
6d3c51d7 |
87 | 4 | 21 | 43 (nothing shared) |
f821c6b2 |
1,487 | 55 | 801 | 60 |
with > |
113 | 2 | 23 | 3 |
Strictly better on both axes: a twentieth of the emissions and fewer frames where the legs
disagree. SI/PSI share comes back to 0.089 %.
Exactly one test fails, and it is the intended-rewind assertion:
due_crosses_an_absolute_slot's assert!(due(ms(750), Some(ms(1_000)), PSI_INTERVAL)). Everything
else passes, including both late_join_matches_a_running_exporter cases. So the trade is explicit
and singular: rewind handling costs 25× PSI on any source with B-frames.
If the rewind property is worth keeping, the two are separable by magnitude — reorder depth is
bounded by the codec's reorder buffer and measured at 180 ms max here, where a rewind is a
discontinuity. A threshold well above the reorder depth, or a high-water mark with an explicit
rewind escape, keeps both.
4. The interleave is not a residue on real content, it is the wall
A note on why the numbers above are table anchor points and not byte comparisons. Comparing the two
legs packet by packet, the longest run they share is 32 packets, even with the continuity counter
masked — and the PID census over the aligned span shows why: the legs render different numbers of
video and audio packets over the same media interval.
That is the fourth item from your #2779 comment, poll_next emitting whichever track has a frame in
hand. On a single-track synthetic fixture it is invisible; on real multi-track content over real
sockets it is not a residue sitting behind the continuity counter, it is what stops a byte
comparison working at all. Our own 1+1 measurements only reached 97 % masked-identical because a
downstream CBR groomer re-places every packet onto a stream-clocked grid, which absorbs the
interleave before anything compares the legs.
Two consequences worth having on the record:
- The residual 2 PAT / 3 SDT disagreements under the
>variant are most likely this: near a slot
boundary the two legs disagree about which frame crosses first. - The opt-in deterministic mode you sketched needs the interleave and the counter. With the
interleave non-deterministic, a byte-identical pair is unreachable no matter what the counter does.
Verdict
Ship it, with > in place of != (or a rewind threshold above the reorder depth). It closes a real
divergence that we can now measure on real content, and the cadence half is a precondition for the
counter work whatever form that takes.
Happy to contribute the two scripts if useful — the anchor-point measurement is a few dozen lines
and is the only way we found to test this property without a byte-identical pair.
(written by Opus 5)
|
Follow-up with the downstream measurement I owed you, since it turns out this PR closes a gap The rig is a 1+1 redundancy pair: one publisher, one relay, two On a single-track source (H.264, no B-frames, no audio, 2 Mb/s CBR):
So the cadence really was the whole of the remaining difference there, and this PR removes it. One caveat worth recording: on a source whose keyframe interval divides the PSI interval exactly On multi-track content with B-frames the picture is the one from my earlier comment:
The gap between the middle and last rows is the PSI inflation seen through a receiver: over the The residual 4.4 % is not yours and not a late-join effect — with both subscribers started at the |
|
Correction to the caveat in my last comment: divisibility is not the explanation, and the defect I went back to a capture from a different host — the same 1+1 rig, a video-only 2 Mb/s source with That capture also shows the mechanism unusually clearly, because it is old enough to predate 31 of 31: every single SDT emission lands on a slot where the other leg has video. PAT and PMT |
Two exporters of one broadcast should render the same packets. Today the MPEG-TS exporter decides when to re-emit a table by measuring forward from its own last emission, so the cadence is anchored to whenever that process started rather than to the media timeline. Start a second exporter of the same broadcast 20 seconds later and the two put PAT/PMT (and SDT/NIT) on different frames, forever.
That breaks the case in #2779: a SMPTE ST 2022-7 pair needs its two legs packet-identical so a receiver can merge them, and a leg restarted for maintenance has to come back rendering exactly what its partner is rendering.
Root cause
due()asked "hasintervalelapsed since the last emission", which carries the phase of whenever the exporter first ran. It now asks "has the timestamp crossed into a laterfloor(timestamp / interval)slot", which is a property of the broadcast alone. Two exporters that last emitted at different points inside the same slot agree on every frame after that.A program with video hid this for PAT/PMT, because the tables are also re-emitted at every video keyframe and that re-anchors both legs to a boundary they share. The two cases where it does not:
A slot boundary also re-emits after a backwards timestamp jump (a rewound timeline), which the elapsed-time version sat out until the old interval came back around.
What this does not fix
The continuity counter, which is the headline of #2779, is still numbered from process state. Making it a function of the broadcast requires each group's per-PID packet count to be a multiple of 16 (a counter that is both globally continuous and independent of the join point forces that), which means padding, which is a much larger and opt-in-shaped change. The details are in #2779; this is the part that is unambiguous and free.
Tests
late_join_matches_a_running_exporter{,_without_video}produce one broadcast, export it twice with the second exporter joining halfway in, and assert the only bytes that disagree in the overlap are continuity counters. The audio-only case fails without this change (one leg renders 752 bytes where the other renders 376: two extra packets, PAT and PMT). The video case passes either way and is there to catch the next field someone mints per process; it is the topology in the report.author_dts_is_join_independent_at_a_peakpins the one piece of carried state that is not a divergence: the monotonic decode-clock bump cannot fire on a frame whose PTS leads everything decoded before it, which every keyframe is, and export only ever tunes in on a keyframe.Cross-package sync
None. No wire format, no public API, no CLI surface, and the MPEG-TS exporter has no JS counterpart.
Fixes part of #2779.
🤖 Generated with Claude Code
(written by Opus 5)