Optimize playback prefetch and decoder scheduling#2014
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
|
hey @greptileai, please re-review the PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
| @@ -633,10 +644,41 @@ impl AVAssetReaderDecoder { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| while let Ok(message) = rx.try_recv() { | |||
| match message { | |||
| VideoDecoderMessage::GetFrame( | |||
| requested_time, | |||
| max_fallback_distance, | |||
| sender, | |||
| ) => { | |||
| let frame = (requested_time * fps as f32).floor() as u32; | |||
| if !sender.is_closed() { | |||
| pending_requests.push(PendingRequest { | |||
| frame, | |||
| max_fallback_distance, | |||
| sender, | |||
There was a problem hiding this comment.
When deferred_requests is non-empty, the loop iteration skips the rx.recv() / rx.try_recv() path entirely. Any new GetFrame messages that arrive on the channel while a deferred cluster is being decoded cannot be read until the deferred queue is fully drained. In a rapid-scrub scenario where a deferred cluster is large or slow to decode, a high-priority new position request could be delayed by the full deferred backlog before the decoder picks it up. Consider draining the channel into the deferred queue (or a priority queue) even during deferred processing so that fresh requests can supersede stale deferred ones.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/rendering/src/decoder/avassetreader.rs
Line: 606-660
Comment:
**New requests stall behind the deferred queue**
When `deferred_requests` is non-empty, the loop iteration skips the `rx.recv()` / `rx.try_recv()` path entirely. Any new `GetFrame` messages that arrive on the channel while a deferred cluster is being decoded cannot be read until the deferred queue is fully drained. In a rapid-scrub scenario where a deferred cluster is large or slow to decode, a high-priority new position request could be delayed by the full deferred backlog before the decoder picks it up. Consider draining the channel into the deferred queue (or a priority queue) even during deferred processing so that fresh requests can supersede stale deferred ones.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
hey @greptileai, please re-review the PR |
| max_fallback_distance, | ||
| sender, | ||
| }); | ||
| if let Some(request) = deferred_requests.pop_front() { |
There was a problem hiding this comment.
When deferred_requests is non-empty, this loop iteration never reads from rx, so fresh scrub requests can sit behind the deferred backlog. Consider always draining rx.try_recv() into pending_requests even during deferred processing (and re-applying the cluster split) so new near-term requests can preempt stale deferred work.
| while deferred_requests.front().is_some_and(|next| { | ||
| next.frame.saturating_sub(last_frame) <= DECODER_REQUEST_CLUSTER_GAP_FRAMES | ||
| }) { | ||
| if let Some(request) = deferred_requests.pop_front() { | ||
| if request.sender.is_closed() { | ||
| continue; | ||
| } | ||
| last_frame = request.frame; | ||
| pending_requests.push(request); | ||
| } | ||
| } |
There was a problem hiding this comment.
When a closed-sender request is popped from deferred_requests and skipped via continue (line 621), last_frame is not updated to that request's frame. The while-loop's next iteration compares next.frame.saturating_sub(last_frame) against the last valid frame rather than the last examined frame. This means a gap that actually spans [valid_frame → closed_frame → next_frame] is measured as [valid_frame → next_frame], which is inflated — it can prematurely terminate the cluster and leave genuinely contiguous frames stranded in deferred_requests. They'll be picked up in the following iteration, but may then be decoded in isolation rather than batched, triggering extra decoder seeks.
To preserve the intended "contiguous run" semantics, last_frame should be updated to the discarded request's frame even when its sender is closed, since the frame number — not the sender's liveness — determines cluster adjacency.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/rendering/src/decoder/avassetreader.rs
Line: 616-626
Comment:
**Closed-sender skip corrupts cluster-gap tracking**
When a closed-sender request is popped from `deferred_requests` and skipped via `continue` (line 621), `last_frame` is not updated to that request's frame. The while-loop's next iteration compares `next.frame.saturating_sub(last_frame)` against the last *valid* frame rather than the last *examined* frame. This means a gap that actually spans `[valid_frame → closed_frame → next_frame]` is measured as `[valid_frame → next_frame]`, which is inflated — it can prematurely terminate the cluster and leave genuinely contiguous frames stranded in `deferred_requests`. They'll be picked up in the following iteration, but may then be decoded in isolation rather than batched, triggering extra decoder seeks.
To preserve the intended "contiguous run" semantics, `last_frame` should be updated to the discarded request's frame even when its sender is closed, since the frame number — not the sender's liveness — determines cluster adjacency.
How can I resolve this? If you propose a fix, please make it concise.
|
hey @greptileai, please re-review the PR |
1 participant
Reduces redundant decode work during scrubbing and transitions by serializing preview prefetch, removing behind-playhead prefetch, and improving decoder allocation. Verified at 60fps with zero playback skips; rendering tests and export checks pass.
Greptile Summary
This PR reduces redundant decoder work during scrubbing and transitions through three complementary changes: serializing preview prefetch into a single cancellable sequential task (started only after confirmed rendering), removing behind-playhead prefetch that traces showed caused false scrub behavior, and splitting discontinuous AVAssetReader request batches into contiguous clusters decoded separately.
editor_instance.rs: Replaces 15 eagerly-spawned concurrent prefetch tasks with one sequentialtokio::spawnthat is properly guarded withtokio::select!for cancellation. Missing segments and media now usecontinueso a gap does not abort the remainder of the prefetch window.avassetreader.rs: Adds aVecDeque<PendingRequest>deferred queue; when sorted pending requests span a gap larger thanDECODER_REQUEST_CLUSTER_GAP_FRAMES(45 frames), the tail cluster is saved for the next loop iteration rather than processed in one seek.multi_position.rs: When a decoder reset is required, the pool selection now prefers the nearest untouched decoder so active decoders keep their position. Backs off to LRU selection once all pool members have been accessed. A dedicated regression test is included.Confidence Score: 5/5
The changes are safe to merge; the core decode-scheduling logic is well-structured and the 60fps validation with zero skipped frames is a strong signal.
No new defects were found in this pass. The previous review flagged two issues in
avassetreader.rs(new channel messages stalling behind the deferred queue, andlast_framenot being updated when a closed-sender request is skipped inside the cluster-drain loop) that remain open; those were already surfaced and are tracked in prior review threads. All other previously flagged items — thebreakvscontinuefor missing segments, and the missingtokio::select!cancellation guard in the prefetch loop — have been addressed.The deferred-request cluster logic in
crates/rendering/src/decoder/avassetreader.rshas two open items from the prior review worth revisiting in a follow-up: high-frequency scrub scenarios where new requests pile up behind a large deferred batch, and thelast_frametracking when closed-sender requests appear mid-cluster.Important Files Changed
continue(notbreak) for missing segments so gaps don't abort the full prefetch run.PREFETCH_BEHIND,prefetched_behindHashSet, and the behind-offset decode loop). Clean removal with no logical residue left behind.deferred_requestsVecDeque and cluster-gap splitting so discontinuous request batches are processed as separate contiguous runs. Two issues from the previous review thread remain in this file: new channel messages can stall behind the deferred queue, andlast_frameis not updated when a closed-sender request is skipped inside the inner cluster-drain loop.Reviews (6): Last reviewed commit: "fix(editor): preserve prefetch across mi..." | Re-trigger Greptile