Skip to content

fix(moq-mux): shift TS passthrough onto the catalog timebase - #5118

Closed
kixelated wants to merge 1 commit into
mainfrom
fix/ts-passthrough-timebase
Closed

kixelated wants to merge 1 commit into
mainfrom
fix/ts-passthrough-timebase

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem

main doesn't compile. #5082 made catalog::Producer::anchor pub(super) and moved every importer onto a per-import catalog::Timebase. #5003 landed the TS passthrough importer on the old self.catalog.anchor(timestamp) call, so moq-mux fails with E0624: method anchor is private (container/ts/passthrough.rs:622). Every CI job on PRs that merge main fails, for example #4039.

Fix

Passthrough takes a timebase from its reservation (reserved.timebase()) and shifts every object's timestamp through it, the same port #5082 made for FLV, fMP4, and MKV. The first object always starts a group, so it anchors the import before the reservation publishes, as before.

Impact

No public API change and no wire change. Passthrough timestamps now follow #5082's timebase rules, verbatim when the import places the catalog clock, like the other importers.

Verification

cargo test -p moq-mux --lib (1180 passed) and just check.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

#5082 made catalog::Producer::anchor private and moved importers onto a
per-import Timebase, while #5003 added the TS passthrough importer on the
old anchor, so main stopped compiling. Passthrough now takes a timebase
from its reservation and shifts every object, like the other importers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 9, 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-10-09T21:42:26.328625Z e996aaa 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.

@coderabbitai

coderabbitai Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 11 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6172c238-5c1b-478f-a254-78f7b120e7f8

📥 Commits

Reviewing files that changed from the base of the PR and between 38066fd and e996aaa.


📒 Files selected for processing (1)
  • rs/moq-mux/src/container/ts/passthrough.rs


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review of #5118 at e996aaa4

Small, correct port of the TS passthrough importer onto #5082's per-import Timebase (reserved.timebase() + clock.shift(...)), matching FLV/fMP4/MKV. It fixes the E0624 main build break.

Blocking: none.

Non-blocking

  • Duplicate of fix(moq-mux): ts passthrough shifts onto the catalog clock #5117. fix(moq-mux): ts passthrough shifts onto the catalog clock #5117 (still open, head a7103ad4) fixes the same break in the same file. Only one should land, so close the other to avoid a merge conflict.
  • passthrough.rs ~608: anchoring now relies on shift() anchoring on the first call, with the comment saying "the first object starts a group". If a stream's first object ever arrived without starting a group, it would still anchor here (fine), but the old explicit reservation.is_some() guard documented the intent. A debug_assert!(self.group.is_none() || self.reservation.is_none()) or similar would keep the invariant checked.
  • The monotonic ticks.max(self.written) guard runs on raw ticks before the shift. That's fine because the shift is a constant offset, but a one-line note would help.
  • CI is pending on this head.

Verdict: MERGE (once CI is green, choosing this or #5117)

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

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: e996aaa

No actionable correctness or security defects found in the diff and relevant surrounding code. The direction is sound: passthrough.rs:299,610–612 reuses the reservation’s existing Timebase, fixes the obsolete private anchor call, and anchors before the reservation is released. Applying one constant offset after the PCR monotonicity guard preserves ordering and intervals; public API, framing, and TS payload bytes stay unchanged.

Non-blocking verification gap: rs/moq-mux/src/container/ts/passthrough_test.rs:64–68 constructs a fresh catalog, so these tests do not exercise the newly relevant nonzero offset. Building on the existing overlap note, retain #5117’s an_import_shifts_onto_a_clock_already_in_use regression if this implementation is selected; it checks a fixed catalog clock, constant offset across every object, and a flagged jump.

Verification limits: GitHub source review only; I did not run builds/tests or independently reproduce the reported 1,180-test pass. Check/Test and platform CI are still running on this SHA. State, head, and existing reviews were rechecked before posting.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #5117, which merged the same Timebase shift with a regression test (an_import_shifts_onto_a_clock_already_in_use); main builds again. Closed at the maintainer's direction.

(Written by Claude Opus 5.5)

@kixelated kixelated closed this Oct 9, 2026
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.

1 participant