Skip to content

fix(rendering): keep AVAssetReader decoding when a frame request only times out - #3010

Open
richiemcilroy wants to merge 3 commits into
mainfrom
fix/avf-fallback-only-on-failure
Open

richiemcilroy wants to merge 3 commits into
mainfrom
fix/avf-fallback-only-on-failure

Conversation

@richiemcilroy

@richiemcilroy richiemcilroy commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

Bug

On macOS, one slow frame on a busy machine switches a recording segment from AVAssetReader to FFmpeg software decoding for the rest of the editor session (more CPU, slower decoding until the editor is reopened).

Evidence

Main push run 39997431468 (macos / build and test) failed 6 cases under heavy runner load (case time 2329 s vs about 1000 s usual), all passing on rerun, for example:

  • seek/vfr-holds/revisit60/avf
  • seek/seek-native/storm30/avf
  • playback/vfr-holds/full/seq30-from0/avf

with segment 0 screen decoded by FFmpeg (software), expected AVAssetReader (fallback reason: AVAssetReader returned no frame). The decoder-fallback failures of playback/basic/camera-hidden/seq30-from0/avf and playback/jitter-camera-40s/full/seq30-from0/avf on #2852 are the same thing.

Mechanism

AsyncVideoDecoderHandle::get_frame_with_timeout (crates/rendering/src/decoder/mod.rs) switched to software decoding whenever the primary decoder's request_frame returned anything other than Ok(Some(frame)). request_frame also returns Ok(None) when a playback (FrameDeadline::Drop) request merely misses its 2 to 4 s deadline ("Frame decode request timed out"), so a single late frame took the same path as a dead reader.

The two cases are distinguishable at the oneshot: a reader that gives up on a request (second failed reader, failed on-demand init, unfulfillable request) drops the sender, so the receiver sees RecvError; a gone decoder thread fails the send; a slow one just hits tokio::time::timeout.

Fix

request_frame now delegates to send_frame_request, which reports Frame, NoFrame (sender dropped or thread gone) or TimedOut. On the macOS primary:

  • NoFrame switches to software immediately, as before (keeps the 8cd61c2 behaviour for readers that stop returning frames).
  • Err(DecodeStalled) (export waited 120 s) also switches, as before.
  • TimedOut returns Ok(None) for that request only. The handle switches only after MAX_TIMED_OUT_ROUNDS (3) consecutive timed-out rounds with no frame delivered in between, so a reader that never answers (stuck thread, reader-permit starvation in a many-segment project) still falls back, after at least 3 full deadlines. Any frame from the primary resets the count. Requests that were already in flight when the previous timeout fired count as the same round, because playback keeps many requests in flight and one slow decode times them out together.

The fallback reason now says which case happened (AVAssetReader returned no frame, AVAssetReader stopped answering, AVAssetReader stalled). Non-macOS paths and the fallback decoder itself are unchanged.

Test

  • New decoder::model::avassetreader_switches_to_software_only_when_its_reader_stops_answering (called from avassetreader_answers_match_the_reference_model, which is what Engine verify runs on macOS). It uses the real AVAssetReader run loop on scripted media with set_vend_delay: three episodes of 2 missed deadlines each, separated by a good frame, keep AVAssetReader and later frames are exact; 3 missed rounds in a row switch to software with reason AVAssetReader stopped answering and the next frame is exact; a reader that fails twice (fail_avf_read_at) switches on the first request with reason AVAssetReader returned no frame.
  • stalled_native_decoder_falls_back_once_and_preserves_offsets_across_clones now runs 3 concurrent rounds against a decoder that never answers: the first 2 return no frame and stay on AVAssetReader, the 3rd falls back once for both clones.
  • No changes to crates/verify/expectations.toml.

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; the previous finding is fixed and no new blocking issues were found.

Summary

The PR separates missed frame deadlines from readers that stop returning frames.

  • AVAssetReader stays active through isolated playback delays.

The previous finding is fixed. No new actionable issues were found. Tests were not run.

Reviews (2) · Last reviewed commit: "fix(rendering): switch to software decod..." · Reviewed by Greptile

@vercel

vercel Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
cap-status Ready Ready Preview Oct 23, 2026 9:03pm UTC

@capy-ai capy-ai 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.

Capy found 1 potential issue (1 medium).

Open in Capy

Comment thread crates/rendering/src/decoder/mod.rs
Comment on lines +1227 to +1230
handle
.get_frame_with_timeout(time(frame), deadline_ms, 2, FrameDeadline::Drop)
.await
.unwrap();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Fallback result goes unchecked

The final timeout loop discards the Option<DecodedFrame> returned by .await.unwrap(). unwrap() handles only the outer Result. The repository guide requires every Option to be explicitly handled, so this must be fixed before merging. Assert None for the first two rounds and Some for the fallback round. That also checks whether fallback serves the request that triggers it.

Context Used: AGENTS.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/rendering/src/decoder/model/mod.rs
Line: 1227-1230

Comment:
**Fallback result goes unchecked**

The final timeout loop discards the `Option<DecodedFrame>` returned by `.await.unwrap()`. `unwrap()` handles only the outer `Result`. The repository guide requires every `Option` to be explicitly handled, so this must be fixed before merging. Assert `None` for the first two rounds and `Some` for the fallback round. That also checks whether fallback serves the request that triggers it.

**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 82fee95. The final loop now checks every round:

  • Rounds 1 and 2 assert Ok(None) and that the handle is still on AVAssetReader.
  • Round 3, which triggers the fallback, asserts Some with the exact requested frame, so it checks that software decoding serves the request that triggered it.

To make round 3 answerable, the scripted vend delay is lifted halfway through that round's deadline. The delay applies to every reader of the script, including the FFmpeg fallback. The native reader is still inside the 1500 ms vend it started in round 1, so it still misses the 250 ms deadline, and only the software decoder can answer in time. The per-episode recovery requests now use get_frame_initial so the 1500 ms vend can't push them past the 2 s playback deadline.

@capy-ai capy-ai 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.

Capy found no issues.

View 2 other findings in Capy.

Open in Capy

@capy-ai capy-ai 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.

Capy found no issues.

View 1 other finding in Capy.

Open in Capy

@richiemcilroy

Copy link
Copy Markdown
Member Author

@greptileai review

This branch was successfully deployed

2 active deployments
Preview – cap-web — 61ca5851 Deployed Oct 23, 2026 by vercel[bot]
Preview – cap-status — 61ca5851 Deployed Oct 23, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant