Add MSMF backend on Windows with DirectShow fallback#48
Open
Add MSMF backend on Windows with DirectShow fallback#48
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a Windows Media Foundation (MSMF) camera backend with automatic DirectShow fallback, backend selection via extraInfo and CCAP_WINDOWS_BACKEND, Rust API extensions for extra_info, build/link updates for MSMF, comprehensive docs, and a Windows-focused test suite. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Provider
participant Selector as Windows Backend<br/>Selector
participant MSMFFactory as MSMF Factory
participant DShowFactory as DirectShow<br/>Factory
participant MSMFImpl as MSMF Backend
participant DShowImpl as DirectShow<br/>Backend
Client->>Provider: open(deviceName, extraInfo)
Provider->>Selector: resolveWindowsBackendPreference(extraInfo/env)
Selector-->>Provider: BackendPreference
Provider->>Selector: isMediaFoundationCameraBackendAvailable()
Selector-->>Provider: available?
alt Preference == MSMF
Provider->>MSMFFactory: createProviderMSMF()
MSMFFactory->>MSMFImpl: instantiate
Provider->>MSMFImpl: open(deviceName)
MSMFImpl-->>Provider: success/failure
else Preference == Auto
Provider->>MSMFFactory: createProviderMSMF() [if available]
MSMFFactory->>MSMFImpl: instantiate
MSMFImpl->>Provider: open result
alt MSMF open fails
Provider->>DShowFactory: createProviderDirectShow()
DShowFactory->>DShowImpl: instantiate
Provider->>DShowImpl: open(deviceName)
DShowImpl-->>Provider: success/failure
end
else Preference == DirectShow
Provider->>DShowFactory: createProviderDirectShow()
DShowFactory->>DShowImpl: instantiate
Provider->>DShowImpl: open(deviceName)
DShowImpl-->>Provider: success/failure
end
Provider-->>Client: final open result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new Windows camera backend based on Media Foundation (MSMF), makes Windows default to MSMF with DirectShow fallback, and updates tests/bindings/docs to cover and expose backend selection.
Changes:
- Implement MSMF capture backend (
ProviderMSMF) and integrate Windows backend selection + fallback logic inProvider. - Add Windows backend selection/comparison tests and wire them into the CMake test suite.
- Document backend selection across README/site docs/CLI help and expose extra-info helpers in Rust bindings.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_windows_backends.cpp | New Windows backend selection + MSMF vs DirectShow comparison tests |
| tests/CMakeLists.txt | Builds and registers the new Windows backend test executable |
| src/ccap_imp_windows_msmf.h | Declares the MSMF provider implementation |
| src/ccap_imp_windows_msmf.cpp | Implements MSMF device enumeration, format negotiation, and frame capture loop |
| src/ccap_core.cpp | Adds Windows backend selection, MSMF availability probing, and fallback open/enumeration logic |
| include/ccap_def.h | Documents nativeHandle type for MSMF frames |
| include/ccap_core.h | Documents extraInfo backend hint and adds state-caching members for fallback switching |
| include/ccap_c.h | Documents extraInfo backend hint for the C API |
| docs/index.html | Updates Windows backend marketing text to MSMF + DirectShow fallback |
| docs/content/rust-bindings.zh.md | Documents Windows backend selection for Rust (ZH) |
| docs/content/rust-bindings.md | Documents Windows backend selection for Rust (EN) |
| docs/content/implementation-details.zh.md | Adds MSMF backend details (ZH) |
| docs/content/implementation-details.md | Adds MSMF backend details (EN) |
| docs/content/documentation.zh.md | Documents Windows default + override options (ZH) |
| docs/content/documentation.md | Documents Windows default + override options (EN) |
| docs/content/cli.zh.md | Documents CCAP_WINDOWS_BACKEND override for CLI (ZH) |
| docs/content/cli.md | Documents CCAP_WINDOWS_BACKEND override for CLI (EN) |
| cli/args_parser.cpp | Adds CLI help line for CCAP_WINDOWS_BACKEND |
| bindings/rust/src/provider.rs | Adds Rust APIs that accept optional extra_info and validates C strings |
| bindings/rust/README.md | Updates Rust README for MSMF + fallback and backend override |
| README.zh-CN.md | Updates top-level docs for MSMF + fallback and backend selection (ZH) |
| README.md | Updates top-level docs for MSMF + fallback and backend selection (EN) |
| CMakeLists.txt | Links Media Foundation libs on Windows and adjusts delay-load behavior |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bindings/rust/src/provider.rs (2)
273-294:⚠️ Potential issue | 🔴 CriticalDo not free the current provider before validating fallible operations.
CString::new(name)andoptional_c_string(extra_info, ...)can fail afterccap_provider_destroy(self.handle)has already run. This leavesself.handledangling andself.is_openedstale. When the Provider is later dropped, the destructor will attempt to destroy an already-freed handle, causing a double-free.Validate input parameters first, then destroy the old provider, and explicitly reset
self.handleandself.is_opened:Safer ordering for open_device_with_extra_info
if let Some(name) = device_name { + let c_name = CString::new(name).map_err(|_| { + CcapError::InvalidParameter("device name contains null byte".to_string()) + })?; + let extra_info = optional_c_string(extra_info, "extra info")?; + // Recreate provider with specific device if !self.handle.is_null() { // If the previous provider was running, stop it and detach callbacks // before destroying the underlying handle. let _ = self.stop_capture(); let _ = self.remove_new_frame_callback(); self.cleanup_callback(); unsafe { sys::ccap_provider_destroy(self.handle); } + self.handle = ptr::null_mut(); + self.is_opened = false; } - let c_name = CString::new(name).map_err(|_| { - CcapError::InvalidParameter("device name contains null byte".to_string()) - })?; - let extra_info = optional_c_string(extra_info, "extra info")?; self.handle = unsafe { sys::ccap_provider_create_with_device(Safer ordering for open_with_index_and_extra_info
+ let extra_info = optional_c_string(extra_info, "extra info")?; + if !self.handle.is_null() { let _ = self.stop_capture(); let _ = self.remove_new_frame_callback(); self.cleanup_callback(); unsafe { sys::ccap_provider_destroy(self.handle); } + self.handle = ptr::null_mut(); + self.is_opened = false; } else { // Clean up any stale callback allocation even if handle is null. self.cleanup_callback(); } - - let extra_info = optional_c_string(extra_info, "extra info")?;Also applies to: 584-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/rust/src/provider.rs` around lines 273 - 294, Validate/convert inputs before destroying the existing provider handle: call CString::new(name) and optional_c_string(extra_info, ...) and check for errors first, then only after successful conversions destroy the old handle (sys::ccap_provider_destroy), assign the new handle to self.handle, and update self.is_opened; when you do destroy the old provider, explicitly reset self.handle to ptr::null_mut() and self.is_opened = false to avoid a dangling handle. Apply the same ordering/fix pattern to the analogous function open_with_index_and_extra_info (and the code region referenced at 584-599): perform fallible conversions up front, only destroy the previous provider after success, and ensure handle/is_opened are updated/reset appropriately.
289-306:⚠️ Potential issue | 🔴 CriticalThe
auto_start=falseparameter is silently ignored in both code paths.The C++
Providerconstructors called byccap_provider_create_with_deviceandccap_provider_create_with_indexinvokeopen(deviceName)andopen(deviceIndex)without specifying theautoStartparameter (src/ccap_core.cpp lines 313, 324). Since the C++open()method defaults toautoStart = true(include/ccap_core.h:100, 109), these constructors always start capture immediately.The Rust wrapper then gates
start_capture()on theauto_startparameter, incorrectly assuming the device is opened but not capturing. Whenauto_start=false, capture is already running but the parameter value is ignored.Required fix: Either:
- Pass
falseto the C++open()call in constructors, or- Add an
autoStartparameter to the C API functions, or- Call
stop_capture()in the Rust wrapper whenauto_start=falseafter the create call succeeds🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/rust/src/provider.rs` around lines 289 - 306, The Rust wrapper is ignoring auto_start because the C++ constructors invoked by ccap_provider_create_with_device / ccap_provider_create_with_index open the device with autoStart=true; after creating/ opening the provider (in the branch that calls ccap_provider_create_with_device and in the paths that call open_with_index_and_extra_info or open) detect when auto_start == false and call self.stop_capture() once the handle is valid and self.is_opened is set (so stop_capture undoes the C++ default start); add this conditional stop after the successful create/open code paths before returning so the Rust auto_start parameter is honored.
🧹 Nitpick comments (2)
include/ccap_core.h (1)
241-258: This changesccap::Provider's public C++ ABI.Adding private fields to an exported class changes its size/layout. If you ship shared binaries, make sure release/package versioning prevents old headers from being mixed with the new library, or force downstream rebuilds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/ccap_core.h` around lines 241 - 258, The change adds new private data members (m_extraInfo, m_frameCallback, m_allocatorFactory, m_maxAvailableFrameSize, m_maxCacheFrameSize, m_requestedWidth, m_requestedHeight, m_requestedFrameRate, m_requestedInternalFormat, m_requestedOutputFormat, m_hasFrameOrientationOverride, m_requestedFrameOrientation) to the exported ccap::Provider class which breaks the public C++ ABI; fix by preserving the original class size/layout—either revert these members into a Pimpl/opaque pointer (move them into an internal Impl struct and add a single std::unique_ptr<void> or std::unique_ptr<Impl> member) or keep the ABI by replacing the concrete new members with an appropriately sized reserved padding field (e.g. a char m_reserved[N] or void* m_reserved[NUM]) sized to accommodate future additions and then move the actual new members into the private Impl; update the class to use the Impl accessor or reinterpret the reserved area internally, and add a comment referencing the reserved field to prevent accidental removal.src/ccap_imp_windows_msmf.h (1)
7-9: Redundant include guards.Using both
#pragma onceand traditional#ifndefguards is redundant. Modern compilers universally support#pragma once, so you could simplify by removing lines 8-9 and the corresponding#endifon line 99.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ccap_imp_windows_msmf.h` around lines 7 - 9, The header uses both `#pragma` once and traditional include guards; remove the redundant traditional guards by deleting the `#ifndef` CAMERA_CAPTURE_MSMF_H and `#define` CAMERA_CAPTURE_MSMF_H near the top and the matching `#endif` at the end of the file so only `#pragma` once remains (leave all other declarations and symbols intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 254-261: The /DELAYLOAD linker flags are currently set as PRIVATE
on target ccap so they won't propagate to consumers when ccap is a static
library; change the target_link_options invocation for the /DELAYLOAD entries
from PRIVATE to PUBLIC (or INTERFACE if you prefer only consumers to get them)
so downstream binaries receive the delay-load directives, and keep
target_link_libraries(ccap PUBLIC delayimp.lib) as-is so the delayimp.lib
dependency is also propagated.
In `@docs/content/cli.zh.md`:
- Around line 74-75: The added line is in Chinese and violates the rule that
docs/ must be English; either translate this sentence into English preserving
the environment variable and values (e.g., mention
CCAP_WINDOWS_BACKEND=auto|msmf|dshow and that it forces Media Foundation or
DirectShow on Windows) or move the localized Chinese variant out of the docs/
tree (e.g., into an i18n or localized folder) and leave an English version in
place; update the sentence that mentions CCAP_WINDOWS_BACKEND accordingly and
submit the change for docs review.
In `@docs/content/documentation.zh.md`:
- Around line 222-225: The new Windows guidance in
docs/content/documentation.zh.md is written in Chinese which violates the rule
that docs/ Markdown must be English; either translate the Chinese paragraph into
English (preserving the exact technical options like "extraInfo", the backend
values `auto`, `msmf`, `dshow`, and the environment variable
`CCAP_WINDOWS_BACKEND=auto|msmf|dshow`) or move this localized content out of
the docs/ tree into a locale-specific directory; update the file so the docs/
version contains the English text describing Media Foundation defaulting,
DirectShow fallback, MSVC 2019+ requirement, and how to force a backend via
`extraInfo` or `CCAP_WINDOWS_BACKEND`.
In `@docs/content/implementation-details.md`:
- Around line 218-221: The docs currently say Media Foundation "automatically
falls back to DirectShow" unconditionally; update the text to clarify the
fallback only occurs when backend selection is set to "auto". Mention that when
callers explicitly request "msmf" (via extraInfo or CCAP_WINDOWS_BACKEND)
Provider::open() will error if MSMF is unavailable or open fails and will not
fall back to DirectShow. Reference the symbols msmf, extraInfo,
CCAP_WINDOWS_BACKEND, Provider::open(), and "auto" so readers know the exact
behavior.
In `@docs/content/implementation-details.zh.md`:
- Around line 201-205: The docs/ entry contains a Chinese section titled
"**Media Foundation 后端：**" which violates the rule that documentation must be in
English; replace this localized paragraph with an English translation (or
remove/move the Chinese copy out of the docs/ tree) and update the PR so the
file contains an English version explaining that Media Foundation is the
preferred modern Windows backend using Source Reader for frame capture and
format negotiation and falls back to DirectShow when unavailable or device open
fails; keep the original Chinese text stored elsewhere if needed and ensure the
changed section header and bullets are in English.
In `@README.zh-CN.md`:
- Around line 184-187: The examples call the Provider constructor twice which
opens the default camera for both msmfProvider and dshowProvider simultaneously;
update the README by making the backend examples mutually exclusive—either wrap
each example in its own scope with explicit cleanup/RAII so the first Provider
is destroyed before creating the second, or present them as alternative usage
snippets (choose one backend per example) and add a note that Provider() opens
the camera immediately and backends must not be instantiated concurrently for
the same device; reference the Provider constructor and the
msmfProvider/dshowProvider examples so maintainers can locate and fix the
snippet.
In `@src/ccap_imp_windows_msmf.cpp`:
- Line 734: On the non-zero-copy code paths where the COM sample is
converted/flipped and released, clear newFrame->nativeHandle (set to nullptr) so
it cannot point to the released sample; locate the branches that perform
conversion/flip (the paths where sample is released instead of retained) and
ensure after creating/copying the frame data you do not leave
newFrame->nativeHandle = sample — explicitly null it and only assign the sample
to nativeHandle on the zero-copy/retain path where the sample lifetime is
guaranteed.
- Around line 670-675: In ProviderMSMF::readLoop() do not reset m_shouldStop at
the start of the worker — remove the line that assigns m_shouldStop = false so a
stop request issued after thread creation but before worker entry is not lost;
rely on start() to initialize m_shouldStop and allow stop()/close() to signal
the worker (which will then unblock from ReadSample()) without being overwritten
by readLoop().
- Around line 39-46: The wideToUtf8() implementation doesn't validate the second
WideCharToMultiByte() call and relies on implicit std::string sizing; change it
to allocate a std::string buffer of size 'length' (or length-1 for the non-null
bytes) explicitly, call WideCharToMultiByte into value.data(), capture its
return (e.g., bytesWritten), check for zero to detect failure, resize the
std::string to bytesWritten (or bytesWritten-1 if you want to drop the trailing
NUL), and return an empty optional or throw/log on failure; reference the
function wideToUtf8, the length variable returned by the first
WideCharToMultiByte, the second WideCharToMultiByte call, and the value
std::string for locating the changes.
In `@tests/test_windows_backends.cpp`:
- Around line 108-123: The helper listCommonDevices currently compares raw lists
from listDevicesForBackend("dshow") and listDevicesForBackend("msmf") using
friendly names, which can include duplicates for identical webcams; change it to
first deduplicate each backend's device list (e.g., convert dshowDevices and
msmfDevices into sets or use std::unordered_set of names) then compute the
intersection so only unique device names present in both backends are returned;
keep the final ordering by applying the existing stable_sort with
virtualCameraRank on the deduplicated intersection before returning from
listCommonDevices.
---
Outside diff comments:
In `@bindings/rust/src/provider.rs`:
- Around line 273-294: Validate/convert inputs before destroying the existing
provider handle: call CString::new(name) and optional_c_string(extra_info, ...)
and check for errors first, then only after successful conversions destroy the
old handle (sys::ccap_provider_destroy), assign the new handle to self.handle,
and update self.is_opened; when you do destroy the old provider, explicitly
reset self.handle to ptr::null_mut() and self.is_opened = false to avoid a
dangling handle. Apply the same ordering/fix pattern to the analogous function
open_with_index_and_extra_info (and the code region referenced at 584-599):
perform fallible conversions up front, only destroy the previous provider after
success, and ensure handle/is_opened are updated/reset appropriately.
- Around line 289-306: The Rust wrapper is ignoring auto_start because the C++
constructors invoked by ccap_provider_create_with_device /
ccap_provider_create_with_index open the device with autoStart=true; after
creating/ opening the provider (in the branch that calls
ccap_provider_create_with_device and in the paths that call
open_with_index_and_extra_info or open) detect when auto_start == false and call
self.stop_capture() once the handle is valid and self.is_opened is set (so
stop_capture undoes the C++ default start); add this conditional stop after the
successful create/open code paths before returning so the Rust auto_start
parameter is honored.
---
Nitpick comments:
In `@include/ccap_core.h`:
- Around line 241-258: The change adds new private data members (m_extraInfo,
m_frameCallback, m_allocatorFactory, m_maxAvailableFrameSize,
m_maxCacheFrameSize, m_requestedWidth, m_requestedHeight, m_requestedFrameRate,
m_requestedInternalFormat, m_requestedOutputFormat,
m_hasFrameOrientationOverride, m_requestedFrameOrientation) to the exported
ccap::Provider class which breaks the public C++ ABI; fix by preserving the
original class size/layout—either revert these members into a Pimpl/opaque
pointer (move them into an internal Impl struct and add a single
std::unique_ptr<void> or std::unique_ptr<Impl> member) or keep the ABI by
replacing the concrete new members with an appropriately sized reserved padding
field (e.g. a char m_reserved[N] or void* m_reserved[NUM]) sized to accommodate
future additions and then move the actual new members into the private Impl;
update the class to use the Impl accessor or reinterpret the reserved area
internally, and add a comment referencing the reserved field to prevent
accidental removal.
In `@src/ccap_imp_windows_msmf.h`:
- Around line 7-9: The header uses both `#pragma` once and traditional include
guards; remove the redundant traditional guards by deleting the `#ifndef`
CAMERA_CAPTURE_MSMF_H and `#define` CAMERA_CAPTURE_MSMF_H near the top and the
matching `#endif` at the end of the file so only `#pragma` once remains (leave all
other declarations and symbols intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 226e5101-4ca9-4707-8ea7-df78e5b46c1a
📒 Files selected for processing (23)
CMakeLists.txtREADME.mdREADME.zh-CN.mdbindings/rust/README.mdbindings/rust/src/provider.rscli/args_parser.cppdocs/content/cli.mddocs/content/cli.zh.mddocs/content/documentation.mddocs/content/documentation.zh.mddocs/content/implementation-details.mddocs/content/implementation-details.zh.mddocs/content/rust-bindings.mddocs/content/rust-bindings.zh.mddocs/index.htmlinclude/ccap_c.hinclude/ccap_core.hinclude/ccap_def.hsrc/ccap_core.cppsrc/ccap_imp_windows_msmf.cppsrc/ccap_imp_windows_msmf.htests/CMakeLists.txttests/test_windows_backends.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vscode/tasks.json (1)
78-91:⚠️ Potential issue | 🟡 MinorInconsistency:
examplesdirectory not cleaned on Windows.The bash command (line 71) cleans
build,build_shared, andexamples, but the PowerShell version only cleansbuildandbuild_shared. This could leave stale artifacts inexampleson Windows.Proposed fix
- "git clean -fdx build; if ($LASTEXITCODE -ne 0 -and (Test-Path 'build')) { Remove-Item -Recurse -Force 'build' }; git clean -fdx build_shared; if ($LASTEXITCODE -ne 0 -and (Test-Path 'build_shared')) { Remove-Item -Recurse -Force 'build_shared' }" + "git clean -fdx build; if ($LASTEXITCODE -ne 0 -and (Test-Path 'build')) { Remove-Item -Recurse -Force 'build' }; git clean -fdx build_shared; if ($LASTEXITCODE -ne 0 -and (Test-Path 'build_shared')) { Remove-Item -Recurse -Force 'build_shared' }; git clean -fdx examples"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/tasks.json around lines 78 - 91, The Windows PowerShell task in .vscode/tasks.json currently cleans "build" and "build_shared" but omits "examples", causing inconsistent cleanup vs the bash task; update the PowerShell "args" command string used by the Windows entry so it also attempts to clean the "examples" directory (mirroring the bash sequence) and preserve the existing error/exit-code handling (check $LASTEXITCODE and Test-Path before Remove-Item) so cleanup behavior for examples matches the bash command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.vscode/tasks.json:
- Around line 78-91: The Windows PowerShell task in .vscode/tasks.json currently
cleans "build" and "build_shared" but omits "examples", causing inconsistent
cleanup vs the bash task; update the PowerShell "args" command string used by
the Windows entry so it also attempts to clean the "examples" directory
(mirroring the bash sequence) and preserve the existing error/exit-code handling
(check $LASTEXITCODE and Test-Path before Remove-Item) so cleanup behavior for
examples matches the bash command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b56fb4c-156f-4068-af85-81f288e05b9d
📒 Files selected for processing (2)
.gitattributes.vscode/tasks.json
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bindings/rust/src/provider.rs (2)
99-123:⚠️ Potential issue | 🟠 MajorCheck the post-construction open state before returning success.
These constructors assume that a non-null
ccap_provider_create_with_*handle means the device is open. It doesn't.src/ccap_c.cppjust returnsnew ccap::Provider(...), and the C++ constructors only callopen(...)internally; they still return a valid object when that open fails. As written, invalid device names/indexes can come back asOk(...)withis_opened = true.Also applies to: 134-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/rust/src/provider.rs` around lines 99 - 123, The constructor assumes a non-null handle implies the device is opened; instead, after creating the handle via sys::ccap_provider_create_with_index (and similarly for the other creators around the 134-162 range), call the C API function that reports open state (e.g., sys::ccap_provider_is_open or equivalent) to verify the provider actually opened the device, set Provider.is_opened based on that result, and return an Err (e.g., CcapError::InvalidDevice with the device identifier) when the provider is not open rather than returning Ok(...) with is_opened = true.
274-319:⚠️ Potential issue | 🟠 MajorMake the reopen path transactional.
Both helpers destroy the current provider before the replacement handle is created and confirmed opened. If the new open fails, the caller loses the current stream entirely; if it succeeds, every Rust-side callback/configuration is still dropped, which bypasses the cached-state handoff the C++
Providernow does during backend switches. Create and validate the replacement first, then swap it in, or expose an FFI open-with-extra-info path on the existing provider.Also applies to: 590-639
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/rust/src/provider.rs` around lines 274 - 319, The reopen logic currently destroys the existing provider (self.handle) and drops Rust-side callbacks/config before creating the new handle, risking loss of stream if creation fails and bypassing C++ cached-state handoff; change this to a transactional swap: first call the FFI create function (sys::ccap_provider_create_with_device or equivalent) into a temporary local handle, validate it (ensure it's non-null and opened), then atomically swap temp into self.handle, update self.is_opened, and only then call cleanup_callback(), stop_capture(), remove_new_frame_callback() on the old handle and destroy it (sys::ccap_provider_destroy); alternatively, add/use an FFI path that performs open-with-extra-info on the existing provider to avoid full replacement. Ensure the methods open_with_index_and_extra_info, start_capture, stop_capture, cleanup_callback, remove_new_frame_callback and the handle/is_opened state are updated consistently and errors are handled without leaving the instance without a valid handle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/rust/build.rs`:
- Around line 199-200: The Windows rebuild inputs in build.rs are missing a
cargo:rerun-if-changed entry for the new MSMF source file
ccap_imp_windows_msmf.cpp; update the section that emits Cargo rebuild hints
(where other Windows sources are registered) to add a matching
cargo:rerun-if-changed line for ccap_imp_windows_msmf.cpp so edits to that file
trigger a rebuild (look for the block that currently lists
ccap_file_reader_windows.cpp and add the new file there).
- Around line 372-379: Add the two missing Windows libs and the delay-load
configuration to build.rs so the Rust link behavior matches CMake: emit rustc
linker directives to link shlwapi and propsys (used by
ccap_file_reader_windows.cpp), add the delay-load linker arguments for mf.dll,
mfplat.dll, mfreadwrite.dll (e.g. /DELAYLOAD:mf.dll etc.), and ensure
delayimp.lib is linked so delay-load support is available; this preserves the
runtime probeLibraryExport fallback in src/ccap_core.cpp by preventing eager
loading of the Media Foundation DLLs.
In `@src/ccap_imp_windows_msmf.cpp`:
- Around line 447-467: The current stride logic throws away a negative
MF_MT_DEFAULT_STRIDE and replaces it with a positive computed width-based
stride; instead preserve the sign returned by
currentType->GetUINT32(MF_MT_DEFAULT_STRIDE) by assigning stride =
static_cast<LONG>(strideValue) when GetUINT32 succeeds (including negative
values), then propagate that value to m_activeStride0; update the consumer of
m_activeStride0 (the zero-copy path) to detect m_activeStride0 < 0 and either
flip the image orientation or force a copy+flip normalization step so bottom-up
frames are rendered correctly; ensure references to info.pixelFormat and
computed fallbacks (width * bpp) are only used when no stride value was
provided.
In `@tests/test_windows_backends.cpp`:
- Around line 71-79: The findProjectRoot function can loop forever on Windows
because parent_path() stops changing at the drive root; modify the loop to stop
when you reach the filesystem root by comparing projectRoot to
projectRoot.root_path() (or detecting when parent_path() equals the current
path) and return {} if not found; update the while condition or add a check
inside the loop referencing findProjectRoot, fs::current_path(),
projectRoot.parent_path(), and projectRoot.root_path() so the function breaks
and returns {} when the root is reached.
---
Outside diff comments:
In `@bindings/rust/src/provider.rs`:
- Around line 99-123: The constructor assumes a non-null handle implies the
device is opened; instead, after creating the handle via
sys::ccap_provider_create_with_index (and similarly for the other creators
around the 134-162 range), call the C API function that reports open state
(e.g., sys::ccap_provider_is_open or equivalent) to verify the provider actually
opened the device, set Provider.is_opened based on that result, and return an
Err (e.g., CcapError::InvalidDevice with the device identifier) when the
provider is not open rather than returning Ok(...) with is_opened = true.
- Around line 274-319: The reopen logic currently destroys the existing provider
(self.handle) and drops Rust-side callbacks/config before creating the new
handle, risking loss of stream if creation fails and bypassing C++ cached-state
handoff; change this to a transactional swap: first call the FFI create function
(sys::ccap_provider_create_with_device or equivalent) into a temporary local
handle, validate it (ensure it's non-null and opened), then atomically swap temp
into self.handle, update self.is_opened, and only then call cleanup_callback(),
stop_capture(), remove_new_frame_callback() on the old handle and destroy it
(sys::ccap_provider_destroy); alternatively, add/use an FFI path that performs
open-with-extra-info on the existing provider to avoid full replacement. Ensure
the methods open_with_index_and_extra_info, start_capture, stop_capture,
cleanup_callback, remove_new_frame_callback and the handle/is_opened state are
updated consistently and errors are handled without leaving the instance without
a valid handle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: adbc4ddc-fe05-408f-a70c-f644f7a4d059
📒 Files selected for processing (9)
CMakeLists.txtREADME.zh-CN.mdbindings/rust/build.rsbindings/rust/src/provider.rsdocs/content/implementation-details.mdinclude/ccap_core.hsrc/ccap_core.cppsrc/ccap_imp_windows_msmf.cpptests/test_windows_backends.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/content/implementation-details.md
- CMakeLists.txt
- README.zh-CN.md
b6e857a to
7cde5e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/content/documentation.zh.md (1)
222-224:⚠️ Potential issue | 🟠 MajorKeep
docs/content in English.The new Windows backend guidance (lines 222–224) is written in Chinese, which violates the repository rule. Please translate this section to English or move the localized version out of the
docs/tree.As per coding guidelines,
.mdfiles indocs/must be English and require review before publishing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/documentation.zh.md` around lines 222 - 224, The documentation contains a Chinese paragraph describing Windows camera backend behavior; replace that localized text with an English translation or move the Chinese version out of the docs/ tree so docs/ remains English-only. Specifically update the Windows backend guidance paragraph that mentions Media Foundation, DirectShow fallback, MSVC requirement, and the ways to force a backend (extraInfo values `auto`, `msmf`, `dshow`, or `backend=<value>` and env var `CCAP_WINDOWS_BACKEND=auto|msmf|dshow`) to be written in English and submit it for review.tests/test_windows_backends.cpp (1)
71-80:⚠️ Potential issue | 🟠 MajorStop the parent walk when you hit the filesystem root.
On Windows,
parent_path()of"C:\"returns"C:\"(unchanged), andhas_parent_path()still returnstrue. This causes an infinite loop if the test runs from outside the repo.Proposed fix
fs::path findProjectRoot() { fs::path projectRoot = fs::current_path(); - while (projectRoot.has_parent_path()) { + while (!projectRoot.empty()) { if (fs::exists(projectRoot / "CMakeLists.txt") && fs::exists(projectRoot / "tests")) { return projectRoot; } - projectRoot = projectRoot.parent_path(); + fs::path parent = projectRoot.parent_path(); + if (parent == projectRoot) { + break; + } + projectRoot = std::move(parent); } return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_windows_backends.cpp` around lines 71 - 80, The loop in findProjectRoot() can become infinite on Windows because parent_path() can equal the current path for roots like "C:\", so change the traversal to detect when the parent does not change (or when we've reached filesystem root) and break: in the while loop compute fs::path parent = projectRoot.parent_path(), if parent == projectRoot (or parent.empty()) then stop the loop and return {} (or break), otherwise proceed to check for "CMakeLists.txt" and "tests" and then set projectRoot = parent; this ensures findProjectRoot() exits at the filesystem root and prevents infinite looping.
🧹 Nitpick comments (1)
src/ccap_core.cpp (1)
244-255: Unreachable code after switch statement.All enum values are handled in the switch cases, making the
return createProviderDirectShow();on line 254 unreachable. Consider removing it or adding adefaultcase to the switch for defensive coding.♻️ Suggested refactor
ProviderImp* createWindowsProvider(WindowsBackendPreference preference) { switch (preference) { case WindowsBackendPreference::MSMF: return isMediaFoundationCameraBackendAvailable() ? createProviderMSMF() : nullptr; case WindowsBackendPreference::DirectShow: return createProviderDirectShow(); case WindowsBackendPreference::Auto: return isMediaFoundationCameraBackendAvailable() ? createProviderMSMF() : createProviderDirectShow(); + default: + break; } return createProviderDirectShow(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ccap_core.cpp` around lines 244 - 255, The final unconditional return in createWindowsProvider is unreachable because all WindowsBackendPreference enum cases are covered; update createWindowsProvider to be defensive by replacing that trailing return with either an explicit default: case in the switch that returns createProviderDirectShow() (or logs/asserts) or add a default: branch that returns createProviderDirectShow(); ensure the function signature and behavior remain the same and reference createWindowsProvider, WindowsBackendPreference, createProviderDirectShow, createProviderMSMF, and isMediaFoundationCameraBackendAvailable when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ccap_imp_windows_msmf.cpp`:
- Around line 447-467: The code currently discards negative MF_MT_DEFAULT_STRIDE
and normalizes stride to positive; preserve the sign from strideValue /
m_activeStride0 and handle bottom-up frames by either (preferred) setting
newFrame->stride[0] = static_cast<uint32_t>(abs(m_activeStride0)) but also set
newFrame->orientation = FrameOrientation::BottomToTop when m_activeStride0 < 0
so zero-copy consumers can interpret row order, or (alternatively) detect
m_activeStride0 < 0 and take the copy+flip path to produce a top-down buffer;
update the logic around reading MF_MT_DEFAULT_STRIDE, the assignment to
newFrame->stride[0], and the zero-copy branch to respect m_activeStride0 sign
and set FrameOrientation::BottomToTop (or force copy+flip) accordingly.
---
Duplicate comments:
In `@docs/content/documentation.zh.md`:
- Around line 222-224: The documentation contains a Chinese paragraph describing
Windows camera backend behavior; replace that localized text with an English
translation or move the Chinese version out of the docs/ tree so docs/ remains
English-only. Specifically update the Windows backend guidance paragraph that
mentions Media Foundation, DirectShow fallback, MSVC requirement, and the ways
to force a backend (extraInfo values `auto`, `msmf`, `dshow`, or
`backend=<value>` and env var `CCAP_WINDOWS_BACKEND=auto|msmf|dshow`) to be
written in English and submit it for review.
In `@tests/test_windows_backends.cpp`:
- Around line 71-80: The loop in findProjectRoot() can become infinite on
Windows because parent_path() can equal the current path for roots like "C:\",
so change the traversal to detect when the parent does not change (or when we've
reached filesystem root) and break: in the while loop compute fs::path parent =
projectRoot.parent_path(), if parent == projectRoot (or parent.empty()) then
stop the loop and return {} (or break), otherwise proceed to check for
"CMakeLists.txt" and "tests" and then set projectRoot = parent; this ensures
findProjectRoot() exits at the filesystem root and prevents infinite looping.
---
Nitpick comments:
In `@src/ccap_core.cpp`:
- Around line 244-255: The final unconditional return in createWindowsProvider
is unreachable because all WindowsBackendPreference enum cases are covered;
update createWindowsProvider to be defensive by replacing that trailing return
with either an explicit default: case in the switch that returns
createProviderDirectShow() (or logs/asserts) or add a default: branch that
returns createProviderDirectShow(); ensure the function signature and behavior
remain the same and reference createWindowsProvider, WindowsBackendPreference,
createProviderDirectShow, createProviderMSMF, and
isMediaFoundationCameraBackendAvailable when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b735bf7-8c0a-4fb5-a6eb-a220efaf5bc0
📒 Files selected for processing (25)
.vscode/tasks.jsonCMakeLists.txtREADME.mdREADME.zh-CN.mdbindings/rust/README.mdbindings/rust/build.rsbindings/rust/src/provider.rscli/args_parser.cppdocs/content/cli.mddocs/content/cli.zh.mddocs/content/documentation.mddocs/content/documentation.zh.mddocs/content/implementation-details.mddocs/content/implementation-details.zh.mddocs/content/rust-bindings.mddocs/content/rust-bindings.zh.mddocs/index.htmlinclude/ccap_c.hinclude/ccap_core.hinclude/ccap_def.hsrc/ccap_core.cppsrc/ccap_imp_windows_msmf.cppsrc/ccap_imp_windows_msmf.htests/CMakeLists.txttests/test_windows_backends.cpp
✅ Files skipped from review due to trivial changes (1)
- cli/args_parser.cpp
🚧 Files skipped from review as they are similar to previous changes (15)
- docs/content/rust-bindings.md
- docs/content/cli.md
- docs/content/rust-bindings.zh.md
- docs/content/cli.zh.md
- bindings/rust/build.rs
- .vscode/tasks.json
- README.md
- bindings/rust/src/provider.rs
- include/ccap_def.h
- docs/content/implementation-details.zh.md
- tests/CMakeLists.txt
- docs/content/implementation-details.md
- docs/content/documentation.md
- docs/index.html
- bindings/rust/README.md
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ccap_imp_windows_msmf.cpp`:
- Around line 647-667: The code is adding converted output formats for MJPG into
DeviceInfo::supportedPixelFormats (via the hasMJPG flag), which violates the
hardware-only contract; update the logic in the mediaTypes processing (the loop
over mediaTypes and the hasMJPG handling) so that when mediaType.isCompressed
(MJPG) is detected you do NOT push BGR24/BGRA32/RGB24/RGBA32 into
info->supportedPixelFormats; instead only record the native compressed
capability (e.g., keep hasMJPG or record MJPG itself) and do not add conversion
output formats after releaseMediaTypes(mediaTypes) — modify the hasMJPG block
(and related code that currently pushes BGR24/BGRA32/RGB24/RGBA32) to stop
injecting non-native formats into supportedPixelFormats while preserving any
MJPG-native indicator needed by callers.
- Around line 152-158: The instance-level COM flags (m_didInitializeCom,
m_didSetup) lead to mismatched CoUninitialize calls if init/teardown occur on
different threads; change setup() and findDeviceNames() to perform thread-local
COM initialization instead of setting instance flags: use a thread_local boolean
(or RAII helper) to call CoInitializeEx/CoUninitialize per thread within those
functions' scope, remove reliance on m_didInitializeCom for destructor cleanup,
and ensure ProviderMSMF::~ProviderMSMF() no longer calls CoUninitialize based on
the instance flag (or assert single-threaded usage if you choose the
alternative). Locate symbols ProviderMSMF, setup(), findDeviceNames(),
m_didInitializeCom, m_didSetup, and ProviderMSMF::~ProviderMSMF to apply this
refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12457dcf-1f9b-482c-834b-c4f005b1bafd
📒 Files selected for processing (4)
bindings/rust/build.rssrc/ccap_imp_windows_msmf.cppsrc/ccap_imp_windows_msmf.htests/test_windows_backends.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/rust/build.rs
6d1a077 to
81bb3ab
Compare
2e4c2b5 to
e7ac5ba
Compare
…y issues Replace per-instance m_didInitializeCom/m_didSetup flags with a static setupCom pattern matching the DirectShow backend. Removes CoUninitialize from the destructor since static COM init is never paired with uninit, avoiding mismatched init/uninit on different threads.
e7ac5ba to
8a64c1a
Compare
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
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.
Summary
Testing
Notes
Full HD webcamcargo testfor Rust bindings is still blocked by a missinglibclang.dllrequired by bindgen in this environmentSummary by CodeRabbit
New Features
API / Bindings
CLI / Docs
Tests