Repository navigation
refactor(rendering): drive the Media Foundation decoder from one request state machine - #2545
Conversation
…rame ReadSample hands back a null sample for stream ticks (gaps in the stream), and read_sample reported those as Ok(None), the same as the end of the stream. The decoder then stopped a batch early and answered with an older frame. Read again until a sample, the end-of-stream flag or an error.
…est state machine The decoder thread mixed batching, a mid-batch readahead slide, deferred requests and four fallback chains in one loop. The policy now lives in FrameRequests, which never touches Media Foundation: the thread takes in requests, asks it for the next step (seek, decode one sample, or wait) and does it. Tests drive it with a fake sample stream. Once the decoder moves past a requested frame, the request gets the newest sample at or below that frame. Requests are served lowest first, each with its own seek when decoding forward cannot reach it, so the "first sample after" fallback only applies to a frame before the stream's first sample. The cache keeps every sample since the last seek and evicts the oldest, so the newest sample at or below a frame is always cached. Behaviour changes: - Several requests in flight (export transitions between two segments of one recording decode both through the same decoder) each get their own frame. The highest used to be the target, and a lower one far from it got the target's frame. - A request behind the decoder with nothing cached at or below it seeks back. Within half the cache size it used to get the next decoded, later frame. - Every request slides the readahead, cache hits included, so a sequential reader no longer drains the cache while the decoder idles. - Readahead stops at its frame count, or before the next sample would evict the one answering the latest request (frames too big for the cache budget used to get a later frame); the per-batch time budget is gone. - Samples outside the old cache window are cached too, so a frame after a VFR gap past the window gets its own sample, not the one before the gap. - A failed seek answers what waits from the cache instead of reading on from the old position as if it had landed. Export output is byte-identical to the previous decoder on the reference recording.
|
@cesarfrancots is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Thanks for the PR. Greptile currently has this at 4/5, and we only merge PRs once Greptile scores the current head 5/5. Please address the remaining Greptile feedback, then comment |
|
@richiemcilroy Thanks. The Greptile feedback is addressed and all review threads are resolved. Greptile now scores the current head ( |
…-machine The request state machine now maps request times with request_frame, like main's other decoder paths.
|
The macOS test finding is fixed here too (merged up from #2541 in 1e10ac5), and the branch includes the latest main. @greptileai review |
…decoder-state-machine
|
@greptileai review |
…decoder-state-machine
|
@greptileai review |
|
@richiemcilroy Greptile is at 5/5 on the current head (8c4630d), and all review threads are resolved. Since the last round I merged the latest main (one conflict in |
…decoder-state-machine
|
@greptileai review |
…decoder-state-machine
|
@greptileai review |
…decoder-state-machine
|
@greptileai review |
|
@greptileai review |
Summary
The Windows Media Foundation decoder thread used to be one 300-line loop. Batching, a mid-batch readahead slide, deferred requests and four separate fallback chains were all tangled together in it, which made its frame selection hard to reason about and impossible to test. This PR splits it into two parts:
FrameRequestsholds the policy and never touches Media Foundation. It owns the cache, the waiting requests and the readahead, and it answersnext_step()with Seek, Decode one sample, or Wait.One answer rule. Once the decoder moves past a requested frame, the request gets the newest sample at or below that frame. VFR samples that round to the same frame resolve the same way no matter when the request arrives. Requests are served lowest first, and each gets its own seek when decoding forward cannot reach it. The cache keeps every sample since the last seek and evicts the oldest, so that newest sample is always in the cache.
Unit tests drive the policy with a fake sample stream. They cover:
Each new rule was mutation-checked: reverting it fails its test.
Behaviour changes
read_sampleno longer returnsOk(None)for a sample that carries no frame. Stream ticks used to look like the end of the stream; it now reads past them.Verification
cargo test -p cap-rendering --lib media_foundation: 13 pass.cargo test -p cap-video-decode --libpasses.cargo clippy -p cap-rendering -p cap-video-decode --all-targets -- -D warningsis clean.a88dca3b…, SSIM 1.0). It matched on every run: three builds, seven exports.scripts/capbench.py(from perf(windows): fence-waited readbacks, overlapped MF export decode, cheaper screen capture #2541) against the perf(windows): fence-waited readbacks, overlapped MF export decode, cheaper screen capture #2541 build. Each row is the median, with a bootstrap 95% CI.One 5-rep run flagged random access as +25% WORSE. A 10-rep, three-way rerun showed that was session noise: the #2541 build itself ranged from 68 to 86 ms across sessions, and the rerun shows +0.6% with a CI of [−3.3%, +2.1%]. Media Foundation returned no frameless samples on this fixture, so the reader change is not a factor here.
The win here is correctness and structure, not speed. The trends lean the right way but stay within noise.
Notes for reviewers
ffmpeg.rskeeps its own readahead and eviction helpers. They are deliberately not shared. The MF cache evicts strictly oldest first, because that is what keeps "newest sample at or below a frame" exact. The FFmpeg helper evicts by direction.🤖 Generated with Claude Code
The PR appears safe to merge; no new actionable issue was found.Summary
The PR moves Media Foundation frame selection into
FrameRequestsand makesread_sampleskip events without a frame.Reviews (18) · Last reviewed commit: "Merge branch 'main' into perf/mf-decoder..." · Reviewed by Greptile