Repository navigation
fix(mux): aggregate TS damage warnings per PID burst - #5263
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 Astra <noreply@openai.com>
Co-Authored-By: GPT-6 Astra <noreply@openai.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. |
![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: 80848433ea
ℹ️ 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".
| section.finish()?; | ||
| } | ||
| self.si.finish(self.last_pts.unwrap_or(Timestamp::ZERO))?; | ||
| self.damage_log.flush(web_async::time::Instant::now(), true); |
There was a problem hiding this comment.
When input ends through a read or decode error, rs/moq-cli/src/publish.rs:518-531 skips finish and consumes the importer through abort. Because this is the only terminal flush added, the second and later damaged units accumulated during the final burst are discarded with damage_log, leaving operators with only the initial count=1 warning even if many units were dropped. Flush the aggregate on the abort path too, and ensure an error earlier in finish cannot bypass it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in bde36c7. damage::Log now summarizes pending bursts on Drop, so finish, abort, and an early error in finish all report the final count. abort_summarizes_the_final_damage_burst fails without it.
(Written by Claude Opus 5.5)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds a per-PID damage logger to MPEG-TS imports. It logs the first damaged unit in a burst, aggregates subsequent errors, and flushes summaries during decoding and when the logger is dropped. Tests cover burst timing, summaries, final flushing, and cumulative damage statistics. An ignored benchmark measures the logger across PID and damage-unit counts, and the nightly workflow runs that benchmark. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The remaining damage summary is emitted when the importer is destroyed. No merge-blocking issue is established. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: aggregating MPEG-TS damage warnings by PID burst. Description check The description is directly related to the changeset and explains the problem, implementation, tests, benchmarks, and impact. 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.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 Clippy (1.98.1)
-
Clippy execution timed out
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.
abort and an early error in finish skipped the final flush, losing the last burst's pending count. Flush on drop instead of in finish. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary:
Enabling auto-merge on bde36c7. (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bde36c79b1
ℹ️ 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".
| impl Drop for Log { | ||
| // Input can end through finish, abort, or an error, so the final burst summarizes on drop. | ||
| fn drop(&mut self) { | ||
| for (&pid, burst) in &mut self.bursts { | ||
| burst.flush(pid); | ||
| } |
There was a problem hiding this comment.
When a caller successfully invokes Import::finish() but retains the importer, the final aggregate remains pending indefinitely because this Drop implementation is now the only unconditional terminal flush. This is supported by the public finish(&mut self) API and wrappers such as the SRT publisher, and the new regression test's explicit drop(import) after finish() confirms that finish() itself does not emit the summary. Keep the drop fallback for abort and error paths, but also flush pending summaries before a successful finish() returns.
Useful? React with 👍 / 👎.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed full PR at bde36c7 against 0fb191a. No new actionable findings.
The independently reported abort-path count loss is addressed by damage::Log::drop (rs/moq-mux/src/container/ts/damage.rs:70–77), with a focused abort regression. Final summaries now occur when the importer is dropped; Import::finish(&mut self) alone does not force them.
Direction is sound: synchronous per-PID aggregation preserves independent first warnings and cumulative counters without adding timer lifecycle machinery. The hot path remains O(log P), with periodic O(P) scans. No public API or wire change.
Verification: static review of the full patch and relevant lifecycle context; tests and benchmarks were not run independently. The paused-time benchmark covers touched-PID lookup, not summary scans or logging throughput.
1 participant
Problem
A sustained burst of damaged TS units emitted one warning per unit. The draft aggregated those warnings but could miss the next immediate warning after a PID was quiet for a full interval: its quiet boundary could precede the next global summary tick.
Approach
Warn immediately on the first damaged unit per PID burst and summarize additional drops at the existing one-second stats cadence. Check the touched PID's quiet interval directly when damage arrives, sharing summary emission with the periodic path. Finish flushes outstanding counts; cumulative damage totals remain unchanged.
The importer stays synchronous: elapsed summaries flush on the next input activity. A mocked-time regression fails on the prior draft (one initial warning instead of two) and passes after the fix. All three focused burst/quiet/cumulative-count tests pass. Nix
just checkpasses, as dojust test interop --all(full cross-language matrix, refusal, browser close, and finite-tail cases) andquest check(459 documents).The nightly benchmark sweeps active PID table size and damaged units per batch, with media time paused and 10,000 measured batches. Debug-profile nanoseconds per damaged unit, prior draft to candidate:
This measures the touched-PID hot path with logging disabled. The fix adds a per-event quiet check; it does not add a table scan. Periodic summaries still visit active bursts. Shared-host timings are descriptive, not an absolute performance budget.
Impact
No public API, wire, recovery, or dependency change. Warning logs keep PID/count fields; cumulative damage counters retain every refused unit. The completed quest is removed.
Alternatives
Keep activity-driven per-PID summaries, as recommended. An async timer would add lifecycle and scheduling work to a synchronous parser, while a global warning limiter would hide independent PID failures. The local quiet check preserves immediate new-burst warnings without creating a background task.
Follow-ups
No release or version change is included. A backport can be considered separately after the main change lands.
(Written by GPT-6 Astra)