fix(mux): refuse damaged TS units without ending ingest (backport #4733)

[kixelated]

Problem

On release, one malformed media packet, PES header, or codec access unit ends MPEG-TS ingest and the SRT session (#4581). PesStart::parse(payload)? and codec errors from the H.264/H.265 splitter propagate out of Import::decode, so a remote sender can kill ingest with one bad packet (unexpected PES header marker bits, h264: forbidden zero bit is not zero, Opus control header truncated).

Ported from #4733 (d8b9b0031)

• Import::damage: refuse the damaged unit on its PID, log a warning (dropped a damaged TS unit), keep ingesting every other PID.
• Routed through it: TEI packets, malformed adaptation fields on media or PCR PIDs (checked before the PCR flag is read, so a malformed field cannot declare a timebase break), malformed PES headers (the PES the start ends is still flushed), and codec errors confined to one access unit (unit_error / private Damaged).
• On any desync (damage or continuity gap), H.264/H.265 cut(None) so the group closes at the break; pictures are refused until the next IDR. A PCR timebase break still salvages and desyncs without the cut.
• Split::reset (H.264/H.265) also drops frames completed but not yet returned, so a refused PES never surfaces a picture on the next call.
• Opus validates every control header in a PES before publishing any packet.
• Also carries the fix for a gap review found in fix(mux): refuse damaged TS units without ending ingest #4733, which lands on main in fix(mux): keep the last good parameter sets when refusing a TS access unit #5153: a refused H.264/H.265 unit puts back the splitter's parameter sets, so a malformed inline SPS is not re-injected into later bare keyframes.
• Publishing, catalog, timestamp, and unsupported-configuration failures stay fatal.
• Docs: doc/bin/cli.md, doc/bin/srt.md.

Left out

• TR 101 290 Health counters (feat(mux): count the TR 101 290 errors of a TS feed at import #4750), not on release.
• StreamStats::damaged and the ts::stats::Log reporting. Damage is surfaced as a warning log instead, so this backport adds no public API.
• The adaptation check runs right after duplicate detection, since there is no Health::packet call to order it after.

Public API and wire impact

• No public API changes. Damaged, unit_error, damage, salvage, lost, adaptation_valid, and opus_packets are private.
• Behavior: a continuity-counter gap or TEI on H.264/H.265 now also closes the group and drops pictures until the next keyframe, matching main. Each break freezes video for up to one GOP.
• No catalog, container, or MoQ wire changes. No version bumps.

Test evidence

Ported the #4733 regression tests, minus the damaged counter assertions (they check published frames and units instead). Before the fix, 12 of the 14 new tests fail on release, e.g. damage must stay local to the unit: unexpected PES header marker bits, h264: forbidden zero bit is not zero, a malformed field reset the program clock, and the group-at-the-break test ending its first group at 2200 ms instead of 1080 ms. After the fix:

• cargo nextest run -p moq-mux: 985 passed, including damaged_{h264,h265}_sps_keeps_the_last_good_parameter_sets, which fail without the restore.
• just check on the current head (918cb13): all checks passed. On the first head, two moq-tokio::broadcast rejoin tests timed out on the wall clock under load and passed when rerun alone.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Grok review of head a254ac12 (backport of #4733 to release)

This is a faithful, well-scoped port. Damage stays on its own PID, publishing and clock errors stay fatal, and there are regression tests for each path. I found nothing blocking.

Non-blocking

1. Warning log volume (import.rs, Import::damage). There's one tracing::warn! per damaged unit, with no rate limit. A sender streaming garbage, or a noisy SRT link, can now produce a warning per packet for as long as the session lasts, where before the session simply ended. Since release doesn't have the StreamStats::damaged counter, consider rate-limiting the warning per PID, or logging the first one and then a periodic count.
2. The unit_error allowlist is narrow for AAC (import.rs, unit_error). Only the three ProgramConfig* variants are confined to one unit, so any other AAC parse error (and every Annexb error, which is confined wholesale) follows the old fatal path. That matches the PR description, but it's worth confirming the AAC ADTS header errors on release are already handled as sync loss and don't reach this point as fatal errors.
3. CI hasn't reported yet. Check, Test, Windows, macOS, and Android are all pending, and the PR mentions two moq-tokio rejoin tests timing out under load. That flake is unrelated to this change, but watch for it on this run.

Verdict: MERGE once CI is green

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: a254ac1

P2: Preserve the last valid parameter sets when refusing an access unit. The new recoverable-error path (import.rs:761–764) resets the splitter, but H.264 reset:233–237 and H.265 reset:237–241 retain parameter sets already replaced before importer validation. For example: publish a good SPS/PPS + IDR, then an IDR carrying a truncated SPS NAL (67), then clean bare IDRs. reconcile_params() caches the truncated SPS before configure_from_sps() rejects it as SpsTooShort; recovery preserves it, so every subsequent bare IDR reinjects the same invalid SPS and is dropped. Video stays frozen until valid parameter sets are repeated, potentially indefinitely. Commit parameter-cache changes only after successful validation, or restore the last validated cache on damage. Add H.264/H.265 regressions with a bare recovery keyframe; the current recovery helper supplies fresh valid parameter sets every GOP.

Direction: PID-local recovery and reuse of cut(None) keep this backport focused without another waiting-state mechanism. The helpers stay private and the wire format is unchanged.

Verification: GitHub-only static inspection of the diff, surrounding importers/splitters, and regression tests. I did not run builds, tests, or live damage replays. Android CI passed; Check/Test and platform checks were still running.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a254ac120b

ℹ️ 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]

Roll back rejected video parameter sets

When an H.264 keyframe contains a malformed inline SPS, the splitter commits that SPS to its retained cache before Import::decode returns SpsParse; classifying the error as recoverable then calls reset(), which deliberately preserves the poisoned cache. If later keyframes omit inline parameter sets, the splitter reinjects the malformed SPS and every keyframe is refused, so one damaged unit can freeze the PID indefinitely instead of recovering at the next keyframe. H.265 has the same failure mode. Preserve the last accepted parameter-set cache, or roll back cache changes when the access unit is rejected.

Useful? React with 👍 / 👎.

[kixelated]

Confirmed and fixed in 918cb13. The splitter adopted the refused keyframe's SPS before Import::decode rejected it, and reset() kept that cache, so every later bare keyframe got the bad SPS re-injected. The H.264/H.265 TS path now takes a snapshot of the retained VPS/SPS/PPS before each PES (Split::params, crate-private) and puts it back (Split::restore) when the unit is refused as Damaged. Regression tests damaged_h264_sps_keeps_the_last_good_parameter_sets and damaged_h265_sps_keeps_the_last_good_parameter_sets failed before the fix (2 pictures published instead of 4). main has the same bug from #4733; the fix is landing there first in #5153.

(Written by Claude Opus 5.5)

[chatgpt-codex-connector]

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-09T22:18:59.521683Z	a254ac1	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]

Automated review of 918cb137 (backport of #4733 onto release)

The port looks faithful: damage is confined to the PID through Import::damage, publishing, catalog, and clock errors still propagate, and the parameter-set params/restore snapshot (918cb13) stops a refused AU's SPS/PPS from being re-injected on a later bare keyframe. I didn't find any blocking issues.

Non-blocking:

• import.rs H.264/H.265 write closure: skip_missing_keyframe now routes producer errors through unit_error, so an error on frame N of a multi-frame PES (for example NotInitialized) is treated as Damaged after earlier frames from the same PES have already been published, and then lost() cuts the group. That's harmless in practice (one AU per PES), but it means "refused whole" in the docs isn't strictly true for a PES carrying several AUs.
• adaptation_valid checks that the optional fields fit, but it doesn't check that an adaptation-only packet (adaptation_field_control == 0b10) declares length 183. That matches main, so it's just something to note if you're tightening this later.
• damage() logs at warn once per damaged packet. A sustained TEI burst on a bad link could flood the logs; it's worth rate-limiting if main doesn't already.
• CI is still pending (Check/Test/Windows/macOS/Android).

Verdict: MERGE once CI is green.

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

[kixelated]

Re the review's P2 (preserve the last valid parameter sets when refusing an access unit): this is the same bug as the P1 inline thread, and it is fixed by the same commit, 918cb13. A refused H.264/H.265 unit now puts back the splitter's retained parameter sets as they were before that PES. The new regressions use a bare recovery keyframe, as suggested, for both codecs. They fail without the fix and pass with it. cargo nextest run -p moq-mux and just check pass on this branch.

main has the same bug (#4733), so the fix is going there first: #5153.

(Written by Claude Opus 5.5)

[kixelated]

Pushed a96df0b (comment and doc only):

• Mirrored the comment fix(mux): keep the last good parameter sets when refusing a TS access unit #5153 added on main (c490360): the parameter-set snapshot is per PES, so with several AUs in one PES a later damaged AU also rolls back parameter sets an earlier, already published AU brought.
• Re the review's note on "refused whole" in doc/bin/cli.md: agreed. With several AUs in one PES, frames before the failing one may already be published. The line now says the unit is "dropped and logged as a warning", which matches main's wording.
• warn per damaged unit and the adaptation-only length-183 check: both match main, so they are left for a follow-up rather than diverging release.

The branch merges cleanly onto the current release (c8a2ba4). The H.265 SPS refactor that landed there keeps the Error variants unit_error matches. CI on a96df0b is green.

The only change since the last non-Grok review here (a254ac1) is 918cb13. It is the same change as #5153's 5f15d18, adapted only to release's reanchor line; range-diff shows no other difference. On main it got an OpenAI review with no findings and a Codex thumbs-up. Holding the merge until the maintainer decides whether that covers this head.

(Written by Claude Opus 5.5)

[kixelated]

Merge decision for a96df0bc: the maintainer counted #5153's OpenAI review and Codex thumbs-up as covering 918cb13, since git range-diff shows the same change apart from release's existing reanchor.apply(...)? line moving inside the closure (not Damaged, so it stays fatal). The later commit only mirrors #5153's comment and fixes the doc/bin/cli.md wording. Auto-merge (merge commit) is pinned to a96df0bc46e8d96d79619c78aed4f16fc7fea47d.

Follow-ups to plan on main first: rate-limit the per-unit warn in Import::damage, and require length 183 for adaptation-only packets in adaptation_valid.

(Written by Claude Opus 5.5)