feat: Improve Media Foundation initialization and error handling #1253
Conversation
WalkthroughThe PR adds Windows Media Foundation initialization compatibility checks and detailed error handling to cap_mediafoundation-utils. It detects Wondershare DemoCreator conflicts and introduces a new public InitError type with variants for third-party conflicts, RoInitialize failures, and MFStartup failures. Initialization callsites in three recording modules are updated to use the new error-aware variants. Changes
Sequence DiagramsequenceDiagram
participant Caller as Recording Module
participant Compat as check_compatibility()
participant Init as thread_init_checked()
participant WinRT as Windows Runtime
rect rgb(200, 220, 255)
Note over Compat,WinRT: New Compatibility Check Phase
Caller->>Compat: Check for conflicts (DLL detection)
alt Wondershare DemoCreator Detected
Compat-->>Caller: InitError::ThirdPartyConflict
Caller->>Caller: Log error, early return
else No Conflicts
Compat-->>Caller: Ok(())
end
end
rect rgb(200, 220, 255)
Note over Init,WinRT: New Error-Aware Init Phase
Caller->>Init: Initialize with error handling
alt RoInitialize Fails
Init-->>Caller: InitError::RoInitializeFailed
else MFStartup Fails
Init-->>Caller: InitError::MFStartupFailed
else Success
Init->>WinRT: RoInitialize + MFStartup
WinRT-->>Init: Ok
Init-->>Caller: Ok(())
Caller->>Caller: Proceed with setup
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes The changes introduce new error-handling logic and compatibility detection in lib.rs, then consistently apply the new API across three recording modules. While the pattern is repetitive across callsites (reducing per-file review load), the core library changes involve Windows FFI and error type design that require careful attention. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mediafoundation-utils/Cargo.toml (1)
6-8: Add missing dependency for thiserror derive.CI fails because
thiserrorisn’t declared in this crate.Apply one of these:
Option A (workspace-managed):
[dependencies] +thiserror = { workspace = true }Option B (explicit version):
[dependencies] +thiserror = "1"
🧹 Nitpick comments (4)
crates/mediafoundation-utils/src/lib.rs (1)
32-51: Minor: simplifyGetModuleHandleWcalls.Pass the
w!()PCWSTR directly; avoid redundantPCWSTR(dll_name.as_ptr()).- let dll_name = windows::core::w!("CaptureGameHook_64.dll"); - let handle = GetModuleHandleW(PCWSTR(dll_name.as_ptr())).ok(); + let dll_name = windows::core::w!("CaptureGameHook_64.dll"); + let handle = GetModuleHandleW(dll_name).ok(); @@ - let dll_name_32 = windows::core::w!("CaptureGameHook.dll"); - let handle_32 = GetModuleHandleW(PCWSTR(dll_name_32.as_ptr())).ok(); + let dll_name_32 = windows::core::w!("CaptureGameHook.dll"); + let handle_32 = GetModuleHandleW(dll_name_32).ok();crates/recording/src/sources/screen_capture/windows.rs (1)
188-201: Optional: rely on thread_init_checked() only.Since
thread_init_checked()already performs compatibility checks, you can drop the explicitcheck_compatibility()block to avoid duplicate work/logs.- // Check for incompatible third-party software before initializing - if let Err(e) = cap_mediafoundation_utils::check_compatibility() { - error!("Third-party software conflict detected: {}", e); - let _ = error_tx.try_send(anyhow!("{}", e)); - return; - } - // Initialize Windows Runtime and Media Foundation with error handling - if let Err(e) = cap_mediafoundation_utils::thread_init_checked() { + if let Err(e) = cap_mediafoundation_utils::thread_init_checked() { error!("Failed to initialize capture thread: {}", e); - let _ = error_tx.send(anyhow!("{}", e)); + let _ = error_tx.try_send(anyhow!("{}", e)); return; }crates/recording/src/output_pipeline/win.rs (1)
66-79: Optional: avoid duplicate compatibility check.
thread_init_checked()performs the pre-check; you can remove the separatecheck_compatibility()block to simplify and reduce double-logging.- // Check for incompatible third-party software before initializing - if let Err(e) = cap_mediafoundation_utils::check_compatibility() { - error!("Third-party software conflict detected: {}", e); - let _ = ready_tx.send(Err(anyhow!("{}", e))); - return; - } - // Initialize Windows Runtime and Media Foundation with error handling if let Err(e) = cap_mediafoundation_utils::thread_init_checked() { error!("Failed to initialize encoder thread: {}", e); let _ = ready_tx.send(Err(anyhow!("{}", e))); return; }crates/recording/src/instant_recording.rs (1)
291-297: Optional: remove redundantcheck_compatibility()call.
thread_init_checked()already calls it; keep only the latter.- { - // Check for incompatible third-party software before initializing - cap_mediafoundation_utils::check_compatibility()?; - - // Initialize Windows Runtime and Media Foundation with error handling - cap_mediafoundation_utils::thread_init_checked()?; - } + { + cap_mediafoundation_utils::thread_init_checked()?; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/mediafoundation-utils/Cargo.toml(1 hunks)crates/mediafoundation-utils/src/lib.rs(1 hunks)crates/recording/src/instant_recording.rs(1 hunks)crates/recording/src/output_pipeline/win.rs(1 hunks)crates/recording/src/sources/screen_capture/windows.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
crates/mediafoundation-utils/src/lib.rs
[error] 22-29: failed to resolve: use of unresolved module or unlinked crate thiserror and cannot find attribute error in this scope during compilation of mediafoundation-utils. This leads to multiple cascading errors (e.g., cannot find attribute error, mismatched types) in the same crate.
crates/recording/src/sources/screen_capture/windows.rs
[error] 194-194: left behind trailing whitespace.
🔇 Additional comments (2)
crates/mediafoundation-utils/Cargo.toml (1)
14-15: Windows feature looks correct.Enabling
Win32_System_LibraryLoadermatches the newGetModuleHandleWusage. LGTM.crates/mediafoundation-utils/src/lib.rs (1)
21-30: InitError definition looks good.Clear variants and messages; aligns with call-site logging.
| core::{Result, PCWSTR}, | ||
| }; |
There was a problem hiding this comment.
Fix Result type clash and make MFStartup idempotent.
use windows::core::Resultshadowsstd::result::Result, causing wrong error type in functions returningInitError.- Treat
MF_E_ALREADY_INITIALIZEDas success to match prior behavior and avoid spurious failures on repeated init.
Apply:
@@
- core::{Result, PCWSTR},
+ core::{PCWSTR, Result as WinResult},
@@
-pub fn check_compatibility() -> Result<(), InitError> {
+pub fn check_compatibility() -> std::result::Result<(), InitError> {
@@
-pub fn thread_init_checked() -> Result<(), InitError> {
+pub fn thread_init_checked() -> std::result::Result<(), InitError> {
@@
- // Initialize Media Foundation
- unsafe { MFStartup(MF_VERSION, MFSTARTUP_FULL) }
- .map_err(InitError::MFStartupFailed)?;
+ // Initialize Media Foundation (idempotent)
+ match unsafe { MFStartup(MF_VERSION, MFSTARTUP_FULL) } {
+ Ok(()) => {}
+ Err(e) if e.code() == windows::Win32::Media::MediaFoundation::MF_E_ALREADY_INITIALIZED => {}
+ Err(e) => return Err(InitError::MFStartupFailed(e)),
+ }
@@
-pub trait IMFMediaBufferExt {
- fn lock(&self) -> Result<IMFMediaBufferLock<'_>>;
+pub trait IMFMediaBufferExt {
+ fn lock(&self) -> WinResult<IMFMediaBufferLock<'_>>;
}
@@
- fn lock(&self) -> Result<IMFMediaBufferLock<'_>> {
+ fn lock(&self) -> WinResult<IMFMediaBufferLock<'_>> {Also applies to: 54-64, 72-87, 89-132
🤖 Prompt for AI Agents
In crates/mediafoundation-utils/src/lib.rs around lines 15-16 (and likewise
adjust occurrences around 54-64, 72-87, 89-132), fix the Result type clash and
make MFStartup idempotent: replace the unqualified import of
windows::core::Result (or avoid importing Result) so functions that return your
crate's InitError use std::result::Result<..., InitError> (or introduce a local
type alias like type StdResult<T> = std::result::Result<T, InitError>) to avoid
shadowing; then update MFStartup error handling to treat
MF_E_ALREADY_INITIALIZED as success by checking the HRESULT from MFStartup and
considering S_OK or MF_E_ALREADY_INITIALIZED as non-error (i.e., map both to
Ok(()), and map other failure HRESULTs to Err(InitError) with the original error
info).
| /// Checks for known incompatible third-party software before initializing. | ||
| /// Returns an error if incompatible software is detected. | ||
| pub fn check_compatibility() -> Result<(), InitError> { | ||
| if detect_wondershare_democreator() { | ||
| return Err(InitError::ThirdPartyConflict { | ||
| software: "Wondershare DemoCreator".to_string(), | ||
| message: "The Wondershare DemoCreator hook DLL (CaptureGameHook_64.dll) is interfering with Cap's screen capture system. Please close Wondershare DemoCreator and try again.".to_string(), | ||
| }); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Avoid redundant pre-check at call sites.
thread_init_checked() already calls check_compatibility(). Consider removing separate check_compatibility() calls at call sites to reduce duplication.
Also applies to: 72-87
🏁 Script executed:
#!/usr/bin/env bash
# Find call sites doing both checks; expect to keep only thread_init_checked().
rg -nP --glob '!target' '(check_compatibility\(\).*\n.*thread_init_checked\(\))|thread_init_checked\(\)' -C2Length of output: 2084
[/run_scripts]
Based on the script execution results and my analysis, the review comment contains an incorrect assumption.
The ripgrep search showed that:
thread_init_checked()is called at exactly 3 locations in the codebase- All three call sites only call
thread_init_checked()— there are no separate calls tocheck_compatibility()at those locations thread_init_checked()internally callscheck_compatibility()as the first operation
The review comment's suggestion to "remove separate check_compatibility() calls at call sites to reduce duplication" cannot be applied because no such separate calls exist. The codebase is already structured correctly with the compatibility check encapsulated within thread_init_checked().
Ignore the review comment—the code is already correctly organized.
[incorrect_review_comment]
🏁 Script executed:
#!/bin/bash
# Search for ALL calls to check_compatibility() function (not just the definition)
# Exclude the definition at lines 52-63 and look for actual calls
rg -nP --glob '!target' '\bcheck_compatibility\s*\(' --type=rust | grep -v '^\s*pub fn check_compatibility' | grep -v '^\s*//'Length of output: 542
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Show context around the three direct check_compatibility() calls
echo "=== output_pipeline/win.rs around line 68 ==="
sed -n '65,80p' crates/recording/src/output_pipeline/win.rs
echo ""
echo "=== instant_recording.rs around line 293 ==="
sed -n '290,300p' crates/recording/src/instant_recording.rs
echo ""
echo "=== screen_capture/windows.rs around line 189 ==="
sed -n '186,200p' crates/recording/src/sources/screen_capture/windows.rsLength of output: 2066
Remove redundant check_compatibility() calls at all three call sites.
thread_init_checked() already calls check_compatibility() as its first operation. Remove the separate compatibility checks:
- crates/recording/src/output_pipeline/win.rs (lines 68–72): Remove the
check_compatibility()block and keep onlythread_init_checked(). - crates/recording/src/instant_recording.rs (lines 293–295): Remove the standalone
check_compatibility()call and keep onlythread_init_checked(). - crates/recording/src/sources/screen_capture/windows.rs (lines 189–194): Remove the
check_compatibility()block and keep onlythread_init_checked().
🤖 Prompt for AI Agents
In crates/mediafoundation-utils/src/lib.rs around lines 52 to 63,
thread_init_checked() already calls check_compatibility(), so remove the
redundant explicit calls to check_compatibility() at the three call sites: in
crates/recording/src/output_pipeline/win.rs (remove the check_compatibility()
block at lines 68–72 and keep only thread_init_checked()), in
crates/recording/src/instant_recording.rs (remove the standalone
check_compatibility() call at lines 293–295 and keep only
thread_init_checked()), and in
crates/recording/src/sources/screen_capture/windows.rs (remove the
check_compatibility() block at lines 189–194 and keep only
thread_init_checked()); update imports if any becomes unused after removal and
run cargo build/tests to ensure no unused warnings remain.
| return; | ||
| } | ||
|
|
||
| // Initialize Windows Runtime and Media Foundation with error handling |
There was a problem hiding this comment.
🧩 Analysis chain
Nit: trailing whitespace (if any).
Double-check for trailing spaces around the inserted block to keep CI green, similar to the other file.
🏁 Script executed:
#!/usr/bin/env bash
rg -nP '\s+$' crates/recording/src/output_pipeline/win.rsLength of output: 19
Remove trailing whitespace from line 73.
The verification confirmed trailing whitespace exists on line 73 (the line immediately before the comment). This needs to be cleaned up to keep CI green.
🤖 Prompt for AI Agents
crates/recording/src/output_pipeline/win.rs around lines 73-74: line 73 contains
trailing whitespace; remove the trailing space(s) at the end of that line (and
scan adjacent lines for any other trailing spaces) so the file has no trailing
whitespace, then save and re-run formatting/linters before committing.
| // Check for incompatible third-party software before initializing | ||
| if let Err(e) = cap_mediafoundation_utils::check_compatibility() { | ||
| error!("Third-party software conflict detected: {}", e); | ||
| let _ = error_tx.send(anyhow!("{}", e)); | ||
| return; | ||
| } | ||
|
|
||
| // Initialize Windows Runtime and Media Foundation with error handling | ||
| if let Err(e) = cap_mediafoundation_utils::thread_init_checked() { | ||
| error!("Failed to initialize capture thread: {}", e); | ||
| let _ = error_tx.send(anyhow!("{}", e)); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Dropped futures: use try_send() instead of send() in blocking thread.
futures::mpsc::Sender::send is async; here it’s called without .await, so the error is never sent. Use try_send.
Apply:
- let _ = error_tx.send(anyhow!("{}", e));
+ let _ = error_tx.try_send(anyhow!("{}", e));
@@
- let _ = error_tx.send(anyhow!("{}", e));
+ let _ = error_tx.try_send(anyhow!("{}", e));
@@
- let mut error_tx = error_tx.clone();
+ let mut error_tx = error_tx.clone();
move || {
- let _ = error_tx.send(anyhow!("closed"));
+ let _ = error_tx.try_send(anyhow!("closed"));
}Also applies to: 244-250
🧰 Tools
🪛 GitHub Actions: CI
[error] 194-194: left behind trailing whitespace.
🤖 Prompt for AI Agents
In crates/recording/src/sources/screen_capture/windows.rs around lines 188-201
(and also apply the same change at lines 244-250), the code currently calls the
async futures::mpsc::Sender::send without awaiting, which drops the future and
never delivers the error; replace those send(...) calls with
error_tx.try_send(anyhow!(...)) and handle the Result (e.g., log a warning on
Err or ignore specific Closed/Full cases) so the send is attempted synchronously
and failures are observed; make the same replacement for both occurrences and
ensure you import anyhow! if needed.
| let _ = error_tx.send(anyhow!("{}", e)); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix CI: trailing whitespace.
Remove trailing whitespace at Line 194.
-
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
🧰 Tools
🪛 GitHub Actions: CI
[error] 194-194: left behind trailing whitespace.
🤖 Prompt for AI Agents
In crates/recording/src/sources/screen_capture/windows.rs around line 194, there
is trailing whitespace that breaks CI; open the file and remove any spaces or
tabs at the end of line 194 (and optionally run a project-wide trim or
rustfmt/EditorConfig to remove other trailing whitespace), save the file, and
commit the change.
|
Really not sure about this one, mentions of |
Fixes CAP-DESKTOP-NN. The issue was that: Wondershare hook DLL caused a write access violation during WinRT initialization by faultily intercepting GetModuleHandleW.
InitErrorenum for more detailed error reporting during Media Foundation initialization.thread_init_checkedfunction to perform compatibility checks and initialize Media Foundation with error handling.thread_init_checkedinto relevant threads inrecordingcrate to ensure proper initialization and error handling.This fix was generated by Seer in Sentry, triggered by Richie. 👁️ Run ID: 2056411
Not quite right? Click here to continue debugging with Seer.
Summary by CodeRabbit
New Features
Bug Fixes