perf(windows): tee-backed MF hardware probe + capability-keyed encoder selection (Plan 035, step 3)#162
Merged
Merged
Conversation
…lity-keyed selection Plan 035 step 3 (issue #156): Windows hardware H.264 was disabled at every session because the old probe (-f null -) could pass while real tee output failed during header creation, so selection hardcoded the software fallback and the shipping path stayed CPU-bound on OpenH264. - The probe now exercises the EXACT production output topology: -f tee with the same fifo isolation (queue_size=512:drop_pkts_on_overflow=1) and the same slave shapes ([f=matroska:onfail=abort] + [f=flv:onfail=ignore: flvflags=no_duration_filesize]) against the null device, using the full production encode args. A pass proves headers, codec tags, and rate control for the real record+stream tee. - Selection is per-session and capability-keyed: the key now includes the FFmpeg binary's byte length + mtime, because app updates replace the bundled ffmpeg IN PLACE and a stale hardware verdict must not outlive the encoder build that produced it. Verdicts (including failures) are cached; a changed binary or output profile re-proves itself. - Fallback diagnostics are exact: rejected probes record the exit status plus a bounded, control-character-stripped stderr tail, and every session that falls back logs that reason ("Using the OpenH264 software H.264 fallback: tee-backed hardware probe failed (exit code N): ..."). - 15s probe timeout fails closed to OpenH264 with its own reason. Not in this change (needs the physical Windows host + Plan 034 metrics): the encoded bridge output that removes the raw FIFO bandwidth (step 2), the 1080p/4K characterization (step 1), and the packaged physical smokes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughWindows Media Foundation H.264 selection now uses a tee-backed hardware probe with per-profile caching. Cache keys include FFmpeg binary metadata, outcomes retain bounded failure reasons, and selection logs either hardware use or OpenH264 fallback. ChangesWindows Media Foundation probe
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EncoderSelection
participant ProbeCache
participant FFmpeg
EncoderSelection->>ProbeCache: Look up profile and FFmpeg metadata key
ProbeCache-->>EncoderSelection: Return cached accepted or rejected outcome
EncoderSelection->>FFmpeg: Run tee probe when cache entry is absent
FFmpeg-->>EncoderSelection: Return exit status and stderr tail
EncoderSelection->>ProbeCache: Store probe outcome and rejection reason
EncoderSelection-->>EncoderSelection: Select Media Foundation hardware or OpenH264 fallback
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/videorc-backend/src/recording.rs (1)
11376-11444: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise cache replay and timeout through the selection path.
The tests only validate the map and formatter. They would still pass if selection re-ran FFmpeg or timeout enforcement broke. Inject the probe runner/timeout and assert one invocation, exact cached failure replay, and fail-closed timeout behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/videorc-backend/src/recording.rs` around lines 11376 - 11444, Extend the media-foundation selection tests around the probe-selection function to inject a controllable probe runner and timeout. Assert that the first failed probe invokes the runner once, subsequent selection with the same windows_media_foundation_probe_key replays the exact cached failure without another invocation, and an exceeded timeout produces the existing fail-closed rejection outcome. Keep the current cache-map and fallback-reason assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/videorc-backend/src/recording.rs`:
- Around line 6336-6337: Update the FFmpeg cache-key metadata fields and their
construction to store modification time at nanosecond precision instead of using
as_millis(), preserving the full value consistently wherever the key is compared
or serialized. Extend the relevant cache regression test to replace the FFmpeg
binary in place with identical length but a different nanosecond mtime, and
verify the Windows Media Foundation verdict is recomputed rather than reused.
- Around line 6358-6386: Bound stderr during probe execution rather than only
trimming it in windows_media_foundation_fallback_reason. Update the probe’s
wait_with_output/process handling to capture at most the needed stderr tail
while the process runs, preserving timeout behavior and the existing 400-byte
fallback-reason limit.
---
Nitpick comments:
In `@crates/videorc-backend/src/recording.rs`:
- Around line 11376-11444: Extend the media-foundation selection tests around
the probe-selection function to inject a controllable probe runner and timeout.
Assert that the first failed probe invokes the runner once, subsequent selection
with the same windows_media_foundation_probe_key replays the exact cached
failure without another invocation, and an exceeded timeout produces the
existing fail-closed rejection outcome. Keep the current cache-map and
fallback-reason assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d22220c-0779-4bb2-8106-630fdbb5d3bd
📒 Files selected for processing (1)
crates/videorc-backend/src/recording.rs
Comment on lines
+6336
to
+6337
| ffmpeg_len: Option<u64>, | ||
| ffmpeg_modified_ms: Option<u128>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant area around the cache key fields and their uses.
grep -nE 'ffmpeg_modified_(ms|ns)|ffmpeg_len|modified\(' crates/videorc-backend/src/recording.rs | sed -n '1,120p'
echo
echo '--- context around the struct / cache key code ---'
sed -n '6310,6425p' crates/videorc-backend/src/recording.rs
echo
echo '--- additional call sites ---'
grep -nE 'ffmpeg_modified_(ms|ns)' -n crates/videorc-backend/src/recording.rsRepository: TheOrcDev/videorc
Length of output: 4500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tests that exercise the cache key and replacement behavior.
grep -nE 'same-size|mtime|ffmpeg|cache|probe' crates/videorc-backend/src/recording.rs | sed -n '1,200p'
echo
echo '--- tests around the relevant section ---'
sed -n '11290,11440p' crates/videorc-backend/src/recording.rsRepository: TheOrcDev/videorc
Length of output: 19629
Preserve nanosecond precision in the FFmpeg cache key. as_millis() can collapse same-size FFmpeg replacements into the same key, so a stale Windows Media Foundation verdict can be reused after an in-place update. Store the full mtime precision and add a same-size replacement regression; the current test only changes the binary length.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/videorc-backend/src/recording.rs` around lines 6336 - 6337, Update the
FFmpeg cache-key metadata fields and their construction to store modification
time at nanosecond precision instead of using as_millis(), preserving the full
value consistently wherever the key is compared or serialized. Extend the
relevant cache regression test to replace the FFmpeg binary in place with
identical length but a different nanosecond mtime, and verify the Windows Media
Foundation verdict is recomputed rather than reused.
Comment on lines
+6358
to
+6386
| fn windows_media_foundation_fallback_reason( | ||
| exit_code: Option<i32>, | ||
| stderr: &str, | ||
| timed_out: bool, | ||
| ) -> String { | ||
| const STDERR_TAIL_MAX: usize = 400; | ||
| if timed_out { | ||
| return "tee-backed hardware probe timed out".to_string(); | ||
| } | ||
| let cleaned: String = stderr | ||
| .chars() | ||
| .map(|character| { | ||
| if character.is_control() { | ||
| ' ' | ||
| } else { | ||
| character | ||
| } | ||
| }) | ||
| .collect(); | ||
| let cleaned = cleaned.split_whitespace().collect::<Vec<_>>().join(" "); | ||
| let tail = if cleaned.len() > STDERR_TAIL_MAX { | ||
| let start = cleaned.len() - STDERR_TAIL_MAX; | ||
| let boundary = (start..cleaned.len()) | ||
| .find(|index| cleaned.is_char_boundary(*index)) | ||
| .unwrap_or(cleaned.len()); | ||
| format!("…{}", &cleaned[boundary..]) | ||
| } else { | ||
| cleaned | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target function and nearby timeout/output handling.
rg -n -C 40 "windows_media_foundation_fallback_reason|wait_with_output\\(|timeout|stderr" crates/videorc-backend/src/recording.rs
# Show a focused slice around the reported lines.
sed -n '6330,6415p' crates/videorc-backend/src/recording.rs
echo '---'
sed -n '6550,6645p' crates/videorc-backend/src/recording.rsRepository: TheOrcDev/videorc
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '6330,6415p' crates/videorc-backend/src/recording.rs
echo '---'
sed -n '6550,6645p' crates/videorc-backend/src/recording.rsRepository: TheOrcDev/videorc
Length of output: 6238
Bound stderr while the probe runs. wait_with_output() still accumulates the full stderr stream before the 400-byte trim, so a noisy failed probe can grow memory unboundedly before the timeout fires.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/videorc-backend/src/recording.rs` around lines 6358 - 6386, Bound
stderr during probe execution rather than only trimming it in
windows_media_foundation_fallback_reason. Update the probe’s
wait_with_output/process handling to capture at most the needed stderr tail
while the process runs, preserving timeout behavior and the existing 400-byte
fallback-reason limit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #156 (Plan 035). This lands the implementable-off-hardware core — step 3 plus step 4's Rust regression tests — and re-enables hardware Media Foundation H.264 behind exactly the proof the plan demands.
What was wrong
select_windows_media_foundation_encoderstoredfalseunconditionally: the old capability probe encoded 3 frames to-f null -, which h264_mf can pass while its real tee output fails during header creation. Result: every Windows session paid for software encoding (OpenH264) even on capable hardware.What changed
Tee-backed probe — the probe keeps the full production encode args (same rate control, GOP, headers) and now outputs through the real tee topology:
-f tee -use_fifo 1 -fifo_options queue_size=512:drop_pkts_on_overflow=1with the production slave shapes[f=matroska:onfail=abort]+[f=flv:onfail=ignore:flvflags=no_duration_filesize], written to the null device. A pass now proves header creation, codec tags, and rate control for the actual record+stream tee — the precise failure mode the old probe missed. Hardware is selected only on a probe pass, per the plan's STOP condition.Capability-keyed, per-session selection with a cache — the verdict cache is keyed by canonical FFmpeg path + binary byte length + mtime (app updates replace the bundled ffmpeg in place — path alone would let a stale verdict outlive the binary; this implements the maintenance note) + width/height/fps/bitrate. Failures are cached like successes, so a rejected probe never re-runs FFmpeg each session; any binary or profile change re-proves itself.
Exact fallback diagnostics — a rejected probe records exit status + a bounded, control-character-stripped stderr tail, and every fallback session logs it:
Using the OpenH264 software H.264 fallback: tee-backed hardware probe failed (exit code 1): …. A 15s timeout fails closed with its own reason.Regression tests
Explicitly deferred (needs the physical Windows host + Plan 034 instrumentation)
Keeping #156 open for those; this PR removes the CPU-bound software-encode half of the bottleneck wherever hardware proves itself.
Verification
cargo test -p videorc-backend: 1326 green (4 new tests + hardened probe pins);cargo clippy -- -D warnings;cargo fmt --check.pnpm check:windows(cargo xwin,x86_64-pc-windows-msvc): the Windows-only selection/probe/timeout code compiles for the real target.#[cfg(windows)]/#[cfg(any(test, windows))]paths + tests), so macOS recording behavior — includingsmoke:recording-matrixcoverage — is untouched by construction.🤖 Generated with Claude Code
Summary by CodeRabbit