Skip to content

chore(quest): abandon unused Opus resync - #3373

Merged
kixelated merged 1 commit into
mainfrom
quest/m0/2849-moq-import-ts-a-truncated-or-spliced-opus-pes-ends-the
Sep 4, 2026
Merged

chore(quest): abandon unused Opus resync#3373
kixelated merged 1 commit into
mainfrom
quest/m0/2849-moq-import-ts-a-truncated-or-spliced-opus-pes-ends-the

Conversation

@kixelated

@kixelated kixelated commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Public API changes

None. This PR now changes quest documentation only.

Test plan

  • nix develop --command env -u RUSTC_WRAPPER just check: passed, 253 quest documents validated.
  • nix develop --command env -u RUSTC_WRAPPER just test: passed, with code suites skipped because no packages changed.

Closes #2849

(Written by GPT-5.6 Sol)

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: c69ac92c-16ce-4754-b3d8-0be32b646f9e

📥 Commits

Reviewing files that changed from the base of the PR and between 3660b17 and 6bbf1e2.

📒 Files selected for processing (2)
  • quest/m0/2849-moq-import-ts-a-truncated-or-spliced-opus-pes-ends-the.md
  • quest/m0/README.md
💤 Files with no reviewable changes (2)
  • quest/m0/2849-moq-import-ts-a-truncated-or-spliced-opus-pes-ends-the.md
  • quest/m0/README.md

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

The pull request removes the #2849 quest plan document. It also removes the #2849 entry from the m0 bug-fix quest list.

Merge Risk: ⚪ Minimal · up to 6bbf1

This change removes completed quest documentation and its tracking-list entry, with no identified runtime or product behavior risk.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR does not implement the coding requirements in issue #2849. It removes the quest documentation, while the issue requires Opus resynchronization, bounded recovery, and continued importing after t… Implement the required Opus resynchronization behavior and tests, or remove the link to issue #2849 and provide a separate issue that explicitly tracks abandoning the proposed fix.
Out of Scope Changes check ⚠️ Warning The documentation-only changes do not address the linked issue's required importer changes. They are outside the implementation scope defined by issue #2849. Align the changes with issue #2849 by modifying the MPEG-TS importer and adding the required tests, or use a different issue and PR scope for abandoning the quest documentation.
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Description check ✅ Passed The description accurately states that the PR abandons quest #2849 and removes related quest documentation. This matches the changeset.
Title check ✅ Passed The title clearly identifies the change as abandoning unused Opus resynchronization work. This matches the removed quest documentation.
Full details: Linked Issues check

Explanation

The PR does not implement the coding requirements in issue #2849. It removes the quest documentation, while the issue requires Opus resynchronization, bounded recovery, and continued importing after truncated or spliced PES data.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch quest/m0/2849-moq-import-ts-a-truncated-or-spliced-opus-pes-ends-the

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

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-04T16:46:53.992986Z 6bbf1e2 New commits
ℹ️ 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

Copy link
Copy Markdown
Collaborator Author

Cross-reference: this PR and #3372 (Closes #2798, the resync observability quest) both change rs/moq-mux/src/container/ts/import.rs, and there is a one-line interaction between them that is easy to lose on merge.

#3372 adds a Stats surface over Resync. It was written against a main where Opus did not go through Resync, so Stream::Opus sits in the None arm of Stream::stats and correctly reports nothing today.

Once both land, Opus does go through Resync, and Stream::Opus needs to move from that None arm into the stream.resync.stats() arm — otherwise Opus resyncs and discarded bytes are silently uncounted, which is exactly the blind spot #2798 exists to close.

Whichever of the two merges second should carry that move plus a test asserting a non-zero Opus resync count in the snapshot. No conflict has occurred so far; the two diffs are in different functions.

🤖 Generated with Claude Code

(Written by Claude Opus 5)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 97f6a44062

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

Comment thread rs/moq-mux/src/container/ts/import.rs Outdated
/// bytes per frame and 48 frames. Beyond that the `au_size` bytes aren't a length, and
/// believing one would have the stream retain arbitrary bytes waiting for an access unit
/// that is never coming.
const MAX_SIZE: usize = 1275 * 48;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include Opus framing overhead in the size cap

au_size is the size of the complete Opus packet, but 1275 limits each encoded frame, not the packet including its TOC and code-3 framing. For example, a conforming 120 ms CBR packet can contain 48 2.5 ms frames and needs two framing bytes, so 48 * 1275 + 2 already exceeds this constant. Such an access unit is classified as invalid, causing the resync loop to drop it and potentially fail an otherwise valid stream. Base the bound on the maximum complete packet, including framing and permitted padding. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L129-L129

Useful? React with 👍 / 👎.

The failure has no known users, and a correct resynchronizer is too complex to justify speculatively.

Co-Authored-By: GPT-5.6 Sol <noreply@openai.com>
@kixelated
kixelated force-pushed the quest/m0/2849-moq-import-ts-a-truncated-or-spliced-opus-pes-ends-the branch from 97f6a44 to 6bbf1e2 Compare September 4, 2026 16:45
@kixelated kixelated changed the title fix(moq-mux): resync Opus instead of ending the session on a truncated PES chore(quest): abandon unused Opus resync Sep 4, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6bbf1e2ccc

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

Comment thread quest/m0/README.md
- [#2405](/quest/m0/2405-js-net-connect-logs-on-every-connection-at-the-wrong.md) - js/net: connect logs print the JWT in the relay URL
- [Rust log redaction](/quest/m0/rust-log-redaction.md) - moq-native logs relay URLs with their query and moq-rtmp logs stream keys
- [#2849](/quest/m0/2849-moq-import-ts-a-truncated-or-spliced-opus-pes-ends-the.md) - moq import ts: a truncated or spliced Opus PES ends the session
- [#3139](/quest/m0/3139-moqsrc-a-rendition-nobody-answers-keeps-the-session-alive.md) - moqsrc: a rendition nobody answers keeps the session alive after the catalog closes

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep the quest until the Opus fix is present

This commit removes the #2849 quest and declares it closed, but its only tree changes are the two quest deletions. In the resulting tree, OpusStream::write still propagates malformed/truncated headers and Opus access unit exceeds PES payload at rs/moq-mux/src/container/ts/import.rs:1892-1895, while Stream::salvages_partial_pes still explicitly excludes Opus at lines 1329-1334. Thus a truncated or spliced Opus PES still ends the session, and deleting this entry loses the remaining work; include the implementation and regression tests before completing the quest.

AGENTS.md reference: quest/AGENTS.md:L113-L114

Useful? React with 👍 / 👎.

@kixelated
kixelated merged commit f5047bc into main Sep 4, 2026
2 checks passed
@kixelated
kixelated deleted the quest/m0/2849-moq-import-ts-a-truncated-or-spliced-opus-pes-ends-the branch September 4, 2026 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

moq import ts: a truncated or spliced Opus PES ends the session

1 participant