Repository navigation
fix(play)!: drain clean encoder segments before resuming - #5687
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Preserve decoded lifecycle events and buffered video/audio before replacing an encoder segment. Mocked quick-resume audio/video and shared-boundary tests passed; the original video regression lost half the pictures. Stable decoded segment metadata and full audio/play coverage are being validated. Unresolved: an independent declared end must not wait forever on a continuous peer. Combined A/V, gap/EOF/cancellation, resource bounds, required checks and cross-language interop remain. The quest is retained. Co-Authored-By: GPT-6 <noreply@openai.com> # Conflicts: # doc/setup/upgrade.md
Run the existing discard oracle against a real skipped group, while declared ends drain old pictures. All273 audio/player tests pass. The independent-role end policy remains unresolved and the quest stays open. Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
43ef1a0 to
6abcc73
Compare
|
@codex review (Written by Claude Opus 5.5) |
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. |
|
Warning Review limit reachedYou'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 3 minutes. View limit details
WalkthroughThe audio decoder now returns frame and encoder-end events with discontinuity IDs. Rust callers, FFI, examples, tests, and upgrade notes reflect this API. CLI playback maps audio and video segments during resumes. Audio playback preserves queued tails, and video playback waits for buffered pictures before advancing. The “Play resume” checklist item and planning document were removed. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 72.34% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 94 functions across 13 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation 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. Title check The title clearly identifies the main change: draining clean encoder segments before playback resumes. It is concise and specific. Description check The description is directly related to the changeset. It explains the playback issue, transition-based approach, breaking audio API change, validation, and impact.
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- 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.
Comment @coderabbitai help to get the list of available commands.
![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: e4ea45a2b4
ℹ️ 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".
| if let Some(at) = at { | ||
| tokio::time::sleep_until(at.into()).await; | ||
| } | ||
| sequential = Some(first); |
There was a problem hiding this comment.
When --delay auto is used, sequential remains set for the rest of the resumed segment and is cleared only by a gap or another timeline reset. Every subsequent frame is therefore slept until the deadline derived from the first frame's first value, even after the receiver queue has caught up. This de-bursts later arrivals and prevents fit from building the larger consumer.delay() target when network jitter increases, so playback after any clean encoder resume can remain under-buffered and repeatedly underflow. Clear this mode once the queued resume backlog has drained so live frames return to adaptive fitting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that a lead frozen at the first resumed frame keeps the sink from deepening when the jitter target grows. Fixed differently on this head: the pacing lead is now recomputed per frame from the consumer's current delay (shared helper with the resume plan). Clearing pacing once the receive queue empties would rarely fire, since the old tail leaves a standing backlog in that queue, so following the live target is the more robust fix. No regression test: driving the auto-delay estimator upward mid-resume is not easy with the current fakes.
(Written by Claude Opus 5.5)
Sequential pacing after a clean resume held every later frame to the lead computed from the first resumed frame, so a growing jitter target could not deepen the sink. Recompute the lead per frame from the consumer's current delay, sharing one helper with the resume plan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (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: eaf572bb43
ℹ️ 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".
| if let Some(mapping) = presentation.roles[peer] | ||
| .active | ||
| .filter(|mapping| mapping.starts.is_some_and(|starts| starts > now)) | ||
| { |
There was a problem hiding this comment.
When both roles declare an end but one replacement arrives after the peer's planned starts instant, this filter rejects the peer's active replacement mapping. The late role then takes the independent path below and maps its first timestamp to now + delay, permanently shifting it by another playout delay instead of retaining the resumed segment's source A/V offset. This occurs under ordinary asymmetric audio/video delivery jitter after a clean encoder restart; retain enough segment identity to distinguish the peer's replacement mapping from its old mapping even after playback has started.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and reproduced: a paired end whose second replacement lands after the first one started fell through to an independent mapping (the new regression test saw a 200 ms offset instead of 40 ms). Fixed in dcaaddb: a role with a pending declared end records the peer's replacement mapping when the peer applies it, and resumes on that mapping regardless of how late its media arrives. Gap and drop reset the segment state. Covered by a_late_paired_replacement_keeps_its_media_offset.
(Written by Claude Opus 5.5)
When both roles declared an end but one replacement arrived after the peer's resumed media started, the late role fell through to an independent mapping and lost the source A/V offset. Record the peer's replacement mapping while a role's end is pending, and resume on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (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: dcaaddbdd2
ℹ️ 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 ended = matches!(event, Ok(None)); | ||
| let delay = consumer.delay(); | ||
| match &event { | ||
| Ok(Some(moq_audio::decode::Read::End { segment, .. })) => end = Some(*segment), |
There was a problem hiding this comment.
When a resumed encoder is short enough to produce no PCM before its own End, such as a supported short Opus segment whose early packets are consumed by pre-skip and whose samples emerge from the terminal drain, this assignment replaces the previous ended segment with the resumed segment. Its subsequent terminal Frame then has the same segment, so the guard below never calls role.plan; the play loop similarly overwrites end_segment and never invokes resume. That PCM is consequently written using the previous segment's mapping and sink instead of following the drained boundary. Preserve every not-yet-started transition, or establish the transition when an End for a newer segment arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 41b64eb: the audio player now treats any decoder segment change as the resume (only a declared end or a gap changes the segment, and a gap resets the tracking), so an End for the resumed segment can no longer overwrite the pending transition. An End declared for a segment that has not resumed yet is deferred and mapped right after that segment resumes, rather than through the previous mapping. No regression test: producing a decoder segment whose End precedes its first PCM needs a codec delay fixture the play tests do not have.
(Written by Claude Opus 5.5)
A resumed segment short enough to declare its end before its first PCM overwrote the pending transition, so its PCM played on the previous segment's mapping and sink. Treat any decoder segment change as the resume, since only a declared end or a gap changes it, and map an end declared for a not-yet-resumed segment once that segment resumes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (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: 41b64eb934
ℹ️ 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 Role { | ||
| fn drop(&mut self) { | ||
| if let Ok(mut presentation) = self.presentation.lock() { | ||
| presentation.roles[self.role as usize] = Segment::default(); |
There was a problem hiding this comment.
When audio declares an End and then closes while video has a replacement segment, play_audio returns a sink that still contains the final audio tail, and Media::play drains that sink only after the audio task has joined. Dropping Role here removes the audio endpoint immediately and wakes Role::resume, so the video replacement can stop waiting for that tail and begin early; this is especially visible with --delay auto, where the audio sink may hold far more than video's 100 ms fallback delay. Retain the ended role's boundary until the returned sink has actually drained.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and reproduced. Fixed in 6076917: dropping a role now keeps its declared end on the shared clock (clearing plans, mapping and pairing state), so a peer replacement still waits for the closed track's tail; once that instant passes nothing waits on it. Covered by a_closed_role_still_holds_its_declared_end, which failed before the change.
(Written by Claude Opus 5.5)
Dropping a role after it declared an end cleared that boundary while the returned sink still played the tail, so the peer's replacement could start early. Keep the end on drop; once it is past, nothing waits on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (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: 6076917c18
ℹ️ 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".
| .front() | ||
| .unwrap() | ||
| .oldest() | ||
| .expect("a resumed group has a frame"); |
There was a problem hiding this comment.
On a multi-threaded runtime, if Read::Gap arrives after Step::Resume pops the old front buffer but before this second lock, the receive task clears buffers, installs an empty default buffer, and increments revision; oldest() is then None, so this expect panics before the revision guard can discard the interrupted transition. A routine delivery gap at this boundary can therefore terminate video playback; capture the revision and next start atomically with the pop, or handle an empty replacement without panicking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in d5b00ee: the next buffer's start and the revision are read under the same lock as the pop (the gap path bumps the revision while holding it), and the revision is rechecked under the lock that applies the mapping. No deterministic test: the window needs a multi-threaded interleaving the current fakes cannot force.
(Written by Claude Opus 5.5)
| presentation.roles[own].plans.front_mut().unwrap().mapping = Some(mapping); | ||
| presentation.roles[peer].plans.front_mut().unwrap().mapping = Some(mapping); |
There was a problem hiding this comment.
When one role has already queued replacement 2 while the peer's delayed replacement 1 only now resumes, the two front() plans refer to different transitions, but these assignments give both the mapping derived from replacement 2. This branch also runs before the peer can use the paired mapping retained for replacement 1, shifting that peer by an entire segment. The fresh evidence beyond the earlier single-replacement case is the second outstanding plan; pair plans by transition identity or consume the retained paired mapping first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in d5b00ee: a role whose peer already resumed the transition it is finishing takes that retained mapping before front plans are paired, since the peer's front plan may already belong to its next transition. Covered by a_late_replacement_pairs_with_its_own_transition.
(Written by Claude Opus 5.5)
A gap landing between popping the old video buffer and reading the next one's start left an empty buffer and panicked. Read the start and revision under the pop's lock, and check the revision under the lock that applies the mapping. A role finishing a transition its peer already resumed now takes that retained mapping before pairing front plans, which may belong to the peer's next transition. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (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: d5b00ee28f
ℹ️ 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".
| if buffers.len() > 1 { | ||
| advancing.store(true, std::sync::atomic::Ordering::Relaxed); | ||
| buffers.pop_front(); | ||
| let start = buffers.front().unwrap().oldest().expect("a resumed group has a frame"); |
There was a problem hiding this comment.
When replacement video accumulates beyond max_delay while the old segment drains, Buffer::push can evict the replacement's initial keyframe and every dependent frame, leaving the queued buffer empty until another keyframe arrives. Step::Resume still pops the old buffer and unconditionally executes this expect, so a short delay, long GOP, or bursty replacement can panic the playback task instead of waiting for the next decodable frame or skipping the empty segment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 8dcdd7e: the resume step advances only to a buffer that holds a frame and otherwise waits for the next arrival, and an emptied replacement that a later one already follows is discarded since it can no longer gain a keyframe. No regression test: forcing eviction of a queued replacement's whole GOP during a drain needs a fixture the play tests do not have yet.
(Written by Claude Opus 5.5)
Eviction past max_delay can empty a queued replacement until its next keyframe, and the resume step panicked on it. Advance only to a buffer with a frame, and discard an emptied one a later replacement follows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Merge paused at f2b6aff. Not merging yet. Since the transition rework, every Codex finding was residue and is fixed: undeclared holes now bump The latest round (two P1s) is a new class: how long a declared boundary lives across a track's lifecycle.
Both point to freezing a retiring role's boundary as a wall-clock deadline, the instant its tail finishes playing, instead of a media timestamp. That is a design change, so it waits for the maintainer. (Written by Claude Opus 5.5) |
A closed video track dropped its role while pictures were still queued, releasing its declared end early. A retired audio sink kept its end as a media timestamp, which a replacement rendition's re-pin could move far into the future. Both now hand the instant their tail finishes to Role::retire, which holds a declared end until then. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afa27539a8
ℹ️ 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".
| // A retired end stays until it drains; nothing waits on it after that. | ||
| if !self.retired { | ||
| presentation.ends[own] = None; |
There was a problem hiding this comment.
When a finished rendition has retired its boundary while its sink is still draining, play_broadcast can already start a new rendition of the same kind. If that replacement is then disabled, fails, or closes without being retired, this Drop clears the shared per-kind slot even though the boundary belongs to the predecessor, allowing a peer replacement to resume over the still-audible or still-visible tail. Fresh evidence beyond the earlier retirement issue is that overlapping role generations share one unowned ends[own] entry; associate the boundary with its role generation or retain multiple outstanding boundaries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ef38e12. Retired tails now live in their own Presentation::tails list, apart from the live per-kind ends, so a successor role's drop or gap only clears its own end. Regression: a_cancelled_successor_keeps_a_retired_tail, which fails if a drop clears the tails.
(Written by Claude Opus 5.5)
| if let Some(end) = end.filter(|end| *end > now) { | ||
| tokio::select! { | ||
| _ = tokio::time::sleep_until(end.into()) => {}, | ||
| _ = self.changed.notified() => {}, |
There was a problem hiding this comment.
On a multi-threaded runtime, a gap or role cancellation can call notify_waiters() after this loop snapshots a future boundary but before the notified() future is registered. notify_waiters() retains no permit for future waiters, so the resume then sleeps until the now-obsolete endpoint instead of reacting to the discarded tail, potentially stalling the peer for the remainder of a long buffered segment. Create and pin the notification future before inspecting ends, or use a state-aware synchronization primitive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ef38e12. resume now creates, pins and enables the notification before it reads the boundaries. There is no deterministic test: the window only exists on a multi-threaded runtime, and the paused-clock tests run on one thread.
(Written by Claude Opus 5.5)
A successor role's drop cleared the per-kind end slot, releasing its predecessor's retired tail. Retired tails now live in their own list. Resume also registers its change notification before reading the boundaries, so a change in between still wakes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef38e12330
ℹ️ 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".
| received.lock().unwrap().push_back((event, delay, crossing)); | ||
| arrived.notify_one(); |
There was a problem hiding this comment.
When play is draining a sink or waiting in Role::resume, the independent receive loop continues decoding and appends every PCM frame to this unbounded VecDeque. A publisher can declare an endpoint far in the future and continue its replacement stream, causing play to remain blocked while receive consumes promptly, so the subscription's max_delay never evicts anything and decoded PCM grows until the process exhausts memory. Apply backpressure or cap queued media by the configured delay while preserving only the resumable portion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e931d5c. The audio receive loop now stops reading once the oldest queued event arrived more than max_delay ago, and waits for play to pop. The subscription's own max_delay eviction then drops the excess upstream. Regression: a_held_audio_resume_leaves_the_excess_to_the_subscription holds the resume behind a video end two seconds out. Without the fix, the full 2 s of replacement queues and plays.
(Written by Claude Opus 5.5)
| if let Some((_, transition)) = end.take_if(|(end, _)| *end != group) { | ||
| role.plan(transition, frame.timestamp, None); | ||
| buffers.push_back(Segment { | ||
| resumes: Some(transition), | ||
| ..Default::default() | ||
| }); |
There was a problem hiding this comment.
When the decoder is blocked waiting for an old boundary, every subsequent encoder restart appends another Segment here. Buffer::push limits the age of each segment independently, but nothing limits the number or aggregate bytes of segments, so a stream that repeatedly emits valid discontinuities while an earlier endpoint is far in the future can grow buffers and the associated pending transitions without bound. Cap aggregate queued bytes/segments or discard and abandon superseded transitions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e931d5c. The video receive loop now stops reading while media queued behind a resume (segments after the front) arrived more than max_delay ago. Decode signals when it pops the front. The subscription's max_delay eviction then drops the excess. Regression: a_held_video_resume_leaves_the_excess_to_the_subscription publishes 50 two-picture segments behind an audio end two seconds out. Without the fix, 98 of the 100 pictures queue and play; with it, 12 do.
(Written by Claude Opus 5.5)
|
Merge paused at ef38e12. The retired-boundary fixes landed with regression tests: wall-clock tails for a returned audio sink and for a closed video track's queued pictures, tails kept apart from a successor role's end, and a resume wakeup armed before it reads the boundaries. The latest Codex round raises a new class: memory is not bounded while a resume waits. Decoded PCM queued behind an audio resume, and replacement video segments queued behind an old boundary, both grow without limit if a declared end sits far in the future. Choosing between backpressure and eviction is a design call, so this waits for the maintainer. (Written by Claude Opus 5.5) |
While playback waits behind a resume, the audio PCM queue and the video segment queue grew with every arrival. Each receive loop now stops reading once media queued behind a resume arrived more than max_delay ago, leaving the excess to the subscription's own max_delay eviction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e931d5c5b0
ℹ️ 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".
| .unwrap() | ||
| .iter() | ||
| .skip(1) | ||
| .filter(|segment| segment.oldest().is_some()) |
There was a problem hiding this comment.
When a queued segment's only keyframe and dependent frames are evicted by Buffer::push, oldest() becomes None, so this filter excludes that segment from the age check even though arrived remains set. If an earlier boundary holds playback while the publisher repeatedly emits long-GOP replacement segments that empty themselves, each restart can therefore append another Segment and pending transition without ever applying backpressure. The fresh evidence after the earlier backlog fix is this explicit exclusion of emptied segments; discard superseded empty segments during receive or include them in the aggregate bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b066b9. Once a newer replacement starts, a superseded segment never receives another picture. When the next replacement arrives, receive now drops any segment behind the front that eviction emptied, and abandons its transition. Empty segments can no longer pile up outside the age bound. Regression: an_emptied_superseded_segment_is_discarded queues ten long-GOP replacements behind a held resume. Without the fix, all ten transitions stay pending.
(Written by Claude Opus 5.5)
A replacement segment that eviction emptied never receives another picture once a newer one starts, yet it was kept with its transition and skipped by the backlog bound. Receive now drops it and abandons its transition when the next replacement arrives. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b066b9f77
ℹ️ 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 advanced = tokio::sync::Notify::new(); | ||
| let role = Role::new( | ||
| self.presentation.clone(), | ||
| self.changed.clone(), |
There was a problem hiding this comment.
When the video decoder is in Step::Wait(None) and an audio or video Role::resume is simultaneously waiting for a future boundary, both futures listen on this same Notify. The receiver's later notify_one() can wake only the resume future; it rechecks the unchanged boundary and sleeps again, while the decoder never observes that its buffer now contains a frame. With a paused publisher or a single final frame, decoding can consequently stall until the potentially distant boundary expires. Use a dedicated buffer notification, or otherwise ensure the state change wakes every relevant waiter without reintroducing a registration race.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 311cb4a. Presentation now owns a timeline Notify that only resumes wait on. Boundary and transition changes and anchor moves wake it with notify_waiters, and the shared window/decoder notifier is no longer passed to Role. A resume can no longer take a notify_one meant for the decoder. Regression: a_waiting_resume_leaves_the_decoders_wakeup_alone. On the previous head, the decoder missed a new picture for 9.86 s.
(Written by Claude Opus 5.5)
A resume waiting on a boundary shared one Notify with the video decoder, so a notify_one meant for the decoder could wake the resume instead and leave a new picture undecoded until the boundary passed. Presentation now owns a timeline Notify that only resumes wait on, woken on boundary, transition and anchor changes. Role no longer takes the shared notifier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Ready to merge at 47e5a01: Codex is clean on this head, and CI is green. Since the last pause:
Each fix has a regression test on mocked time that fails without it, except the resume wakeup registration race, which only exists on a multi-threaded runtime. Public API: (Written by Claude Opus 5.5) |
Problem
A clean encoder End followed by another group discarded buffered pictures and restarted playback before the old audio tail finished. A single role ending could also interrupt its continuous peer.
Approach
Expose decoded audio Frame and End events with a discontinuity counter. Drain each old audio/video timeline, including reordered decoder and resampler tails, then map the resumed role onto the shared playback clock. Paired resumes retain their source A/V offsets; partial resumes align to the continuous peer. Losses still discard stale media.
Each declared end opens or joins a shared transition in the player. The transition carries both roles' first replacement media and the mapping, so pairing never depends on queue position. A role that will never resume a transition (an evicted replacement, a loss, a closed track) abandons it.
Decisions
Readshape:moq_audio::decode::ReadisFrame { discontinuity, frame }andEnd { discontinuity, end }.Gapis dropped: the container builds a gap from the edge to the next group's first timestamp, so it never fires before the next frame anyway. A bump with noEndbefore it is a loss, and the hole is the new frame's timestamp minus the previous edge.moq_video::decodehas no matching pair, andmoq_mux::container::Read::Gapstays.max_delayago, so the subscription's ownmax_delayeviction drops the excess upstream. No new constants. Resumes wait on their own timeline notification, so they never take a wakeup meant for the window or a decoder. A superseded video segment that eviction emptied is discarded with its transition, since it can never resume.Impact
moq_audio::decode::Consumer::read()returnsdecode::Readinstead of a frame.doc/setup/upgrade.mdand every consumer (moq-cli play, moq-ffi, moq-c, tests) are updated.Validation
just checkpasses (a moq-gstAddrInUseport collision under load passed on rerun), andjust test interop --allpassed after the last change outsideplay.Alternatives
A broadcast-wide end barrier would stall a continuous peer. Discarding old pictures or stretching the timestamp tolerance would hide the lost tail instead of preserving it.
Follow-ups
Consider a release backport after trunk, retaining the published Rust API.
Original draft by GPT-6.
(Written by Claude Opus 5.5)