Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors FaceDetectorYN interop: removes old cve_* P/Invoke and C wrappers, adds objdetect_* C wrappers and new P/Invoke signatures, and converts the managed FaceDetectorYN to a Ptr-backed factory-based implementation with updated lifetime and detect flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Client/Test
participant Managed as FaceDetectorYN (managed)
participant PInvoke as NativeMethods (P/Invoke)
participant Extern as OpenCvSharpExtern (C API)
participant OpenCV as OpenCV::FaceDetectorYN
Test->>Managed: FaceDetectorYN.Create(model, config, inputSize, ...)
Managed->>PInvoke: objdetect_FaceDetectorYN_create(...)
PInvoke->>Extern: call objdetect_FaceDetectorYN_create C wrapper
Extern->>OpenCV: cv::FaceDetectorYN::create(...)
OpenCV-->>Extern: cv::Ptr<cv::FaceDetectorYN> (wrapped)
Extern-->>PInvoke: ExceptionStatus + out ptr
PInvoke-->>Managed: return pointer
Managed->>Managed: store Ptr wrapper
Test->>Managed: Detect(image)
Managed->>PInvoke: objdetect_Ptr_FaceDetectorYN_get(ptr)
PInvoke->>Extern: objdetect_Ptr_FaceDetectorYN_get
Extern->>OpenCV: return raw cv::FaceDetectorYN*
Managed->>PInvoke: objdetect_FaceDetectorYN_detect(obj, image, faces, out rv)
PInvoke->>Extern: call detect wrapper
Extern->>OpenCV: obj->detect(...)
OpenCV-->>Extern: faces
Extern-->>PInvoke: ExceptionStatus + return value
PInvoke-->>Managed: detection result
Managed->>PInvoke: objdetect_Ptr_FaceDetectorYN_delete(ptr) (on dispose)
PInvoke->>Extern: delete cv::Ptr
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/OpenCvSharp/Internal/PInvoke/NativeMethods/objdetect/NativeMethods_objdetect_FaceDetectorYN.cs (1)
25-26:[Pure]attribute is incorrect on the delete method.The
objdetect_Ptr_FaceDetectorYN_deletemethod has side effects (memory deallocation), so the[Pure]attribute is semantically misleading. The[Pure]attribute indicates a method has no observable side effects, which is not the case for a delete operation.Suggested fix
- [Pure, DllImport(DllExtern, CallingConvention = CallingConvention.Cdecl, BestFitMapping = false, ThrowOnUnmappableChar = true, ExactSpelling = true)] + [DllImport(DllExtern, CallingConvention = CallingConvention.Cdecl, BestFitMapping = false, ThrowOnUnmappableChar = true, ExactSpelling = true)] public static extern ExceptionStatus objdetect_Ptr_FaceDetectorYN_delete(IntPtr ptr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenCvSharp/Internal/PInvoke/NativeMethods/objdetect/NativeMethods_objdetect_FaceDetectorYN.cs` around lines 25 - 26, The method declaration for objdetect_Ptr_FaceDetectorYN_delete incorrectly has the [Pure] attribute despite performing side effects (memory deallocation); remove the [Pure] attribute from the extern method declaration so the signature only includes the DllImport and other relevant attributes, ensuring objdetect_Ptr_FaceDetectorYN_delete no longer claims purity.src/OpenCvSharp/Modules/objdetect/FaceDetectorYN.cs (1)
81-86: Consider nullingptrafter disposingdetectorPtr.After
detectorPtr?.Dispose()is called, the inheritedptrfield still holds a raw pointer that becomes dangling. WhiledetectorPtris set to null,ptrremains set. If any code path accessesptrafter disposal (despiteThrowIfDisposedchecks), it could lead to undefined behavior.Suggested fix
protected override void DisposeManaged() { detectorPtr?.Dispose(); detectorPtr = null; + ptr = IntPtr.Zero; base.DisposeManaged(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenCvSharp/Modules/objdetect/FaceDetectorYN.cs` around lines 81 - 86, In DisposeManaged of FaceDetectorYN, after calling detectorPtr?.Dispose() and before/after nulling detectorPtr, also set the inherited raw pointer field ptr to IntPtr.Zero (or null equivalent) so the dangling native pointer is cleared; update DisposeManaged to null detectorPtr and then assign ptr = IntPtr.Zero to prevent any access to a stale pointer (ensure this change preserves calls to base.DisposeManaged and still allows ThrowIfDisposed to function correctly).src/OpenCvSharpExtern/objdetect_FaceDetectorYN.h (1)
42-48: Consider adding null check before dereferencingptr.The
objdetect_Ptr_FaceDetectorYN_getfunction dereferencesptrwithout a null check. While the managed code should ensure valid pointers are passed, defensive programming could prevent crashes from invalid states.Optional defensive fix
CVAPI(ExceptionStatus) objdetect_Ptr_FaceDetectorYN_get( cv::Ptr<cv::FaceDetectorYN>* ptr, cv::FaceDetectorYN** returnValue) { BEGIN_WRAP + if (!ptr) { + *returnValue = nullptr; + return ExceptionStatus::Occurred; + } *returnValue = ptr->get(); END_WRAP }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenCvSharpExtern/objdetect_FaceDetectorYN.h` around lines 42 - 48, The objdetect_Ptr_FaceDetectorYN_get function dereferences ptr without validation; add checks in objdetect_Ptr_FaceDetectorYN_get to verify ptr is not null and ptr->get() is not null before assigning to *returnValue, and if either is null set *returnValue = nullptr and return the appropriate error ExceptionStatus (or use the existing error handling macros used by BEGIN_WRAP/END_WRAP) so you avoid undefined dereference in cv::Ptr<cv::FaceDetectorYN>* ptr and when calling ptr->get().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/OpenCvSharp/Internal/PInvoke/NativeMethods/objdetect/NativeMethods_objdetect_FaceDetectorYN.cs`:
- Around line 25-26: The method declaration for
objdetect_Ptr_FaceDetectorYN_delete incorrectly has the [Pure] attribute despite
performing side effects (memory deallocation); remove the [Pure] attribute from
the extern method declaration so the signature only includes the DllImport and
other relevant attributes, ensuring objdetect_Ptr_FaceDetectorYN_delete no
longer claims purity.
In `@src/OpenCvSharp/Modules/objdetect/FaceDetectorYN.cs`:
- Around line 81-86: In DisposeManaged of FaceDetectorYN, after calling
detectorPtr?.Dispose() and before/after nulling detectorPtr, also set the
inherited raw pointer field ptr to IntPtr.Zero (or null equivalent) so the
dangling native pointer is cleared; update DisposeManaged to null detectorPtr
and then assign ptr = IntPtr.Zero to prevent any access to a stale pointer
(ensure this change preserves calls to base.DisposeManaged and still allows
ThrowIfDisposed to function correctly).
In `@src/OpenCvSharpExtern/objdetect_FaceDetectorYN.h`:
- Around line 42-48: The objdetect_Ptr_FaceDetectorYN_get function dereferences
ptr without validation; add checks in objdetect_Ptr_FaceDetectorYN_get to verify
ptr is not null and ptr->get() is not null before assigning to *returnValue, and
if either is null set *returnValue = nullptr and return the appropriate error
ExceptionStatus (or use the existing error handling macros used by
BEGIN_WRAP/END_WRAP) so you avoid undefined dereference in
cv::Ptr<cv::FaceDetectorYN>* ptr and when calling ptr->get().
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/OpenCvSharp/Internal/PInvoke/NativeMethods/objdetect/NativeMethods_FaceDetectorYN.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/objdetect/NativeMethods_objdetect_FaceDetectorYN.cssrc/OpenCvSharp/Modules/objdetect/FaceDetectorYN.cssrc/OpenCvSharpExtern/OpenCvSharpExtern.vcxprojsrc/OpenCvSharpExtern/face_detector_yn.cppsrc/OpenCvSharpExtern/face_detector_yn.hsrc/OpenCvSharpExtern/objdetect.cppsrc/OpenCvSharpExtern/objdetect_FaceDetectorYN.h
💤 Files with no reviewable changes (3)
- src/OpenCvSharp/Internal/PInvoke/NativeMethods/objdetect/NativeMethods_FaceDetectorYN.cs
- src/OpenCvSharpExtern/face_detector_yn.h
- src/OpenCvSharpExtern/face_detector_yn.cpp
Contributor
|
Please create a new release @shimat and thus new NuGet version. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close #1830
Summary
Refactor
FaceDetectorYNwrapper to follow the established conventions of this codebase, and fix theNO_OBJDETECTslim-build regression introduced by #1830.Changes
C++ (
OpenCvSharpExtern)face_detector_yn.{h,cpp}→objdetect_FaceDetectorYN.hobjdetect_QRCodeDetector.h,objdetect_HOGDescriptor.h, etc..cppfile is removed; the header is now included fromobjdetect.cppmodule_ClassName_methodNameconvention:cveFaceDetectorYNCreate→objdetect_FaceDetectorYN_createcveFaceDetectorYNRelease→objdetect_Ptr_FaceDetectorYN_deletecveFaceDetectorYNDetect→objdetect_FaceDetectorYN_detectobjdetect_Ptr_FaceDetectorYN_getforcv::PtrunwrappingExceptionStatuswithBEGIN_WRAP/END_WRAP, matching all other wrappersclone()helper forcv::Ptrownership, consistent withfeatures2d_SIFT_createetc.NO_OBJDETECTbuild: the entire header is now guarded by#ifndef NO_OBJDETECT, so slim builds that exclude the objdetect module compile cleanly without stubs or undefined-type referencesC# (
OpenCvSharp)NativeMethods_FaceDetectorYN.cs→NativeMethods_objdetect_FaceDetectorYN.cs[DllImport]entries updated to match new function names andExceptionStatus-based signaturesFaceDetectorYN.csto follow theSIFT/BRISKpattern:private; replaced by apublic static FaceDetectorYN Create(...)factory methodPtrclass backed byobjdetect_Ptr_FaceDetectorYN_get/objdetect_Ptr_FaceDetectorYN_delete, instead of a raw_sharedPtrfield with manual lifetime trackingNativeMethods.HandleException(...)Motivation
The original implementation was contributed by a third party (PR #1830) and fixed a real build failure, but its naming conventions, file layout, error-handling style, and memory-management pattern were inconsistent with the rest of the codebase. This PR retains the intent of the fix while bringing everything in line with the existing conventions.
Summary by CodeRabbit