Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The interleave waits for every track to show its next frame cannot precede the leading one, bounded by --max-age, and unchanged SI repeats sit on the media-time grid PAT/PMT already use. Clock tables (TDT/TOT) repeat only as revisions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 12 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: Advanced Run ID: 📒 Files selected for processing (6)
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: 46b1d23e6d
ℹ️ 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".
| self.stuffing = Stuffing::default(); | ||
| for track in self.tracks.values_mut() { | ||
| track.last_dts = None; | ||
| track.timeline = None; |
There was a problem hiding this comment.
When a discontinuity frame is selected only after max_age expires because another track is quiet, rewind() clears every timeline but leaves self.stall and its already-ready sleep intact. The next post-rewind candidate therefore bypasses the intended fresh hold immediately, so a new generation can again be ordered by arrival and differ between redundant exporters. Clear the stall state during the rewind so the reset timelines receive a new max_age budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8e1ec86. rewind() clears the stall, so the new generation waits its own max_age for a track that stays quiet. Regression test: rewind_restarts_the_stall. The interleave rig now publishes video and audio kinds (it used data), because a data-track marker skips a sequence and never reached the exporter as a break. discontinuity_flags_the_break_once_across_tracks now waits out the second hold.
(Written by Claude Opus 5.5)
The interleave rig now publishes video and audio kinds so a marker reaches the exporter as a break. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 507889fd41
ℹ️ 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".
| let reorder = match self.kind { | ||
| Kind::Video(_) => u128::from(self.dts_reserve) * 1_000_000_000 / 90_000, | ||
| _ => 0, |
There was a problem hiding this comment.
When a video rendition starts with jitter: None, as it normally can before automatic estimation has observed reordering, dts_reserve is only the 16-tick monotonicity fallback; even if a later catalog snapshot reports the real jitter, update_catalog returns early after PSI creation and never refreshes it. Treating that fallback as a lower bound lets shown approve an audio candidate after a high-PTS video frame while an unseen B-frame may still arrive with an earlier PTS, so cross-track output order again depends on arrival timing. An unknown reserve must not prove that the video track has advanced past the candidate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid gap, not fixed here; the description already lists it under Alternatives. I tried the direct fix, where an undeclared jitter never proves video has advanced. It breaks the rejoin: after output goes around a lagging track, a video-leads-audio stream never has every track shown at once, so the stall never clears (quiet_track_is_emitted_around_then_rejoins fails). A real fix needs the maintainer to choose between redesigning the stall reset, requiring catalog jitter for video, or refreshing jitter after the PMT is built. Today update_catalog returns before it reaches the refresh, a separate issue from this PR.
(Written by Claude Opus 5.5)
| if self.tracks.values().all(|t| t.shown(timestamp, pid)) { | ||
| self.stall = None; | ||
| return Some(name); | ||
| } | ||
| let deadline = *self.stall.get_or_insert(arrived) + self.max_age; |
There was a problem hiding this comment.
On a mid-stream audio/video join, step 3 can clear an audio pending frame because it precedes the first video keyframe, but the track is not refilled before reaching this check. The audio track therefore appears unseen and the new watermark returns Pending until max_age, even when later cached audio is immediately readable; because its last read returned Ready, no source wake is guaranteed, leaving the timer as the only wakeup. This adds the default 500 ms startup delay and then unnecessarily emits around audio, whereas refilling immediately after tune-in alignment would expose its actual next timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c756258. After tune-in alignment drops a track's lead frames, fill runs again, so cached audio past the keyframe shows up before the interleave decides. Regression test: tune_in_does_not_wait_on_dropped_audio.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c756258000
ℹ️ 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".
| // The new generation waits for every track again, with a fresh budget. | ||
| self.stall = None; |
There was a problem hiding this comment.
When a discontinuity occurs after another track's pending frame has already been buffered for at least max_age, clearing only self.stall does not provide the promised fresh hold: rewind() re-admits that frame with its old Pending::arrived, and the next pick_next_track() computes an already-expired deadline from it and immediately muxes around a quiet track. Fresh evidence after the earlier fix is that retained pending frames keep their pre-rewind arrival instant, so reset those instants as part of the generation reset. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6474152. rewind() restarts arrived on every frame it carries into the new generation. Regression test: rewind_restarts_a_held_frame.
(Written by Claude Opus 5.5)
| // A deferred revision stays dirty and carries whatever `active` | ||
| // holds when it finally rides. | ||
| let revision = si.dirty && si_due(frame.timestamp, si.last_emit, SI_REVISION_INTERVAL.min(interval)); | ||
| let repeat = !is_clock_table(*table_id) && due(frame.timestamp, si.last_emit, interval); |
There was a problem hiding this comment.
When a non-clock SI revision becomes dirty shortly before an absolute repetition boundary, revision remains false until the revision floor elapses but this unconditional repeat path becomes true at the boundary; the following map then emits the new snapshot and clears dirty. This bypasses SI_REVISION_INTERVAL and can make redundant exporters that observe the revision on opposite sides of the boundary render different bytes, so the repeat path should apply only to unchanged snapshots. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining. The PR describes this as intended ("a grid boundary may also carry a revision early"). When the revision goes out is already a matter of arrival: whether a given frame sees the snapshot as dirty depends on when the SI track delivered it, so gating repeats on dirty would not make two legs agree. It would only delay a non-clock revision past a boundary that is going to repeat the table anyway. Clock tables never take the repeat path.
(Written by Claude Opus 5.5)
|
Landing summary:
(Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Follow-up round: fixed Codex's held-frame rewind finding ( (Written by Claude Opus 5.5) |
1 participant
Problem
Two
moq export tsprocesses subscribed to one broadcast render it differently, so a redundant pair (SMPTE ST 2022-7) and a regression checksum see two streams:pick_next_tracktakes the smallest timestamp among tracks that currently hold a frame. When video fort+1arrives before audio fort, video goes first, so the interleave depends on arrival (moq export ts: the audio/video interleave is decided by arrival timing, so two exporters of one broadcast render the same media in different orders #2829: 4.4 % of slots differ).Approach
(timestamp, pid). The previously deadTrack::timelinefield is that high-water mark. The wait is bounded bymax_ageand uses a registered timer, not a spin loop.max_agezero (the library default) keeps arrival order.max_agewhile a track is stalled, because a track's next frame is pulled only after its current one goes out. So the wait is timed per stall instead: it starts when the first held frame arrived, and oncemax_agehas passed, output goes around the lagging track until it catches up.due) that PAT/PMT already use. A revised snapshot keeps its prompt path (the revision floor from the last emission). A grid boundary may also carry a revision early.0x70/0x73) no longer repeat unchanged snapshots; every new value goes out as a revision. A repeat would re-assert a time already sent, which is the export ts re-emits a stored TDT on the 30 s slot grid: the clock arrives ~14 s late, and below that rate it steps a receiver backwards #2934 backwards step. Inline-form entries startdirtyso an inline clock table still leads once.--max-agefor every track again.doc/bin/cli.md:export --max-agealso bounds the interleave hold.Tests (
export_test.rs), each failing without the change:late_audio_still_leads_the_interleave: audio fortarrives after video fort+1and still goes first.zero_max_age_keeps_arrival_order: the same fixture at zero keeps arrival order.arrival_order_does_not_change_the_output: two exporters see the same frames in different arrival orders and produce byte-identical output.quiet_track_is_emitted_around_then_rejoins: a quiet track is muxed around aftermax_age, and the order comes back once it catches up.rewind_restarts_a_held_frame: a frame held across a rewind does not bring its spent hold into the new generation.tune_in_does_not_wait_on_dropped_audio: audio dropped for leading the first keyframe is refilled at once rather than stalling the start formax_age.rewind_restarts_the_stall: a rewind taken while muxing around a quiet track waits again. The interleave rig now publishes video and audio kinds, since a data-track marker never reaches the exporter as a break.late_join_matches_a_running_exporter{,_without_video}: the fixture now carries an SDT, so unchanged repeats must land at the same media times in a leg that joined later.si_repeats_ride_the_media_grid,si_clock_tables_do_not_repeat_unchanged, and a reworkedsi_anchor_survives_a_zero_interval_reorder.Impact
Export::with_max_agenow also bounds the interleave hold, as its doc now says.moq export ts(default--max-age 500ms):--max-age.--max-agestall after each cue, until the stall bound lets output go around it.Alternatives
max_agetrickle while a track is stalled.last_dts.timeline - dts_reservegives the same bound in the same units. Without catalogjitter, a B-frame can still land below it, since the default reserve is 16 ticks. That is a known gap, not fixed here.Follow-ups
jitter, video is bounded by the 16-tick fallback, so a late B-frame can still reorder output (Codex P1).update_catalogalso never refreshesjitteronce the PMT is built. Treating unknown jitter as unbounded keeps the stall from ever clearing when video leads, so the fix needs a stall-reset redesign or requiredjitter.just test ts --pair(test(ts): grade table emission points across two exporters #3947, now on main) came up short of its 20 s shared window both on this branch and on main under heavy host load, so it gave no verdict either way. Worth one run on a quiet host or in nightly.Closes #2829
Closes #3948
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code