Optimize error callback implementation #8
Conversation
|
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. Warning Rate limit exceeded@wysaid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCentralized error reporting added (new ErrorCode::InitializationFailed); switched error descriptions to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Provider as Platform Provider (Apple/Linux/Win)
participant Core as ccap::reportError
participant Callback as Registered ErrorCallback
participant Log as Logger
Provider->>Core: reportError(ErrorCode, std::string_view description)
alt Callback registered
Core->>Callback: invoke(ErrorCode, description)
Callback-->>Core: return
else No callback
Core->>Log: CCAP_LOG_E(errorCodeToString(ErrorCode).data(), description.data())
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the error reporting mechanism by refactoring error callback implementation and replacing manual logging with a centralized error reporting system. The key improvements include transitioning from member-based to global error reporting, changing parameter types for better performance, and consolidating error handling across all platforms.
- Refactored error callback from class member to global function with
std::string_viewparameter - Replaced manual
CCAP_LOG_Ecalls with centralizedreportErrorfunction calls - Updated all platform-specific implementations (Windows, Linux, macOS) to use consistent error reporting
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ccap_imp.h | Moved reportError from protected member to global function declaration |
| src/ccap_imp.cpp | Implemented global reportError function with simplified error handling logic |
| src/ccap_imp_windows.cpp | Replaced all manual error logging with reportError calls throughout DirectShow implementation |
| src/ccap_imp_linux.cpp | Updated V4L2 implementation to use centralized error reporting |
| src/ccap_imp_apple.mm | Modified AVFoundation implementation to use consistent error reporting |
| include/ccap_def.h | Changed ErrorCallback parameter from const std::string& to std::string_view |
| src/ccap_c.cpp | Updated C wrapper to handle new string_view parameter and fixed include order |
| examples/desktop/*.cpp | Updated all examples to use new string_view callback signature |
| .vscode/tasks.json | Added code formatting task configuration |
| .vscode/settings.json | Added C++ file association settings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (6)
src/ccap_imp_windows.cpp (2)
474-488: Bug: setGrabberOutputSubtype returns false on successThe function returns false when SetMediaType succeeds, which inverts the success condition. It also always returns false at the end, making the return value meaningless.
Apply this diff:
bool ProviderDirectShow::setGrabberOutputSubtype(GUID subtype) { if (m_sampleGrabber) { AM_MEDIA_TYPE mt; ZeroMemory(&mt, sizeof(mt)); mt.majortype = MEDIATYPE_Video; mt.subtype = subtype; mt.formattype = FORMAT_VideoInfo; HRESULT hr = m_sampleGrabber->SetMediaType(&mt); - if (SUCCEEDED(hr)) return false; - - reportError(ErrorCode::UnsupportedPixelFormat, "Set media type failed"); + if (SUCCEEDED(hr)) { + return true; + } + reportError(ErrorCode::UnsupportedPixelFormat, "Set media type failed"); } - return false; + return false; }
704-712: Bug: returning HRESULT from a bool function (open) after IVideoWindow QI failureSame issue as above: returning hr from a bool function can incorrectly indicate success.
Apply this diff:
- if (FAILED(hr)) { - CCAP_LOG_E("ccap: QueryInterface IVideoWindow failed, result=0x%lx\n", hr); - return hr; - } + if (FAILED(hr)) { + CCAP_LOG_E("ccap: QueryInterface IVideoWindow failed, result=0x%lx\n", hr); + return false; + }src/ccap_imp_apple.mm (2)
341-349: Bug: input creation error handling is broken and leaks success pathYou allocate with initWithDevice:error:, then on error call deviceInputWithDevice:error: but ignore its return and never reset error. This can report failure even if the second call succeeds, and leaves _videoInput potentially nil.
Apply this simpler, correct pattern:
- // Create input device - _videoInput = [[AVCaptureDeviceInput alloc] initWithDevice:_device error:&error]; - if (error) { - [AVCaptureDeviceInput deviceInputWithDevice:self.device error:&error]; - if (error) { - reportError(ErrorCode::DeviceOpenFailed, "Open camera failed: " + std::string([error.localizedDescription UTF8String])); - return NO; - } - } + // Create input device + _videoInput = [AVCaptureDeviceInput deviceInputWithDevice:_device error:&error]; + if (!_videoInput) { + reportError(ErrorCode::DeviceOpenFailed, "Open camera failed: " + std::string([error.localizedDescription UTF8String])); + return NO; + }
983-990: Fix unsafe NSString construction from std::string_viewVerified only one occurrence of this pattern in src/ccap_imp_apple.mm (line 986).
deviceName.data()is not guaranteed to be NUL-terminated, which can lead to truncation or out-of-bounds reads. Please replace it with one of the following safe approaches:• Minimal copy + UTF-8 C-string conversion:
--- a/src/ccap_imp_apple.mm +++ b/src/ccap_imp_apple.mm @@ -983,7 +983,11 @@ @autoreleasepool { m_imp = [[CameraCaptureObjc alloc] initWithProvider:this]; if (!deviceName.empty()) { - [m_imp setCameraName:@(deviceName.data())]; + // Ensure null-termination before creating NSString + std::string nameCopy(deviceName); + NSString *name = [NSString stringWithUTF8String:nameCopy.c_str()]; + [m_imp setCameraName:name]; } [m_imp setResolution:CGSizeMake(m_frameProp.width, m_frameProp.height)]; return [m_imp open];• Or length-aware initializer without extra copy:
--- a/src/ccap_imp_apple.mm +++ b/src/ccap_imp_apple.mm @@ -983,7 +983,11 @@ @autoreleasepool { m_imp = [[CameraCaptureObjc alloc] initWithProvider:this]; if (!deviceName.empty()) { - [m_imp setCameraName:@(deviceName.data())]; + NSString *name = [[NSString alloc] initWithBytes:deviceName.data() + length:deviceName.size() + encoding:NSUTF8StringEncoding]; + [m_imp setCameraName:name]; } [m_imp setResolution:CGSizeMake(m_frameProp.width, m_frameProp.height)]; return [m_imp open];src/ccap_imp_linux.cpp (2)
90-97: Bug: defaulting to /dev/video0 may not match chosen deviceWhen deviceName is empty, you set m_deviceName from findDeviceNames()[0] but hardcode m_devicePath to /dev/video0. The first enumerated name might correspond to a different node (e.g., /dev/video2).
Apply this diff to select the first available /dev/video* node:
- m_deviceName = devices[0]; - m_devicePath = "/dev/video0"; // Default to first device + m_deviceName = devices[0]; + m_devicePath.clear(); + for (const auto& entry : std::filesystem::directory_iterator("/dev")) { + const std::string filename = entry.path().filename().string(); + if (filename.rfind("video", 0) == 0 && isVideoDevice(entry.path().string())) { + m_devicePath = entry.path().string(); + break; + } + } + if (m_devicePath.empty()) { + reportError(ErrorCode::NoDeviceFound, "No video device nodes found under /dev"); + return false; + }
514-521: Bug: frameIndex is incremented twice per frameframe->frameIndex is assigned/incremented at Line 519 and again at Lines 658-659. This double-increments m_frameIndex and may confuse consumers.
Apply this diff to remove the duplicate increment at the end:
- frame->frameIndex = m_frameIndex++; + frame->frameIndex = m_frameIndex++; @@ - frame->frameIndex = m_frameIndex++; + // (removed) duplicate incrementAlternatively, keep only the later assignment if you want to set after zero-copy/convert decision.
Also applies to: 658-659
🧹 Nitpick comments (9)
include/ccap_def.h (1)
275-276: Ensure safe null-termination and lifetime when bridging std::string_view to C callbackIn your C bridge (src/ccap_c.cpp around line 380), you’re doing:
g_cGlobalErrorCallbackWrapper->callback( convert_error_code_to_c(errorCode), description.data(), g_cGlobalErrorCallbackWrapper->userData);However, std::string_view does not guarantee a null-terminated buffer nor that its data outlives the call. This can lead to undefined behavior if
errorDescriptionpoints to a temporary or a non-null-terminated buffer, or if callbacks are ever deferred asynchronously.To address this, please choose one of the following:
Extend the C callback signature to carry a length parameter:
• UpdateCcapErrorCallbackininclude/ccap_c.hto
typedef void (*CcapErrorCallback)(CcapErrorCode, const char* /*data*/, size_t /*len*/, void*);
• Propagate the length inccap_set_error_callbackand the invocation insrc/ccap_c.cpp.Materialize a null-terminated string before invoking the C callback:
• Insrc/ccap_c.cpp, convert the view to an owningstd::string str(description);
• Passstr.c_str()(and keepstralive until after the call).Also verify that no callbacks are deferred beyond the lifetime of the string you pass. If you ever queue the callback for asynchronous execution, materialize the string at enqueue time.
examples/desktop/3-capture_callback.cpp (1)
40-43: Good switch to std::string_view; add an explicit include to avoid reliance on transitive headersSome toolchains won’t provide operator<< for std::string_view unless <string_view> is included explicitly in this TU.
Add near other includes:
#include <string_view>src/ccap_imp_windows.cpp (4)
309-317: Include HRESULT in error reporting for enumerator creation failuresYou already include the HRESULT in the CreateClassEnumerator failure path. Consider doing the same for the preceding CoCreateInstance failure to aid diagnostics.
Apply this diff to include HRESULT in the message:
- if (FAILED(result)) { - reportError(ErrorCode::NoDeviceFound, "Create system device enumerator failed"); + if (FAILED(result)) { + reportError(ErrorCode::NoDeviceFound, "Create system device enumerator failed, result=0x" + std::to_string(result));
681-684: Nit: inconsistent "ccap:" prefix in error messageMost messages omit the "ccap:" prefix after centralization. Consider removing it here for consistency.
- reportError(ErrorCode::InvalidDevice, "ccap: No video capture device: " + (deviceName.empty() ? unavailableMsg : deviceName)); + reportError(ErrorCode::InvalidDevice, "No video capture device: " + (deviceName.empty() ? unavailableMsg : deviceName));
588-593: Optional: also report SetFormat failure via reportError to keep error surfaces consistentCurrently SetFormat failure is only logged. Consider reporting it as DeviceOpenFailed for consistency across backends, unless this is expected and handled upstream.
- } else { - CCAP_LOG_E("ccap: SetFormat failed, result=0x%lx\n", setFormatResult); - } + } else { + reportError(ErrorCode::DeviceOpenFailed, "SetFormat failed, result=0x" + std::to_string(setFormatResult)); + }
470-490: HandlesetGrabberOutputSubtypereturn value increateStreamThe call to
setGrabberOutputSubtype(subtype)increateStream(src/ccap_imp_windows.cpp:583) currently ignores its boolean result. If setting the grabber’s media type fails, downstream code may misbehave. Check the return value and bail out on failure:--- a/src/ccap_imp_windows.cpp +++ b/src/ccap_imp_windows.cpp @@ -580,7 +580,11 @@ bool ProviderDirectShow::createStream() { m_frameProp.cameraPixelFormat = pixFormatInfo.pixelFormat; } - setGrabberOutputSubtype(subtype); + // Fail early if the requested pixel format isn’t supported + if (!setGrabberOutputSubtype(subtype)) { + // propagate failure to caller + return false; + } auto setFormatResult = streamConfig->SetFormat(mediaType); if (SUCCEEDED(setFormatResult)) {• Location to update:
src/ccap_imp_windows.cpp, around line 583
• Suggestion: validatesetGrabberOutputSubtypebefore continuingsrc/ccap_imp_apple.mm (1)
669-699: Explicit fallthrough markers for unsupported pixel formatsThe switch intentionally falls through after reporting UnsupportedPixelFormat for I420/I420f. Add [[fallthrough]] to silence -Wimplicit-fallthrough and document intent. Also consider whether reporting these as “errors” is desired, since the code transparently falls back.
case PixelFormat::I420: - reportError(ErrorCode::UnsupportedPixelFormat, "I420 is not supported on macOS, fallback to NV12"); + reportError(ErrorCode::UnsupportedPixelFormat, "I420 is not supported on macOS, fallback to NV12"); + [[fallthrough]]; case PixelFormat::NV12: _cvPixelFormat = kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange; internalFormat = PixelFormat::NV12; break; case PixelFormat::I420f: - reportError(ErrorCode::UnsupportedPixelFormat, "I420f is not supported on macOS, fallback to NV12f"); + reportError(ErrorCode::UnsupportedPixelFormat, "I420f is not supported on macOS, fallback to NV12f"); + [[fallthrough]]; case PixelFormat::NV12f:If frequent, you might downgrade these to warnings instead of errors to avoid noisy callbacks.
src/ccap_imp_linux.cpp (2)
244-258: Misclassified capability errors: use a device capability error, not UnsupportedPixelFormatLack of VIDEO_CAPTURE/STREAMING is a device capability issue, not a pixel format problem. Consider DeviceOpenFailed (or a dedicated capability code, if available).
- reportError(ErrorCode::UnsupportedPixelFormat, "Device does not support video capture"); + reportError(ErrorCode::DeviceOpenFailed, "Device does not support video capture"); ... - reportError(ErrorCode::UnsupportedPixelFormat, "Device does not support streaming"); + reportError(ErrorCode::DeviceOpenFailed, "Device does not support streaming");
119-131: Consider chaining setupDevice() failure reason to the callerYou report "Failed to setup device ..." after setupDevice() returns false. Since setupDevice() already emits detailed reasons via reportError, consider including errno details here or downgrading this to a log to avoid redundant callbacks.
- if (!setupDevice()) { + if (!setupDevice()) { ::close(m_fd); m_fd = -1; - reportError(ErrorCode::DeviceOpenFailed, "Failed to setup device " + m_devicePath); + // Detailed errors already reported inside setupDevice(); keep this as a log. + CCAP_LOG_E("ccap: Failed to setup device %s", m_devicePath.c_str()); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.vscode/settings.json(1 hunks).vscode/tasks.json(1 hunks)examples/desktop/0-print_camera.cpp(1 hunks)examples/desktop/1-minimal_example.cpp(1 hunks)examples/desktop/2-capture_grab.cpp(1 hunks)examples/desktop/3-capture_callback.cpp(1 hunks)include/ccap_def.h(1 hunks)src/ccap_c.cpp(2 hunks)src/ccap_imp.cpp(1 hunks)src/ccap_imp.h(1 hunks)src/ccap_imp_apple.mm(7 hunks)src/ccap_imp_linux.cpp(8 hunks)src/ccap_imp_windows.cpp(13 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/ccap_imp.h (1)
src/ccap_imp.cpp (2)
reportError(193-199)reportError(193-193)
examples/desktop/2-capture_grab.cpp (1)
src/ccap_core.cpp (2)
setErrorCallback(44-47)setErrorCallback(44-44)
examples/desktop/0-print_camera.cpp (1)
src/ccap_core.cpp (2)
setErrorCallback(44-47)setErrorCallback(44-44)
src/ccap_c.cpp (1)
src/ccap_core.cpp (2)
setErrorCallback(44-47)setErrorCallback(44-44)
examples/desktop/3-capture_callback.cpp (1)
src/ccap_core.cpp (2)
setErrorCallback(44-47)setErrorCallback(44-44)
examples/desktop/1-minimal_example.cpp (1)
src/ccap_core.cpp (2)
setErrorCallback(44-47)setErrorCallback(44-44)
src/ccap_imp_linux.cpp (1)
src/ccap_imp.cpp (2)
reportError(193-199)reportError(193-193)
src/ccap_imp.cpp (3)
src/ccap_core.cpp (2)
getErrorCallback(49-52)getErrorCallback(49-49)include/ccap_utils.h (1)
errorLogEnabled(105-105)src/ccap_utils.cpp (2)
errorCodeToString(250-283)errorCodeToString(250-250)
src/ccap_imp_windows.cpp (1)
src/ccap_imp.cpp (2)
reportError(193-199)reportError(193-193)
🔇 Additional comments (25)
examples/desktop/0-print_camera.cpp (1)
64-67: LGTM: example updated to std::string_viewThe lambda signature correctly reflects the public API change and streams std::string_view safely (provided <string_view> is included via headers).
.vscode/tasks.json (1)
234-246: LGTM: adds a useful “Format All Code” taskTask wiring mirrors existing patterns (bash -l, cwd to scripts/). Looks good.
examples/desktop/1-minimal_example.cpp (1)
16-19: LGTM: error callback signature migrated to std::string_viewConsistent with the API alias change; usage is correct.
examples/desktop/2-capture_grab.cpp (1)
23-26: LGTM: error callback signature migrated to std::string_viewConsistent with the API alias change across examples.
.vscode/settings.json (1)
117-121: VSCode file associations: LGTMHelpful for libc++ STL headers; benign editor-only change.
src/ccap_c.cpp (1)
13-13: Include reorder: no functional impactOrdering change is fine; no action needed.
src/ccap_imp.h (1)
132-133: Centralized reportError API looks goodPromotes reuse and decouples ProviderImp from error reporting; string_view is an appropriate choice here.
src/ccap_imp_windows.cpp (6)
205-218: Good switch to centralized error reporting on COM init failureRouting the COM init failure through reportError with InternalError is consistent with the PR goals.
298-305: Centralized error reporting for device enumerator creation LGTMMapping CoCreateInstance failure to NoDeviceFound and returning early is appropriate.
596-602: Good: unified error reporting on SampleGrabber AddFilter failureUsing DeviceOpenFailed here is consistent with other OS backends.
632-642: Good: centralized error on GetConnectedMediaType failureAppropriate error code and early return.
1035-1047: Good: standardized error on Run() failureUsing DeviceStartFailed aligns with the new error scheme.
447-472: End-to-end graph creation error mapping looks consistentCreate FilterGraph, CaptureGraphBuilder, and AddFilter paths now map to DeviceOpenFailed with clear messages.
src/ccap_imp_apple.mm (5)
273-276: LGTM: proper error code for authorization failureUsing DeviceOpenFailed with a clear message is appropriate.
324-327: LGTM: unified error for missing video deviceNoDeviceFound is the right category here.
357-359: LGTM: error mapping when session can't add inputDeviceOpenFailed with a crisp message matches the scheme.
472-474: LGTM: error mapping when session can't add outputConsistent with input add failure mapping.
977-981: LGTM: “already opened” now uses centralized error reporterAccurate classification and message.
src/ccap_imp_linux.cpp (7)
81-86: LGTM: centralized report when already openedAccurate error code and message.
112-116: LGTM: invalid device now emits InvalidDevice with contextClear message including the requested device name.
323-331: LGTM: negotiateFormat now reports failures via DeviceStartFailedConsistent with “start”-phase errors.
357-361: LGTM: report failure when re-fetching format after VIDIOC_S_FMTAppropriate categorization and return path.
381-411: Good: Memory allocation failures mapped to MemoryAllocationFailedREQBUFS/QUERYBUF/mmap failures are consistently funneled through reportError with strerror(errno).
435-445: LGTM: start streaming errors mapped correctlyQBUF/STREAMON errors use DeviceStartFailed with strerror(errno).
178-204: LGTM: start() now reports when device isn’t openedDeviceStartFailed with “Device not opened” is helpful for users.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/ccap_imp.h (1)
14-23: Include <string_view> to support reportError declaration.This header declares reportError(ErrorCode, std::string_view) but does not include <string_view>, leading to compilation errors depending on include order.
Apply this diff:
#include "ccap_utils.h" #include <atomic> #include <condition_variable> #include <deque> #include <mutex> #include <optional> #include <queue> +#include <string_view>Also applies to: 133-140
src/ccap_imp.cpp (1)
121-126: Timeout can overshoot requested duration; wait in smaller, remaining slicesAs written, a 500 ms timeout can block ~1000 ms. Wait in min(remaining, 1000 ms) slices and update the progress log accordingly.
Apply this diff:
- for (uint32_t waitedTime = 0; waitedTime < timeoutInMs; waitedTime += 1000) { - waitSuccess = m_frameCondition.wait_for(lock, std::chrono::milliseconds(1000), - [this]() { return m_grabFrameWaiting && !m_availableFrames.empty(); }); - if (waitSuccess) break; - CCAP_LOG_V("ccap: Waiting for new frame... %u ms\n", waitedTime); - } + for (uint32_t waitedTime = 0; waitedTime < timeoutInMs;) { + const uint32_t remaining = timeoutInMs - waitedTime; + const uint32_t slice = remaining < 1000 ? remaining : 1000; + waitSuccess = m_frameCondition.wait_for( + lock, + std::chrono::milliseconds(slice), + [this]() { return m_grabFrameWaiting && !m_availableFrames.empty(); }); + waitedTime += slice; + if (waitSuccess) break; + CCAP_LOG_V("ccap: Waiting for new frame... %u ms\n", waitedTime); + }
♻️ Duplicate comments (6)
include/ccap_def.h (1)
40-45: Add missing <string_view> include and clarify ErrorCallback lifetime semantics.This header exposes std::string_view in the public API (ErrorCallback and errorCodeToString) but does not include <string_view>, which can cause compile failures depending on include order. Also, please document that the view’s lifetime is limited to the duration of the callback and may not be null-terminated.
Apply this diff:
#include <cstdint> #include <functional> #include <memory> #include <string> +#include <string_view> #include <vector> @@ /** * @brief Error callback function type for C++ interface * @param errorCode The error code that occurred - * @param errorDescription English description of the error + * @param errorDescription Error description as std::string_view (may not be null-terminated) + * @note The view is only valid during the callback invocation; do not store it or assume null-termination. */ using ErrorCallback = std::function<void(ErrorCode errorCode, std::string_view errorDescription)>;Also applies to: 274-279
src/ccap_c.cpp (1)
376-383: Fix two issues: potential deadlock and passing non-null-terminated data() to C callback.
- The lambda locks g_cErrorCallbackMutex while invoking user callback, risking deadlock on re-entrancy.
- std::string_view::data() is not guaranteed null-terminated; C callback expects const char* C-string.
Copy the wrapper under lock and invoke outside; materialize description to std::string to ensure null-termination.
Apply this diff:
- ccap::setErrorCallback([](ccap::ErrorCode errorCode, std::string_view description) { - std::lock_guard<std::mutex> lock(g_cErrorCallbackMutex); - if (g_cGlobalErrorCallbackWrapper && g_cGlobalErrorCallbackWrapper->callback) { - g_cGlobalErrorCallbackWrapper->callback(convert_error_code_to_c(errorCode), - description.data(), - g_cGlobalErrorCallbackWrapper->userData); - } - }); + ccap::setErrorCallback([](ccap::ErrorCode errorCode, std::string_view description) { + // Copy wrapper under lock; call outside to avoid deadlocks on re-entrancy. + std::shared_ptr<ErrorCallbackWrapper> localWrapper; + { + std::lock_guard<std::mutex> lock(g_cErrorCallbackMutex); + localWrapper = g_cGlobalErrorCallbackWrapper; + } + if (localWrapper && localWrapper->callback) { + // Ensure null-terminated string for the C callback. + std::string descCopy(description); + localWrapper->callback(convert_error_code_to_c(errorCode), + descCopy.c_str(), + localWrapper->userData); + } + });src/ccap_imp.cpp (1)
195-200: Fix unsafe printf with string_view and guard against exceptions escaping from user callbackUsing "%s" with string_view.data() assumes null-termination. Also, user callbacks may throw; prevent exceptions from crossing this boundary.
Apply this diff:
-void reportError(ErrorCode errorCode, std::string_view description) { - if (ErrorCallback globalCallback = getErrorCallback()) { - globalCallback(errorCode, description); - } else if (ccap::errorLogEnabled()) { - CCAP_LOG_E("ccap error code %s: %s\n", errorCodeToString(errorCode).data(), description.data()); - } -} +void reportError(ErrorCode errorCode, std::string_view description) { + if (ErrorCallback globalCallback = getErrorCallback()) { + // Prevent exceptions from escaping across library boundaries. + try { + globalCallback(errorCode, description); + } catch (const std::exception& e) { + if (ccap::errorLogEnabled()) { + CCAP_LOG_E("ccap error callback threw exception: %s\n", e.what()); + } + } catch (...) { + if (ccap::errorLogEnabled()) { + CCAP_LOG_E("ccap error callback threw unknown exception\n"); + } + } + } else if (ccap::errorLogEnabled()) { + const auto codeStr = errorCodeToString(errorCode); + CCAP_LOG_E("ccap error code %.*s: %.*s\n", + static_cast<int>(codeStr.size()), codeStr.data(), + static_cast<int>(description.size()), description.data()); + } +}src/ccap_imp_windows.cpp (3)
311-312: Hex formatting of HRESULT is incorrect
std::to_string(result) prints decimal, not hex. Use std::hex formatting.Apply this diff:
- reportError(ErrorCode::NoDeviceFound, "CreateClassEnumerator CLSID_VideoInputDeviceCategory failed, result=0x" + std::to_string(result)); + { + std::ostringstream oss; + oss << "CreateClassEnumerator CLSID_VideoInputDeviceCategory failed, result=0x" + << std::hex << std::uppercase << static_cast<unsigned long>(result); + reportError(ErrorCode::NoDeviceFound, oss.str()); + }Additionally, ensure the header is included:
// Add near other includes #include <sstream>
684-685: Consistent message style: drop redundant “ccap:” prefix in reportError message
Most error strings reported via reportError omit the “ccap:” prefix. Align for consistency.Apply this diff:
- reportError(ErrorCode::InvalidDevice, "ccap: No video capture device: " + (deviceName.empty() ? unavailableMsg : deviceName)); + reportError(ErrorCode::InvalidDevice, "No video capture device: " + (deviceName.empty() ? std::string(unavailableMsg) : std::string(deviceName)));
610-614: Bug: returning HRESULT from a bool function (false positives on failure)
Returning hr from a bool function converts non-zero HRESULTs to true. Return false on failure.Apply this diff:
- if (FAILED(hr)) { - reportError(ErrorCode::DeviceOpenFailed, "Add null renderer filter to graph failed"); - return hr; - } + if (FAILED(hr)) { + reportError(ErrorCode::DeviceOpenFailed, "Add null renderer filter to graph failed"); + return false; + }
🧹 Nitpick comments (4)
src/ccap_imp.h (1)
41-47: Clarify default pixel format comment for non-Apple platforms.The else-branch applies to both Windows and Linux; the comment currently says “Windows default,” which can be misleading.
Apply this diff:
- PixelFormat::BGR24 ///< Windows default + PixelFormat::BGR24 ///< Non-Apple default (Windows/Linux)src/ccap_imp.cpp (1)
114-115: Consider a more accurate ErrorCode for “grab when not started”DeviceStartFailed typically denotes a start operation that failed. Here the device simply isn’t started. InternalError (or a dedicated code if added later) better communicates the state.
Apply this diff:
- ccap::reportError(ErrorCode::DeviceStartFailed, "Grab called when camera is not started"); + ccap::reportError(ErrorCode::InternalError, "Grab called when camera is not started");src/ccap_imp_windows.cpp (1)
710-714: Do not fail open() solely because IVideoWindow is unavailable
Removing the ActiveMovie window is a nice-to-have; cameras without IVideoWindow support can still capture. Recommend logging/reporting but not failing open().Apply this diff:
- if (FAILED(hr)) { - CCAP_LOG_E("ccap: QueryInterface IVideoWindow failed, result=0x%lx\n", hr); - ccap::reportError(ErrorCode::DeviceOpenFailed, "QueryInterface IVideoWindow failed"); - return false; - } + if (FAILED(hr)) { + CCAP_LOG_E("ccap: QueryInterface IVideoWindow failed, result=0x%lx\n", hr); + ccap::reportError(ErrorCode::DeviceOpenFailed, "QueryInterface IVideoWindow failed"); + // Non-fatal: continue without altering AutoShow + } else { + videoWindow->put_AutoShow(false); + } - videoWindow->put_AutoShow(false);src/ccap_core.cpp (1)
80-80: Typo in log message
Destructor logs “VideoFrame::VideoFrameFrame()”; correct to “VideoFrame::~VideoFrame()”.Apply this diff:
-VideoFrame::~VideoFrame() { CCAP_LOG_V("ccap: VideoFrame::VideoFrameFrame() called, this=%p\n", this); } +VideoFrame::~VideoFrame() { CCAP_LOG_V("ccap: VideoFrame::~VideoFrame() called, this=%p\n", this); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
include/ccap_c.h(1 hunks)include/ccap_def.h(2 hunks)src/ccap_c.cpp(3 hunks)src/ccap_core.cpp(5 hunks)src/ccap_imp.cpp(3 hunks)src/ccap_imp.h(2 hunks)src/ccap_imp_apple.mm(11 hunks)src/ccap_imp_linux.cpp(13 hunks)src/ccap_imp_windows.cpp(16 hunks)src/ccap_utils.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ccap_imp_apple.mm
- src/ccap_imp_linux.cpp
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/ccap_imp.cpp (3)
src/ccap_core.cpp (2)
getErrorCallback(49-52)getErrorCallback(49-49)include/ccap_utils.h (1)
errorLogEnabled(105-105)src/ccap_utils.cpp (2)
errorCodeToString(250-285)errorCodeToString(250-250)
src/ccap_c.cpp (1)
src/ccap_core.cpp (2)
setErrorCallback(44-47)setErrorCallback(44-44)
src/ccap_core.cpp (2)
src/ccap_imp.cpp (18)
reportError(195-201)reportError(195-195)set(29-74)set(29-29)get(76-92)get(76-76)grab(108-142)grab(108-108)setNewFrameCallback(94-100)setNewFrameCallback(94-94)setFrameAllocator(102-106)setFrameAllocator(102-102)setMaxAvailableFrameSize(144-144)setMaxAvailableFrameSize(144-144)size(21-21)size(21-21)setMaxCacheFrameSize(146-146)setMaxCacheFrameSize(146-146)include/ccap_core.h (1)
Provider(59-228)
src/ccap_imp.h (1)
src/ccap_imp.cpp (2)
reportError(195-201)reportError(195-195)
src/ccap_imp_windows.cpp (1)
src/ccap_imp.cpp (2)
reportError(195-201)reportError(195-195)
⏰ 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). (5)
- GitHub Check: Build ARM64 (Release-gcc)
- GitHub Check: Build (Release-clang)
- GitHub Check: Windows Build (Debug)
- GitHub Check: Build (Debug-gcc)
- GitHub Check: Build (Release-gcc)
🔇 Additional comments (8)
include/ccap_def.h (1)
245-247: New error code addition looks correct and consistent with C API.InitializationFailed = 0x1006 is added in order and matches the mapping introduced elsewhere.
src/ccap_utils.cpp (1)
264-266: Handled new error code in errorCodeToString — looks good.The new case for ErrorCode::InitializationFailed returns a stable string literal and fits the existing pattern.
include/ccap_c.h (1)
76-76: C enum addition aligns with C++ ErrorCode (0x1006).CCAP_ERROR_INITIALIZATION_FAILED is correctly inserted and value-matched.
src/ccap_c.cpp (2)
13-13: Include reordering appears safe.No apparent dependency or ODR implications from moving ccap_utils_c.h; proceed.
484-485: Static assertion for InitializationFailed mapping — good safety check.Ensures C/C++ enum compatibility at compile time.
src/ccap_imp_windows.cpp (1)
214-215: Centralized error reporting adoption looks good
Replacing scattered CCAP_LOG_E paths with reportError and standardized ErrorCode mapping improves consistency and API surface for clients.Also applies to: 302-303, 454-455, 461-462, 468-470, 485-485, 495-496, 501-502, 592-593, 600-602, 606-608, 618-620, 641-643, 657-658, 692-693, 697-698, 704-705, 713-714, 740-741
src/ccap_core.cpp (2)
70-77: Good: allocation failure now reported via centralized error callback
Early return with MemoryAllocationFailed avoids UB in downstream paths and leaves allocator in a known state.
127-132: Good: m_imp null-guards with InitializationFailed improve robustness
Systematically guarding all public entry points and surfacing InitializationFailed plus clear messages is the right tradeoff.Also applies to: 139-146, 148-155, 159-165, 168-171, 191-195, 197-204, 211-217, 221-227, 229-235, 237-243, 245-251, 253-259
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/ccap_imp_windows.cpp (4)
309-313: Hex formatting bug: HRESULT printed in decimal despite "0x" prefixstd::to_string(result) yields decimal text; the message pretends hex. Format as hex and pad for clarity.
Apply this diff:
- reportError(ErrorCode::NoDeviceFound, "CreateClassEnumerator CLSID_VideoInputDeviceCategory failed, result=0x" + std::to_string(result)); + { + std::ostringstream oss; + oss << "CreateClassEnumerator CLSID_VideoInputDeviceCategory failed, result=0x" + << std::hex << std::uppercase << std::setw(8) << std::setfill('0') + << static_cast<unsigned long>(result); + reportError(ErrorCode::NoDeviceFound, oss.str()); + }Add missing headers near the top of this file:
#include <sstream> #include <iomanip>
481-488: Bug: setGrabberOutputSubtype returns false on success (inverted condition)Currently returns false when SUCCEEDED(hr) and never returns true, masking success.
Apply this diff:
- HRESULT hr = m_sampleGrabber->SetMediaType(&mt); - if (SUCCEEDED(hr)) return false; - - reportError(ErrorCode::UnsupportedPixelFormat, "Set media type failed"); + HRESULT hr = m_sampleGrabber->SetMediaType(&mt); + if (SUCCEEDED(hr)) return true; + reportError(ErrorCode::UnsupportedPixelFormat, "Set media type failed");Function will still end with
return false;on failure paths.
683-686: Consistency: drop “ccap:” prefix in error messages routed through reportErrorElsewhere error strings omit the prefix; keep logging style internal to logger.
Apply this diff:
- reportError(ErrorCode::InvalidDevice, "ccap: No video capture device: " + std::string(deviceName.empty() ? unavailableMsg : deviceName)); + reportError( + ErrorCode::InvalidDevice, + std::string("No video capture device: ") + + (deviceName.empty() ? std::string("unavailable") : std::string(deviceName)) + );
610-614: Bug: returning HRESULT from a bool function (false positives on failure)
return hr;implicitly converts non-zero failures totrue. Must returnfalseon failure.Apply this diff:
- reportError(ErrorCode::DeviceOpenFailed, "Add null renderer filter to graph failed"); - return hr; + reportError(ErrorCode::DeviceOpenFailed, "Add null renderer filter to graph failed"); + return false;
🧹 Nitpick comments (4)
src/ccap_imp_windows.cpp (4)
592-593: Standardize error-reporting calls and enrich messages with HRESULT context
- Mixed usage of
reportErrorandccap::reportErrorin the same file. Sinceusing namespace ccap;is present, prefer one style throughout for consistency.- Several failures already log
hrvia CCAP_LOG_*; consider including the HRESULT hex and, optionally, the system message in the string passed toreportErrortoo. This makes downstream callbacks more actionable without requiring logs.Optional helper you can add to this TU or a shared utility:
static std::string formatHr(HRESULT hr) { std::ostringstream oss; oss << "0x" << std::hex << std::uppercase << std::setw(8) << std::setfill('0') << static_cast<unsigned long>(hr); return oss.str(); } // If you want the system message as well: static std::string formatHrWithMessage(HRESULT hr) { LPVOID msgBuf = nullptr; DWORD len = FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, nullptr, hr, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), reinterpret_cast<LPSTR>(&msgBuf), 0, nullptr); std::string msg = len ? std::string(static_cast<LPCSTR>(msgBuf), len) : std::string("Unknown error"); if (msg.ends_with("\r\n")) msg.resize(msg.size() - 2); LocalFree(msgBuf); return formatHr(hr) + " (" + msg + ")"; }Then, for example at Line 703:
reportError(ErrorCode::DeviceOpenFailed, std::string("QueryInterface IMediaControl failed, hr=") + formatHr(hr));Also applies to: 627-628, 642-642, 690-693, 697-697, 703-704, 712-713, 739-740, 1046-1047
1041-1046: Use InitializationFailed error when start() is called before open()This aligns with the PR’s new error code and improves diagnosability.
Apply this diff:
- if (!m_isOpened) return false; + if (!m_isOpened) { + reportError(ErrorCode::InitializationFailed, "Camera is not opened"); + return false; + }
572-585: Optional: check SetFormat return more explicitly and early-outYou already log and report error on failure. Consider returning false immediately to simplify the control flow.
Example:
- auto setFormatResult = streamConfig->SetFormat(mediaType); - if (SUCCEEDED(setFormatResult)) { + auto setFormatResult = streamConfig->SetFormat(mediaType); + if (SUCCEEDED(setFormatResult)) { // ... - } else { + } else { CCAP_LOG_E("ccap: SetFormat failed, result=0x%lx\n", setFormatResult); ccap::reportError(ErrorCode::UnsupportedPixelFormat, "SetFormat failed"); + return false; }
590-596: Nit: avoid mixing qualified and unqualified reportErrorLine 593 uses
ccap::reportErrorwhile nearby calls are unqualified. Pick one style to reduce visual noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ccap_imp_windows.cpp(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ccap_imp_windows.cpp (1)
src/ccap_imp.cpp (2)
reportError(195-201)reportError(195-195)
⏰ 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). (6)
- GitHub Check: Build (Release-gcc)
- GitHub Check: Build ARM64 (Debug-gcc)
- GitHub Check: Build ARM64 (Release-gcc)
- GitHub Check: Build (Debug-clang)
- GitHub Check: Build (Release-clang)
- GitHub Check: Build (Debug-gcc)
🔇 Additional comments (1)
src/ccap_imp_windows.cpp (1)
205-218: LGTM: centralized and clearer error reporting on key failure pathsGood migration to reportError for COM init, graph creation, filter addition, media type verification, and double-open guard. Messages are concise and actionable.
Also applies to: 450-471, 597-603, 604-609, 635-644, 654-658
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/ccap_imp_linux.cpp (4)
90-97: Bug: defaulting m_devicePath to "/dev/video0" can open the wrong deviceWhen deviceName is empty, you pick the first enumerated device name but hard-code m_devicePath = "/dev/video0". directory_iterator has no defined order; the “first” found device may not be /dev/video0, leading to a name/path mismatch and opening the wrong camera.
Apply a deterministic selection and bind name/path from the same entry:
- auto devices = findDeviceNames(); - if (devices.empty()) { - reportError(ErrorCode::NoDeviceFound, "No video devices found"); - return false; - } - m_deviceName = devices[0]; - m_devicePath = "/dev/video0"; // Default to first device + // Pick the first valid /dev/video* deterministically + std::vector<std::string> candidates; + for (const auto& entry : std::filesystem::directory_iterator("/dev")) { + const std::string filename = entry.path().filename().string(); + if (filename.rfind("video", 0) == 0 && isVideoDevice(entry.path().string())) { + candidates.emplace_back(entry.path().string()); + } + } + std::sort(candidates.begin(), candidates.end()); // ensures video0 < video1 < ... + if (candidates.empty()) { + reportError(ErrorCode::NoDeviceFound, "No video devices found"); + return false; + } + m_devicePath = candidates.front(); + m_deviceName = getDeviceDescription(m_devicePath); + if (m_deviceName.empty()) m_deviceName = m_devicePath;Note: add
#include <algorithm>for std::sort.
393-413: Fix: cleanup on partial allocation failures to avoid mmaps leakingIf VIDIOC_QUERYBUF or mmap fails for i > 0, earlier mmapped buffers remain mapped. Call releaseBuffers() and (optionally) ask the driver to drop buffer allocations before returning.
Apply:
- if (ioctl(m_fd, VIDIOC_QUERYBUF, &buf) < 0) { - reportError(ErrorCode::MemoryAllocationFailed, "Query device buffer failed: " + std::string(strerror(errno))); - return false; - } + if (ioctl(m_fd, VIDIOC_QUERYBUF, &buf) < 0) { + reportError(ErrorCode::MemoryAllocationFailed, "Query device buffer failed: " + std::string(strerror(errno))); + releaseBuffers(); + // Hint the driver to free any requested buffers + struct v4l2_requestbuffers zero = {}; + zero.count = 0; zero.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; zero.memory = V4L2_MEMORY_MMAP; + ioctl(m_fd, VIDIOC_REQBUFS, &zero); + return false; + } ... - if (m_buffers[i].start == MAP_FAILED) { - reportError(ErrorCode::MemoryAllocationFailed, "Memory mapping failed: " + std::string(strerror(errno))); - return false; - } + if (m_buffers[i].start == MAP_FAILED) { + reportError(ErrorCode::MemoryAllocationFailed, "Memory mapping failed: " + std::string(strerror(errno))); + releaseBuffers(); + struct v4l2_requestbuffers zero = {}; + zero.count = 0; zero.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; zero.memory = V4L2_MEMORY_MMAP; + ioctl(m_fd, VIDIOC_REQBUFS, &zero); + return false; + }
189-193: Ensure resource cleanup when startStreaming() failsIf negotiateFormat/allocateBuffers succeed but startStreaming() fails, buffers remain mmapped. Do cleanup before returning.
Apply:
- if (!negotiateFormat() || !allocateBuffers() || !startStreaming()) { - reportError(ErrorCode::DeviceStartFailed, "Failed to start streaming"); - return false; - } + if (!negotiateFormat()) { + reportError(ErrorCode::DeviceStartFailed, "Failed to negotiate format"); + return false; + } + if (!allocateBuffers()) { + reportError(ErrorCode::DeviceStartFailed, "Failed to allocate buffers"); + return false; + } + if (!startStreaming()) { + reportError(ErrorCode::DeviceStartFailed, "Failed to start streaming"); + releaseBuffers(); // cleanup partially allocated resources + return false; + }
520-521: Critical Issue: DuplicateframeIndexAssignment insrc/ccap_imp_linux.cppVerification shows two separate occurrences of the post-increment assignment, meaning each frame winds up advancing the index twice:
- src/ccap_imp_linux.cpp:520
frame->frameIndex = m_frameIndex++;- src/ccap_imp_linux.cpp:661
frame->frameIndex = m_frameIndex++;Both lines match the pattern
frame->frameIndex = m_frameIndex++per the ripgrep output, confirming the double increment persists.To fix, remove one of the duplicate assignments so that
m_frameIndexonly advances once per captured frame. For example, you could disable the later assignment:--- a/src/ccap_imp_linux.cpp +++ b/src/ccap_imp_linux.cpp @@ -660,7 +660,7 @@ // … prior context … buf = m_camBufs[m_currentBufIndex]; - frame->frameIndex = m_frameIndex++; + // Avoid double-increment; frameIndex already set at line 520. + // frame->frameIndex = m_frameIndex++; frame->sizeInBytes = buf.bytesused; // … following context …Please apply this change (or remove the earlier assignment instead—whichever aligns with your intended frame-indexing logic) and confirm that only one
frameIndexassignment remains.src/ccap_imp_apple.mm (2)
333-349: Fix: unconditional unlock and redundant input creationTwo issues here:
unlockForConfigurationis called even iflockForConfigurationfailed.- The second call to
deviceInputWithDevice:error:is redundant; check_videoInput/erroronce.These can cause runtime warnings and mask the real open failure.
Apply this diff:
- NSError* error = nil; - [_device lockForConfiguration:&error]; - if ([_device isFocusModeSupported:AVCaptureFocusModeContinuousAutoFocus]) { - [_device setFocusMode:AVCaptureFocusModeContinuousAutoFocus]; - CCAP_NSLOG_V(@"ccap: Set focus mode to continuous auto focus"); - } - [_device unlockForConfiguration]; - - // Create input device - _videoInput = [[AVCaptureDeviceInput alloc] initWithDevice:_device error:&error]; - if (error) { - [AVCaptureDeviceInput deviceInputWithDevice:self.device error:&error]; - if (error) { - reportError(ErrorCode::DeviceOpenFailed, "Open camera failed: " + std::string([error.localizedDescription UTF8String])); - return NO; - } - } + NSError* error = nil; + if ([_device lockForConfiguration:&error]) { + if ([_device isFocusModeSupported:AVCaptureFocusModeContinuousAutoFocus]) { + [_device setFocusMode:AVCaptureFocusModeContinuousAutoFocus]; + CCAP_NSLOG_V(@"ccap: Set focus mode to continuous auto focus"); + } + [_device unlockForConfiguration]; + } else { + CCAP_NSLOG_W(@"ccap: lockForConfiguration failed: %@", error.localizedDescription); + // Continue; some devices still work without tweaking focus settings. + } + + // Create input device + _videoInput = [AVCaptureDeviceInput deviceInputWithDevice:_device error:&error]; + if (!_videoInput || error) { + reportError(ErrorCode::DeviceOpenFailed, std::string("Open camera failed: ") + [error.localizedDescription UTF8String]); + return NO; + }Note: See my separate comment about
std::string_viewlifetimes when building error strings with concatenation.
446-458: RefactorreportErrorto own dynamically constructed descriptionsAfter inspecting the code, we confirmed that:
reportError(ErrorCode errorCode, std::string_view description)is declared insrc/ccap_imp.h:133and defined insrc/ccap_imp.cpp:193–197. It forwards the view synchronously to the global callback or log (no internal copy) before the call completes.- Call sites like
•src/ccap_imp_apple.mm:346•reportError(ErrorCode::DeviceOpenFailed, "Open camera failed: " + std::string([error.localizedDescription UTF8String]));src/ccap_imp_apple.mm:457•reportError(ErrorCode::UnsupportedPixelFormat, "Preferred pixel format not supported, fallback to: " + fallbackInfo.description);src/ccap_imp_apple.mm:647(and similar uses inreportError(ErrorCode::FrameRateSetFailed, "Desired fps (" + std::to_string(fps) + ") not supported, using fallback");ccap_imp_windows.cppandccap_imp_linux.cpp) build temporarystd::strings, bind them tostd::string_view, then destroy the temporaries at the end of the full expression. While the immediate callback or log invocation is safe, any storage or asynchronous use of the passed view will dangle.Mandatory refactor required:
- Change the API to take ownership of dynamic messages:
• Overload or replace withso that dynamically constructed strings remain valid untilvoid reportError(ErrorCode errorCode, std::string description);reportErrorreturns.
• Keep astd::string_viewoverload for static, literal messages to avoid unnecessary copies.- Update all call sites passing concatenated or
std::to_stringresults to use the rvalue‐std::stringoverload (either implicitly via overload resolution or by explicitly constructing astd::string).Key call sites to update:
- src/ccap_imp_apple.mm: lines 346, 457, 647
- src/ccap_imp_windows.cpp: lines 312, 593
- src/ccap_imp_linux.cpp: lines 114, 122, 190
♻️ Duplicate comments (4)
src/ccap_imp_windows.cpp (4)
311-313: Use hexadecimal formatting for HRESULT and keep messages consistentstd::to_string prints decimal, while the log above uses hex. Format as hex here to match expectations.
- // result is formatted as decimal by std::to_string, don't prefix with 0x - reportError(ErrorCode::NoDeviceFound, "CreateClassEnumerator CLSID_VideoInputDeviceCategory failed, result=" + std::to_string(result)); + { + std::ostringstream oss; + oss << "CreateClassEnumerator CLSID_VideoInputDeviceCategory failed, hr=0x" + << std::hex << std::uppercase << static_cast<unsigned long>(result); + reportError(ErrorCode::NoDeviceFound, oss.str()); + }
475-489: Bug: setGrabberOutputSubtype returns false on success (inverted condition) and always false overallCurrent logic returns false when SUCCEEDED(hr) and then falls through to return false in all cases. This masks success and breaks callers.
bool ProviderDirectShow::setGrabberOutputSubtype(GUID subtype) { if (m_sampleGrabber) { AM_MEDIA_TYPE mt; ZeroMemory(&mt, sizeof(mt)); mt.majortype = MEDIATYPE_Video; mt.subtype = subtype; mt.formattype = FORMAT_VideoInfo; HRESULT hr = m_sampleGrabber->SetMediaType(&mt); - if (SUCCEEDED(hr)) return false; - - reportError(ErrorCode::UnsupportedPixelFormat, "Set media type failed"); + if (SUCCEEDED(hr)) { + return true; + } + reportError(ErrorCode::UnsupportedPixelFormat, "Set media type failed"); } return false; }Follow-up: consider checking the return at the call site to bail early on failure:
// Around line 584 if (!setGrabberOutputSubtype(subtype)) { return false; }
601-615: Good: corrected return type on failure pathReturning false (vs HRESULT) fixes the earlier bool-return mismatch. Thanks for addressing it.
684-686: Remove “ccap:” prefix from user-facing error for consistencyOther reportError messages don’t prefix with “ccap:”. Keep it consistent.
- reportError(ErrorCode::InvalidDevice, "ccap: No video capture device: " + std::string(deviceName.empty() ? unavailableMsg : deviceName)); + reportError(ErrorCode::InvalidDevice, "No video capture device: " + std::string(deviceName.empty() ? unavailableMsg : deviceName));Optional: consider renaming unavailableMsg to a neutral string (e.g., "unavailable") to avoid embedding “ccap” in messages.
🧹 Nitpick comments (22)
src/ccap_imp_linux.cpp (4)
83-85: Minor: error code semantics for "already opened"Using DeviceOpenFailed works, though this is more of an invalid-state error than an open failure. If you have or plan a more specific code (e.g., InvalidState/AlreadyOpened), consider using it for clearer telemetry. Otherwise, keep as-is.
251-258: Consider using a capability-related error code instead of UnsupportedPixelFormatThese branches flag missing V4L2 capabilities (VIDEO_CAPTURE/STREAMING) as UnsupportedPixelFormat. That conflates device capability with format negotiation. If your ErrorCode set allows, prefer a capability-focused code (e.g., DeviceCapabilityUnsupported or DeviceOpenFailed with a distinct description). Improves analytics and user guidance.
326-328: Enrich the error message with device contextIncluding the device path helps users correlate failures on multi-camera systems.
Apply:
- reportError(ErrorCode::DeviceStartFailed, "Get device format failed: " + std::string(strerror(errno))); + reportError(ErrorCode::DeviceStartFailed, + "Get device format (VIDIOC_G_FMT) failed on " + m_devicePath + ": " + std::string(strerror(errno)));
357-361: Same as above: add device context on post-set VIDIOC_G_FMT failureApply:
- reportError(ErrorCode::DeviceStartFailed, "Get device format failed after set: " + std::string(strerror(errno))); + reportError(ErrorCode::DeviceStartFailed, + "Get device format failed after set on " + m_devicePath + ": " + std::string(strerror(errno)));src/ccap_imp_windows.cpp (10)
213-215: Include HRESULT in COM init failure for faster diagnosticsAdd the failing hr to the message (hex) so callers can triage COM init issues without reproducing.
- reportError(ErrorCode::InternalError, "COM initialization failed"); + { + std::ostringstream oss; + oss << "COM initialization failed, hr=0x" << std::hex << std::uppercase + << static_cast<unsigned long>(hr); + reportError(ErrorCode::InternalError, oss.str()); + }Add once at the top with the other headers:
#include <sstream>
301-304: Surface HRESULT on CoCreateInstance failureReturn path hides the specific failure code. Include result (hex) for parity with other logs.
- if (FAILED(result)) { - reportError(ErrorCode::NoDeviceFound, "Create system device enumerator failed"); + if (FAILED(result)) { + std::ostringstream oss; + oss << "Create system device enumerator failed, hr=0x" + << std::hex << std::uppercase << static_cast<unsigned long>(result); + reportError(ErrorCode::NoDeviceFound, oss.str()); return; }
587-595: Use hex for SetFormat result to match logging and Windows normsThe log line prints 0x%lx while reportError uses decimal. Align both to hex.
- } else { - CCAP_LOG_E("ccap: SetFormat failed, result=0x%lx\n", setFormatResult); - reportError(ErrorCode::UnsupportedPixelFormat, "SetFormat failed, result=" + std::to_string(setFormatResult)); + } else { + CCAP_LOG_E("ccap: SetFormat failed, result=0x%lx\n", setFormatResult); + std::ostringstream oss; + oss << "SetFormat failed, hr=0x" << std::hex << std::uppercase + << static_cast<unsigned long>(setFormatResult); + reportError(ErrorCode::UnsupportedPixelFormat, oss.str()); }
617-621: Include hr in RenderStream failureAdds valuable context when diagnosing graph connection issues.
- if (FAILED(hr)) { - reportError(ErrorCode::DeviceOpenFailed, "Render stream failed"); + if (FAILED(hr)) { + std::ostringstream oss; + oss << "Render stream failed, hr=0x" << std::hex << std::uppercase + << static_cast<unsigned long>(hr); + reportError(ErrorCode::DeviceOpenFailed, oss.str()); return false; }
636-644: Surface hr on GetConnectedMediaType failureThe HRESULT is available in-scope; include it for uniformity.
- } else { - reportError(ErrorCode::DeviceOpenFailed, "Get connected media type failed"); + } else { + std::ostringstream oss; + oss << "Get connected media type failed, hr=0x" << std::hex << std::uppercase + << static_cast<unsigned long>(hr); + reportError(ErrorCode::DeviceOpenFailed, oss.str()); return false; }
671-675: Add hr to “bind failed” InvalidDevice errorYou already log hr; echo it in reportError for consumers that don’t have logs.
- if (!deviceName.empty()) { - reportError(ErrorCode::InvalidDevice, "\"" + std::string(deviceName) + "\" is not a valid video capture device, bind failed"); - return true; // stop enumeration when returning true - } + if (!deviceName.empty()) { + std::ostringstream oss; + oss << "\"" << deviceName << "\" is not a valid video capture device, bind failed, hr=0x" + << std::hex << std::uppercase << static_cast<unsigned long>(hr); + reportError(ErrorCode::InvalidDevice, oss.str()); + return true; // stop enumeration when returning true + }
701-706: Include hr on IMediaControl QI failureAdds crucial detail when COM QI fails.
- if (FAILED(hr)) { - reportError(ErrorCode::DeviceOpenFailed, "QueryInterface IMediaControl failed"); + if (FAILED(hr)) { + std::ostringstream oss; + oss << "QueryInterface IMediaControl failed, hr=0x" << std::hex << std::uppercase + << static_cast<unsigned long>(hr); + reportError(ErrorCode::DeviceOpenFailed, oss.str()); return false; }
712-715: Use hex HRESULT in IVideoWindow QI failure and keep message consistentMatch the logging style and other error paths.
- if (FAILED(hr)) { - CCAP_LOG_E("ccap: QueryInterface IVideoWindow failed, result=0x%lx\n", hr); - reportError(ErrorCode::DeviceOpenFailed, "QueryInterface IVideoWindow failed, result=" + std::to_string(hr)); - return false; - } + if (FAILED(hr)) { + CCAP_LOG_E("ccap: QueryInterface IVideoWindow failed, result=0x%lx\n", hr); + std::ostringstream oss; + oss << "QueryInterface IVideoWindow failed, hr=0x" + << std::hex << std::uppercase << static_cast<unsigned long>(hr); + reportError(ErrorCode::DeviceOpenFailed, oss.str()); + return false; + }
1044-1049: Report hr on start failureExpose the IMediaControl::Run() HRESULT to pinpoint root causes.
- if (!m_isRunning) { - reportError(ErrorCode::DeviceStartFailed, "Start video capture failed"); + if (!m_isRunning) { + std::ostringstream oss; + oss << "Start video capture failed, hr=0x" << std::hex << std::uppercase + << static_cast<unsigned long>(hr); + reportError(ErrorCode::DeviceStartFailed, oss.str()); } else { CCAP_LOG_V("ccap: IMediaControl->Run() succeeded.\n"); }
311-313: Optional: factor a small helper to format HRESULTs onceYou repeat the “oss << 0x… << std::hex …” pattern several times. A tiny helper keeps messages concise and consistent.
Example (place in an anonymous namespace at top of file):
static std::string hresultHex(HRESULT hr) { std::ostringstream oss; oss << "0x" << std::hex << std::uppercase << static_cast<unsigned long>(hr); return oss.str(); }Then:
reportError(code, "SetFormat failed, hr=" + hresultHex(setFormatResult));Also applies to: 592-594, 713-714
src/ccap_imp_apple.mm (8)
250-276: Centralized auth error is good; avoid blocking the main thread during permission requestSwitching to
reportErrorat Line 274 is aligned with the PR goals. However, ifopen()is invoked on the main thread and authorization is not determined,dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER)will block UI while the OS permission sheet is up. Consider returning early (with an “authorization pending” status) or running the wait on a background thread to keep UI responsive.Would you like me to draft an async authorization flow so
open()never blocks the main thread?
320-327: Explicit nil check helps clarity in no-device pathObjective‑C will return NO for
hasMediaType:when_device == nil, but making the nil check explicit improves readability and avoids surprising behavior on refactors.Apply this diff:
- if (![_device hasMediaType:AVMediaTypeVideo]) { /// No video device found + if (!_device || ![_device hasMediaType:AVMediaTypeVideo]) { /// No video device found
646-649: Message accuracy and the same string_view lifetime risk
- If
bestRangeis null (Line 647), no fallback is actually applied. The message “using fallback” is misleading.- This call also builds a temporary
std::stringfor astd::string_viewparameter (lifetime risk noted elsewhere).Recommend adjusting the message (and addressing the lifetime topic globally):
Apply this diff:
- reportError(ErrorCode::FrameRateSetFailed, "Desired fps (" + std::to_string(fps) + ") not supported, using fallback"); + reportError(ErrorCode::FrameRateSetFailed, "Desired fps (" + std::to_string(fps) + ") not supported by the active format/resolution");Optionally, set
prop.fpsto the actual enforced fps (if determinable) to keep state consistent.
671-685: Make fallthrough explicit in pixel-format switch
case PixelFormat::I420:andcase PixelFormat::I420f:intentionally fall through after reporting the error. Add[[fallthrough]];to silence -Wimplicit-fallthrough and make intent explicit.Apply this diff:
case PixelFormat::I420: - reportError(ErrorCode::UnsupportedPixelFormat, "I420 is not supported on macOS, fallback to NV12"); + reportError(ErrorCode::UnsupportedPixelFormat, "I420 is not supported on macOS, fallback to NV12"); + [[fallthrough]]; @@ case PixelFormat::I420f: - reportError(ErrorCode::UnsupportedPixelFormat, "I420f is not supported on macOS, fallback to NV12f"); + reportError(ErrorCode::UnsupportedPixelFormat, "I420f is not supported on macOS, fallback to NV12f"); + [[fallthrough]];
737-738: “No connections available” might be transient — consider lowering severity or gatingIf
flushResolutioncan be called before outputs/connections are fully established in any flow, this could spam the error callback. Consider downgrading to a warning or reporting it once per open attempt.
794-797: Throttle provider-nil error to avoid per-frame error stormsIf
_provideris nil, this path fires on every captured frame. Throttle to once to avoid overwhelming clients of the error callback.Apply this diff:
- if (!_provider) { - reportError(ErrorCode::InitializationFailed, "CameraCaptureObjc captureOutput - provider is nil"); - return; - } + if (!_provider) { + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + reportError(ErrorCode::InitializationFailed, "CameraCaptureObjc captureOutput - provider is nil"); + }); + return; + }
978-983: “Already opened” path is correct; consider a more specific ErrorCode (if available)
DeviceOpenFailedworks, but anAlreadyOpened/InvalidStatecode would be more precise, if defined.
1024-1027: Prefer a “NotOpened”/“InvalidState” error for getDeviceInfo when closed
InitializationFailedis serviceable, but a state-specific code would make downstream handling cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
include/ccap_def.h(4 hunks)src/ccap_core.cpp(5 hunks)src/ccap_imp.cpp(3 hunks)src/ccap_imp_apple.mm(15 hunks)src/ccap_imp_linux.cpp(11 hunks)src/ccap_imp_windows.cpp(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- include/ccap_def.h
- src/ccap_imp.cpp
- src/ccap_core.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/ccap_imp_windows.cpp (1)
src/ccap_imp.cpp (2)
reportError(193-199)reportError(193-193)
src/ccap_imp_linux.cpp (1)
src/ccap_imp.cpp (2)
reportError(193-199)reportError(193-193)
🔇 Additional comments (16)
src/ccap_imp_linux.cpp (7)
246-248: LGTM: capability query failure reports errno and uses DeviceOpenFailedGood mapping and message construction; errno detail is very helpful for field debugging.
382-384: LGTM: VIDIOC_REQBUFS failure mapped to MemoryAllocationFailed with errnoAppropriate code and message.
387-389: LGTM: detecting insufficient buffersClear message; right error code.
455-455: LGTM: reports DeviceStopFailed in addition to loggingGood to propagate stop failures to the centralized error channel.
641-641: LGTM: destructor-path QBUF failure reports FrameCaptureFailedBoth logging and callback are useful here; destructor-path errors are otherwise easy to miss.
657-657: LGTM: QBUF failure after copy reported as FrameCaptureFailedConsistent with the destructor path.
122-124: Confirm callback contract for std::string_view lifetimesMany calls build a temporary std::string (e.g., "...: " + std::string(strerror(errno))) and pass it as std::string_view to reportError(). That’s fine only if ErrorCallback consumes the view synchronously and never stores it. Please ensure the public docs for ErrorCallback explicitly state that the description view is valid only during the callback invocation.
Would you like me to add a brief note in include/ccap_def.h next to the ErrorCallback typedef?
src/ccap_imp_windows.cpp (5)
451-472: Good: standardized error reporting on graph creation pathsSwapping scattered logs for reportError with specific ErrorCode values improves consistency and downstream handling.
496-503: Good: precise error codes on SampleGrabber creation and QI failureClear mapping to DeviceOpenFailed and early returns look correct.
655-659: Good: prevent double-openClear error message and correct return flow.
696-699: Good: propagate stream creation failure via reportErrorClear reason and early exit.
739-741: Nice: centralize per-frame GetPointer failure via reportErrorThis keeps callback failures visible to API users without spamming logs.
src/ccap_imp_apple.mm (4)
299-306: Session preset fallback: warning level is appropriateChanging an unsupported preset to a warning (Line 301) while falling back to
AVCaptureSessionPresetHighis reasonable and keeps the app resilient.
352-359: Centralized error on addInput failure: goodUsing
reportError(DeviceOpenFailed, "Session cannot add input device")(Line 357) provides a consistent failure path.
468-475: Centralized error on addOutput failure: goodConsistent with the new error-callback flow. No issues.
1041-1042: Start without an opened device: consistent error pathReporting
DeviceStartFailedwith a clear message (Line 1041) aligns with the centralized error handling. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ccap_imp_windows.cpp (1)
709-717: Leak: IVideoWindow is not released on the success pathWhen QueryInterface(IID_IVideoWindow) succeeds, videoWindow is never released. Add Release() after put_AutoShow(false) to avoid a COM refcount leak.
hr = m_graph->QueryInterface(IID_IVideoWindow, (LPVOID*)&videoWindow); if (FAILED(hr)) { CCAP_LOG_E("ccap: QueryInterface IVideoWindow failed, result=0x%lx\n", hr); reportError(ErrorCode::DeviceOpenFailed, "QueryInterface IVideoWindow failed, result=" + std::to_string(hr)); return false; } - videoWindow->put_AutoShow(false); + videoWindow->put_AutoShow(false); + videoWindow->Release();src/ccap_imp_linux.cpp (1)
198-201: Leak on start() failure: free driver buffers when startStreaming() (or prior steps) failWhen negotiateFormat/allocateBuffers/startStreaming fails, mmapped buffers requested via allocateBuffers() remain until close(). Release them on failure to prevent resource leakage.
- if (!negotiateFormat() || !allocateBuffers() || !startStreaming()) { - reportError(ErrorCode::DeviceStartFailed, "Failed to start streaming"); - return false; - } + if (!negotiateFormat()) { + reportError(ErrorCode::DeviceStartFailed, "Failed to negotiate format"); + return false; + } + if (!allocateBuffers()) { + reportError(ErrorCode::DeviceStartFailed, "Failed to allocate buffers"); + // allocateBuffers() already cleaned up on failure when needed + return false; + } + if (!startStreaming()) { + reportError(ErrorCode::DeviceStartFailed, "Failed to start streaming"); + // Clean up buffers requested earlier; ensure driver drops them + releaseAndFreeDriverBuffers(); + return false; + }
🧹 Nitpick comments (7)
src/ccap_imp_windows.cpp (4)
311-313: Unify HRESULT formatting (hex) for diagnosticsThe comment intentionally avoids 0x-prefix with decimal, but other logs in this file print HRESULTs in hex (e.g., 0x%lx). For consistency and better Windows troubleshooting, prefer hex formatting in error messages as well.
Apply this minimal change:
- // result is formatted as decimal by std::to_string, don't prefix with 0x - reportError(ErrorCode::NoDeviceFound, "CreateClassEnumerator CLSID_VideoInputDeviceCategory failed, result=" + std::to_string(result)); + std::ostringstream oss; + oss << "CreateClassEnumerator CLSID_VideoInputDeviceCategory failed, result=0x" + << std::hex << std::uppercase << static_cast<unsigned long>(result); + reportError(ErrorCode::NoDeviceFound, oss.str());And add this include once at the top (near other headers):
#include <sstream>
592-594: SetFormat failure: confirm intended control flowOn SetFormat failure, the code logs and reports the error but continues building the graph. If the intention is to proceed and let downstream negotiation pick another type, this is fine; otherwise, consider bailing out to avoid partial graph states.
If failure should abort:
- } else { - CCAP_LOG_E("ccap: SetFormat failed, result=0x%lx\n", setFormatResult); - reportError(ErrorCode::UnsupportedPixelFormat, "SetFormat failed, result=" + std::to_string(setFormatResult)); - } + } else { + CCAP_LOG_E("ccap: SetFormat failed, result=0x%lx\n", setFormatResult); + reportError(ErrorCode::UnsupportedPixelFormat, "SetFormat failed"); + return false; + }
684-686: Clarify fallback device message; avoid “ccap unavailable by now”The fallback currently uses unavailableMsg = "ccap unavailable by now", producing messages like “No video capture device: ccap unavailable by now”. Consider a neutral placeholder.
-constexpr const char* unavailableMsg = "ccap unavailable by now"; +constexpr const char* unavailableMsg = "<unspecified>";
1046-1047: Include HRESULT in start failure for better debuggingStart failure reports without the HRESULT. Add hr (prefer hex) to the message.
- if (!m_isRunning) { - reportError(ErrorCode::DeviceStartFailed, "Start video capture failed"); + if (!m_isRunning) { + std::ostringstream oss; + oss << "Start video capture failed, hr=0x" << std::hex << std::uppercase << static_cast<unsigned long>(hr); + reportError(ErrorCode::DeviceStartFailed, oss.str());src/ccap_imp_linux.h (1)
93-94: API addition looks good; consider noexceptreleaseAndFreeDriverBuffers is a cleanup helper; declare it noexcept to document it won’t throw during teardown.
- void releaseAndFreeDriverBuffers(); + void releaseAndFreeDriverBuffers() noexcept;src/ccap_imp_linux.cpp (2)
90-99: Deterministic default device selection: add exception safetystd::filesystem::directory_iterator may throw; wrap the iteration to avoid throwing across API boundaries.
- std::vector<std::string> candidates; - for (const auto& entry : std::filesystem::directory_iterator("/dev")) { + std::vector<std::string> candidates; + try { + for (const auto& entry : std::filesystem::directory_iterator("/dev")) { const std::string filename = entry.path().filename().string(); if (filename.rfind("video", 0) == 0 && isVideoDevice(entry.path().string())) { candidates.emplace_back(entry.path().string()); } - } + } + } catch (const std::exception& e) { + reportError(ErrorCode::NoDeviceFound, std::string("Enumerating /dev failed: ") + e.what()); + }Also applies to: 103-106
254-266: Account for device_caps when checking capabilitiesSome drivers set V4L2_CAP_DEVICE_CAPS; in that case device_caps contains the relevant bits. Use (device_caps ? device_caps : capabilities).
- if (!(m_caps.capabilities & V4L2_CAP_VIDEO_CAPTURE)) { + const auto caps = (m_caps.capabilities & V4L2_CAP_DEVICE_CAPS) ? m_caps.device_caps : m_caps.capabilities; + if (!(caps & V4L2_CAP_VIDEO_CAPTURE)) { reportError(ErrorCode::UnsupportedPixelFormat, "Device does not support video capture"); return false; } - if (!(m_caps.capabilities & V4L2_CAP_STREAMING)) { + if (!(caps & V4L2_CAP_STREAMING)) { reportError(ErrorCode::UnsupportedPixelFormat, "Device does not support streaming"); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/ccap_imp_apple.mm(15 hunks)src/ccap_imp_linux.cpp(13 hunks)src/ccap_imp_linux.h(2 hunks)src/ccap_imp_windows.cpp(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ccap_imp_apple.mm
🧰 Additional context used
🧬 Code graph analysis (3)
src/ccap_imp_linux.h (1)
src/ccap_imp_linux.cpp (2)
releaseAndFreeDriverBuffers(438-449)releaseAndFreeDriverBuffers(438-438)
src/ccap_imp_windows.cpp (1)
src/ccap_imp.cpp (2)
reportError(193-199)reportError(193-193)
src/ccap_imp_linux.cpp (1)
src/ccap_imp.cpp (2)
reportError(193-199)reportError(193-193)
⏰ 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). (2)
- GitHub Check: Build ARM64 (Release-gcc)
- GitHub Check: Build (Release-gcc)
🔇 Additional comments (11)
src/ccap_imp_windows.cpp (3)
214-214: Good: centralized COM init failure reportingUsing reportError here aligns with the new error-callback flow and avoids silent COM init failures.
483-486: Fix confirmed: success path now returns trueThe inverted success check in SetMediaType is corrected. Returning true on SUCCEEDED(hr) is the right behavior; error path reports via reportError and falls through to false.
611-615: Good: bool-returning path no longer returns HRESULTReplaces the problematic return hr; with return false;, preventing false-positive success on failure.
src/ccap_imp_linux.h (2)
104-104: Style-only brace normalization is fineInitializer normalization to {} is consistent with the rest of the codebase.
109-109: Style-only brace normalization is fineSame as above for m_currentFormat.
src/ccap_imp_linux.cpp (6)
16-16: Good: added for deterministic orderingThis supports the sorted candidate selection logic in open().
84-86: Appropriate guard against double-openEarly-failing with a clear error code is correct here.
409-411: Good: robust cleanup on QUERYBUF/MMAP failurereleaseAndFreeDriverBuffers() makes allocation failure paths leak-safe.
Also applies to: 419-421
438-449: Helper implementation looks correctUnmaps local buffers and hints the driver to free; ignoring the ioctl return here is appropriate during teardown.
460-461: Clear error signaling on QBUF/STREAMON failureStraightforward and consistent with centralized reporting.
Also applies to: 466-469
662-663: Good: centralized error reporting for requeue failuresBoth in normal and destructor paths, errors are reported via FrameCaptureFailed, keeping diagnostics consistent.
Also applies to: 678-679
Summary by CodeRabbit
Breaking Changes
Refactor
New Features
Chores