feat(moq-mux): count and log MPEG-TS audio resyncs - #3372
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The recovery path zeroed its own evidence: `Resync::recovered` reset the discard count and neither call site logged, so a feed losing audio to a damaged frame header looked identical to a healthy one. `Resync` now keeps cumulative counters and warns once per completed resync with the PID, track suffix, and bytes discarded. `ts::Import::stats()` exposes them per elementary stream PID, and `moq import ts` logs the streams whose counters moved. Closes #2798 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. WalkthroughThe MPEG-TS importer now exposes per-PID audio resync statistics for AAC and legacy audio streams. It records resyncs, discarded bytes, and unconfirmed publications, including in-progress scans and EOF-drained frames. The CLI compares statistics during decoding and logs changed counters and final drain results. Tests cover damaged headers, loop wraps, active scans, and unconfirmed EOF publications. The completed quest entry was removed. Merge Risk: ⚪ Minimal · up to MPEG-TS publishing now reports audio resync, discarded-byte, and unconfirmed-frame counters without changing recovery or downstream discontinuity behavior. The statistics flow and covered recovery cases indicate no current merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
✨ Simplify code
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 |
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. |
![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: 4f344b3dc1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5099689a8
ℹ️ 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".
1 participant
Summary
recovers from a damaged MP2/AC-3/E-AC-3/AAC frame header by scanning to the next confirmed
sync instead of ending the session, but
Resync::recovereddid nothing except resetdiscardedto 0. Neither call site (AacStream,LegacyStream) logged, andts::Importhad no stats surface at all, so a feed dropping audio looked exactly like a healthy one:
the timeline just stepped 24 ms to 48 ms. Only the budget-exhausted give-up surfaced, as an
error string.
Resyncnow carries cumulative counters andtracing::warn!s once per completed resyncwith the PID, the track suffix, the bytes discarded, and the running resync count.
ts::Import::stats()returns a per-PID snapshot.moq <side> import tslogs the streamswhose counters moved, per input chunk plus once more after the end-of-input drain, so a live
feed reports as it degrades rather than only at exit.
vouched for (a joined tail drained at end of stream, where no successor can ever confirm it)
is a substitution rather than a gap, so it is counted separately as
unconfirmed.container::Producer::discontinuity()is still never called bythe TS importer. A marker group per lost 24 ms frame changes downstream behaviour and is a
separate decision.
Public API changes
Additive only, so this targets
main. Nothing renamed, removed, or resignatured.moq_mux::container::ts::Import::stats(&self) -> ts::Statsmoq_mux::container::ts::Stats-#[non_exhaustive],pub streams: BTreeMap<u16, StreamStats>, plusis_empty()moq_mux::container::ts::StreamStats-#[non_exhaustive],pub track: &'static str,pub resyncs: u64,pub discarded: u64,pub unconfirmed: u64How it is shaped and why:
stats()reads counters the demuxer already keeps and clonesthem. No registry, no
Arc, no interior mutability, and no callback: the caller polls atwhatever cadence it wants and owns the reporting policy, which is what the "avoid callback
parameters" rule asks for.
BTreeMap<u16, ..>return. The map alone is the smallersurface today, but the TS layer also loses packet sync (
Import::decodereacquires the188-byte stride), and that is an importer-wide counter with no PID to hang it on. Wrapping
the map in a
#[non_exhaustive]Statsleaves room to add it without a breaking change;returning the map directly would not.
#[non_exhaustive]on both is case 3 inrs/CLAUDE.md: these grow additive, defaultablecounters. Both derive
Defaultso an external caller still has a construction path, andPartialEqis what makes "log it when it changes" a one-liner.is_empty()isthe whole-feed health check and change detection stays cheap.
Statsis the snapshot,StreamStatsis one elementary stream's share.Every field is a plain cumulative count, because the signal worth alarming on is the
derivative, not the value.
Opus:
Statsreports nothing for Opus PIDs, because onmaintoday Opus does not gothrough
Resyncat all (its framing is length-declared, not self-describing). The siblingquest #2849 is routing Opus through
Resync; when it lands, the one-line change is to moveStream::Opusfrom theNonearm ofStream::statsinto thestream.resync.stats()armbeside AAC and Legacy. Nothing else in this surface needs to change.
Test plan
New assertions on the existing fixtures in
rs/moq-mux/src/container/ts/import.rs:legacy_resyncs_past_damaged_header(the one-byte-damage fixture from fix(moq-mux): resync TS audio instead of aborting the broadcast #2751): exactly oneresync on the MP2 PID, charged the damaged frame's 72 bytes.
aac_resyncs_past_damaged_header: the same for ADTS, charged 47 bytes.legacy_drains_a_joined_frame_at_end_of_stream:unconfirmed: 1, no resync.legacy_survives_a_looping_file_wrap(the realac3.tsfixture, looped): the snapshot isempty. Worth calling out, because the issue expected this fixture to show an unconfirmed
frame and it does not: the continuity counter catches that wrap a packet before the codec
would ever see spliced bytes, so the tail is dropped rather than scanned past. The
unconfirmed counter is exercised by the drain fixture above, which is the only path that
publishes a frame nothing vouched for. The assertion is kept as a regression guard on that
reasoning.
What was actually run, and what was not. This machine is running ~14 concurrent agent
builds and the shared
target/was garbage-collected mid-session, so a fulljust check/just testdid not finish in the time available. Scoped runs instead:cargo test -p moq-mux --lib -- container::ts::import- 63 passed, 0 failed.cargo check -p moq-cli- clean.rustfmt --config-path rs/.rustfmt.tomlover both changed files - clean.cargo clippy -p moq-mux --all-targets -- -D warningswas started but had not finished.CI is therefore the first full
just check/just testrun on this branch. Nothing here isplatform-gated or feature-gated, so there is no manual
just rs macos/windowsgap.Cross-package sync
rs/moq-cli->doc/bin/cli.md: skipped. No flag, argument, subcommand, or positionalchanged; the CLI only gained a log line, and that row covers the invocation surface.
moq-ffisurface changed, so nodrafts/,js/, orlanguage-binding rows apply.
Quest
Completes and deletes
quest/m0/2798-moq-import-ts-an-audio-resync-is-silent-no-log-no-counter.mdand its
quest/m0/README.mdentry.Closes #2798
🤖 Generated with Claude Code
(Written by Claude Opus 5)