Repository navigation
Conversation
|
@Addin 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}
ff0a562 to
29ea30e
Compare
|
@greptileai review |
|
@greptileai review |
|
Hi @richiemcilroy can I have a review on this, or should this be closed? |
|
Thanks! Can you rebase on main to clear the conflicts? Then we'll take a look. |
Read Windows capture back through a staging ring with DO_NOT_WAIT instead of a blocking Map plus a second full-frame copy, and let the fragmented/instant M4S muxer encode from the captured D3D11 texture through Media Foundation with the existing FFmpeg encoder as fallback. Also removes the unused win_segmented muxers and the never-invoked DiskSpaceCallback plumbing.
- Keep the readback budget authoritative by default; CAP_READBACK_BLOCKING opts into an unbounded Map. - Negotiate MFT types inside the candidate loop and handle WARP without VIDEO_SUPPORT. - Validate harness --path values and count failures only for exercised paths.
- Evict idle stale-size staging textures before reporting StagingBusy. - Size the benchmark copy from each mapped frame and rebuild old_frame on resize. - Require METransformDrainComplete and return errors on failed or timed-out drains. - Count drained samples in the encoder health output counters. - Keep benchmark runs alive across capture idle timeouts.
- Block in Map by default and keep the readback deadline behind CAP_READBACK_BUDGET_MS. - Gate the Media Foundation texture path to opt-in Instant recordings; Studio and future recordings fall back to FFmpeg. - Output BT.709 limited-range video from the MF path and tag the DASH muxer with BT.709 metadata. - Send END_OF_STREAM before DRAIN and treat a slow drain as non-fatal. - Preserve the existing MFT selection and keyframe behavior for the MP4/camera encoders.
6341fe6 to
6557c8d
Compare
|
Rebased onto the rebased #2940 branch, which is now on current Local @greptileai review |
| if let Err(error) = | ||
| muxer.write_packet(packet, cap_mediafoundation_ffmpeg::MF_TIMEBASE) | ||
| { | ||
| failed_writes += 1; | ||
| if failed_writes == 1 || failed_writes.is_multiple_of(300) { | ||
| warn!( | ||
| failed_writes, | ||
| "Failed to write Media Foundation sample: {error}" | ||
| ); | ||
| } | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
When CAP_MF_TEXTURE_INPUT is enabled, this callback drops failed packet conversions and failed write_packet calls but returns Ok(()). The worker can then finish successfully even though video samples were lost. SegmentedPacketMuxer::finish does not remember earlier write errors, so a later successful trailer cannot catch them.
Save the first output error and return it after draining, or propagate it through encoder.run.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/output_pipeline/win_fragmented_m4s.rs
Line: 867-878
Comment:
**Lost samples look successful**
When `CAP_MF_TEXTURE_INPUT` is enabled, this callback drops failed packet conversions and failed `write_packet` calls but returns `Ok(())`. The worker can then finish successfully even though video samples were lost. `SegmentedPacketMuxer::finish` does not remember earlier write errors, so a later successful trailer cannot catch them.
Save the first output error and return it after draining, or propagate it through `encoder.run`.
---
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 615e65a and rebased #2999 onto it (new head 5cb3e12).
The Media Foundation sample callback now propagates packet-conversion, mutex-poison, and write_packet errors through encoder.run instead of warning and returning Ok(()). The existing failure path then disables the texture path, emits MuxerCrashed, finalizes the segments already written, and uses FFmpeg for the next recording, so a lost sample can no longer be reported as a successful encode.
@greptileai review
There was a problem hiding this comment.
Confirmed—the fix addresses the finding. Packet-conversion, muxer-lock, and write_packet failures now propagate through H264Encoder::run; its error path disables future texture input, emits MuxerCrashed, drains the video queue, and returns the error. The Media Foundation backend still finalizes the segments already written, so these lost samples are no longer reported as a successful encode. No further finding on this change.
|
The only red check is the macOS From the sandbox artifact:
@richiemcilroy could you rerun the failed job? The runner appears to have been delivering ~1 fps instead of the expected 30 fps. |
|
One more flaky check: |
Cache the video processor input/output views at converter creation, replace the single staging pair with a reusable ring, and keep CPU upload/readback copies outside the GPU mutex. Share one D3D11 converter across pool workers so camera recording uses one device instead of one per worker. Collapse contiguous plane copies into a single memcpy and add a benchmark example that compares the D3D11 path with single-threaded swscale.
Allow selecting the input format, worker counts, and single-thread, pool-only, or D3D11-only runs so the benchmark can isolate transfer, conversion, and concurrency costs.
Map the staging output with DO_NOT_WAIT and release the shared GPU mutex between retries so other camera workers can submit their blits and copies while one readback completes. The 4-worker 1080p pool drops from about 3.6 ms/frame to 2.2 ms/frame on the GTX 950M.
Four staging slots match the default camera pool so workers spend less time waiting for an upload/readback pair; deeper rings showed no additional gain.
Media Foundation hardware encoders need a video-capable D3D11 device to attach their DXGI device manager. Without it the texture-native screen encoder falls back to FFmpeg and the full-frame readback path. Keep the display-notch tests below the platform helpers so clippy accepts the module layout.
Time the Media Foundation encoder wait/input/process/output stages, the native camera upload/mux stages, and the screen D3D11 readback itself instead of including pacer waits in the conversion metric. Also make the camera benchmark run on Windows and honor an externally supplied RUST_LOG.
6557c8d to
5cb3e12
Compare
Summary
Optimizes the Windows D3D11 frame-conversion hot path and adds the instrumentation used to verify it.
MapwithD3D11_MAP_FLAG_DO_NOT_WAITand release the shared context between retries so other workers can submit GPU work.BENCH_INPUT,BENCH_WORKERS,BENCH_SINGLE_ONLY,BENCH_POOL_ONLY,BENCH_D3D11_ONLY) and report the selected adapter.D3D11_CREATE_DEVICE_VIDEO_SUPPORTon capture devices.Performance comparison
GTX 950M / i7-6700HQ, YUYV422 -> NV12, release build. Before is
8a71a02c; after ise8333db6, the last converter commit before the readbackFlushcorrectness fix. Medians of three four-worker runs and two single-thread runs.from_configpool, 720pfrom_configpool, 1080pfrom_configis the production camera pool. At maximum submitted throughput it is within run-to-run noise of the old four-device configuration while using one D3D11 device instead of four; at the 30 fps camera rate the pool does not contend, so the single-thread latency and contention rows are the relevant gains. Without the readback polling changes, sharing the converter would have been about 2x slower.Additional after-only measurements:
DO_NOT_WAITpolling at maximum throughput uses 148% of one core (18.5% of the 8-thread machine); at the 30 fps production rate the workers idle between frames, so no backoff was addedThe MF hardware encoder still falls back on this GPU: NVIDIA MFT returns
0x8000FFFFand Intel QSV returnsE_INVALIDARG. The FFmpeg fallback remains.Greptile review fixes
frame-converter: flush the immediate context after queuing the output copy and before theDO_NOT_WAITpolling loop, so standalone conversions cannot spin on work that was never submitted.capture_pipeline: build the WARP fallback withoutD3D11_CREATE_DEVICE_VIDEO_SUPPORT.win_fragmented_m4s: after draining, return the Media Foundation encoder error so a failed encoder cannot be finalized as a successful recording.scap-ffmpeg(base perf(recording): cut Windows readback copies and encode from textures #2940): create the mapped readbackAVBufferRefwithAV_BUFFER_FLAG_READONLY.Dependency
Stacked on #2940 (readback/MF groundwork). Until that merges, this diff includes its readback/MF commits. If #2940 lands with a merge commit, GitHub will reduce this PR to the converter/capture commits automatically; if it is squash-merged, this branch will need a rebase onto the new
main.Validation
cargo fmt --all -- --checkcargo clippy -p cap-frame-converter -p cap-enc-mediafoundation -p cap-recording --all-targets -- -D warningscargo test -p cap-recording --lib: 686 passed, 0 failed, 4 ignoredcap-frame-converterandcap-enc-mediafoundationtests passd3d11-benchmarkrerun after the lazy-staging change; numbers above are unchanged from the eager-allocation baselineThe texture-input path should preserve output errors before this PR merges.
Findings
Fix with agent prompt
Summary
Optimizes Windows frame conversion and adds the stacked readback and Media Foundation work.
Reviews (4) · Last reviewed commit: "fix(recording): report MF encoder failur..." · Reviewed by Greptile