feat(rust): Add high-performance Rust bindings with SIMD acceleration #44
Conversation
Co-authored-by: wysaid <1430725+wysaid@users.noreply.github.com>
…SCode tasks Co-authored-by: wysaid <1430725+wysaid@users.noreply.github.com>
- Fixed async.rs compilation errors: - Use open_device() instead of open() with parameters - Fix async stop() method return type - Add missing InternalError variant to error enum - Fix mutable borrow in spawn_blocking - Clean up warnings: - Remove unused imports (CString, std::ptr) from frame.rs - Fix unused variable by prefixing with underscore - Remove unused tokio_test import from async.rs - Fix unused Result warning in minimal_example.rs - Add comprehensive documentation: - Document all enum variants for PixelFormat, FrameOrientation, PropertyName, ColorConversionBackend - Document all struct fields in Resolution, DeviceInfo, VideoFrameInfo - Document all missing methods and associated functions - Add doc comment for InternalError variant and Unknown error code field - Add GitHub Actions workflow (.github/workflows/rust.yml): - Cross-platform testing (Ubuntu, Windows, macOS) - Rust formatting and clippy checks - Build C library as prerequisite - Test rust bindings and examples - Comprehensive CI pipeline for rust-specific checks All compilation errors resolved, warnings cleaned up, and comprehensive CI workflow added as requested. Co-authored-by: wysaid <1430725+wysaid@users.noreply.github.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a complete Rust bindings crate for the ccap camera library: build integration (CMake/Cargo/build.rs), bindgen FFI, synchronous Provider API, optional async wrapper, conversion/utils modules, examples, tests, CI workflow, and developer tasks/docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client App
participant Async as AsyncProvider (Rust)
participant Sync as Provider (Rust)
participant C as ccap C library
participant Dev as Device/Driver
Note over App,Async: async feature enabled (optional)
App->>Async: new()/open()/start()
Async->>Sync: lock & delegate open/start
Sync->>C: ccap_open / ccap_start_capture
C->>Dev: initialize stream
Dev-->>C: emit frame (C pointer)
C-->>Sync: deliver frame pointer
Sync->>Async: return VideoFrame / send via channel
Async-->>App: yield VideoFrame (await or stream)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (20)
CMakeLists.txt (1)
350-355: Consider respecting CMAKE_BUILD_TYPE for Rust builds.The Rust build always uses
--releasemode, but CMake might be configured for Debug builds. For consistency and to enable debug symbols in Rust code when developing:🔎 Proposed enhancement to match build type
# Add custom target to build Rust bindings add_custom_target(ccap-rust - COMMAND ${CARGO_CMD} build --release + COMMAND ${CARGO_CMD} build $<$<CONFIG:Release>:--release> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/bindings/rust COMMENT "Building Rust bindings" DEPENDS ccap )bindings/rust/RUST_INTEGRATION_SUMMARY.md (1)
29-47: Specify language identifier for code fence.The fenced code block lacks a language identifier, which impacts syntax highlighting and automated processing.
🔎 Proposed fix
### 项目结构 -``` +```text bindings/rust/ ├── Cargo.toml # 包配置bindings/rust/Cargo.toml (1)
17-17: Consider reducing tokio feature scope.Using
features = ["full"]includes all tokio features, which increases compile time and binary size. Since this is optional (behind the "async" feature), evaluate if a more minimal feature set would suffice.Typical minimal async features might be:
["rt", "sync", "time", "macros"]. Review the actual usage insrc/async.rsto determine the minimum required set..vscode/tasks.json (1)
1227-1326: Consider adding dependency on C library build.The Rust binding tasks execute
cargocommands without ensuring the underlying C++ccaplibrary is built first. Since the Rust bindings link against the C++ library (viabuild.rs), these tasks may fail if the library hasn't been built.Consider adding
dependsOnfor tasks like "Build Rust Bindings" to depend on "Build Project (Debug)" or "Build Project (Release)":🔎 Example enhancement
{ "label": "Build Rust Bindings", "type": "shell", "command": "cargo", "args": ["build"], "options": { "cwd": "${workspaceFolder}/bindings/rust" }, "group": "build", "problemMatcher": "$rustc", "dependsOn": ["Build Project (Debug)"] }bindings/rust/examples/minimal_example.rs (3)
5-7: Global error callback leaks memory by design.Based on the relevant code snippet from
provider.rs, the error callback intentionally leaks memory (line 308-347 note). While acceptable for examples, consider documenting this behavior or providing an example of proper lifecycle management for production use.Add a comment explaining the memory leak:
// Set error callback to receive error notifications // Note: This callback is global and persists for the application lifetime Provider::set_error_callback(|error_code, description| { eprintln!("Error occurred - Code: {}, Description: {}", error_code, description); });
18-21: Early return suppresses error information.When
is_started()returns false, the function returnsOk(()), which masks the actual failure. This makes debugging harder for users running the example.🔎 Consider returning an error instead
if !provider.is_started() { eprintln!("Failed to start camera!"); - return Ok(()); + return Err(ccap::CcapError::OperationFailed("Camera failed to start".to_string())); }
32-40: Error handling silently exits on frame grab failures.The example returns
Ok(())on bothNone(line 34) andErr(line 38) cases, suppressing the actual error status. While this prevents example crashes, it could confuse users trying to diagnose issues.For a more informative example, consider propagating errors:
Ok(None) => { return Err(ccap::CcapError::OperationFailed( format!("Timeout grabbing frame {}", i) )); } Err(e) => { return Err(e); }.github/workflows/rust.yml (1)
6-6: Consider expanding path filters to include C library changes.The workflow only triggers on
bindings/rust/**changes, but modifications to the C library headers (include/ccap*.h) or core implementation could break the Rust bindings. Consider adding these paths to the filter.🔎 Proposed enhancement
push: branches: [ main, develop ] - paths: [ 'bindings/rust/**' ] + paths: + - 'bindings/rust/**' + - 'include/ccap*.h' + - 'src/**' pull_request: branches: [ main, develop ] - paths: [ 'bindings/rust/**' ] + paths: + - 'bindings/rust/**' + - 'include/ccap*.h' + - 'src/**'Also applies to: 9-9
bindings/rust/build.rs (1)
37-38: Document or remove commented v4l2 linking.The commented-out v4l2 linking should either be removed if not needed, or documented why it's disabled but preserved. This helps future maintainers understand the decision.
🔎 Suggested clarification
#[cfg(target_os = "linux")] { - // v4l2 might not be available on all systems - // println!("cargo:rustc-link-lib=v4l2"); + // Note: v4l2 linking is not required as the C library statically links it + // or handles it internally. Keeping this comment for reference. println!("cargo:rustc-link-lib=stdc++"); }bindings/rust/examples/capture_callback.rs (1)
27-31: Simplify dead conditional logic.The conditional always evaluates to
0regardless of the number of devices, making theifstatement unnecessary. Either simplify this or add actual device selection logic.🔎 Proposed simplification
- // Select camera device (automatically use first device for testing) - let device_index = if devices.len() == 1 { - 0 - } else { - 0 // Just use first device for now - }; + // Select camera device (use first device for this example) + let device_index = 0;bindings/rust/examples/capture_grab.rs (1)
37-37: Use FileOperationFailed for directory creation errors.Directory creation failure is mapped to
InvalidParameter, butFileOperationFailed(used in capture_callback.rs line 64) is more semantically accurate for filesystem operations.🔎 Proposed fix
fs::create_dir_all(capture_dir) - .map_err(|e| ccap::CcapError::InvalidParameter(format!("Failed to create directory: {}", e)))?; + .map_err(|e| ccap::CcapError::FileOperationFailed(format!("Failed to create directory: {}", e)))?;bindings/rust/examples/print_camera.rs (1)
9-9: Fix plural grammar in output message.The message uses singular "device" even when multiple devices are found. Consider making it grammatically correct.
🔎 Proposed fix
if !devices.is_empty() { - println!("## Found {} video capture device:", devices.len()); + println!("## Found {} video capture device{}:", devices.len(), if devices.len() == 1 { "" } else { "s" }); for (index, name) in devices.iter().enumerate() { println!(" {}: {}", index, name); }bindings/rust/src/error.rs (2)
99-99: Empty strings in error conversion lose context information.When converting from integer error codes,
InvalidDeviceandInvalidParameterare created with empty strings, losing any context about what device or parameter failed. Consider preserving the error code or adding a helper method that accepts the context string.This is acceptable for a
From<i32>impl since the integer code contains no string context. However, you might want to add a separate constructor:impl CcapError { pub fn invalid_device_with_code(code: i32) -> Self { CcapError::InvalidDevice(format!("Error code: {}", code)) } pub fn invalid_parameter_with_code(code: i32) -> Self { CcapError::InvalidParameter(format!("Error code: {}", code)) } }Also applies to: 107-107
6-7: Consider whether CcapError::None variant is necessary.Having a
Nonevariant in an error enum is unusual in Rust - typically success is represented byOk(())rather thanOk(CcapError::None). This might be mirroring the C API's error codes, but could lead to confusion.If this variant is never actually used in practice, consider removing it. If it's required for C FFI compatibility, add a doc comment explaining why it exists:
/// No error occurred /// /// Note: This variant exists for compatibility with the C API's error code system, /// but should typically not appear in Rust code as success is represented by Ok(()). None,bindings/rust/src/utils.rs (2)
51-56: Misleading function name and documentation.The docstring says "Save frame as BMP file" but this delegates to
dump_frame_to_file, which uses "automatic format detection" according to its own docs. This creates confusion about the actual output format.Consider either:
- Renaming to reflect actual behavior, or
- Implementing actual BMP-specific logic if the intent is BMP output
197-206: Handle I/O errors gracefully and avoid silent fallback to valid index.Two issues in this interactive selection:
- Line 199:
unwrap()onflush()will panic on I/O errors- Lines 205-206:
unwrap_or(0)silently falls back to index 0 on parse failure, but 0 is a valid device index. Users may not realize their input was invalid.🔎 Proposed fix
- io::stdout().flush().unwrap(); + io::stdout().flush() + .map_err(|e| CcapError::InvalidParameter(format!("Failed to flush stdout: {}", e)))?; let mut input = String::new(); io::stdin().read_line(&mut input) .map_err(|e| CcapError::InvalidParameter(format!("Failed to read input: {}", e)))?; - let selected_index = input.trim().parse::<usize>() - .unwrap_or(0); + let selected_index = input.trim().parse::<usize>() + .map_err(|e| CcapError::InvalidParameter(format!("Invalid device index: {}", e)))?;bindings/rust/src/async.rs (1)
88-95: Panic on second call and redundant feature gate.
- Line 88: The
#[cfg(feature = "async")]is redundant since this is already inside a#[cfg(feature = "async")]impl block- Lines 91-92:
expect()will panic ifframe_stream()is called twice. Consider returningOption<impl Stream>orResultinstead.🔎 Proposed fix
- #[cfg(feature = "async")] /// Create a stream of frames - pub fn frame_stream(&mut self) -> impl futures::Stream<Item = VideoFrame> { - let receiver = self.frame_receiver.take() - .expect("Frame stream can only be created once"); - - tokio_stream::wrappers::UnboundedReceiverStream::new(receiver) + pub fn frame_stream(&mut self) -> Option<impl futures::Stream<Item = VideoFrame>> { + self.frame_receiver.take() + .map(tokio_stream::wrappers::UnboundedReceiverStream::new) }bindings/rust/src/types.rs (1)
86-104: Preferimpl From<PixelFormat> for sys::CcapPixelFormatoverimpl Into.The Rust API guidelines recommend implementing
Fromrather thanInto, asFromautomatically provides anIntoimplementation. This is also what Clippy warns about withclippy::from_over_into.🔎 Proposed fix
-impl Into<sys::CcapPixelFormat> for PixelFormat { - fn into(self) -> sys::CcapPixelFormat { +impl From<PixelFormat> for sys::CcapPixelFormat { + fn from(format: PixelFormat) -> Self { - match self { + match format { PixelFormat::Unknown => sys::CcapPixelFormat_CCAP_PIXEL_FORMAT_UNKNOWN, // ... rest unchanged } } }bindings/rust/src/convert.rs (2)
66-88: Consider validating source buffer size and preventing overflow.
No validation that
src_data.len()is sufficient for the given dimensions. For YUYV422, expected size iswidth * height * 2. Passing undersized buffers to C could cause buffer overreads.
width * height * 3could theoretically overflow for very large dimensions (e.g., 37000×37000), though this is unlikely in practice for camera frames.🔎 Proposed validation example
pub fn yuyv422_to_rgb24( src_data: &[u8], width: u32, height: u32, ) -> Result<Vec<u8>> { + let expected_src_size = (width as usize) + .checked_mul(height as usize) + .and_then(|v| v.checked_mul(2)) + .ok_or(CcapError::InvalidParameter("dimensions overflow".to_string()))?; + + if src_data.len() < expected_src_size { + return Err(CcapError::InvalidParameter( + format!("source buffer too small: {} < {}", src_data.len(), expected_src_size) + )); + } + let dst_size = (width * height * 3) as usize;
5-5: Unused import.
CStringis imported but never used in this file (onlyCStris used).🔎 Proposed fix
-use std::ffi::{CStr, CString}; +use std::ffi::CStr;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bindings/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/workflows/rust.yml.vscode/tasks.jsonCMakeLists.txtbindings/rust/.gitignorebindings/rust/Cargo.tomlbindings/rust/README.mdbindings/rust/RUST_INTEGRATION_SUMMARY.mdbindings/rust/build.rsbindings/rust/build_and_test.shbindings/rust/examples/capture_callback.rsbindings/rust/examples/capture_grab.rsbindings/rust/examples/minimal_example.rsbindings/rust/examples/print_camera.rsbindings/rust/src/async.rsbindings/rust/src/convert.rsbindings/rust/src/error.rsbindings/rust/src/frame.rsbindings/rust/src/lib.rsbindings/rust/src/provider.rsbindings/rust/src/types.rsbindings/rust/src/utils.rsbindings/rust/tests/integration_tests.rsbindings/rust/wrapper.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-27T19:15:25.440Z
Learnt from: CR
Repo: wysaid/CameraCapture PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T19:15:25.440Z
Learning: Applies to dev.cmake : `dev.cmake` in project root (git-ignored) overrides default CMake options. Remove it for standard builds/tests; keep only when verifying custom settings
Applied to files:
CMakeLists.txt
🧬 Code graph analysis (12)
bindings/rust/src/error.rs (1)
bindings/rust/src/types.rs (4)
from(35-52)from(116-122)from(150-159)from(208-213)
bindings/rust/examples/minimal_example.rs (3)
bindings/rust/src/provider.rs (3)
set_error_callback(309-348)new(17-27)pixel_format(298-301)bindings/rust/src/utils.rs (1)
select_camera(182-215)bindings/rust/src/frame.rs (5)
index(143-145)width(123-125)height(128-130)data_size(138-140)pixel_format(133-135)
bindings/rust/src/async.rs (1)
bindings/rust/src/provider.rs (8)
new(17-27)find_device_names(286-288)open(141-153)start(176-178)stop(181-183)is_opened(186-188)is_started(171-173)grab_frame(232-243)
bindings/rust/src/utils.rs (4)
bindings/rust/src/provider.rs (1)
new(17-27)bindings/rust/src/async.rs (1)
new(25-34)include/ccap_utils.h (1)
LogLevel(80-140)bindings/rust/src/types.rs (3)
to_c_enum(57-59)to_c_enum(144-146)to_c_enum(177-184)
bindings/rust/examples/print_camera.rs (2)
bindings/rust/src/provider.rs (3)
new(17-27)with_device_name(43-56)device_info(166-168)bindings/rust/src/utils.rs (1)
set_log_level(218-222)
bindings/rust/src/lib.rs (2)
include/ccap_utils.h (1)
LogLevel(80-140)bindings/rust/src/provider.rs (1)
version(266-277)
bindings/rust/src/convert.rs (2)
bindings/rust/src/types.rs (2)
from_c_enum(62-64)from_c_enum(187-195)bindings/rust/src/frame.rs (3)
from_c_ptr(49-51)width(123-125)height(128-130)
bindings/rust/build.rs (3)
bindings/rust/src/error.rs (1)
from(92-112)bindings/rust/src/types.rs (4)
from(35-52)from(116-122)from(150-159)from(208-213)bindings/rust/src/provider.rs (1)
new(17-27)
bindings/rust/examples/capture_grab.rs (6)
bindings/rust/examples/capture_callback.rs (1)
main(6-100)bindings/rust/examples/minimal_example.rs (1)
main(3-46)bindings/rust/examples/print_camera.rs (1)
main(56-74)bindings/rust/src/utils.rs (2)
set_log_level(218-222)dump_frame_to_directory(102-142)bindings/rust/src/provider.rs (2)
set_error_callback(309-348)new(17-27)bindings/rust/src/frame.rs (2)
width(123-125)height(128-130)
bindings/rust/src/frame.rs (3)
include/ccap_def.h (1)
DeviceInfo(372-389)bindings/rust/src/types.rs (6)
from_c_enum(62-64)from_c_enum(187-195)from(35-52)from(116-122)from(150-159)from(208-213)bindings/rust/src/error.rs (1)
from(92-112)
bindings/rust/examples/capture_callback.rs (3)
bindings/rust/src/utils.rs (1)
dump_frame_to_directory(102-142)bindings/rust/src/provider.rs (3)
set_error_callback(309-348)new(17-27)with_device(30-40)bindings/rust/src/frame.rs (4)
index(143-145)width(123-125)height(128-130)data_size(138-140)
bindings/rust/src/provider.rs (4)
include/ccap_def.h (1)
DeviceInfo(372-389)bindings/rust/src/types.rs (4)
from(35-52)from(116-122)from(150-159)from(208-213)bindings/rust/src/frame.rs (4)
width(123-125)height(128-130)from_c_ptr(49-51)from_c_ptr_ref(54-56)bindings/rust/src/lib.rs (1)
version(38-40)
🪛 actionlint (1.7.9)
.github/workflows/rust.yml
35-35: shellcheck reported issue in this script: SC2046:warning:4:8: Quote this to prevent word splitting
(shellcheck)
99-99: shellcheck reported issue in this script: SC2046:warning:4:8: Quote this to prevent word splitting
(shellcheck)
🪛 GitHub Actions: Rust CI
bindings/rust/examples/minimal_example.rs
[warning] 1-1: Formatted error callback logging blocks for consistency.
bindings/rust/src/async.rs
[warning] 1-1: Code formatting updates: added/adjusted comments and reordered imports for async module.
[warning] 1-1: Async feature code formatting: introduced explicit braces and alignment in spawn_blocking usage.
[warning] 1-1: Updated frame_stream creation to align with new receiver extraction style.
bindings/rust/examples/print_camera.rs
[warning] 1-1: Code style changes: adjusted spacing and formatting in provider/device info output paths.
bindings/rust/src/convert.rs
[warning] 1-1: File unchanged semantics; diff indicates reformatting only.
bindings/rust/build.rs
[warning] 1-1: Formatting/style changes detected in rust build script (diffs introduced by CI).
[warning] 1-1: Build profile string formatting expanded to multi-line for readability.
[warning] 1-1: Adjusted cargo:rustc-link-search logic to print multiple lines with explicit build_type.
[warning] 1-1: Expanded platform-specific linking blocks with consistent formatting.
[warning] 1-1: Added re-run triggers for header/include changes in build script.
bindings/rust/examples/capture_grab.rs
[warning] 1-1: Formatted Camera error logging and provider setup calls; minor style adjustments.
bindings/rust/examples/capture_callback.rs
[warning] 1-1: Code style changes: formatted eprintln! calls across error callback blocks.
🪛 GitHub Check: Build and Test
bindings/rust/build.rs
[warning] 66-66:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 52-52:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 45-45:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 38-38:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 31-31:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 16-16:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 6-6:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
bindings/rust/examples/capture_callback.rs
[warning] 29-29:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/examples/capture_callback.rs
[warning] 9-9:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/examples/capture_callback.rs
[warning] 1-1:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/examples/capture_callback.rs
🪛 GitHub Check: CodeQL
.github/workflows/rust.yml
[warning] 16-65: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
[warning] 66-119: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 LanguageTool
bindings/rust/README.md
[style] ~180-~180: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request. ## License Thi...
(FEEL_FREE_TO_STYLE_ME)
🪛 markdownlint-cli2 (0.18.1)
bindings/rust/RUST_INTEGRATION_SUMMARY.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
bindings/rust/build_and_test.sh
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: macOS Build (Release-static)
- GitHub Check: Build Fedora (Debug-shared)
- GitHub Check: Build (Release-gcc-shared)
- GitHub Check: Build (Debug-gcc-shared)
- GitHub Check: Build Fedora (Debug-static)
- GitHub Check: Build Fedora (Release-static)
- GitHub Check: Build (Release-gcc-static)
- GitHub Check: Build ARM64 (Debug-gcc-shared)
- GitHub Check: Build ARM64 (Release-gcc-shared)
- GitHub Check: Build (Debug-clang-shared)
- GitHub Check: Build ARM64 (Debug-gcc-static)
- GitHub Check: Build (Release-clang-static)
- GitHub Check: Build (Debug-clang-static)
- GitHub Check: Windows Build VS2022 (Release-shared)
- GitHub Check: Windows Build MinGW (Debug-static)
- GitHub Check: Windows Build VS2022 (Release-static)
- GitHub Check: Windows Build MinGW (Release-shared)
- GitHub Check: Windows Build VS2022 (Debug-shared)
- GitHub Check: Windows Build MinGW (Debug-shared)
- GitHub Check: Windows Build MinGW (Release-static)
🔇 Additional comments (12)
bindings/rust/.gitignore (1)
1-2: LGTM!The ignore patterns appropriately exclude Rust build artifacts (
target/) and example output (image_capture/).bindings/rust/tests/integration_tests.rs (2)
7-12: LGTM!The test correctly verifies that a newly created provider is not opened, which aligns with the expected behavior shown in the code snippet from
provider.rs:16-26.
35-41: LGTM!The round-trip conversion test for
PixelFormatensures type safety between Rust and C enums, which is critical for FFI correctness.bindings/rust/wrapper.h (1)
1-3: Header files are properly configured and accessible.All three included headers exist in the include directory and are correctly referenced in the build.rs configuration with proper include path settings and include guards in place.
bindings/rust/build.rs (1)
63-66: DerivingDefaulton opaque handle types is not a practical risk given the API design.The C headers define
CcapProviderandCcapVideoFrameas forward-declared opaque types. Bindgen does generateDefaultderives for these, which is technically unsafe since a default/null pointer would be invalid. However, the Rust codebase mitigates this risk:
- The public API wraps opaque types in safe wrappers (
Providerstruct) that never use.default()- The low-level
sysmodule containing raw opaque types is marked for advanced users only- Concrete types that actually call
.default()in the code (CcapDeviceNamesList,CcapDeviceInfo) are safe data containersNo action required; the current design prevents unsafe usage of
Defaulton opaque handles.bindings/rust/src/lib.rs (1)
1-40: LGTM!The crate structure is well-organized with appropriate module separation, feature-gated async support, and clear public API surface. The
#![warn(missing_docs)]and#![warn(rust_2018_idioms)]lints are good hygiene for a library crate.bindings/rust/src/utils.rs (1)
225-251: LGTM!The
LogLevelenum properly mirrors the C API levels and theto_c_enumconversion is correct.bindings/rust/src/async.rs (1)
72-86: LGTM!The
grab_frame_timeoutcorrectly usesspawn_blockingto offload the blocking FFI call, andblocking_lock()is the right choice insidespawn_blocking. The timeout-to-millis conversion and error propagation are handled properly.bindings/rust/src/types.rs (1)
162-196: LGTM!The
ColorConversionBackendenum correctly maps to and from C API values. The fallback toCpufor unknown values is a reasonable defensive choice for FFI code.bindings/rust/src/convert.rs (1)
10-47: LGTM!The backend management methods (
backend(),set_backend(),backend_name(),is_backend_available()) are well-implemented with proper error handling and C string conversion.bindings/rust/src/provider.rs (2)
376-419: LGTM with memory leak caveat.The
set_new_frame_callbackimplementation correctly handles the callback type throughBox::into_rawand reconstruction. The memory leak is acceptable for callback registration that lives for the provider's lifetime, though addingDropcleanup or a dedicated unregister mechanism would be ideal for long-running applications.
439-448: LGTM!The
Dropimplementation correctly releases the C handle and nullifies the pointer to prevent double-free.
| if [ ! -d "build" ]; then | ||
| mkdir -p build/Debug | ||
| cd build/Debug | ||
| cmake ../.. -DCMAKE_BUILD_TYPE=Debug | ||
| make -j$(nproc 2>/dev/null || echo 4) | ||
| else | ||
| echo "Build directory exists, assuming library is built" | ||
| fi |
There was a problem hiding this comment.
Verify library existence, not just build directory.
The script assumes the C library is built if the build directory exists, but doesn't verify the library file actually exists in that case. If someone manually creates build/ or a build fails partway, this will skip rebuilding even though the library is missing.
🔎 Proposed fix
- if [ ! -d "build" ]; then
+ if true; then # Always try to build if library not found
mkdir -p build/Debug
cd build/Debug
cmake ../.. -DCMAKE_BUILD_TYPE=Debug
make -j$(nproc 2>/dev/null || echo 4)
- else
- echo "Build directory exists, assuming library is built"
fiOr better, remove the else branch entirely since the outer check already verified the library doesn't exist.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
| futures = { version = "0.3", optional = true } | ||
|
|
||
| [build-dependencies] | ||
| bindgen = "0.68" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
bindgen latest stable version MSRV minimum supported rust version
💡 Result:
Latest stable bindgen: 0.72.1. Minimum supported Rust version (MSRV): 1.70.0. [1][2]
Sources:
- bindgen (docs.rs) — release & MSRV info. [1]
- rust-bindgen GitHub README — MSRV statement. [2]
[1] https://docs.rs/crate/bindgen/latest
[2] https://github.com/rust-lang/rust-bindgen
bindgen 0.68 is outdated; consider upgrading with MSRV implications.
The current version (0.68) is several releases behind the latest stable (0.72.1), which offers bug fixes and improvements. However, the latest bindgen requires Rust 1.70.0 MSRV, while the project specifies 1.65+. If keeping Rust 1.65 compatibility is important, stay with 0.68 or an intermediate version. Otherwise, upgrade to 0.72.1 and update the rust-version requirement to 1.70 or higher.
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive Rust bindings for the CameraCapture library, providing a memory-safe, high-performance interface with optional async support. The implementation wraps the underlying C API with idiomatic Rust patterns including RAII resource management, Result-based error handling, and zero-copy data access.
Key changes:
- Complete Rust FFI bindings with safe wrappers for camera capture operations
- Hardware-accelerated pixel format conversion support (AVX2, NEON, Apple Accelerate)
- Optional async/await interface using tokio
- Cross-platform build configuration integrated with CMake
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
bindings/rust/src/lib.rs |
Main library entry point with module organization and public API exports |
bindings/rust/src/error.rs |
Error type definitions and conversions from C error codes |
bindings/rust/src/types.rs |
Type-safe Rust enums for pixel formats, properties, and backend selection |
bindings/rust/src/frame.rs |
VideoFrame wrapper with lifetime management and data access methods |
bindings/rust/src/provider.rs |
Main Provider API for camera control and frame capture |
bindings/rust/src/utils.rs |
Utility functions for logging, file I/O, and device selection |
bindings/rust/src/convert.rs |
Pixel format conversion functions leveraging SIMD acceleration |
bindings/rust/src/async.rs |
Tokio-based async wrapper for non-blocking camera operations |
bindings/rust/build.rs |
Build script for generating FFI bindings via bindgen |
bindings/rust/Cargo.toml |
Package manifest with dependencies and feature flags |
bindings/rust/tests/integration_tests.rs |
Integration tests for core API functionality |
bindings/rust/examples/*.rs |
Example programs demonstrating various usage patterns |
bindings/rust/README.md |
Documentation with usage examples and API reference |
bindings/rust/build_and_test.sh |
Build automation script for C library and Rust bindings |
CMakeLists.txt |
CMake integration for optional Rust bindings build |
.github/workflows/rust.yml |
CI workflow for cross-platform testing |
bindings/rust/src/utils.rs
Outdated
| use crate::types::PixelFormat; | ||
| use crate::frame::VideoFrame; | ||
| use crate::sys; | ||
| use std::ffi::{ CString}; |
There was a problem hiding this comment.
Unused import: CString is imported but never used in this file. Consider removing it to clean up the imports.
| use std::ffi::{ CString}; |
| pub fn set_new_frame_callback<F>(&mut self, callback: F) -> Result<()> | ||
| where | ||
| F: Fn(&VideoFrame) -> bool + Send + Sync + 'static, | ||
| { | ||
| use std::os::raw::c_void; | ||
|
|
||
| unsafe extern "C" fn new_frame_callback_wrapper( | ||
| frame: *const sys::CcapVideoFrame, | ||
| user_data: *mut c_void, | ||
| ) -> bool { | ||
| if user_data.is_null() || frame.is_null() { | ||
| return false; | ||
| } | ||
|
|
||
| let callback = &*(user_data as *const Box<dyn Fn(&VideoFrame) -> bool + Send + Sync>); | ||
|
|
||
| // Create a temporary VideoFrame wrapper that doesn't own the frame | ||
| let video_frame = VideoFrame::from_c_ptr_ref(frame as *mut sys::CcapVideoFrame); | ||
| callback(&video_frame) | ||
| } | ||
|
|
||
| // Store the callback to prevent it from being dropped | ||
| let callback_box = Box::new(callback); | ||
| let callback_ptr = Box::into_raw(callback_box); | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_provider_set_new_frame_callback( | ||
| self.handle, | ||
| Some(new_frame_callback_wrapper), | ||
| callback_ptr as *mut c_void, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(()) | ||
| } else { | ||
| // Clean up on failure | ||
| unsafe { | ||
| let _ = Box::from_raw(callback_ptr); | ||
| } | ||
| Err(CcapError::CaptureStartFailed) | ||
| } | ||
| } |
There was a problem hiding this comment.
Memory leak: The callback pointer is converted to a raw pointer and never freed. When set_new_frame_callback is called multiple times or when the Provider is dropped, the previous callback is leaked. Store the callback pointer in the Provider struct and free it in the Drop implementation or when a new callback is set.
bindings/rust/src/provider.rs
Outdated
| if success { | ||
| Ok(()) | ||
| } else { | ||
| Err(CcapError::CaptureStartFailed) |
There was a problem hiding this comment.
Incorrect error handling: When remove_new_frame_callback fails, it returns CcapError::CaptureStartFailed which is semantically incorrect. This should return a more appropriate error like CaptureStopFailed or a new error variant specific to callback removal.
| Err(CcapError::CaptureStartFailed) | |
| Err(CcapError::CaptureStopFailed) |
| use crate::error::{CcapError, Result}; | ||
| use crate::types::{PixelFormat, ColorConversionBackend}; | ||
| use crate::frame::VideoFrame; | ||
| use crate::sys; | ||
| use std::ffi::{CStr, CString}; | ||
|
|
||
| /// Color conversion utilities | ||
| pub struct Convert; | ||
|
|
||
| impl Convert { | ||
| /// Get current color conversion backend | ||
| pub fn backend() -> ColorConversionBackend { | ||
| let backend = unsafe { sys::ccap_convert_get_backend() }; | ||
| ColorConversionBackend::from_c_enum(backend) | ||
| } | ||
|
|
||
| /// Set color conversion backend | ||
| pub fn set_backend(backend: ColorConversionBackend) -> Result<()> { | ||
| let success = unsafe { | ||
| sys::ccap_convert_set_backend(backend.to_c_enum()) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(()) | ||
| } else { | ||
| Err(CcapError::BackendSetFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Get backend name as string | ||
| pub fn backend_name() -> Result<String> { | ||
| let backend_ptr = unsafe { sys::ccap_convert_get_backend_name() }; | ||
| if backend_ptr.is_null() { | ||
| return Err(CcapError::InternalError("Failed to get backend name".to_string())); | ||
| } | ||
|
|
||
| let backend_cstr = unsafe { CStr::from_ptr(backend_ptr) }; | ||
| backend_cstr | ||
| .to_str() | ||
| .map(|s| s.to_string()) | ||
| .map_err(|_| CcapError::StringConversionError("Invalid backend name".to_string())) | ||
| } | ||
|
|
||
| /// Check if backend is available | ||
| pub fn is_backend_available(backend: ColorConversionBackend) -> bool { | ||
| unsafe { sys::ccap_convert_is_backend_available(backend.to_c_enum()) } | ||
| } | ||
|
|
||
| /// Convert frame to different pixel format | ||
| pub fn convert_frame( | ||
| src_frame: &VideoFrame, | ||
| dst_format: PixelFormat, | ||
| ) -> Result<VideoFrame> { | ||
| let dst_ptr = unsafe { | ||
| sys::ccap_convert_frame(src_frame.as_c_ptr(), dst_format.to_c_enum()) | ||
| }; | ||
|
|
||
| if dst_ptr.is_null() { | ||
| Err(CcapError::ConversionFailed) | ||
| } else { | ||
| Ok(VideoFrame::from_c_ptr(dst_ptr)) | ||
| } | ||
| } | ||
|
|
||
| /// Convert YUYV422 to RGB24 | ||
| pub fn yuyv422_to_rgb24( | ||
| src_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_yuyv422_to_rgb24( | ||
| src_data.as_ptr(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert YUYV422 to BGR24 | ||
| pub fn yuyv422_to_bgr24( | ||
| src_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_yuyv422_to_bgr24( | ||
| src_data.as_ptr(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert RGB24 to BGR24 | ||
| pub fn rgb24_to_bgr24( | ||
| src_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_rgb24_to_bgr24( | ||
| src_data.as_ptr(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert BGR24 to RGB24 | ||
| pub fn bgr24_to_rgb24( | ||
| src_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_bgr24_to_rgb24( | ||
| src_data.as_ptr(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert MJPEG to RGB24 | ||
| pub fn mjpeg_to_rgb24( | ||
| src_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_mjpeg_to_rgb24( | ||
| src_data.as_ptr(), | ||
| src_data.len(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert MJPEG to BGR24 | ||
| pub fn mjpeg_to_bgr24( | ||
| src_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_mjpeg_to_bgr24( | ||
| src_data.as_ptr(), | ||
| src_data.len(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert NV12 to RGB24 | ||
| pub fn nv12_to_rgb24( | ||
| y_data: &[u8], | ||
| uv_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_nv12_to_rgb24( | ||
| y_data.as_ptr(), | ||
| uv_data.as_ptr(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert NV12 to BGR24 | ||
| pub fn nv12_to_bgr24( | ||
| y_data: &[u8], | ||
| uv_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_nv12_to_bgr24( | ||
| y_data.as_ptr(), | ||
| uv_data.as_ptr(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert YV12 to RGB24 | ||
| pub fn yv12_to_rgb24( | ||
| y_data: &[u8], | ||
| u_data: &[u8], | ||
| v_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_yv12_to_rgb24( | ||
| y_data.as_ptr(), | ||
| u_data.as_ptr(), | ||
| v_data.as_ptr(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Convert YV12 to BGR24 | ||
| pub fn yv12_to_bgr24( | ||
| y_data: &[u8], | ||
| u_data: &[u8], | ||
| v_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_size = (width * height * 3) as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| let success = unsafe { | ||
| sys::ccap_convert_yv12_to_bgr24( | ||
| y_data.as_ptr(), | ||
| u_data.as_ptr(), | ||
| v_data.as_ptr(), | ||
| dst_data.as_mut_ptr(), | ||
| width, | ||
| height, | ||
| ) | ||
| }; | ||
|
|
||
| if success { | ||
| Ok(dst_data) | ||
| } else { | ||
| Err(CcapError::ConversionFailed) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage: The Convert struct and all its conversion functions lack test coverage. Given that this is a core feature of the library (hardware-accelerated conversions), tests should be added to verify: 1) Backend selection works correctly, 2) Conversion functions produce valid output, 3) Error cases are handled properly.
bindings/rust/README.md
Outdated
| println!("Camera opened successfully!"); | ||
|
|
||
| // Capture a frame | ||
| if let Some(frame) = provider.grab_frame_blocking()? { |
There was a problem hiding this comment.
Inconsistent API naming: The method is named grab_frame_blocking in the README example, but the actual method in Provider is grab_frame. Either update the README to use the correct method name or add a grab_frame_blocking alias for consistency with the async API.
| if let Some(frame) = provider.grab_frame_blocking()? { | |
| if let Some(frame) = provider.grab_frame()? { |
bindings/rust/src/provider.rs
Outdated
|
|
||
| /// Set pixel format | ||
| pub fn set_pixel_format(&mut self, format: PixelFormat) -> Result<()> { | ||
| self.set_property(PropertyName::PixelFormatOutput, format as u32 as f64) |
There was a problem hiding this comment.
Incorrect type conversion: Using format as u32 as f64 to convert PixelFormat enum to a property value is incorrect. The enum doesn't derive Copy or have a guaranteed integer representation. Use format.to_c_enum() as u32 as f64 instead.
| self.set_property(PropertyName::PixelFormatOutput, format as u32 as f64) | |
| self.set_property( | |
| PropertyName::PixelFormatOutput, | |
| format.to_c_enum() as u32 as f64, | |
| ) |
bindings/rust/src/provider.rs
Outdated
| /// Get current pixel format (convenience getter) | ||
| pub fn pixel_format(&self) -> Result<PixelFormat> { | ||
| let format_val = self.get_property(PropertyName::PixelFormatOutput)? as u32; | ||
| Ok(PixelFormat::from(format_val)) |
There was a problem hiding this comment.
Incorrect type conversion: Converting format_val as u32 to PixelFormat using PixelFormat::from() is incorrect. The From implementation expects sys::CcapPixelFormat, not u32. This will cause a compilation error. Use PixelFormat::from_c_enum(format_val as sys::CcapPixelFormat) instead.
| Ok(PixelFormat::from(format_val)) | |
| Ok(PixelFormat::from_c_enum(format_val as sys::CcapPixelFormat)) |
| ``` | ||
|
|
||
| ## Building | ||
|
|
There was a problem hiding this comment.
Incorrect API usage in documentation: The method provider.set_resolution() is called with a Resolution struct, but the actual implementation expects two separate u32 parameters: set_resolution(width: u32, height: u32). Update the example to use provider.set_resolution(640, 480).
| pub struct AsyncProvider { | ||
| provider: Arc<Mutex<SyncProvider>>, | ||
| frame_receiver: Option<mpsc::UnboundedReceiver<VideoFrame>>, | ||
| _frame_sender: mpsc::UnboundedSender<VideoFrame>, |
There was a problem hiding this comment.
Redundant field in AsyncProvider: The _frame_sender field is stored but never used. The sender is only used to keep the channel alive, but since the receiver is stored and owns the channel, the sender can be dropped. Consider removing this field or document why it needs to be kept.
bindings/rust/src/utils.rs
Outdated
| let selected_index = input.trim().parse::<usize>() | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
Unreachable code in error handling: The unwrap_or(0) will never be executed because parse() returns a Result, and if parsing fails, the error is not propagated - it just defaults to 0. However, the subsequent bounds check makes this safe. Consider using unwrap_or(0) or unwrap_or_else(|_| 0) for clarity, or better yet, handle the error explicitly with proper error messaging.
| let selected_index = input.trim().parse::<usize>() | |
| .unwrap_or(0); | |
| let selected_index = match input.trim().parse::<usize>() { | |
| Ok(index) => index, | |
| Err(_) => { | |
| println!("Invalid input, using the first device: {}", devices[0]); | |
| 0 | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
bindings/rust/Cargo.toml (3)
17-17: Consider narrowing tokio features for a lighter footprint.The
"full"feature set includes I/O drivers, filesystem, process, signal handling, and other heavy components that a camera capture library likely doesn't need. For a library crate, consider specifying only the required features (e.g.,["rt", "sync", "macros"]) to reduce compile times and binary size for downstream users.🔎 Proposed refinement
-tokio = { version = "1.0", features = ["full"], optional = true } +tokio = { version = "1.0", features = ["rt", "sync", "macros"], optional = true }
16-19: Consider adding patch version constraints for stability.Version specifications like
"1.0"allow any1.xrelease, which could introduce unexpected behavior changes. For better stability, consider specifying minimum patch versions (e.g.,"1.0.0") or using more conservative ranges.
28-32: Consider enforcing mutual exclusivity between static-link and build-source.The features
static-linkandbuild-sourceappear to be mutually exclusive by design (one for development, one for distribution), but nothing prevents both from being enabled. Whilebuild.rschecksCARGO_FEATURE_BUILD_SOURCEfirst and may handle this gracefully, explicit documentation or compile-time enforcement would clarify the intended usage pattern.bindings/rust/build.rs (2)
59-60: C++17 requirement should be documented.The build script requires C++17, but this minimum compiler version requirement isn't documented in the package metadata or README. Consider documenting the compiler requirements for users building from source.
79-81: Multiple library search paths could mask linking issues.The build adds three search paths with fallbacks (specific build type, Debug, Release). While this provides flexibility during development, it could mask issues if the linker finds a library from an unexpected build configuration. Consider documenting this behavior or adding a warning when falling back to a different build type.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bindings/rust/Cargo.tomlbindings/rust/build.rs
🧰 Additional context used
🪛 GitHub Actions: Rust CI
bindings/rust/build.rs
[error] 12-12: unused variable: is_packaged
🪛 GitHub Check: Build and Test
bindings/rust/build.rs
[warning] 97-97:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 79-79:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 73-73:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 65-65:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 52-52:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 47-47:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 40-40:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 24-24:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 12-12:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
[warning] 5-5:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/build.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Build (Release-gcc-static)
- GitHub Check: Build (Release-clang-shared)
- GitHub Check: Build Fedora (Release-static)
- GitHub Check: Build (Debug-clang-shared)
- GitHub Check: Build Fedora (Debug-static)
- GitHub Check: Build (Release-clang-static)
- GitHub Check: Build Fedora (Release-shared)
- GitHub Check: Build (Debug-clang-static)
- GitHub Check: Build Fedora (Debug-shared)
- GitHub Check: Build ARM64 (Release-gcc-static)
- GitHub Check: Windows Build MinGW (Debug-static)
- GitHub Check: Windows Build VS2022 (Release-static)
- GitHub Check: Windows Build MinGW (Release-shared)
- GitHub Check: Windows Build MinGW (Release-static)
- GitHub Check: Windows Build VS2022 (Release-shared)
- GitHub Check: Windows Build MinGW (Debug-shared)
🔇 Additional comments (5)
bindings/rust/Cargo.toml (3)
1-14: Package metadata looks good.The package configuration is well-structured with appropriate metadata for publishing to crates.io.
21-23: Build dependencies are appropriately specified.The build dependencies are correctly configured for FFI binding generation and C++ compilation.
34-48: Example configurations are correct.The example binaries are properly configured with explicit paths.
bindings/rust/build.rs (2)
122-134: Bindgen configuration is well-structured.The bindgen setup correctly uses allowlists to generate bindings only for ccap symbols, and derives useful traits (Default, Debug, PartialEq, Eq) for the generated types.
26-35: The review comment is incorrect. The code already has comprehensive SIMD feature detection and dispatch logic in place:AVX2: Runtime CPUID detection via
hasAVX2_()checks for AVX, OSXSAVE, and AVX2 support with_xgetbv()validation. All AVX2 functions are guarded with#if ENABLE_AVX2_IMP(x86_64-only compile gate) andcanUseAVX2()runtime checks before execution.NEON: Platform-specific
hasNEON_()with compile-time gate (#if ENABLE_NEON_IMPfor ARM64 only). NEON is mandatory on ARM64 per architecture spec, but the code includes platform-specific checks (Apple/Windows always true; Android/Linux checks HWCAP or assumes AArch64 support). All NEON functions are guarded with#if ENABLE_NEON_IMPandcanUseNEON()runtime checks.Dispatch: Every conversion function conditionally selects the appropriate SIMD backend based on detected capabilities before calling any SIMD variant.
No changes needed—the implementation already prevents executing unsupported SIMD instructions on incompatible hardware.
Likely an incorrect or invalid review comment.
bindings/rust/build.rs
Outdated
| let (ccap_root, is_packaged) = if manifest_path.join("native").exists() { | ||
| (manifest_path.join("native"), true) | ||
| } else { | ||
| (manifest_path.parent().unwrap().parent().unwrap().to_path_buf(), false) |
There was a problem hiding this comment.
Chained unwrap() could panic in unexpected directory structures.
The .parent().unwrap().parent().unwrap() chain will panic if the manifest directory doesn't have at least two parent directories. While unlikely in the expected repository structure, this creates a fragile assumption.
🔎 Proposed fix with better error handling
- (manifest_path.parent().unwrap().parent().unwrap().to_path_buf(), false)
+ manifest_path.parent()
+ .and_then(|p| p.parent())
+ .expect("Cargo manifest must be at least 2 directories deep (bindings/rust/)")
+ .to_path_buf()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bindings/rust/build.rs around line 15, the expression using
.parent().unwrap().parent().unwrap() is fragile and can panic if the manifest
path lacks two parent directories; replace the chained unwraps with safe
handling: retrieve the second ancestor via manifest_path.parent().and_then(|p|
p.parent()) or manifest_path.ancestors().nth(2), map it to a PathBuf on success,
and on failure return a clear error (or call panic!/eprint! and exit) with a
descriptive message indicating the expected repository layout and the
problematic manifest_path; ensure the code does not use plain unwrap() so it
fails with an actionable error instead of an obscure panic.
| println!("cargo:rerun-if-changed=../../include/ccap_c.h"); | ||
| println!("cargo:rerun-if-changed=../../include/ccap_utils_c.h"); | ||
| println!("cargo:rerun-if-changed=../../include/ccap_convert_c.h"); |
There was a problem hiding this comment.
Rerun triggers use hardcoded paths that break in packaged mode.
The rerun-if-changed directives use hardcoded relative paths (../../include/*) that assume repository layout. When the crate is packaged (with is_packaged == true and ccap_root pointing to native/), these paths won't exist, causing the rerun triggers to fail silently.
🔎 Proposed fix using ccap_root
- println!("cargo:rerun-if-changed=wrapper.h");
- println!("cargo:rerun-if-changed=../../include/ccap_c.h");
- println!("cargo:rerun-if-changed=../../include/ccap_utils_c.h");
- println!("cargo:rerun-if-changed=../../include/ccap_convert_c.h");
+ println!("cargo:rerun-if-changed=wrapper.h");
+ println!("cargo:rerun-if-changed={}/include/ccap_c.h", ccap_root.display());
+ println!("cargo:rerun-if-changed={}/include/ccap_utils_c.h", ccap_root.display());
+ println!("cargo:rerun-if-changed={}/include/ccap_convert_c.h", ccap_root.display());Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bindings/rust/build.rs around lines 117 to 119, the cargo:rerun-if-changed
directives use hardcoded relative paths (../../include/...) which break when the
crate is packaged; update these three println! invocations to build the
include-file paths from the existing ccap_root variable (or PathBuf formed from
it) so they point to ccap_root.join("include/<file>") and print the resulting
path (e.g., path.display()) instead of the hardcoded ../../ paths; ensure the
code uses the correct string/Display output so the paths exist both in repo and
packaged (is_packaged) modes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/rust.yml (1)
21-24: Add explicit permissions block to follow least-privilege principle.Both CodeQL and previous reviews have flagged the missing permissions block as a security concern. The workflow should explicitly set minimal permissions for GITHUB_TOKEN.
🔎 Proposed fix
env: CARGO_TERM_COLOR: always +permissions: + contents: read + jobs:
🧹 Nitpick comments (1)
.github/workflows/rust.yml (1)
52-56: Consider adding Cargo dependency caching for faster builds.The Rust toolchain setup looks good, but you could improve CI performance by caching Cargo dependencies and build artifacts. The
actions-rust-lang/setup-rust-toolchain@v1action includes built-in caching, but it can be further optimized.🔎 Suggested enhancement
The action you're using already includes some caching, but you can add explicit Cargo caching for better performance:
- name: Install Rust toolchain uses: actions-rust-lang/setup-rust-toolchain@v1 with: toolchain: stable components: clippy, rustfmt + cache: true + cache-directories: | + ~/.cargo/bin/ + ~/.cargo/registry/index/ + ~/.cargo/registry/cache/ + ~/.cargo/git/db/ + bindings/rust/target/Alternatively, add a separate caching step:
- name: Cache Cargo dependencies uses: actions/cache@v3 with: path: | ~/.cargo/bin/ ~/.cargo/registry/index/ ~/.cargo/registry/cache/ ~/.cargo/git/db/ bindings/rust/target/ key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} restore-keys: | ${{ runner.os }}-cargo-
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/rust.yml
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/rust.yml
[warning] 28-100: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
[warning] 101-132: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Build ARM64 (Debug-gcc-static)
- GitHub Check: Build ARM64 (Release-gcc-shared)
- GitHub Check: Build Fedora (Debug-shared)
- GitHub Check: Build (Release-gcc-shared)
- GitHub Check: Build Fedora (Release-shared)
- GitHub Check: Build Fedora (Debug-static)
- GitHub Check: Build (Release-clang-shared)
- GitHub Check: Build (Debug-clang-static)
- GitHub Check: Build (Release-clang-static)
- GitHub Check: macOS Build (Debug-static)
- GitHub Check: macOS Build (Debug-shared)
- GitHub Check: macOS Build (Release-shared)
- GitHub Check: Windows Build MinGW (Debug-shared)
- GitHub Check: Windows Build MinGW (Release-shared)
- GitHub Check: Windows Build MinGW (Debug-static)
- GitHub Check: Windows Build MinGW (Release-static)
- GitHub Check: Windows Build VS2022 (Debug-static)
- GitHub Check: Windows Build VS2022 (Debug-shared)
- GitHub Check: Windows Build VS2022 (Release-static)
- GitHub Check: Windows Build VS2022 (Release-shared)
🔇 Additional comments (6)
.github/workflows/rust.yml (6)
1-19: LGTM! Well-configured workflow triggers.The workflow name, branch triggers, and path filters are appropriate for the Rust bindings CI pipeline. The path filters will ensure this workflow only runs when relevant code changes.
34-50: LGTM! Platform-specific dependency installation is correct.The conditional steps correctly install required build tools for each platform. Ubuntu gets cmake, build-essential, and pkg-config; Windows gets cmake via choco; macOS gets cmake via brew.
58-77: LGTM! Platform-specific C++ library build steps are correct.The Unix and Windows build steps correctly handle platform differences:
- Unix builds in
build/Debugto match build.rs expectations for Makefiles- Windows builds in
buildwhich createsbuild/Debug/ccap.libfor MSVC- Both disable examples and tests to speed up CI builds
79-95: LGTM! Rust validation steps are well-structured.The formatting and clippy checks run only on Ubuntu (efficient), while builds and tests run on all platforms (thorough). The
--features static-linkflag is correctly applied to all Rust commands.
111-115: Verify build-source works on Windows/macOS without cmake installation.The build-source job only installs system dependencies on Ubuntu, while the static-link job explicitly installs cmake on all platforms. Although GitHub-hosted runners come with cmake pre-installed, this inconsistency raises a question: does the build.rs script with the build-source feature successfully compile the C++ library on Windows and macOS without explicit cmake installation?
Consider explicitly installing cmake on all platforms for consistency and to ensure the build-source feature works reliably:
🔎 Suggested enhancement for consistency
- name: Install system dependencies (Ubuntu) if: matrix.os == 'ubuntu-latest' run: | sudo apt-get update sudo apt-get install -y build-essential pkg-config + + - name: Install system dependencies (Windows) + if: matrix.os == 'windows-latest' + run: choco install cmake + + - name: Install system dependencies (macOS) + if: matrix.os == 'macos-latest' + run: brew install cmakeIf testing confirms the current setup works without these additions, please add a comment explaining that cmake is pre-installed on GitHub runners and the build-source feature doesn't require explicit installation.
117-132: LGTM! Build-source validation is correctly configured.The job correctly validates the build-source distribution path by:
- Not pre-building the C++ library (letting build.rs handle it via the cc crate)
- Using
--no-default-features --features build-sourceto disable static linking- Running both build and test to ensure the feature works end-to-end
The helpful comment on lines 122-123 clearly explains the intentional difference from the static-link job.
Critical fixes: - Add GitHub Actions workflow permissions (contents: read) - Fix memory leak in Provider callback management - Add callback_ptr field to track callback lifecycle - Implement cleanup_callback() method - Clean up callbacks in Drop implementation - Fix open_device ignoring device_name parameter - Fix pixel format conversion type errors in set/get operations - Fix VideoFrame data plane size calculations - Plane 0: stride * height - Chroma planes: stride * (height + 1) / 2 Important fixes: - Remove strict device count assertion in tests - Add bounds checking for DeviceInfo arrays - Fix README example code - Use open_device() instead of open() - Use grab_frame(timeout) instead of grab_frame_blocking() Minor fixes: - Add ConversionFailed error variant - Clean up unused imports in utils.rs - Document AsyncProvider's reserved _frame_sender field - Fix shell script quoting in build_and_test.sh - Fix remove_new_frame_callback error type All critical and major issues from PR review have been addressed.
- Remove non-existent functions: ccap_convert_frame, ccap_convert_is_backend_available, ccap_convert_get_backend_name - Remove non-existent conversion functions: MJPEG, YV12, yuyv422 - Fix NV12/I420/YUYV conversion functions to match C API signatures (add stride parameters) - Use correct constant CCAP_CONVERT_FLAG_DEFAULT instead of non-existent NONE - Remove unused ConversionFailed error variant - Fix unused_variables warning in build.rs - All conversion functions now match actual C API in include/ccap_convert_c.h
- Run cargo fmt to fix all code formatting issues - Separate SIMD file compilation in build.rs with proper flags: - ccap_convert_avx2.cpp with -mavx2 -mfma for x86/x86_64 - ccap_convert_neon.cpp for aarch64 - This fixes GitHub Actions CI failures: - Formatting check failure - Build-source mode compilation failure
- Add #[allow(clippy::too_many_arguments)] to functions with 8 parameters - Change Into<T> implementation to From<T> (clippy::from_over_into) - Remove unnecessary double cast (u32 as u32 as f64 -> u32 as f64) All clippy checks now pass with -D warnings.
Compress multi-line call to single line as cargo fmt prefers.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
bindings/rust/build.rs (2)
15-24: Fragile path traversal will panic with unclear error message.The chained
.parent().unwrap().parent().unwrap()(lines 17-20) will panic if the manifest directory doesn't have two parent directories. While unlikely in normal repository structure, this creates a fragile assumption that will fail with an unclear "calledOption::unwrap()on aNonevalue" message.🔎 Proposed fix with better error handling
} else { ( manifest_path - .parent() - .unwrap() - .parent() - .unwrap() - .to_path_buf(), + .ancestors() + .nth(2) + .expect("Cargo manifest must be at least 2 directories deep (expected bindings/rust/Cargo.toml)") + .to_path_buf(), false, ) };
165-168: Hardcoded paths break rerun triggers in packaged mode.The
rerun-if-changeddirectives (lines 166-168) use hardcoded../../include/*paths that assume repository layout. When the crate is packaged for crates.io (ccap_rootpoints tonative/), these paths won't exist, causing rebuild triggers to silently fail.🔎 Proposed fix using ccap_root
// Tell cargo to invalidate the built crate whenever the wrapper changes println!("cargo:rerun-if-changed=wrapper.h"); - println!("cargo:rerun-if-changed=../../include/ccap_c.h"); - println!("cargo:rerun-if-changed=../../include/ccap_utils_c.h"); - println!("cargo:rerun-if-changed=../../include/ccap_convert_c.h"); + println!("cargo:rerun-if-changed={}/include/ccap_c.h", ccap_root.display()); + println!("cargo:rerun-if-changed={}/include/ccap_utils_c.h", ccap_root.display()); + println!("cargo:rerun-if-changed={}/include/ccap_convert_c.h", ccap_root.display());bindings/rust/README.md (1)
147-155: Outdated API call in error handling example.Line 150 still uses
provider.grab_frame_blocking(), which was flagged in previous reviews as non-existent. The synchronous API requires a timeout parameter:provider.grab_frame(timeout_ms). This should be updated to match the corrected usage shown in the Basic Usage example (line 53).🔎 Proposed fix
```rust -match provider.grab_frame_blocking() { +match provider.grab_frame(3000) { Ok(Some(frame)) => { /* process frame */ }, Ok(None) => println!("No frame available"), Err(e) => eprintln!("Capture error: {:?}", e), }</details> </blockquote></details> <details> <summary>bindings/rust/src/async.rs (1)</summary><blockquote> `95-104`: **Frame stream implementation is incomplete.** This was already flagged in previous reviews - the receiver will never yield items since nothing sends to the channel. The doc comment at lines 17-18 acknowledges this limitation. </blockquote></details> <details> <summary>bindings/rust/src/frame.rs (1)</summary><blockquote> `191-193`: **Document thread-safety assumptions for unsafe Send+Sync.** The previous review flagged that these unsafe impls require documented justification. The underlying C library's thread-safety guarantees should be verified and documented in a `// SAFETY:` comment. At minimum, add safety documentation: ```diff -// Make VideoFrame Send + Sync if the underlying C library supports it -unsafe impl Send for VideoFrame {} -unsafe impl Sync for VideoFrame {} +// SAFETY: VideoFrame wraps a CcapVideoFrame pointer. The underlying ccap library +// guarantees that video frame data is immutable after creation and frame handles +// can be safely shared across threads. The frame's lifetime is managed by +// reference counting in the C library. +unsafe impl Send for VideoFrame {} +unsafe impl Sync for VideoFrame {}bindings/rust/src/provider.rs (1)
361-373: Global callback memory leak is acknowledged but not addressed.The previous review suggested using
OnceLockto avoid repeated leaks on multiple calls. The current implementation will leak memory each timeset_error_callbackis called. While acceptable for single-call usage, consider theOnceLockpattern if users might call this multiple times.
🧹 Nitpick comments (6)
bindings/rust/src/error.rs (1)
4-89: Consider using thiserror to reduce boilerplate.The manual
DisplayandErrorimplementations (lines 64-89) work correctly but could be simplified using thethiserrorcrate that's already in your dependencies. This would reduce boilerplate, ensure consistent error message formatting, and align with Rust ecosystem conventions.🔎 Example with thiserror
+use thiserror::Error; + /// Error types for ccap operations -#[derive(Debug)] +#[derive(Debug, Error)] pub enum CcapError { /// No error occurred + #[error("No error")] None, /// No camera device found + #[error("No camera device found")] NoDeviceFound, /// Invalid device specified + #[error("Invalid device: {0}")] InvalidDevice(String), /// Camera device open failed + #[error("Camera device open failed")] DeviceOpenFailed, // ... (apply pattern to remaining variants) /// Unknown error with error code + #[error("Unknown error: {code}")] Unknown { /// Error code from the underlying system code: i32, }, } - -impl std::fmt::Display for CcapError { - // ... (remove manual Display impl) -} - -impl std::error::Error for CcapError {}bindings/rust/examples/minimal_example.rs (1)
17-17: Consider safe integer conversion to prevent potential overflow.Line 17 casts
camera_index(usize) directly toi32. While unlikely in practice, on 64-bit systems with large indices, this could overflow and produce negative or incorrect values. Consider using a checked conversion for robustness.🔎 Proposed fix with safe conversion
- let mut provider = Provider::with_device(camera_index as i32)?; + let device_index = i32::try_from(camera_index) + .map_err(|_| ccap::CcapError::InvalidParameter( + format!("Device index {} is too large", camera_index) + ))?; + let mut provider = Provider::with_device(device_index)?;bindings/rust/examples/capture_callback.rs (1)
37-37: Consider safe integer conversion to prevent potential overflow.Line 37 has the same integer cast issue as minimal_example.rs:
device_index as i32could overflow on 64-bit systems. Consider usingi32::try_from()for safer conversion.🔎 Proposed fix
- let mut provider = Provider::with_device(device_index as i32)?; + let device_index_i32 = i32::try_from(device_index) + .map_err(|_| ccap::CcapError::InvalidParameter( + format!("Device index {} is too large", device_index) + ))?; + let mut provider = Provider::with_device(device_index_i32)?;bindings/rust/src/async.rs (1)
95-97: Redundant feature gate.The
#[cfg(feature = "async")]on line 95 is unnecessary since the entireimpl AsyncProviderblock starting at line 27 is already gated by the same feature.🔎 Proposed fix
- #[cfg(feature = "async")] /// Create a stream of frames pub fn frame_stream(&mut self) -> impl futures::Stream<Item = VideoFrame> {bindings/rust/src/frame.rs (1)
154-178: Consider documenting FFI overhead in convenience methods.Each convenience method (
width(),height(), etc.) makes a separate FFI call viainfo(). For performance-critical code paths where multiple properties are needed, users should callinfo()once and access fields directly. A doc comment would help clarify this.bindings/rust/src/provider.rs (1)
14-14: Add safety documentation for unsafe impl Send.Similar to
VideoFrame, theSendimpl forProviderassumes thread-safety of the underlying C library. Add a// SAFETY:comment documenting the invariants.+// SAFETY: Provider's handle operations are thread-safe in the ccap C library. +// The callback_ptr is only accessed while holding appropriate ownership. unsafe impl Send for Provider {}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/rust.ymlbindings/rust/README.mdbindings/rust/build.rsbindings/rust/build_and_test.shbindings/rust/examples/capture_callback.rsbindings/rust/examples/capture_grab.rsbindings/rust/examples/minimal_example.rsbindings/rust/examples/print_camera.rsbindings/rust/src/async.rsbindings/rust/src/convert.rsbindings/rust/src/error.rsbindings/rust/src/frame.rsbindings/rust/src/lib.rsbindings/rust/src/provider.rsbindings/rust/src/types.rsbindings/rust/src/utils.rsbindings/rust/tests/integration_tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- bindings/rust/examples/print_camera.rs
- bindings/rust/build_and_test.sh
- .github/workflows/rust.yml
- bindings/rust/examples/capture_grab.rs
- bindings/rust/src/types.rs
🧰 Additional context used
🧬 Code graph analysis (6)
bindings/rust/tests/integration_tests.rs (4)
bindings/rust/src/async.rs (2)
new(30-39)is_opened(66-69)bindings/rust/src/provider.rs (4)
new(18-29)is_opened(209-211)version(291-302)with_device(32-46)bindings/rust/src/lib.rs (1)
version(38-40)bindings/rust/src/types.rs (2)
from_c_enum(62-64)from_c_enum(197-207)
bindings/rust/src/lib.rs (2)
include/ccap_utils.h (1)
LogLevel(80-140)bindings/rust/src/provider.rs (1)
version(291-302)
bindings/rust/src/async.rs (1)
bindings/rust/src/provider.rs (8)
new(18-29)find_device_names(311-313)open(147-159)start(199-201)stop(204-206)is_opened(209-211)is_started(194-196)grab_frame(257-268)
bindings/rust/src/convert.rs (1)
bindings/rust/src/types.rs (2)
from_c_enum(62-64)from_c_enum(197-207)
bindings/rust/src/utils.rs (2)
include/ccap_utils.h (1)
LogLevel(80-140)bindings/rust/src/types.rs (3)
to_c_enum(57-59)to_c_enum(148-150)to_c_enum(185-194)
bindings/rust/examples/minimal_example.rs (4)
bindings/rust/examples/print_camera.rs (1)
main(62-83)bindings/rust/src/provider.rs (4)
set_error_callback(334-373)new(18-29)with_device(32-46)pixel_format(323-326)bindings/rust/src/utils.rs (1)
select_camera(201-234)bindings/rust/src/frame.rs (5)
index(176-178)width(154-156)height(159-161)data_size(171-173)pixel_format(164-168)
🪛 GitHub Actions: Rust CI
bindings/rust/src/convert.rs
[error] 205-205: this function has too many arguments (8/7) during cargo clippy linting. Command: cargo clippy --all-targets --features static-link -- -D warnings
🪛 LanguageTool
bindings/rust/README.md
[style] ~180-~180: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request. ## License Thi...
(FEEL_FREE_TO_STYLE_ME)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Build (Release-clang-shared)
- GitHub Check: Build ARM64 (Release-gcc-shared)
- GitHub Check: Build (Release-gcc-static)
- GitHub Check: Build (Debug-gcc-shared)
- GitHub Check: Build (Debug-gcc-static)
- GitHub Check: Build (Debug-clang-shared)
- GitHub Check: Static Link (Dev) (windows-latest)
- GitHub Check: Static Link (Dev) (ubuntu-latest)
- GitHub Check: Static Link (Dev) (macos-latest)
- GitHub Check: macOS Build (Debug-static)
- GitHub Check: Windows Build MinGW (Debug-static)
- GitHub Check: Windows Build MinGW (Release-shared)
- GitHub Check: Windows Build MinGW (Release-static)
- GitHub Check: Windows Build VS2022 (Release-static)
- GitHub Check: Windows Build VS2022 (Release-shared)
- GitHub Check: Windows Build VS2022 (Debug-shared)
🔇 Additional comments (8)
bindings/rust/README.md (1)
68-90: The async API signatures in the example are correct. The review shows:
AsyncProvider::open(&self, device_name: Option<&str>, auto_start: bool)matches the implementation at line 48 ofsrc/async.rsAsyncProvider::grab_frame()matches the implementation at line 91, which defaults to a 0ms timeoutNo issues found.
bindings/rust/src/async.rs (2)
17-25: Acknowledged incomplete feature with proper documentation.The doc comment and
#[allow(dead_code)]appropriately document that frame streaming is reserved for future implementation. The current approach is acceptable for initial release.
78-88: Correct async-to-sync bridge pattern.Using
spawn_blockingwithblocking_lock()is the right approach to avoid blocking the async runtime while calling synchronous FFI code.bindings/rust/src/frame.rs (2)
24-35: Proper bounds checking implemented.The use of
.min()to clamp counts to array lengths prevents potential panics from malformed C data. This addresses the previous review concern.
93-106: Plane size calculations now correctly account for stride × height.The fix properly computes plane sizes: full height for plane 0, and
(height + 1) / 2for chroma planes, which is correct for common YUV formats like NV12 and I420.bindings/rust/src/provider.rs (3)
161-186: Device name parameter is now properly handled.The implementation correctly destroys the old handle, creates a new provider with the specified device name, and handles the auto_start flag. This addresses the previous review concern.
322-326: Type conversion is now correct.Using
PixelFormat::from_c_enum(format_val as sys::CcapPixelFormat)properly converts the property value to the enum type.
469-477: Callback cleanup and Drop implementation are correct.The
cleanup_callbackmethod properly frees the boxed callback, andDropensures resources are released. This addresses the previous memory leak concerns for frame callbacks.Also applies to: 480-491
| ) -> Result<Vec<u8>> { | ||
| let dst_stride = (width * 3) as usize; | ||
| let dst_size = dst_stride * height as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| unsafe { | ||
| sys::ccap_convert_yuyv_to_rgb24( | ||
| src_data.as_ptr(), | ||
| src_stride as c_int, | ||
| dst_data.as_mut_ptr(), | ||
| dst_stride as c_int, | ||
| width as c_int, | ||
| height as c_int, | ||
| sys::CcapConvertFlag_CCAP_CONVERT_FLAG_DEFAULT, | ||
| ) | ||
| }; | ||
|
|
||
| Ok(dst_data) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.h" | head -20Repository: wysaid/CameraCapture
Length of output: 878
🏁 Script executed:
fd -e h -e rs "convert" | grep -E "\.(h|rs)$" | head -20Repository: wysaid/CameraCapture
Length of output: 238
🏁 Script executed:
git ls-files | grep -E "bindings/rust/src/convert\.rs"Repository: wysaid/CameraCapture
Length of output: 92
🏁 Script executed:
cat include/ccap_convert.h | head -100Repository: wysaid/CameraCapture
Length of output: 3532
🏁 Script executed:
cat include/ccap_convert_c.h | head -100Repository: wysaid/CameraCapture
Length of output: 3536
🏁 Script executed:
cat -n bindings/rust/src/convert.rs | head -250Repository: wysaid/CameraCapture
Length of output: 8617
🏁 Script executed:
cat include/ccap_convert_c.h | tail -150Repository: wysaid/CameraCapture
Length of output: 6348
🏁 Script executed:
grep -A 5 "ccap_convert_yuyv_to_rgb24\|ccap_convert_nv12_to_rgb24\|ccap_convert_i420_to_rgb24" include/ccap_convert_c.hRepository: wysaid/CameraCapture
Length of output: 999
Remove misleading Result wrapper—C functions return void with no failure path.
All conversion functions return Result<Vec<u8>> despite the underlying C FFI functions returning void and having no error indication mechanism. The return type should be Vec<u8> directly since there is no error case to handle.
This applies to all conversion methods in this file: yuyv_to_rgb24, yuyv_to_bgr24, rgb_to_bgr, bgr_to_rgb, nv12_to_rgb24, nv12_to_bgr24, i420_to_rgb24, i420_to_bgr24, and any others following this pattern.
🤖 Prompt for AI Agents
In bindings/rust/src/convert.rs around lines 48 to 66, the conversion functions
currently return Result<Vec<u8>> even though the underlying C FFI conversion
functions return void and cannot fail; change the signature of yuyv_to_rgb24
(and all other conversion functions listed: yuyv_to_bgr24, rgb_to_bgr,
bgr_to_rgb, nv12_to_rgb24, nv12_to_bgr24, i420_to_rgb24, i420_to_bgr24, etc.) to
return Vec<u8> directly, remove Result usage and any Ok(...) wrappers, and
eliminate unused Result/any error imports so each function allocates the dst
buffer, calls the unsafe FFI, and returns the Vec<u8> without wrapping it in
Result.
| let callback = Arc::new(Mutex::new(callback)); | ||
|
|
||
| unsafe extern "C" fn error_callback_wrapper( | ||
| error_code: sys::CcapErrorCode, | ||
| description: *const c_char, | ||
| user_data: *mut std::ffi::c_void, | ||
| ) { | ||
| if user_data.is_null() || description.is_null() { | ||
| return; | ||
| } | ||
|
|
||
| let callback = &*(user_data as *const Arc<Mutex<dyn Fn(u32, &str) + Send + Sync>>); | ||
| let desc_cstr = std::ffi::CStr::from_ptr(description); | ||
| if let Ok(desc_str) = desc_cstr.to_str() { | ||
| if let Ok(cb) = callback.lock() { | ||
| cb(error_code, desc_str); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Store the callback to prevent it from being dropped | ||
| let callback_ptr = Box::into_raw(Box::new(callback)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n bindings/rust/src/provider.rs | sed -n '330,370p'Repository: wysaid/CameraCapture
Length of output: 1658
🏁 Script executed:
# Check if tests exercise the error callback
rg -n "set_error_callback|error_callback" bindings/rust --type rust -A 3 -B 1Repository: wysaid/CameraCapture
Length of output: 2937
🏁 Script executed:
# Look for the complete context around the callback setup
rg -n "Box::into_raw" bindings/rust --type rust -B 10 -A 5Repository: wysaid/CameraCapture
Length of output: 2449
Fix unsound pointer cast in error callback setup.
Line 352 unsafely casts a pointer to Arc<Mutex<F>> (concrete type) as a pointer to Arc<Mutex<dyn Fn>> (trait object). These pointer types have incompatible layouts: the trait object pointer is a fat pointer requiring two words (data + vtable), but the concrete pointer is a single word. This is undefined behavior.
Use the trait object type from the start:
- let callback = Arc::new(Mutex::new(callback));
+ let callback: Arc<Mutex<dyn Fn(u32, &str) + Send + Sync>> = Arc::new(Mutex::new(callback));This ensures Arc wraps the trait object directly, making the cast at line 352 valid and safe.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bindings/rust/src/provider.rs around lines 341 to 362, the code unsafely
casts a pointer to Arc<Mutex<F>> into a pointer to Arc<Mutex<dyn Fn(u32, &str) +
Send + Sync>>, which is UB because trait objects are fat pointers; instead
construct and store the trait object type from the start: change the local
callback variable to be an Arc<Mutex<dyn Fn(u32, &str) + Send + Sync>> (by
boxing/cloning the closure into that trait object), then
Box::into_raw(Box::new(callback_trait_object)) so the raw pointer has the
correct fat-pointer layout, and update any uses of the pointer and the unsafe
wrapper to treat user_data as *const Arc<Mutex<dyn Fn(u32, &str) + Send +
Sync>>; remove the earlier incorrect cast. Ensure all clones/ownership
adjustments still prevent drop and that locking and invocation remain unchanged.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
bindings/rust/src/convert.rs (1)
43-66:Resultwrapper is misleading—C FFI functions returnvoid.All conversion functions (including this one) return
Result<Vec<u8>>but the underlying C functions likeccap_convert_yuyv_to_rgb24returnvoidwith no error indication. TheOk(dst_data)on line 65 always succeeds, making theResulttype misleading to callers.Either change the return type to
Vec<u8>directly, or add actual validation (e.g., input size checks) that could produce errors.CMakeLists.txt (1)
404-406: Circular dependency betweenccapandccap-rusttargets.Line 394 declares
ccap-rustwithDEPENDS ccap, meaningccap-rustdepends onccap. Line 405 then addsadd_dependencies(ccap ccap-rust), creating the reverse dependency. This circular ordering will cause CMake build failures.Remove line 405 (
add_dependencies(ccap ccap-rust)) or introduce a separate aggregation target (e.g.,ccap-all) that depends on bothccapandccap-rustif you want a unified build command.🔎 Proposed fix
# Add to main build if requested if (CCAP_IS_ROOT_PROJECT) - add_dependencies(ccap ccap-rust) + # Create an aggregation target instead of circular dependency + add_custom_target(ccap-all DEPENDS ccap ccap-rust) endif ()bindings/rust/src/provider.rs (1)
338-359: Undefined behavior: pointer layout mismatch in callback reconstruction.Line 338 creates
Arc<Mutex<F>>(concrete type), but line 349 castsuser_datato*const Arc<Mutex<dyn Fn(u32, &str) + Send + Sync>>(trait object). These have incompatible layouts—the trait object pointer is a fat pointer (two words: data + vtable), but the concrete pointer is a single word.Construct the trait object type from the start:
🔎 Proposed fix
pub fn set_error_callback<F>(callback: F) where F: Fn(u32, &str) + Send + Sync + 'static, { use std::os::raw::c_char; use std::sync::{Arc, Mutex}; - let callback = Arc::new(Mutex::new(callback)); + let callback: Arc<Mutex<dyn Fn(u32, &str) + Send + Sync>> = Arc::new(Mutex::new(callback));
🧹 Nitpick comments (2)
CMakeLists.txt (1)
390-395: Cargo always builds in--releasemode regardless of CMake build type.When CMake is configured for Debug, the C++
ccaplibrary will have a debug postfix (ccapdon MSVC per line 178). However,cargo build --releasehere will link against release artifacts. This may cause linking failures or mismatched symbols in Debug configurations.Consider passing the build profile based on
CMAKE_BUILD_TYPE:🔎 Proposed fix
+ # Determine Cargo profile based on CMake build type + if (CMAKE_BUILD_TYPE STREQUAL "Debug") + set(CARGO_PROFILE "") + else () + set(CARGO_PROFILE "--release") + endif () + # Add custom target to build Rust bindings add_custom_target(ccap-rust - COMMAND ${CARGO_CMD} build --release + COMMAND ${CARGO_CMD} build ${CARGO_PROFILE} WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/bindings/rust COMMENT "Building Rust bindings" DEPENDS ccap )bindings/rust/src/utils.rs (1)
217-226:flush().unwrap()can panic; parse error handling is silent.Line 219 uses
.unwrap()which can panic if stdout flush fails (e.g., broken pipe). Consider usinglet _ = io::stdout().flush();or propagating the error.Line 226 silently defaults to index 0 on parse failure without informing the user. The bounds check at line 228 partially addresses this, but the user receives no feedback that their input was invalid.
🔎 Proposed improvement
print!("Enter the index of the device you want to use: "); use std::io::{self, Write}; - io::stdout().flush().unwrap(); + let _ = io::stdout().flush(); let mut input = String::new(); io::stdin() .read_line(&mut input) .map_err(|e| CcapError::InvalidParameter(format!("Failed to read input: {}", e)))?; - let selected_index = input.trim().parse::<usize>().unwrap_or(0); + let selected_index = match input.trim().parse::<usize>() { + Ok(index) => index, + Err(_) => { + println!("Invalid input, using the first device: {}", devices[0]); + return Ok(0); + } + };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/tasks.jsonCMakeLists.txtbindings/rust/src/convert.rsbindings/rust/src/provider.rsbindings/rust/src/types.rsbindings/rust/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .vscode/tasks.json
- bindings/rust/src/types.rs
🧰 Additional context used
🧬 Code graph analysis (3)
bindings/rust/src/utils.rs (2)
include/ccap_utils.h (1)
LogLevel(80-140)bindings/rust/src/types.rs (3)
to_c_enum(57-59)to_c_enum(148-150)to_c_enum(185-194)
bindings/rust/src/provider.rs (5)
include/ccap_def.h (1)
DeviceInfo(425-442)bindings/rust/src/types.rs (5)
from(35-52)from(87-103)from(116-126)from(154-167)from(220-225)bindings/rust/src/error.rs (1)
from(92-114)bindings/rust/src/frame.rs (6)
width(154-156)height(159-161)from_c_ptr(52-57)info(87-138)from_c_ptr_ref(60-65)drop(182-188)bindings/rust/src/lib.rs (1)
version(38-40)
bindings/rust/src/convert.rs (2)
bindings/rust/src/types.rs (2)
from_c_enum(62-64)from_c_enum(197-207)bindings/rust/src/frame.rs (2)
width(154-156)height(159-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Windows Build VS2022 (Release-static)
- GitHub Check: Windows Build MinGW (Release-static)
- GitHub Check: Build ARM64 (Debug-gcc-static)
- GitHub Check: Windows Build VS2022 (Debug-shared)
- GitHub Check: Build ARM64 (Debug-gcc-shared)
- GitHub Check: Build (Release-gcc-static)
- GitHub Check: Build ARM64 (Release-gcc-shared)
- GitHub Check: Build (Debug-gcc-shared)
- GitHub Check: Windows Build MinGW (Debug-static)
- GitHub Check: Build (Release-clang-static)
- GitHub Check: Build Fedora (Debug-shared)
- GitHub Check: Windows Build VS2022 (Release-shared)
- GitHub Check: Windows Build MinGW (Release-shared)
- GitHub Check: Build Fedora (Release-static)
- GitHub Check: Build ARM64 (Release-gcc-static)
- GitHub Check: Build Fedora (Release-shared)
- GitHub Check: Windows VS2022 Release - No File Playback
- GitHub Check: Build (Release-gcc-shared)
- GitHub Check: macOS Release - No File Playback
- GitHub Check: macOS Build (Release-shared)
🔇 Additional comments (7)
bindings/rust/src/convert.rs (1)
9-40: LGTM!The backend management and feature detection methods are well-implemented with appropriate error handling for
set_backend.bindings/rust/src/utils.rs (3)
13-29: LGTM!The two-step pattern (check return code, then convert) correctly handles the FFI string conversion with appropriate error handling.
62-106: LGTM!The two-step buffer sizing pattern is correctly implemented: first call gets required size, second call fills the buffer. Error handling covers both the FFI failure and UTF-8 conversion cases.
245-270: LGTM!The
LogLevelenum provides a clean Rust abstraction over the C API's bitmask-based log levels. Theto_c_enummethod correctly maps to the underlying C constants.bindings/rust/src/provider.rs (3)
161-186: LGTM!The
open_devicemethod now properly handles thedevice_nameparameter by recreating the provider with the specified device, addressing the previous review feedback.
319-323: LGTM!The
pixel_formatmethod now correctly usesPixelFormat::from_c_enumfor the type conversion, addressing the previous review feedback.
477-488: LGTM!The
Dropimplementation properly cleans up the callback before destroying the handle, preventing resource leaks.
| unsafe { | ||
| sys::ccap_convert_yuyv_to_rgb24( | ||
| src_data.as_ptr(), | ||
| src_stride as c_int, | ||
| dst_data.as_mut_ptr(), | ||
| dst_stride as c_int, | ||
| width as c_int, | ||
| height as c_int, | ||
| sys::CcapConvertFlag_CCAP_CONVERT_FLAG_DEFAULT, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Missing input buffer size validation—potential out-of-bounds read.
The function passes src_data.as_ptr() to the C FFI without validating that src_data.len() >= src_stride * height. If the caller provides an undersized buffer, the C code will read beyond the slice bounds, causing undefined behavior.
Add size validation before the unsafe block:
🔎 Proposed fix
pub fn yuyv_to_rgb24(
src_data: &[u8],
src_stride: usize,
width: u32,
height: u32,
) -> Result<Vec<u8>> {
+ let expected_src_size = src_stride * height as usize;
+ if src_data.len() < expected_src_size {
+ return Err(CcapError::InvalidParameter(format!(
+ "source buffer too small: {} < {}",
+ src_data.len(),
+ expected_src_size
+ )));
+ }
+
let dst_stride = (width * 3) as usize;Committable suggestion skipped: line range outside the PR's diff.
| pub fn nv12_to_rgb24( | ||
| y_data: &[u8], | ||
| y_stride: usize, | ||
| uv_data: &[u8], | ||
| uv_stride: usize, | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>> { | ||
| let dst_stride = (width * 3) as usize; | ||
| let dst_size = dst_stride * height as usize; | ||
| let mut dst_data = vec![0u8; dst_size]; | ||
|
|
||
| unsafe { | ||
| sys::ccap_convert_nv12_to_rgb24( | ||
| y_data.as_ptr(), | ||
| y_stride as c_int, | ||
| uv_data.as_ptr(), | ||
| uv_stride as c_int, | ||
| dst_data.as_mut_ptr(), | ||
| dst_stride as c_int, | ||
| width as c_int, | ||
| height as c_int, | ||
| sys::CcapConvertFlag_CCAP_CONVERT_FLAG_DEFAULT, | ||
| ) | ||
| }; | ||
|
|
||
| Ok(dst_data) | ||
| } |
There was a problem hiding this comment.
Same input validation issue applies to planar format conversions.
Similar to yuyv_to_rgb24, this function and i420_to_* variants pass buffer pointers to C FFI without validating sizes. For NV12:
y_dataneeds at leasty_stride * heightbytesuv_dataneeds at leastuv_stride * (height / 2)bytes
Add validation for all input planes before the unsafe block to prevent out-of-bounds reads.
🤖 Prompt for AI Agents
In bindings/rust/src/convert.rs around lines 145 to 172, the NV12 conversion
calls the C FFI without validating input buffer sizes; add checks before the
unsafe block to ensure y_data.len() >= y_stride * height and uv_data.len() >=
uv_stride * (height / 2) (use height as usize and integer division for the UV
plane), and return an Err with a descriptive message if either check fails so we
never pass invalid pointers to the FFI. Ensure the same style/Result error type
used elsewhere in the file is used for the returned error.
| callback_ptr: Option<*mut std::ffi::c_void>, | ||
| } | ||
|
|
||
| unsafe impl Send for Provider {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, explore the repository structure
echo "=== Repository structure ==="
git ls-files | head -20
echo -e "\n=== Looking for C API headers ==="
fd -t f -e h . | head -20
echo -e "\n=== Checking provider.rs location and structure ==="
find . -name "provider.rs" -type fRepository: wysaid/CameraCapture
Length of output: 1394
🏁 Script executed:
#!/bin/bash
# Check the Provider struct definition and surrounding code
echo "=== Provider struct definition ==="
head -100 bindings/rust/src/provider.rs | cat -nRepository: wysaid/CameraCapture
Length of output: 4210
🏁 Script executed:
#!/bin/bash
# Search for C API headers related to provider/camera
echo "=== Looking for C API provider headers ==="
fd -t f -e h . | xargs grep -l "provider\|Provider\|camera\|Camera" 2>/dev/null | head -10Repository: wysaid/CameraCapture
Length of output: 358
🏁 Script executed:
#!/bin/bash
# Check the main C API header for CcapProvider definition and documentation
echo "=== Checking ccap_c.h for CcapProvider ==="
cat include/ccap_c.h | head -200Repository: wysaid/CameraCapture
Length of output: 8957
🏁 Script executed:
#!/bin/bash
# Search for CcapProvider definition and any thread-safety comments
echo "=== Searching for CcapProvider definition ==="
rg -n "CcapProvider\|typedef.*Provider" include/ -C 3Repository: wysaid/CameraCapture
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Search for any thread-safety documentation in C headers
echo "=== Searching for thread-safety comments ==="
rg -n "thread|Thread|concurrent|Concurrent|thread-safe|thread safe" include/ -C 2Repository: wysaid/CameraCapture
Length of output: 3500
🏁 Script executed:
#!/bin/bash
# Check if there are any mutex/lock patterns in the C API implementation
echo "=== Searching for synchronization primitives in source ==="
rg -n "mutex|Mutex|lock|Lock|atomic|Atomic|critical" src/ --type c --type h -C 1 | head -100Repository: wysaid/CameraCapture
Length of output: 2883
🏁 Script executed:
#!/bin/bash
# Double-check the exact thread-safety statement in the header
grep -A 5 "not thread-safe" include/ccap_core.hRepository: wysaid/CameraCapture
Length of output: 380
🏁 Script executed:
#!/bin/bash
# Check if there's any synchronization wrapper in the Rust binding
rg -n "Mutex|RwLock|Arc" bindings/rust/src/provider.rsRepository: wysaid/CameraCapture
Length of output: 263
Remove unsafe impl Send or wrap Provider in Arc<Mutex<>>.
The underlying C++ Provider is explicitly documented as not thread-safe: "This class is not thread-safe. It is recommended to use it in a single thread." The current unsafe impl Send allows transferring the non-thread-safe handle across thread boundaries without synchronization, violating the C API contract and risking undefined behavior. Either remove the Send impl or wrap the handle with Arc<Mutex<>> for actual thread-safe sharing.
| let callback = &*(user_data as *const Box<dyn Fn(&VideoFrame) -> bool + Send + Sync>); | ||
|
|
||
| // Create a temporary VideoFrame wrapper that doesn't own the frame | ||
| let video_frame = VideoFrame::from_c_ptr_ref(frame as *mut sys::CcapVideoFrame); | ||
| callback(&video_frame) | ||
| } | ||
|
|
||
| // Store the callback to prevent it from being dropped | ||
| let callback_box = Box::new(callback); | ||
| let callback_ptr = Box::into_raw(callback_box); |
There was a problem hiding this comment.
Same UB: concrete Box<F> cast to Box<dyn Fn...> trait object.
Line 428 creates Box::new(callback) where callback is the concrete type F. Line 420 then casts user_data to *const Box<dyn Fn(&VideoFrame) -> bool + Send + Sync>. These have incompatible layouts (thin vs fat pointer).
🔎 Proposed fix
// Store the callback to prevent it from being dropped
- let callback_box = Box::new(callback);
+ let callback_box: Box<dyn Fn(&VideoFrame) -> bool + Send + Sync> = Box::new(callback);
let callback_ptr = Box::into_raw(callback_box);🤖 Prompt for AI Agents
In bindings/rust/src/provider.rs around lines 420 to 429, the code boxes the
concrete closure type F then later casts the raw pointer to a Box<dyn
Fn(&VideoFrame) -> bool + Send + Sync>, causing undefined behavior due to
incompatible layouts; fix by boxing the closure as a trait object up front
(i.e., create a Box<dyn Fn(&VideoFrame)->bool + Send + Sync> from the closure,
ensuring F: Fn(&VideoFrame)->bool + Send + Sync + 'static so coercion succeeds),
store/convert that boxed trait object with Box::into_raw, and keep the raw
pointer cast and user_data access matching the Box<dyn ...> type so the layouts
agree and the callback isn't dropped.
- Remove unused PixelFormat import in capture_grab.rs - Simplify device_index selection logic in capture_callback.rs - Remove unnecessary type cast (device_index as i32) - Add cache: false to Rust toolchain setup to avoid Cargo.toml lookup errors Fixes clippy warnings: - unused-imports - if-same-then-else - unnecessary-cast
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/rust.yml (3)
55-60: Consider enabling Rust caching to improve CI performance.The workflow explicitly sets
cache: false, which disables caching of Rust dependencies and build artifacts. This increases CI runtime.🔎 Proposed fix
- name: Install Rust toolchain uses: actions-rust-lang/setup-rust-toolchain@v1 with: toolchain: stable components: clippy, rustfmt - cache: falseRemoving
cache: falseenables the action's default caching behavior, which caches cargo registry, git dependencies, and target directories.
88-91: Consider running clippy for build-source feature as well.Clippy currently only checks code with the
static-linkfeature enabled. Conditional compilation blocks under thebuild-sourcefeature won't be linted, potentially missing issues in distribution mode.🔎 Suggested addition
Add a clippy check in the
build-sourcejob as well:- name: Run clippy (Source) if: matrix.os == 'ubuntu-latest' working-directory: bindings/rust run: cargo clippy --all-targets --no-default-features --features build-source -- -D warnings
121-125: Consider enabling Rust caching to improve CI performance.Same issue as the
static-linkjob:cache: falsedisables caching and slows down CI.🔎 Proposed fix
- name: Install Rust toolchain uses: actions-rust-lang/setup-rust-toolchain@v1 with: toolchain: stable - cache: false
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/rust.ymlbindings/rust/examples/capture_callback.rsbindings/rust/examples/capture_grab.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- bindings/rust/examples/capture_callback.rs
- bindings/rust/examples/capture_grab.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Build (Debug-clang-static)
- GitHub Check: macOS Build (Debug-static)
- GitHub Check: Build Fedora (Release-static)
- GitHub Check: Build Fedora (Release-shared)
- GitHub Check: macOS Build (Release-static)
- GitHub Check: Build Fedora (Debug-shared)
- GitHub Check: Build Source (Dist) (ubuntu-latest)
- GitHub Check: Build Source (Dist) (windows-latest)
- GitHub Check: Build Source (Dist) (macos-latest)
- GitHub Check: Static Link (Dev) (macos-latest)
- GitHub Check: Static Link (Dev) (windows-latest)
- GitHub Check: Windows Build MinGW (Release-shared)
- GitHub Check: Windows Build MinGW (Debug-static)
- GitHub Check: Windows Build MinGW (Debug-shared)
- GitHub Check: Windows Build MinGW (Release-static)
- GitHub Check: Windows VS2022 Release - No File Playback
- GitHub Check: Windows Build VS2022 (Release-shared)
- GitHub Check: Windows Build VS2022 (Release-static)
- GitHub Check: Static Link (Dev) (windows-latest)
- GitHub Check: Static Link (Dev) (macos-latest)
🔇 Additional comments (4)
.github/workflows/rust.yml (4)
1-19: LGTM! Trigger configuration is well-designed.The workflow triggers and path filters are appropriate, ensuring CI runs when Rust bindings or their C++ dependencies change.
21-25: LGTM! Permissions follow least-privilege principle.The explicit
contents: readpermission addresses the security concern flagged by CodeQL.
27-135: Workflow structure effectively tests both build modes.The separation into
static-link(development) andbuild-source(distribution) jobs provides good coverage for both usage scenarios of the Rust bindings.
115-119: Thebuild-sourcejob correctly omits cmake as it compiles C++ directly with thecccrate.The build.rs script uses
cc::Build::new()to compile C++ sources directly without requiring CMake. It includes only the project's standard headers frominclude/andsrc/directories, not cmake-generated files. Thebuild-essentialandpkg-configpackages are sufficient since system compilers (g++, clang, MSVC) are pre-installed on GitHub runners.
Problem Analysis: - Build Source (Dist) mode on macOS/Windows was failing with undefined symbols - Root cause: ccap_file_reader_apple.mm and ccap_file_reader_windows.cpp were missing - These files are required when CCAP_ENABLE_FILE_PLAYBACK is enabled (default ON in CMake) - The FileReaderApple/Windows classes are referenced in ccap_imp_apple.mm and ccap_imp_windows.cpp Changes: - Add ccap_file_reader_apple.mm to macOS platform sources - Add ccap_file_reader_windows.cpp to Windows platform sources - Define CCAP_ENABLE_FILE_PLAYBACK=1 to enable file playback functionality Verification: - Linux build-source mode: ✅ cargo build --no-default-features --features build-source - Linux tests: ✅ cargo test --no-default-features --features build-source - All 11 tests passed This fix ensures build-source mode includes all required source files for the file playback feature, resolving linker errors on macOS and Windows.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bindings/rust/build.rs (2)
15-21: Chainedunwrap()could panic in unexpected directory structures.The
.parent().unwrap().parent().unwrap()chain will panic if the manifest directory doesn't have at least two parent directories. While unlikely in the expected repository structure, this creates a fragile assumption.🔎 Proposed fix with better error handling
- ( - manifest_path - .parent() - .unwrap() - .parent() - .unwrap() - .to_path_buf(), - false, - ) + ( + manifest_path + .parent() + .and_then(|p| p.parent()) + .expect("Cargo manifest must be at least 2 directories deep (bindings/rust/)") + .to_path_buf(), + false, + )
170-174: Rerun triggers use hardcoded paths that break in packaged mode.The
rerun-if-changeddirectives use hardcoded relative paths (../../include/*) that assume repository layout. When the crate is packaged (withccap_rootpointing tonative/), these paths won't exist, causing the rerun triggers to fail silently.🔎 Proposed fix using ccap_root
println!("cargo:rerun-if-changed=wrapper.h"); - println!("cargo:rerun-if-changed=../../include/ccap_c.h"); - println!("cargo:rerun-if-changed=../../include/ccap_utils_c.h"); - println!("cargo:rerun-if-changed=../../include/ccap_convert_c.h"); + println!("cargo:rerun-if-changed={}/include/ccap_c.h", ccap_root.display()); + println!("cargo:rerun-if-changed={}/include/ccap_utils_c.h", ccap_root.display()); + println!("cargo:rerun-if-changed={}/include/ccap_convert_c.h", ccap_root.display());
🧹 Nitpick comments (1)
bindings/rust/build.rs (1)
176-189: UpdateCargoCallbacksto use the new constructor API.Starting in bindgen 0.68,
CargoCallbacksrequires explicit instantiation with::new(). The current code uses the deprecated unit-struct form, which triggers deprecation warnings.Proposed fix
let bindings = bindgen::Builder::default() .header("wrapper.h") .clang_arg(format!("-I{}/include", ccap_root.display())) - .parse_callbacks(Box::new(bindgen::CargoCallbacks)) + .parse_callbacks(Box::new(bindgen::CargoCallbacks::new())) .allowlist_function("ccap_.*")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bindings/rust/build.rs
🧰 Additional context used
🧬 Code graph analysis (1)
bindings/rust/build.rs (2)
bindings/rust/src/types.rs (5)
from(35-52)from(87-103)from(116-126)from(154-167)from(220-225)bindings/rust/src/error.rs (1)
from(92-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Build (Release-gcc-static)
- GitHub Check: Build ARM64 (Release-gcc-shared)
- GitHub Check: Build ARM64 (Debug-gcc-static)
- GitHub Check: Build (Release-clang-static)
- GitHub Check: Build Source (Dist) (windows-latest)
- GitHub Check: Windows Build MinGW (Release-shared)
- GitHub Check: Static Link (Dev) (ubuntu-latest)
- GitHub Check: Windows Build VS2022 (Release-static)
- GitHub Check: Windows Build MinGW (Release-static)
- GitHub Check: Windows Build VS2022 (Debug-shared)
- GitHub Check: Build Source (Dist) (macos-latest)
- GitHub Check: Static Link (Dev) (windows-latest)
- GitHub Check: Windows Build MinGW (Debug-static)
- GitHub Check: Windows VS2022 Release - No File Playback
- GitHub Check: Windows Build VS2022 (Release-shared)
- GitHub Check: macOS Build (Release-shared)
- GitHub Check: macOS Build (Debug-shared)
- GitHub Check: Static Link (Dev) (windows-latest)
- GitHub Check: Static Link (Dev) (macos-latest)
🔇 Additional comments (5)
bindings/rust/build.rs (5)
1-7: LGTM!Standard imports and manifest directory initialization are correct.
29-82: LGTM!The build-from-source configuration is well-structured with appropriate platform-specific handling, C++17 standard, and ARC support for macOS.
112-142: LGTM!The pre-built library linking logic with Debug/Release fallback paths is well-designed for development workflows.
144-168: LGTM!Platform-specific library linking is comprehensive and appropriate for the underlying C++ library's dependencies.
191-195: LGTM!Standard bindgen output handling with appropriate error messages.
Problem Analysis: - Windows builds were failing with type mismatch errors in error.rs and provider.rs - Root cause: CcapErrorCode constants have platform-dependent types - Linux/macOS: c_uint (u32) via GCC/Clang - Windows: c_int (i32) via MSVC - Original code assumed u32 and cast i32 to u32, causing type errors on Windows Error Details: - error.rs:97: expected u32, found i32 for CcapErrorCode constants - provider.rs:353: error callback expected u32 but received i32 Changes: 1. error.rs: Convert i32 to CcapErrorCode type before matching - Use 'let code_u = code as CcapErrorCode' to handle platform differences - This ensures constants match regardless of underlying type 2. provider.rs: Change error callback signature from Fn(u32, &str) to Fn(i32, &str) - Use i32 as the common error code type across all platforms - Cast sys::CcapErrorCode to i32 when calling the callback Verification: - Linux: ✅ cargo build --features static-link - Linux: ✅ cargo clippy --all-targets --features static-link -- -D warnings - Linux: ✅ cargo build --no-default-features --features build-source - Expected to fix Windows CI failures This ensures the Rust bindings work correctly on all platforms despite C enum type differences between compilers.
- Fix CMakeLists.txt circular dependency (remove ccap <- ccap-rust dep) - Add Windows Media Foundation libraries for video playback support - Fix error type in set_new_frame_callback (use InvalidParameter) - Document global error callback memory management behavior Fixes: - Windows build failures in CI - Circular dependency preventing CMake configuration - Incorrect error type returned from callback registration
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
bindings/rust/src/provider.rs (3)
14-14: Verify thatSendis safe for non-thread-safe C API.Previous reviews flagged this
unsafe impl Sendbecause the underlying C++ Provider is documented as "not thread-safe." However,Sendonly allows transferring ownership between threads (not concurrent access, which would requireSync). If the C API can safely be used from any single thread (just not concurrently), thenSendis appropriate. If there's thread-local state or platform restrictions, this may be unsound.Based on learnings from past reviews.
347-368: Critical UB: Incompatible pointer cast remains unfixed.This critical issue was flagged in previous reviews but remains unresolved. Line 347 creates
Arc<Mutex<F>>(concrete type), line 368 boxes it asBox<Arc<Mutex<F>>>, but line 358 reconstructs it as*const Arc<Mutex<dyn Fn...>>(trait object).The problem:
Arc<Mutex<F>>andArc<Mutex<dyn Fn...>>have different memory layouts. The latter requires a fat pointer (data + vtable), while the former is a thin pointer. Casting between them is undefined behavior.🔎 Required fix: box the trait object from the start
- let callback = Arc::new(Mutex::new(callback)); + let callback: Arc<Mutex<dyn Fn(i32, &str) + Send + Sync>> = Arc::new(Mutex::new(callback));This ensures the Arc wraps the trait object directly, making the cast at line 358 valid.
Based on learnings from past reviews.
429-438: Critical UB: Incompatible Box cast remains unfixed.This critical issue was flagged in previous reviews but remains unresolved. Line 437 creates
Box::new(callback)wherecallbackis the concrete typeF, but line 429 reconstructs it as*const Box<dyn Fn(&VideoFrame) -> bool + Send + Sync>.The problem:
Box<F>is a thin pointer, whileBox<dyn Fn...>is a fat pointer requiring two words (data + vtable). Casting between them is undefined behavior.🔎 Required fix: box as trait object from the start
- let callback_box = Box::new(callback); + let callback_box: Box<dyn Fn(&VideoFrame) -> bool + Send + Sync> = Box::new(callback); let callback_ptr = Box::into_raw(callback_box);This ensures the Box contains the trait object directly, making the cast at line 429 valid.
Based on learnings from past reviews.
bindings/rust/build.rs (3)
15-21: Chained unwrap() remains fragile and will cause cryptic build panics.This issue was flagged in previous reviews but remains unresolved. The
.parent().unwrap().parent().unwrap()chain will panic with an unhelpful error if the manifest isn't at least two directories deep.🔎 Proposed fix with explicit error message
- ( - manifest_path - .parent() - .unwrap() - .parent() - .unwrap() - .to_path_buf(), - false, - ) + ( + manifest_path + .parent() + .and_then(|p| p.parent()) + .expect("Cargo manifest must be at least 2 directories deep (bindings/rust/)") + .to_path_buf(), + false, + )Based on learnings from past reviews.
84-97: AVX2 flags won't compile on Windows MSVC (still unresolved).This issue was flagged in previous reviews but remains unresolved. The flags
-mavx2and-mfmaare GCC/Clang-specific and are silently ignored on MSVC, resulting in non-SIMD code that defeats the PR's performance goals.🔎 Proposed fix with compiler detection
#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] { let mut avx2_build = cc::Build::new(); avx2_build .file(ccap_root.join("src/ccap_convert_avx2.cpp")) .include(ccap_root.join("include")) .include(ccap_root.join("src")) .cpp(true) - .std("c++17") - .flag("-mavx2") - .flag("-mfma") - .compile("ccap_avx2"); + .std("c++17"); + + if avx2_build.get_compiler().is_like_msvc() { + avx2_build.flag("/arch:AVX2"); + } else { + avx2_build.flag("-mavx2").flag("-mfma"); + } + + avx2_build.compile("ccap_avx2"); }Based on learnings from past reviews.
176-178: Hardcoded rerun paths break in packaged mode (still unresolved).This issue was flagged in previous reviews but remains unresolved. The hardcoded
../../include/*paths assume repository layout and will fail when the crate is packaged withccap_rootpointing tonative/.🔎 Proposed fix using ccap_root
println!("cargo:rerun-if-changed=wrapper.h"); - println!("cargo:rerun-if-changed=../../include/ccap_c.h"); - println!("cargo:rerun-if-changed=../../include/ccap_utils_c.h"); - println!("cargo:rerun-if-changed=../../include/ccap_convert_c.h"); + println!("cargo:rerun-if-changed={}/include/ccap_c.h", ccap_root.display()); + println!("cargo:rerun-if-changed={}/include/ccap_utils_c.h", ccap_root.display()); + println!("cargo:rerun-if-changed={}/include/ccap_convert_c.h", ccap_root.display());Based on learnings from past reviews.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CMakeLists.txtbindings/rust/build.rsbindings/rust/src/provider.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (1)
bindings/rust/src/provider.rs (6)
bindings/rust/src/async.rs (5)
new(30-39)open(48-51)start(54-57)stop(60-63)is_opened(66-69)include/ccap_def.h (1)
DeviceInfo(425-442)bindings/rust/src/error.rs (1)
from(92-118)bindings/rust/src/types.rs (5)
from(35-52)from(87-103)from(116-126)from(154-167)from(220-225)bindings/rust/src/frame.rs (6)
width(154-156)height(159-161)from_c_ptr(52-57)pixel_format(164-168)from_raw(75-84)drop(182-188)bindings/rust/src/lib.rs (1)
version(38-40)
🪛 GitHub Check: Static Link (Dev) (ubuntu-latest)
bindings/rust/src/provider.rs
[warning] 453-453:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/src/provider.rs
[warning] 337-337:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/src/provider.rs
[warning] 328-328:
Diff in /home/runner/work/CameraCapture/CameraCapture/bindings/rust/src/provider.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Windows Build VS2026 (Release-static)
- GitHub Check: Windows Build MinGW (Release-static)
- GitHub Check: Windows Build MinGW (Debug-shared)
- GitHub Check: Windows VS2022 Release - No File Playback
- GitHub Check: macOS Build (Release-static)
- GitHub Check: macOS Build (Release-shared)
- GitHub Check: macOS Release - No File Playback
- GitHub Check: Build ARM64 (Debug-gcc-static)
- GitHub Check: Build Source (Dist) (windows-latest)
- GitHub Check: Build ARM64 (Debug-gcc-shared)
- GitHub Check: Build (Release-clang-shared)
- GitHub Check: Build Fedora (Debug-static)
- GitHub Check: Build ARM64 (Release-gcc-static)
- GitHub Check: Build Source (Dist) (macos-latest)
- GitHub Check: Build (Release-gcc-shared)
- GitHub Check: Build (Release-clang-static)
- GitHub Check: Build Fedora (Release-shared)
- GitHub Check: Build ARM64 (Release-gcc-shared)
- GitHub Check: Build Source (Dist) (macos-latest)
| if let Some(callback_ptr) = self.callback_ptr.take() { | ||
| unsafe { | ||
| let _ = Box::from_raw( | ||
| callback_ptr as *mut Box<dyn Fn(&VideoFrame) -> bool + Send + Sync>, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Cleanup cast must match the stored type.
Line 479 casts callback_ptr as *mut Box<dyn Fn(&VideoFrame) -> bool + Send + Sync>. This is only correct if set_new_frame_callback stores a Box<dyn Fn...> (as suggested in the fix for that function). If the current code in set_new_frame_callback is used (which stores Box<F>), then this cast also exhibits undefined behavior.
Once the fix to set_new_frame_callback (lines 429-438) is applied, this cleanup will be correct.
Based on learnings from past reviews.
🤖 Prompt for AI Agents
In bindings/rust/src/provider.rs around lines 476 to 481, the Box::from_raw cast
used to drop the stored callback must match the exact type that was stored by
set_new_frame_callback; update the cleanup to cast to the same pointer type that
set_new_frame_callback stores (if you changed set_new_frame_callback to store
Box<dyn Fn(&VideoFrame) -> bool + Send + Sync> keep the current cast, otherwise
change the cast to the concrete Box<F> type previously used), and ensure
set_new_frame_callback and this drop path use the same boxed type so the
from_raw call is safe and defined.
- Fix formatting issues (trailing spaces in doc comments) - Fix Windows MSVC linker errors by: * Always compiling ccap_convert_neon.cpp (provides hasNEON() symbol) * Using MSVC-compatible flags (/arch:AVX2) for AVX2 builds * Removing -mavx2/-mfma flags that are GCC/Clang specific The hasNEON() function is needed on all platforms (returns false on non-ARM). Previously it was only compiled on aarch64, causing undefined symbol errors on Windows x86_64.
Add robust path conversion in utils.rs that safely handles Windows paths containing Unicode characters. The new path_to_cstring() helper function: - Uses to_string_lossy() on Windows to prevent UTF-8 conversion panics - Maintains standard to_str() conversion on Unix-like systems - Updates dump_frame_to_file(), dump_frame_to_directory(), and save_rgb_data_as_bmp() functions to use the new helper This resolves Windows path handling issues where Unicode characters in file paths would cause CString conversion failures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bindings/rust/src/utils.rs (1)
1-6: LGTM!All imports are necessary and used appropriately throughout the file. The CString import is utilized by the
path_to_cstringhelper function.
🧹 Nitpick comments (2)
bindings/rust/src/utils.rs (2)
84-122: Consider extracting the common two-phase buffer allocation pattern.Both
dump_frame_to_fileanddump_frame_to_directoryshare nearly identical logic for the two-phase buffer size query and data retrieval pattern. Extracting this into a helper function would reduce duplication and improve maintainability.🔎 Example refactoring approach
fn call_with_buffer_allocation<F>( operation: F, error_msg: &str, ) -> Result<String> where F: Fn(*mut i8, usize) -> i32, { // First call to get required buffer size let buffer_size = operation(std::ptr::null_mut(), 0); if buffer_size <= 0 { return Err(CcapError::FileOperationFailed(error_msg.to_string())); } // Second call to get actual result let mut buffer = vec![0u8; buffer_size as usize]; let result_len = operation(buffer.as_mut_ptr() as *mut i8, buffer.len()); if result_len <= 0 { return Err(CcapError::FileOperationFailed(error_msg.to_string())); } buffer.truncate(result_len as usize); String::from_utf8(buffer) .map_err(|_| CcapError::StringConversionError("Invalid output path string".to_string())) }Then simplify both functions to use this helper.
Also applies to: 125-168
226-239: Improve user feedback for invalid input.The current code silently defaults to 0 when parsing fails, which can confuse users who enter non-numeric input. They'll see "Invalid index" even though the actual issue was a parse error, not an out-of-bounds index.
🔎 Clearer error handling for user input
- let selected_index = input.trim().parse::<usize>().unwrap_or(0); - - if selected_index >= devices.len() { - println!("Invalid index, using the first device: {}", devices[0]); - Ok(0) - } else { - println!("Using device: {}", devices[selected_index]); - Ok(selected_index) - } + let selected_index = match input.trim().parse::<usize>() { + Ok(index) if index < devices.len() => { + println!("Using device: {}", devices[index]); + index + } + Ok(index) => { + println!("Index {} is out of range, using the first device: {}", index, devices[0]); + 0 + } + Err(_) => { + println!("Invalid input (expected a number), using the first device: {}", devices[0]); + 0 + } + }; + + Ok(selected_index)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bindings/rust/src/utils.rs
🧰 Additional context used
🧬 Code graph analysis (1)
bindings/rust/src/utils.rs (3)
bindings/rust/src/frame.rs (3)
data(141-151)width(154-156)height(159-161)include/ccap_utils.h (1)
LogLevel(80-140)bindings/rust/src/types.rs (3)
to_c_enum(57-59)to_c_enum(148-150)to_c_enum(185-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Build Source (Dist) (ubuntu-latest)
- GitHub Check: Static Link (Dev) (windows-latest)
- GitHub Check: Windows Build MinGW (Release-static)
- GitHub Check: Windows Build VS2022 (Debug-static)
- GitHub Check: Build ARM64 (Release-gcc-shared)
- GitHub Check: Build ARM64 (Debug-gcc-static)
- GitHub Check: Build ARM64 (Debug-gcc-shared)
- GitHub Check: Build (Debug-clang-static)
- GitHub Check: Build (Release-clang-shared)
- GitHub Check: Build (Release-clang-static)
- GitHub Check: Build ARM64 (Release-gcc-static)
- GitHub Check: Build (Debug-clang-shared)
- GitHub Check: Build Fedora (Release-shared)
- GitHub Check: Build Fedora (Release-static)
- GitHub Check: Build (Release-gcc-shared)
- GitHub Check: Build Source (Dist) (macos-latest)
- GitHub Check: Static Link (Dev) (windows-latest)
- GitHub Check: Static Link (Dev) (macos-latest)
🔇 Additional comments (5)
bindings/rust/src/utils.rs (5)
62-81: LGTM! Excellent Windows Unicode path handling.The platform-specific path conversion correctly addresses Windows UTF-16 to UTF-8 issues using
to_string_lossy()while maintaining strict validation on Unix systems. This matches the fix described in the commit message.
171-204: LGTM!The parameter count is justified by the BMP format requirements, and the
clippy::too_many_argumentssuppression is appropriate. The implementation correctly wraps the C API with proper error handling.
8-9: LGTM!The
Utilsstruct as a namespace for utility functions is a standard Rust pattern, andsave_frame_as_bmpappropriately delegates todump_frame_to_filefor implementation.Also applies to: 54-59
32-52: All 13 PixelFormat enum variants are correctly included in the string_to_pixel_format mapping. The implementation is complete with proper case-insensitive matching and error handling for unknown formats.
265-276: LogLevel constant mappings are correct.The Rust LogLevel enum correctly maps to the C API constants defined in ccap_utils_c.h. Each variant (None, Error, Warning, Info, Verbose) maps to the corresponding CCAP_LOG_LEVEL_* constant (0, 1, 3, 7, 15) via the to_c_enum conversion function. The implementation is complete and consistent with the C API.
This PR introduces Rust bindings for the CameraCapture library, providing a high-performance and lightweight alternative to existing solutions.
Key Features
image, noopencv), making it ideal for embedded systems and CLI tools.VideoFrameInfolifetime issues) to prevent use-after-free errors.tokiointegration for modern async Rust applications.Changes
bindings/rustdirectory containing the Cargo project.Provider,VideoFrame, andConvertstructs.convertmodule.build.rsto correctly link against Debug/Release builds of the C++ library.Comparison with
nokhwaUnlike
nokhwa, which focuses on broad compatibility and ease of use (often at the cost of heavy dependencies and generic performance), this binding focuses on raw performance and minimal footprint, making it suitable for computer vision and high-throughput video processing tasks.Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.