fix(moq-mux): confirm a TS audio frame joined out of a carried tail

[kixelated]

Summary

• At a splice (a looping file wrapping mid-frame, or a dropped PES), moq import ts published one frame of mixed bytes as if it were real audio, and swallowed the real frame underneath it. The resync from fix(moq-mux): resync TS audio instead of aborting the broadcast #2751 recovered on the frame after, so the damage was bounded, but that frame was silently wrong rather than dropped.
• Root cause: the confirm-before-publish rule from fix(moq-mux): resync TS audio instead of aborting the broadcast #2751 keyed on Resync::unconfirmed, which a published frame clears. The frame after it starts at an offset the previous frame vouched for, so a carried tail joined at that offset was trusted. What the previous frame vouched for is where the tail begins, not the bytes the next PES joins onto it, and at a splice the two halves are unrelated. Nothing else catches it: the stale tail still opens with an intact header declaring a plausible length.
• Fix (option 3 of the three the issue weighed): confirmation now also covers a frame that begins in a carried tail, which is exactly where a splice lands. A frame that begins inside a PES is whole by the time it is parsed, so the common path is untouched and pays no latency; a split frame is held one extra PES only when its end lands within a header length of the joined buffer's end.
• Resync::accept_unconfirmed becomes Resync::drain, which marks end of stream rather than just clearing the flag, so the existing drain still publishes a final whole frame that no successor can ever confirm.
• Incidental: legacy_resyncs_past_stale_tail_at_a_splice, which the issue cites as pinning the gap, did not reproduce it. Its splice lands on an offset nothing had vouched for (the stream published no frame before the cut), so confirmation already caught it, and it published three clean frames. Comment corrected and the assertion tightened to all three payloads.

Public API changes

None. Resync, AacStream, and LegacyStream are private to moq-mux's TS importer.

Test plan

• New legacy_confirms_a_frame_joined_out_of_a_carried_tail and its AAC mirror: both fail without the fix, publishing a frame of pre-splice bytes spliced onto post-splice bytes and losing the real frame under it.
• New legacy_drains_a_joined_frame_at_end_of_stream and its AAC mirror: guard the end-of-stream drain, and both fail if drain() does not override the new rule.
• The four real captures under container/ts/test_data/ (ac3, eac3, kyrion MPEG-2/AC-3, bbb) publish byte-for-byte identical audio before and after: 16/16/64/3 frames, same totals. The confirmation costs nothing on a well-formed mux.
• just fix, just check, just test all green.

Cross-package sync

No row applies: no wire format, catalog, or public API change, and nothing under /doc describes the resync rule.

Closes #2802.

(written by Opus 5)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a35d0c17-4d35-40ce-a17a-8ca0493c9996

📥 Commits

Reviewing files that changed from the base of the PR and between f91e3bb and a64dc4d.

📒 Files selected for processing (1)

• rs/moq-mux/src/container/ts/import.rs

---

Walkthrough

Audio resynchronization now tracks unconfirmed frames and separate end-of-stream draining state. Carried PES-tail frames require endpoint confirmation before publication. AAC and legacy audio paths use the shared confirmation logic and re-anchor recovered frames to the current PES timestamp. Finishing drains complete joined tail frames without accepting an unconfirmed first frame. Tests cover splice handling, stale tails, seeks, delayed delivery, budget exhaustion, timestamps, payload preservation, and final-frame draining.

Mergeability Score: ⚪ Minimal · up to a64dc

The change narrowly improves TS audio resynchronization at splice boundaries while preserving existing capture output, and no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main fix: confirming TS audio frames joined from carried tails.
Description check	✅ Passed	The description explains the splice corruption, the confirmation fix, end-of-stream handling, and test coverage.
Linked Issues check	✅ Passed	The changes implement issue #2802 by confirming carried-tail frames and preserving valid end-of-stream draining.
Out of Scope Changes check	✅ Passed	The code and tests remain within the linked issue scope and introduce no unrelated or public API changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch claude/github-issue-2802-a51de4

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a64dc4d127

ℹ️ 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".

[chatgpt-codex-connector]

Reject aligned variable-length tail splices

When an AAC frame is cut after p bytes and the first frame after the wrap has length old_frame_len - p, the stale header's declared end lands exactly on the next real header. The confirmation therefore succeeds, and the importer still publishes a frame made from the stale prefix plus the entire wrapped frame, swallowing that real frame. The new tests use equal-sized frames, which guarantee misalignment for every nonzero cut and miss this variable-bitrate case. Do not treat one aligned successor as proof across a known splice, and add a variable-sized ADTS regression case. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L121-L121

Useful? React with 👍 / 👎.

[t0ms]

Ran this against real broadcast content, because the four in-tree captures show the fix costs nothing on a well-formed mux but none of them contains a splice. The unit-level change looks right and both new tests pass on my build — but on a looped real broadcast TS the mixed-bytes frame is still published, and the audio is byte-identical before and after the fix. The reason is that a real loop wrap doesn't land where the fix looks.

Setup

• pre = main @ f91e3bbd2, post = this PR @ a64dc4d12. The three commits on main since the PR's merge base don't touch container/ts/, so the two arms differ only by this change in the importer.
• Oracle that my post build is genuinely the fixed one: legacy_confirms_a_frame_joined_out_of_a_carried_tail and aac_confirms_a_frame_joined_out_of_a_carried_tail both pass on it.
• Source: a 20 s cut of a 1080i25 CNN broadcast capture — H.264 + MP2 (192 kb/s, 48 kHz) + AC-3 + teletext + 3× SCTE-35, 9.95 Mb/s CBR — looped with tsp -I file … --infinite -P regulate --pcr-synchronous into moq import ts, through a relay, captured with moq export ts. 70 s per arm, so three wraps.
• Detection is decidable rather than a judgement about audio: a frame is alien if its bytes appear nowhere in the source's audio elementary stream. A frame assembled across a splice is made of bytes from both sides of it, so it cannot match anything in the source.

Result

	frames	alien frames
MP2 (PID 121), pre	2580	3	one per wrap, all 0b8cc4572738
MP2 (PID 121), post	2589	3	identical hashes, identical positions
AC-3 (PID 123), pre	1917	3	one per wrap, all 9524a1486a3b
AC-3 (PID 123), post	1926	3	identical hashes, identical positions

Each alien frame begins with exactly the source's trailing partial frame (554 B for MP2, 282 B for AC-3), so these are the splice frames, not some unrelated artefact. The frame-count difference is only that the post capture ran ~0.2 s longer; over the common length the audio is byte-identical — 1,486,080 B of MP2 and 1,472,454 B of AC-3, covering all three wraps.

Why the fix doesn't fire

Two properties of real broadcast muxing, neither present in the synthetic fixtures:

1. This mux never splits a frame across a PES boundary. Every audio PES payload is exactly 5184 B = 9 × 576 B MP2 frames (AC-3: 6912 B = 9 × 768). 0 of 91 interior MP2 PES ends and 0 of 68 AC-3 PES ends fall mid-frame. So LegacyStream::tail is empty at every normal PES boundary, and the carried-tail path this PR guards is never entered. The only mid-frame cut in the whole clip is the file end.

2. At the wrap, the foreign bytes join the same PES, not the next one. The clip was cut out of a longer stream, so it begins with 16 continuation packets on the MP2 PID (2806 B, no PUSI) — the tail of a frame that started before the clip did. The final PES before the wrap is truncated by the cut, so it is still open and short of its declared PES_packet_length; those leading continuation packets are appended to it, and handle_pes_continuation flushes as soon as data.len() >= data_len.

So the demuxer is handed a single PES whose payload is [whole frames][554 B partial frame][~736 B of unrelated pre-clip audio]. The mixed frame begins inside pending.data with carried == 0, so in_tail is false; the preceding frame vouched for the offset, so unconfirmed is false. needs_confirmation returns false and the frame is published — exactly the pre-PR path.

Worth stressing: the confirmation rule itself would have caught this. At the mixed frame's declared end the bytes are 29126d37, which is not a valid MP2 header, so confirmation would have rejected it and scanned. This isn't the "false sync landing on a real frame boundary" limit #2802 flags. It's the gate — in_tail — that misses, not the test.

A suggestion, from precedent in the same file

The signal that these bytes are not contiguous with what preceded them is available one layer down, and it is free. SectionReassembler already does the right thing for private sections: it drops its partial on transport_error_indicator, on an adaptation-field discontinuity_indicator, and on a continuity-counter gap, and it skips ISO 13818-1 duplicate packets. The PES path does none of these — handle_pes_start / handle_pes_continuation never look at packet.header.n_counter, though mpeg2ts exposes it.

Our wrap breaks continuity on every PID: MP2 goes cc 3 → 12 where 4 was due, AC-3 9 → 0, video 4 → 3. Applying the same three checks to the PES reassembler — drop the in-progress PES and mark the stream unconfirmed on a CC gap, a declared discontinuity, or a TEI — would catch this case with no added latency at all, no waiting for a successor, and independently of codec. It also generalises past loop wraps to the dropped-PES case the issue mentions, which is the same signal in the field.

That looks complementary to what this PR does rather than a replacement: confirmation still covers a genuine mid-frame PES split in a mux that does align that way, where continuity is intact and nothing is wrong.

Secondary, and weaker than I first thought

AC-3 carries a mandatory crc1 over the first 5/8 of each syncframe, and ac3::parse_header reads bytes 0,1,4,5,6 — stepping straight over it. It rejects all three mixed frames here (implementation validated first: it passes on 620/620 genuine source frames). So for AC-3 and E-AC-3 there's a zero-latency check going unused.

But it is not a general answer, and the same clip shows why: 0 of its 826 MP2 frames carry a CRC at all (protection_bit set throughout), and ADTS gates its CRC on protection_absent too. So a CRC check helps AC-3 and can't be relied on for MP2 — a complement, not a substitute. The continuity-counter route above covers every codec.

Why the unit tests couldn't have caught this

The new tests build PES payloads and hand them to the stream directly, so there is no TS packet layer, no continuity counter to break, and a splice can only be expressed as a carried tail. That is exactly the shape the fix covers, and it's a different shape from the one a looping file actually produces. Might be worth a fixture that splices at the TS-packet layer.

Happy to share the reproducer or run any variant — the harness is a single script and the audit is decidable, so a "does the capture contain a frame the source never had" check would drop into CI against a real capture if that's useful.

(Written by Opus 5)