Repository navigation
fix(rendering): keep AVAssetReader decoding when a frame request only times out - #3010
richiemcilroy wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| handle | ||
| .get_frame_with_timeout(time(frame), deadline_ms, 2, FrameDeadline::Drop) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this 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)
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.There was a problem hiding this comment.
Fixed in 82fee95. The final loop now checks every round:
- Rounds 1 and 2 assert
Ok(None)and that the handle is still onAVAssetReader. - Round 3, which triggers the fallback, asserts
Somewith 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.
…es the request that triggered it
…itial frame request
|
@greptileai review |
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/avfseek/seek-native/storm30/avfplayback/vfr-holds/full/seq30-from0/avfwith
segment 0 screen decoded by FFmpeg (software), expected AVAssetReader (fallback reason: AVAssetReader returned no frame). Thedecoder-fallbackfailures ofplayback/basic/camera-hidden/seq30-from0/avfandplayback/jitter-camera-40s/full/seq30-from0/avfon #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'srequest_framereturned anything other thanOk(Some(frame)).request_framealso returnsOk(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 hitstokio::time::timeout.Fix
request_framenow delegates tosend_frame_request, which reportsFrame,NoFrame(sender dropped or thread gone) orTimedOut. On the macOS primary:NoFrameswitches 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.TimedOutreturnsOk(None)for that request only. The handle switches only afterMAX_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
decoder::model::avassetreader_switches_to_software_only_when_its_reader_stops_answering(called fromavassetreader_answers_match_the_reference_model, which is what Engine verify runs on macOS). It uses the real AVAssetReader run loop on scripted media withset_vend_delay: three episodes of 2 missed deadlines each, separated by a good frame, keepAVAssetReaderand later frames are exact; 3 missed rounds in a row switch to software with reasonAVAssetReader stopped answeringand the next frame is exact; a reader that fails twice (fail_avf_read_at) switches on the first request with reasonAVAssetReader returned no frame.stalled_native_decoder_falls_back_once_and_preserves_offsets_across_clonesnow runs 3 concurrent rounds against a decoder that never answers: the first 2 return no frame and stay onAVAssetReader, the 3rd falls back once for both clones.crates/verify/expectations.toml.

Confidence 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.
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