Repository navigation
Add OpenCV 5 face, img_hash, and line_descriptor APIs - #2051
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds OpenCV face-analysis and facemark APIs, one-shot image hashing, and binary line-descriptor detection, matching, drawing, native interop, and tests. ChangesFace algorithms and prediction collectors
Facemark training and utilities
One-shot image hashing
Binary line descriptors and matching
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/OpenCvSharp/Modules/face/FaceRecognizer/PredictCollector.cs (1)
102-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
Combinehelper to construct the dictionary.Reusing the
Combinehelper here enforces the array length consistency check (as done inGetResults) and cleanly converts the outputs without redundantZipand array materialization logic.♻️ Proposed refactor
public IReadOnlyDictionary<int, double> GetResultsMap() { ThrowIfDisposed(); using var labels = new StdVector<int>(); using var distances = new StdVector<double>(); NativeMethods.HandleException( NativeMethods.face_StandardCollector_getResultsMap( Handle, labels.CvPtr, distances.CvPtr)); - return labels.ToArray().Zip(distances.ToArray()).ToDictionary(x => x.First, x => x.Second); + return Combine(labels.ToArray(), distances.ToArray()).ToDictionary(x => x.Label, x => x.Distance); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OpenCvSharp/Modules/face/FaceRecognizer/PredictCollector.cs` around lines 102 - 111, Update GetResultsMap to use the existing Combine helper when converting labels and distances into the result dictionary. Preserve the current native call and disposal flow, while replacing the direct Zip/ToDictionary conversion so Combine enforces matching output lengths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OpenCvSharp/Modules/face/Facemark/FacemarkTrain.cs`:
- Around line 58-67: Change FacemarkTrain.GetData(IntPtr items) from a public
API to an internal-only binding, preventing callers from supplying arbitrary
native pointers. Add or use typed per-algorithm data wrapper methods that
validate and marshal their data before invoking the native getData path, while
preserving disposal and native exception handling.
---
Nitpick comments:
In `@src/OpenCvSharp/Modules/face/FaceRecognizer/PredictCollector.cs`:
- Around line 102-111: Update GetResultsMap to use the existing Combine helper
when converting labels and distances into the result dictionary. Preserve the
current native call and disposal flow, while replacing the direct
Zip/ToDictionary conversion so Combine enforces matching output lengths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c6b6f94-f361-4d12-8bc3-adb242c14e05
📒 Files selected for processing (39)
src/OpenCvSharp/Internal/PInvoke/NativeMethods/NativeMethods_img_hash.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/NativeMethods_line_descriptor.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/NativeMethods_stdvector.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/face/FacemarkKazemiParamsData.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/face/NativeMethods_face_Additional.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/face/NativeMethods_face_FaceRecognizer.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/face/NativeMethods_face_Facemark.cssrc/OpenCvSharp/Internal/Vectors/VectorOfKeyLine.cssrc/OpenCvSharp/Modules/face/BIF.cssrc/OpenCvSharp/Modules/face/Cv2.Face.cssrc/OpenCvSharp/Modules/face/FaceRecognizer/FaceRecognizer.cssrc/OpenCvSharp/Modules/face/FaceRecognizer/PredictCollector.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkFaceDetector.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkKazemi.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkTrain.cssrc/OpenCvSharp/Modules/face/MACE.cssrc/OpenCvSharp/Modules/img_hash/Cv2.ImgHash.cssrc/OpenCvSharp/Modules/line_descriptors/BinaryDescriptor.cssrc/OpenCvSharp/Modules/line_descriptors/BinaryDescriptorMatcher.cssrc/OpenCvSharp/Modules/line_descriptors/Cv2.LineDescriptor.cssrc/OpenCvSharp/Modules/line_descriptors/DrawLinesMatchesFlags.cssrc/OpenCvSharpExtern/face.cppsrc/OpenCvSharpExtern/face_Additional.hsrc/OpenCvSharpExtern/face_FaceRecognizer.hsrc/OpenCvSharpExtern/face_Facemark.hsrc/OpenCvSharpExtern/img_hash.hsrc/OpenCvSharpExtern/line_descriptor.hsrc/OpenCvSharpExtern/std_vector.htest/OpenCvSharp.Tests/face/BIFTest.cstest/OpenCvSharp.Tests/face/FaceUtilitiesTest.cstest/OpenCvSharp.Tests/face/FacemarkAAMTest.cstest/OpenCvSharp.Tests/face/FacemarkKazemiTest.cstest/OpenCvSharp.Tests/face/LBPHFaceRecognizerTest.cstest/OpenCvSharp.Tests/face/MACETest.cstest/OpenCvSharp.Tests/face/StandardCollectorTest.cstest/OpenCvSharp.Tests/img_hash/ImgHashFunctionsTest.cstest/OpenCvSharp.Tests/line_descriptor/BinaryDescriptorTest.cs
- redesign FacemarkTrain.GetData(void*): the generic raw-pointer binding is removed entirely (it was unreferenced) in favor of a typed FacemarkAAM.GetData(out Point2f[] s0), since FacemarkAAM is the only concrete algorithm that populates anything through it (FacemarkLBF's override ignores the pointer and always returns false) - reuse the Combine helper in StandardCollector.GetResultsMap, matching GetResults, instead of a separate Zip/ToDictionary conversion While fixing the GetData exposure, found that FacemarkAAM/FacemarkLBF/ FacemarkKazemi (like the saliency classes fixed in #2052) crash when calling Algorithm.Write/Read: cv::face::Facemark inherits cv::Algorithm *virtually*, so the raw pointer these classes hand to the generic core_Algorithm_write/ read bridge (typed as cv::Algorithm*) is not the same address as the correctly-adjusted pointer the compiler would produce from the concrete type. Fixed by giving all three classes their own write/read bridge that calls write()/read() on the pointer at its native concrete type, plus regression tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Pushed a follow-up commit addressing CodeRabbit's review:
While redesigning Full suite: 1544 total, 1518 passed, 26 skipped, 0 failed - no regressions. Not addressed: the docstring coverage warning (23.08% vs. the 80% threshold) - that's a broad documentation pass across the 39 files in this PR, out of scope for a targeted review-feedback fix. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/OpenCvSharp.Tests/face/FacemarkAAMTest.cs`:
- Around line 186-213: Use a unique Guid-based filename for each temporary file
in FacemarkAAMTest.cs lines 186-213, FacemarkKazemiTest.cs lines 21-48, and
FacemarkLBFTest.cs lines 21-48, updating the corresponding ReadWrite test setup
while preserving the existing cleanup logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea04b6f9-5adb-4ba9-b38a-a0975998111e
📒 Files selected for processing (10)
src/OpenCvSharp/Internal/PInvoke/NativeMethods/face/NativeMethods_face_Facemark.cssrc/OpenCvSharp/Modules/face/FaceRecognizer/PredictCollector.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkKazemi.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cssrc/OpenCvSharp/Modules/face/Facemark/FacemarkTrain.cssrc/OpenCvSharpExtern/face_Facemark.htest/OpenCvSharp.Tests/face/FacemarkAAMTest.cstest/OpenCvSharp.Tests/face/FacemarkKazemiTest.cstest/OpenCvSharp.Tests/face/FacemarkLBFTest.cs
💤 Files with no reviewable changes (1)
- src/OpenCvSharp/Modules/face/Facemark/FacemarkTrain.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/OpenCvSharp/Modules/face/Facemark/FacemarkKazemi.cs
- src/OpenCvSharpExtern/face_Facemark.h
Comment on lines
+60
to
+67
| public override void Write(FileStorage fs) | ||
| { | ||
| ThrowIfDisposed(); | ||
| ArgumentNullException.ThrowIfNull(fs); | ||
|
|
||
| NativeMethods.HandleException(NativeMethods.face_FacemarkAAM_write(Handle, fs.CvPtr)); | ||
| GC.KeepAlive(fs); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add disposal checks for FileStorage and FileNode instances.
Passing a disposed FileStorage or FileNode object exposes its CvPtr as IntPtr.Zero, which causes an unmanaged segmentation fault when dereferenced by the native write or read functions. Always call ThrowIfDisposed() on these wrapper objects before interacting with their native pointers.
src/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cs#L60-L67: Callfs.ThrowIfDisposed();immediately after the null check inWrite.src/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cs#L76-L83: Callfn.ThrowIfDisposed();immediately after the null check inRead.src/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cs#L264-L279: Callfn.ThrowIfDisposed();immediately after the null check inParams.Read.src/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cs#L285-L299: Callfs.ThrowIfDisposed();immediately after the null check inParams.Write.src/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cs#L60-L67: Callfs.ThrowIfDisposed();immediately after the null check inWrite.src/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cs#L76-L83: Callfn.ThrowIfDisposed();immediately after the null check inRead.src/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cs#L202-L217: Callfn.ThrowIfDisposed();immediately after the null check inParams.Read.src/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cs#L223-L237: Callfs.ThrowIfDisposed();immediately after the null check inParams.Write.
📍 Affects 2 files
src/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cs#L60-L67(this comment)src/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cs#L76-L83src/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cs#L264-L279src/OpenCvSharp/Modules/face/Facemark/FacemarkAAM.cs#L285-L299src/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cs#L60-L67src/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cs#L76-L83src/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cs#L202-L217src/OpenCvSharp/Modules/face/Facemark/FacemarkLBF.cs#L223-L237
Comment on lines
+186
to
+213
| [Fact] | ||
| public void ReadWrite() | ||
| { | ||
| // cv::face::Facemark inherits Algorithm virtually; this exercises the P/Invoke plumbing | ||
| // that keeps the native pointer at its concrete type across the write/read call, rather | ||
| // than reinterpreting it as cv::Algorithm* (which would corrupt memory). | ||
| using var facemark = FacemarkAAM.Create(); | ||
|
|
||
| var fileName = Path.Combine(Path.GetTempPath(), "facemark_aam_test.yml"); | ||
| try | ||
| { | ||
| using (var fs = new FileStorage(fileName, FileStorage.Modes.Write)) | ||
| { | ||
| fs.Write("marker", 1); | ||
| facemark.Write(fs); | ||
| } | ||
|
|
||
| using var fs2 = new FileStorage(fileName, FileStorage.Modes.Read); | ||
| var root = fs2.Root(); | ||
| Assert.NotNull(root); | ||
| facemark.Read(root); | ||
| } | ||
| finally | ||
| { | ||
| if (File.Exists(fileName)) | ||
| File.Delete(fileName); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use unique filenames to prevent parallel test collisions.
Hardcoding static temporary file names can lead to file-lock conflicts or data corruption if the test suite is executed concurrently (e.g., across multiple CI workers or assemblies) or if a previous test run aborts before cleanup. Incorporate a Guid into the file names.
test/OpenCvSharp.Tests/face/FacemarkAAMTest.cs#L186-L213: Changevar fileName = Path.Combine(Path.GetTempPath(), "facemark_aam_test.yml");tovar fileName = Path.Combine(Path.GetTempPath(), $"facemark_aam_test_{Guid.NewGuid():N}.yml");.test/OpenCvSharp.Tests/face/FacemarkKazemiTest.cs#L21-L48: Change the hardcoded name to include a unique Guid suffix.test/OpenCvSharp.Tests/face/FacemarkLBFTest.cs#L21-L48: Change the hardcoded name to include a unique Guid suffix.
📍 Affects 3 files
test/OpenCvSharp.Tests/face/FacemarkAAMTest.cs#L186-L213(this comment)test/OpenCvSharp.Tests/face/FacemarkKazemiTest.cs#L21-L48test/OpenCvSharp.Tests/face/FacemarkLBFTest.cs#L21-L48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/OpenCvSharp.Tests/face/FacemarkAAMTest.cs` around lines 186 - 213, Use a
unique Guid-based filename for each temporary file in FacemarkAAMTest.cs lines
186-213, FacemarkKazemiTest.cs lines 21-48, and FacemarkLBFTest.cs lines 21-48,
updating the corresponding ReadWrite test setup while preserving the existing
cleanup logic.
- add fs.ThrowIfDisposed()/fn.ThrowIfDisposed() before dereferencing CvPtr in FacemarkAAM/FacemarkLBF/FacemarkKazemi's Write/Read overrides and in FacemarkAAM.Params/FacemarkLBF.Params's Read/Write, so a disposed FileStorage/FileNode throws ObjectDisposedException instead of handing native a null pointer to dereference - use a Guid-suffixed temp filename in the three Facemark ReadWrite regression tests to avoid collisions under parallel test execution Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Pushed a follow-up commit addressing the second round of CodeRabbit review (from the write/read bridge fix):
Full suite: 1544 total, 1518 passed, 26 skipped, 0 failed - no regressions. Still not addressed: the docstring coverage warning (23.08% vs. 80% threshold) - unchanged from prior rounds, out of scope for a targeted review-feedback pass. |
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
faceAPIs, including BIF, MACE, prediction collectors, trainable facemarks, FacemarkKazemi, AAM runtime configuration, and dataset utilitiesimg_hashone-shot functions underCv2.ImgHashline_descriptorOpenCV declares
BinaryDescriptor::EDLineDetectorandEDLineParaminside the class's private section, so they cannot be referenced by external C++ callers and are intentionally not exposed.Testing
cmake --build src\build --config Release --target OpenCvSharpExterndotnet build src\OpenCvSharp\OpenCvSharp.csproj -c Release --no-restore(0 warnings, 0 errors)dotnet test test\OpenCvSharp.Tests\OpenCvSharp.Tests.csproj -c Release --no-build(1,514 passed, 26 skipped, 0 failed)git diff --checkCloses #2014
Summary by CodeRabbit
Cv2.ImgHashhashing helpers (average, block-mean, color-moment, Marr-Hildreth, pHash, radial-variance).BIF,MACE,FacemarkTrainworkflow, Kazemi, LBF/AAM model read/write, AAM config fitting/data access, plusCv2.Facedataset/landmark utilities.PredictCollector/StandardCollector, including aPredictoverload.BinaryDescriptor,BinaryDescriptorMatcher, andCv2.LineDescriptordrawing helpers.