Repository navigation
OpenCV 5.x API coverage: photo, text, saliency, shape, superres, ximgproc gaps - #2052
Merged
Merged
Conversation
…ge sweep Implements the coverage gaps identified in #2015: - superres: wire up SuperResolution.OpticalFlow (native bridge already existed) - shape: ShapeContextDistanceExtractor.GetCostExtractor/GetTransformAlgorithm - ximgproc: findEllipses, RadonTransform, ScanSegment, ContourFitting (+ fourierDescriptor/transformFD/contourSampling), DisparityFilter/ DisparityWLSFilter, SparseMatchInterpolator/EdgeAwareInterpolator/ RICInterpolator (StereoMatcher gains a generic Ptr_get for the createRightMatcher/DisparityFilter.Filter overloads that return/accept it) - photo: ColorCorrectionModel (ccm) plus its 7 enums, and gammaCorrection - text: the ER-filter pipeline (ERFilter/ERFilterCallback, computeNMChannels, detectRegions) and the non-Tesseract OCR decoders (OCRHMMDecoder, OCRBeamSearchDecoder, OCRHolisticWordRecognizer), scoped to file/classifier- based construction - implementing ClassifierCallback/ERFilter::Callback in managed code would need a native->managed virtual dispatch trampoline this repo has no precedent for. ERStat and the ERStat-vector overloads of erGrouping/MSERsToERStats are skipped: ERStat is a tree-node struct with internal parent/child/sibling pointers that isn't safely marshalable as a vector element: detectRegions covers the same end-to-end use case with Point-based regions instead. While adding StaticSaliencySpectralResidual.Write/Read coverage, found that it (and every other saliency class: StaticSaliencyFineGrained, MotionSaliencyBinWangApr2014, ObjectnessBING) crashed when calling Algorithm.Write/Read: cv::saliency::Saliency 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 four classes their own write/read bridge that calls write()/read() on the pointer at its native concrete type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 (12)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe change expands OpenCV 5.x coverage across photo, text, saliency, shape, superres, and ximgproc modules with managed wrappers, native exports, P/Invoke bindings, build wiring, and tests. ChangesOpenCV 5.x API coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Cv2Text
participant NativeMethods
participant OpenCVText
Caller->>Cv2Text: Create OCR decoder
Cv2Text->>NativeMethods: Invoke decoder creation binding
NativeMethods->>OpenCVText: Create native decoder
OpenCVText-->>NativeMethods: Return decoder pointer
NativeMethods-->>Cv2Text: Return managed handle
Caller->>Cv2Text: Run image or image plus mask
Cv2Text->>NativeMethods: Invoke OCR run binding
NativeMethods->>OpenCVText: Run decoder and populate outputs
OpenCVText-->>NativeMethods: Return text and component data
NativeMethods-->>Cv2Text: Convert native outputs
Cv2Text-->>Caller: Return OCR results
Possibly related issues
🚥 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: 9
🧹 Nitpick comments (2)
src/OpenCvSharpExtern/OpenCvSharpExtern.vcxproj (1)
186-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort header includes alphabetically for maintainability.
The newly added
ximgprocheaders are slightly out of alphabetical order.ximgproc_ContourFitting.handximgproc_DisparityFilter.hshould be placed beforeximgproc_EdgeBoxes.h.♻️ Proposed refactor
- <ClInclude Include="ximgproc_EdgeBoxes.h" /> - <ClInclude Include="ximgproc_EdgeFilter.h" /> - <ClInclude Include="ximgproc_FastLineDetector.h" /> - <ClInclude Include="ximgproc_ContourFitting.h" /> - <ClInclude Include="ximgproc_DisparityFilter.h" /> + <ClInclude Include="ximgproc_ContourFitting.h" /> + <ClInclude Include="ximgproc_DisparityFilter.h" /> + <ClInclude Include="ximgproc_EdgeBoxes.h" /> + <ClInclude Include="ximgproc_EdgeFilter.h" /> + <ClInclude Include="ximgproc_FastLineDetector.h" />🤖 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/OpenCvSharpExtern/OpenCvSharpExtern.vcxproj` around lines 186 - 190, Reorder the ximgproc header entries in the project file alphabetically, placing ximgproc_ContourFitting.h and ximgproc_DisparityFilter.h before ximgproc_EdgeBoxes.h while preserving the remaining entries.test/OpenCvSharp.Tests/saliency/MotionSaliencyTest.cs (1)
70-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the temporary file after the test completes.
The test writes a file to the temporary directory but does not clean it up, which pollutes the environment over successive test runs. Consider wrapping the file operations in a
try/finallyblock to ensure deletion.♻️ Proposed refactor
[Fact] public void ReadWrite() { // cv::saliency::Saliency 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 - see StaticSaliencyTest's // SpectralResidual_ReadWrite for the full explanation). using var saliency = MotionSaliencyBinWangApr2014.Create(); saliency.SetImagesize(64, 48); var fileName = Path.Combine(Path.GetTempPath(), "motion_saliency_bin_wang_apr2014_test.yml"); - using (var fs = new FileStorage(fileName, FileStorage.Modes.Write)) - { - fs.Write("marker", 1); - saliency.Write(fs); - } - - using var fs2 = new FileStorage(fileName, FileStorage.Modes.Read); - var root = fs2.Root(); - Assert.NotNull(root); - saliency.Read(root); + try + { + using (var fs = new FileStorage(fileName, FileStorage.Modes.Write)) + { + fs.Write("marker", 1); + saliency.Write(fs); + } + + using var fs2 = new FileStorage(fileName, FileStorage.Modes.Read); + var root = fs2.Root(); + Assert.NotNull(root); + saliency.Read(root); + } + finally + { + if (File.Exists(fileName)) + { + File.Delete(fileName); + } + } }🤖 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/saliency/MotionSaliencyTest.cs` around lines 70 - 91, Update the ReadWrite test around the fileName and FileStorage operations to delete the generated temporary YAML file in a finally block, ensuring cleanup even when writing or reading fails while preserving the existing assertions and disposal behavior.
🤖 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/text/OCRBeamSearchDecoder.cs`:
- Around line 84-152: The OCR Run overloads do not validate the decoder receiver
before invoking native code. Add ThrowIfDisposed() on the receiver at the start
of every Run overload in OCRBeamSearchDecoder.cs (84-152) and OCRHMMDecoder.cs
(83-151), alongside the existing Mat validation; apply the same receiver check
to every Run overload in OCRTesseract.cs and OCRHolisticWordRecognizer.cs.
In `@src/OpenCvSharp/Modules/text/OCRHolisticWordRecognizer.cs`:
- Around line 47-57: Add ThrowIfDisposed() at the beginning of both Run
overloads in src/OpenCvSharp/Modules/text/OCRHolisticWordRecognizer.cs:47-57 and
src/OpenCvSharp/Modules/text/OCRHolisticWordRecognizer.cs:90-103, before any
native invocation or Handle use; no other changes are required.
In `@src/OpenCvSharp/Modules/ximgproc/DisparityWLSFilter.cs`:
- Around line 62-168: Add GC.KeepAlive(this) after the native operations in the
Lambda, SigmaColor, LRCthresh, and DepthDiscontinuityRadius property accessors,
and in GetConfidenceMap and GetROI. Ensure each getter and setter places the
keep-alive call at the end of its block while preserving the existing return
values and disposal checks.
In `@src/OpenCvSharp/Modules/ximgproc/ScanSegment.cs`:
- Around line 92-100: The ScanSegment OutputArray wrappers lack readiness
validation before native writes and metadata synchronization afterward. In
ScanSegment.cs lines 92-100, update GetLabels to call
labelsOut.ThrowIfNotReady() before the native invocation and labelsOut.Fix()
after it; in lines 110-118, apply the same sequence to image around its native
call.
In `@src/OpenCvSharpExtern/text_ERFilter.h`:
- Around line 165-177: Update text_detectRegions_rects so a null filename is
converted to an empty string before constructing cv::String, while preserving
the provided filename when non-null. Apply the guard at the
cv::text::detectRegions call and leave the remaining arguments unchanged.
In `@test/OpenCvSharp.Tests/saliency/ObjectnessBINGTest.cs`:
- Around line 91-104: Update the serialization test around ObjectnessBING.Read
so the bing.Base value is changed after writing and before reading, then assert
that bing.Base is restored to the originally serialized value. Keep the existing
FileStorage write/read flow and marker serialization unchanged.
In `@test/OpenCvSharp.Tests/saliency/StaticSaliencyTest.cs`:
- Around line 74-85: Ensure temporary YAML files are deleted even when the
saliency read/write tests fail: wrap the file operations in
SpectralResidual_ReadWrite and FineGrained_ReadWrite with try/finally cleanup,
checking File.Exists(fileName) before deleting. Apply this to both affected
ranges in test/OpenCvSharp.Tests/saliency/StaticSaliencyTest.cs: lines 74-85 and
134-145.
In `@test/OpenCvSharp.Tests/text/OCRDecoderTest.cs`:
- Around line 21-29: Update OCRHMMDecoderCreate_MissingFile_Throws and the other
OCR missing-file tests to use Assert.Throws<OpenCVException> instead of
Assert.ThrowsAny<Exception>, ensuring they validate the expected native OpenCV
failure rather than unrelated binding or load errors. Add an exception-message
assertion only if the missing-file text is stable across supported OpenCV
versions.
In `@test/OpenCvSharp.Tests/ximgproc/XimgProcTest.cs`:
- Around line 484-486: Update the XImgProc.TransformFD test call to assign the
Mat.Eye(...) result to a local Mat using declaration before passing it as the
transform matrix, ensuring the temporary native resource is disposed while
preserving the existing assertion and fdContour setting.
---
Nitpick comments:
In `@src/OpenCvSharpExtern/OpenCvSharpExtern.vcxproj`:
- Around line 186-190: Reorder the ximgproc header entries in the project file
alphabetically, placing ximgproc_ContourFitting.h and ximgproc_DisparityFilter.h
before ximgproc_EdgeBoxes.h while preserving the remaining entries.
In `@test/OpenCvSharp.Tests/saliency/MotionSaliencyTest.cs`:
- Around line 70-91: Update the ReadWrite test around the fileName and
FileStorage operations to delete the generated temporary YAML file in a finally
block, ensuring cleanup even when writing or reading fails while preserving the
existing assertions and disposal behavior.
🪄 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: 4804ceff-7eb8-419d-83cf-9d4d671e6b2d
📒 Files selected for processing (95)
src/OpenCvSharp/Cv2/Cv2_photo.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/photo/NativeMethods_photo_ColorCorrectionModel.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/saliency/NativeMethods_saliency_MotionSaliencyBinWangApr2014.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/saliency/NativeMethods_saliency_ObjectnessBING.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/saliency/NativeMethods_saliency_StaticSaliencyFineGrained.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/saliency/NativeMethods_saliency_StaticSaliencySpectralResidual.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/shape/NativeMethods_shape_ShapeDistanceExtractor.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/shape/NativeMethods_shape_ShapeTransformer.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/stereo/NativeMethods_stereo_StereoMatcher.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/superres/NativeMethods_superres_DenseOpticalFlowExt.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/text/NativeMethods_text_ERFilter.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/text/NativeMethods_text_OCRBeamSearchDecoder.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/text/NativeMethods_text_OCRHMMDecoder.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/text/NativeMethods_text_OCRHolisticWordRecognizer.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/ximgproc/NativeMethods_ximgproc.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/ximgproc/NativeMethods_ximgproc_ContourFitting.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/ximgproc/NativeMethods_ximgproc_DisparityFilter.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/ximgproc/NativeMethods_ximgproc_ScanSegment.cssrc/OpenCvSharp/Internal/PInvoke/NativeMethods/ximgproc/NativeMethods_ximgproc_SparseMatchInterpolator.cssrc/OpenCvSharp/Modules/photo/CcmType.cssrc/OpenCvSharp/Modules/photo/ColorCheckerType.cssrc/OpenCvSharp/Modules/photo/ColorCorrectionModel.cssrc/OpenCvSharp/Modules/photo/ColorSpace.cssrc/OpenCvSharp/Modules/photo/DistanceType.cssrc/OpenCvSharp/Modules/photo/InitialMethodType.cssrc/OpenCvSharp/Modules/photo/LinearizationType.cssrc/OpenCvSharp/Modules/saliency/MotionSaliencyBinWangApr2014.cssrc/OpenCvSharp/Modules/saliency/ObjectnessBING.cssrc/OpenCvSharp/Modules/saliency/StaticSaliencyFineGrained.cssrc/OpenCvSharp/Modules/saliency/StaticSaliencySpectralResidual.cssrc/OpenCvSharp/Modules/shape/HistogramCostExtractor.cssrc/OpenCvSharp/Modules/shape/ShapeContextDistanceExtractor.cssrc/OpenCvSharp/Modules/shape/ShapeTransformer.cssrc/OpenCvSharp/Modules/stereo/StereoMatcher.cssrc/OpenCvSharp/Modules/superres/BroxOpticalFlow.cssrc/OpenCvSharp/Modules/superres/DenseOpticalFlowExt.cssrc/OpenCvSharp/Modules/superres/DualTVL1OpticalFlow.cssrc/OpenCvSharp/Modules/superres/FarnebackOpticalFlow.cssrc/OpenCvSharp/Modules/superres/PyrLKOpticalFlow.cssrc/OpenCvSharp/Modules/superres/SuperResolution.cssrc/OpenCvSharp/Modules/text/ClassifierType.cssrc/OpenCvSharp/Modules/text/Cv2.Text.cssrc/OpenCvSharp/Modules/text/DecoderMode.cssrc/OpenCvSharp/Modules/text/ERFilter.cssrc/OpenCvSharp/Modules/text/ERFilterCallback.cssrc/OpenCvSharp/Modules/text/ERFilterNMMode.cssrc/OpenCvSharp/Modules/text/ErGroupingModes.cssrc/OpenCvSharp/Modules/text/OCRBeamSearchDecoder.cssrc/OpenCvSharp/Modules/text/OCRBeamSearchDecoderClassifierCallback.cssrc/OpenCvSharp/Modules/text/OCRHMMDecoder.cssrc/OpenCvSharp/Modules/text/OCRHMMDecoderClassifierCallback.cssrc/OpenCvSharp/Modules/text/OCRHolisticWordRecognizer.cssrc/OpenCvSharp/Modules/text/OCRTesseract.cssrc/OpenCvSharp/Modules/text/OcrEngineMode.cssrc/OpenCvSharp/Modules/text/PageSegMode.cssrc/OpenCvSharp/Modules/ximgproc/ContourFitting.cssrc/OpenCvSharp/Modules/ximgproc/Cv2.XImgProc.cssrc/OpenCvSharp/Modules/ximgproc/DisparityFilter.cssrc/OpenCvSharp/Modules/ximgproc/DisparityWLSFilter.cssrc/OpenCvSharp/Modules/ximgproc/EdgeAwareInterpolator.cssrc/OpenCvSharp/Modules/ximgproc/RICInterpolator.cssrc/OpenCvSharp/Modules/ximgproc/ScanSegment.cssrc/OpenCvSharp/Modules/ximgproc/SparseMatchInterpolator.cssrc/OpenCvSharpExtern/OpenCvSharpExtern.vcxprojsrc/OpenCvSharpExtern/OpenCvSharpExtern.vcxproj.filterssrc/OpenCvSharpExtern/photo.cppsrc/OpenCvSharpExtern/photo_ColorCorrectionModel.hsrc/OpenCvSharpExtern/saliency_MotionSaliencyBinWangApr2014.hsrc/OpenCvSharpExtern/saliency_ObjectnessBING.hsrc/OpenCvSharpExtern/saliency_StaticSaliencyFineGrained.hsrc/OpenCvSharpExtern/saliency_StaticSaliencySpectralResidual.hsrc/OpenCvSharpExtern/shape_ShapeDistanceExtractor.hsrc/OpenCvSharpExtern/shape_ShapeTransformer.hsrc/OpenCvSharpExtern/stereo_StereoMatcher.hsrc/OpenCvSharpExtern/superres.hsrc/OpenCvSharpExtern/text.cppsrc/OpenCvSharpExtern/text_ERFilter.hsrc/OpenCvSharpExtern/text_OCRBeamSearchDecoder.hsrc/OpenCvSharpExtern/text_OCRHMMDecoder.hsrc/OpenCvSharpExtern/text_OCRHolisticWordRecognizer.hsrc/OpenCvSharpExtern/ximgproc.cppsrc/OpenCvSharpExtern/ximgproc.hsrc/OpenCvSharpExtern/ximgproc_ContourFitting.hsrc/OpenCvSharpExtern/ximgproc_DisparityFilter.hsrc/OpenCvSharpExtern/ximgproc_ScanSegment.hsrc/OpenCvSharpExtern/ximgproc_SparseMatchInterpolator.htest/OpenCvSharp.Tests/photo/ColorCorrectionModelTest.cstest/OpenCvSharp.Tests/saliency/MotionSaliencyTest.cstest/OpenCvSharp.Tests/saliency/ObjectnessBINGTest.cstest/OpenCvSharp.Tests/saliency/StaticSaliencyTest.cstest/OpenCvSharp.Tests/shape/ShapeContextDistanceExtractorTest.cstest/OpenCvSharp.Tests/superres/SuperResTest.cstest/OpenCvSharp.Tests/text/ERFilterTest.cstest/OpenCvSharp.Tests/text/OCRDecoderTest.cstest/OpenCvSharp.Tests/ximgproc/XimgProcTest.cs
Comment on lines
+84
to
+152
| public override void Run( | ||
| Mat image, | ||
| out string outputText, | ||
| out Rect[] componentRects, | ||
| out string?[] componentTexts, | ||
| out float[] componentConfidences, | ||
| ComponentLevels componentLevel = ComponentLevels.Word) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(image); | ||
| image.ThrowIfDisposed(); | ||
|
|
||
| using var outputTextString = new StdString(); | ||
| using var componentRectsVector = new StdVector<Rect>(); | ||
| using var componentTextsVector = new VectorOfString(); | ||
| using var componentConfidencesVector = new StdVector<float>(); | ||
| NativeMethods.HandleException( | ||
| NativeMethods.text_OCRBeamSearchDecoder_run1( | ||
| Handle, | ||
| image.CvPtr, | ||
| outputTextString.CvPtr, | ||
| componentRectsVector.CvPtr, | ||
| componentTextsVector.CvPtr, | ||
| componentConfidencesVector.CvPtr, | ||
| (int)componentLevel)); | ||
| outputText = outputTextString.ToString(); | ||
| componentRects = componentRectsVector.ToArray(); | ||
| componentTexts = componentTextsVector.ToArray(); | ||
| componentConfidences = componentConfidencesVector.ToArray(); | ||
|
|
||
| GC.KeepAlive(image); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public override void Run( | ||
| Mat image, | ||
| Mat mask, | ||
| out string outputText, | ||
| out Rect[] componentRects, | ||
| out string?[] componentTexts, | ||
| out float[] componentConfidences, | ||
| ComponentLevels componentLevel = ComponentLevels.Word) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(image); | ||
| ArgumentNullException.ThrowIfNull(mask); | ||
| image.ThrowIfDisposed(); | ||
| mask.ThrowIfDisposed(); | ||
|
|
||
| using var outputTextString = new StdString(); | ||
| using var componentRectsVector = new StdVector<Rect>(); | ||
| using var componentTextsVector = new VectorOfString(); | ||
| using var componentConfidencesVector = new StdVector<float>(); | ||
| NativeMethods.HandleException( | ||
| NativeMethods.text_OCRBeamSearchDecoder_run2( | ||
| Handle, | ||
| image.CvPtr, | ||
| mask.CvPtr, | ||
| outputTextString.CvPtr, | ||
| componentRectsVector.CvPtr, | ||
| componentTextsVector.CvPtr, | ||
| componentConfidencesVector.CvPtr, | ||
| (int)componentLevel)); | ||
| outputText = outputTextString.ToString(); | ||
| componentRects = componentRectsVector.ToArray(); | ||
| componentTexts = componentTextsVector.ToArray(); | ||
| componentConfidences = componentConfidencesVector.ToArray(); | ||
|
|
||
| GC.KeepAlive(image); | ||
| GC.KeepAlive(mask); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'src/OpenCvSharp/Modules/text/OCRBeamSearchDecoder.cs' 'src/OpenCvSharp/Modules/text/OCRHMMDecoder.cs' 'src/OpenCvSharp/Modules/**/OCR*.cs' 'src/OpenCvSharp/Modules/**/Algorithm*.cs' 'src/OpenCvSharp/**/*.cs' | sed -n '1,200p'
printf '\nClass outlines:\n'
ast-grep outline src/OpenCvSharp/Modules/text/OCRBeamSearchDecoder.cs --view expanded || true
printf '\n---\n'
ast-grep outline src/OpenCvSharp/Modules/text/OCRHMMDecoder.cs --view expanded || true
printf '\nNearby Run methods with ThrowIfDisposed:\n'
rg -n "ThrowIfDisposed\\(\\)" src/OpenCvSharp/Modules/text src/OpenCvSharp/Modules | sed -n '1,200p'Repository: shimat/opencvsharp
Length of output: 31548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'BaseOCR:\n'
sed -n '1,220p' src/OpenCvSharp/Modules/text/BaseOCR.cs
printf '\nOCRBeamSearchDecoder:\n'
sed -n '1,220p' src/OpenCvSharp/Modules/text/OCRBeamSearchDecoder.cs
printf '\nOCRHMMDecoder:\n'
sed -n '1,220p' src/OpenCvSharp/Modules/text/OCRHMMDecoder.cs
printf '\nOCRTesseract:\n'
sed -n '1,180p' src/OpenCvSharp/Modules/text/OCRTesseract.cs
printf '\nOCRHolisticWordRecognizer:\n'
sed -n '1,180p' src/OpenCvSharp/Modules/text/OCRHolisticWordRecognizer.csRepository: shimat/opencvsharp
Length of output: 29066
Add ThrowIfDisposed() to every OCR Run overload.
These wrappers validate Mat inputs but skip the receiver disposal check before native calls.
src/OpenCvSharp/Modules/text/OCRBeamSearchDecoder.cssrc/OpenCvSharp/Modules/text/OCRHMMDecoder.cssrc/OpenCvSharp/Modules/text/OCRTesseract.cssrc/OpenCvSharp/Modules/text/OCRHolisticWordRecognizer.cs
📍 Affects 2 files
src/OpenCvSharp/Modules/text/OCRBeamSearchDecoder.cs#L84-L152(this comment)src/OpenCvSharp/Modules/text/OCRHMMDecoder.cs#L83-L151
🤖 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/text/OCRBeamSearchDecoder.cs` around lines 84 - 152,
The OCR Run overloads do not validate the decoder receiver before invoking
native code. Add ThrowIfDisposed() on the receiver at the start of every Run
overload in OCRBeamSearchDecoder.cs (84-152) and OCRHMMDecoder.cs (83-151),
alongside the existing Mat validation; apply the same receiver check to every
Run overload in OCRTesseract.cs and OCRHolisticWordRecognizer.cs.
Source: Coding guidelines
Comment on lines
+92
to
+100
| public virtual void GetLabels(OutputArray labelsOut) | ||
| { | ||
| ThrowIfDisposed(); | ||
|
|
||
| NativeMethods.HandleException( | ||
| NativeMethods.ximgproc_ScanSegment_getLabels(Handle, labelsOut.Proxy)); | ||
|
|
||
| GC.KeepAlive(labelsOut.Source); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Missing validation and synchronization for OutputArray buffers. When C++ writes to or reallocates an OutputArray, the wrapper must be validated beforehand with ThrowIfNotReady() and its internal dimensions/pointers must be updated afterward with Fix(). Omitting Fix() causes the returned Mat in C# to appear empty or carry stale metadata. Based on learnings, InputArray/OutputArray-appropriate mechanisms should be used for validation.
src/OpenCvSharp/Modules/ximgproc/ScanSegment.cs#L92-L100: addlabelsOut.ThrowIfNotReady();before the native call andlabelsOut.Fix();after it.src/OpenCvSharp/Modules/ximgproc/ScanSegment.cs#L110-L118: addimage.ThrowIfNotReady();before the native call andimage.Fix();after it.
📍 Affects 1 file
src/OpenCvSharp/Modules/ximgproc/ScanSegment.cs#L92-L100(this comment)src/OpenCvSharp/Modules/ximgproc/ScanSegment.cs#L110-L118
🤖 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/ximgproc/ScanSegment.cs` around lines 92 - 100, The
ScanSegment OutputArray wrappers lack readiness validation before native writes
and metadata synchronization afterward. In ScanSegment.cs lines 92-100, update
GetLabels to call labelsOut.ThrowIfNotReady() before the native invocation and
labelsOut.Fix() after it; in lines 110-118, apply the same sequence to image
around its native call.
Source: Learnings
- add ThrowIfDisposed() to every OCRHMMDecoder/OCRBeamSearchDecoder/ OCRHolisticWordRecognizer Run overload before touching the native handle - add GC.KeepAlive(this) to DisparityWLSFilter's property accessors and GetConfidenceMap/GetROI, matching the belt-and-suspenders pattern already used elsewhere (Mat, SparseMat, SimpleBlobDetector, ImageCollection) - guard text_detectRegions_rects against a null filename before wrapping it in cv::String - strengthen the OCR/ER-filter missing-file tests to assert the specific OpenCVException the native side actually throws, instead of Exception - delete the temporary FileStorage files the saliency Read/Write regression tests write to the temp directory - materialize the Mat.Eye(...) MatExpr passed to TransformFD into a local so it isn't finalizer-collected - fix ximgproc header include ordering in the vcxproj Not applied: the suggestion to add ThrowIfNotReady()/Fix() calls in ScanSegment.GetLabels/GetLabelContourMask - neither method exists anywhere in this codebase, so this would not compile. Not applied: the suggestion to change ObjectnessBING.Base before Read and assert it reverts - ObjectnessBING never overrides Algorithm::read/write (confirmed against opencv_contrib source), so nothing is actually restored; asserting a round-trip here would be asserting something untrue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Pushed a follow-up commit addressing CodeRabbit's review:
Re: the failing macOS (arm64) job - unrelated to this PR. The xUnit run itself reports |
shimat
added a commit
that referenced
this pull request
Jul 16, 2026
- 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>
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
superres: wire upSuperResolution.OpticalFlow(the native bridge and P/Invoke declarations already existed; only the C# property body was missing)shape:ShapeContextDistanceExtractor.GetCostExtractor/GetTransformAlgorithm(setters already existed, getters didn't)ximgproc:findEllipses,RadonTransform,ScanSegment,ContourFitting(+fourierDescriptor/transformFD/contourSampling),DisparityFilter/DisparityWLSFilter,SparseMatchInterpolator/EdgeAwareInterpolator/RICInterpolatorStereoMatchergains a genericPtr_get, needed becausecreateRightMatcherreturns a base-typedPtr<StereoMatcher>andDisparityFilter.Filtertakes aStereoMatcher-shaped confidence pathphoto:ColorCorrectionModel(cv::ccm) plus its 7 enums (CcmType,ColorCheckerType,ColorSpace,LinearizationType,DistanceType,InitialMethodType), andgammaCorrectiontext: the ER-filter pipeline (ERFilter,ERFilterCallback,computeNMChannels,detectRegions) and the non-Tesseract OCR decoders (OCRHMMDecoder,OCRBeamSearchDecoder,OCRHolisticWordRecognizer)Two things were scoped out, both because the underlying C++ shape can't be marshaled safely, not because they were skipped for convenience:
ERStatand theERStat-vector overloads oferGrouping/MSERsToERStats:ERStatis a component-tree node with raw internalparent/child/next/prevpointers, so it isn't safely representable as a marshaled vector element.detectRegions(implemented here) covers the same end-to-end grouping use case usingPoint-based regions instead, so nothing is lost in practice.ERFilter::Callback/OCRHMMDecoder::ClassifierCallback/OCRBeamSearchDecoder::ClassifierCallbackin managed code: this would require a native-to-managed virtual dispatch trampoline, which this repo has no existing precedent for.ERFilter/OCRHMMDecoder/OCRBeamSearchDecoderare still fully constructible via the file-based classifier overloads and via the variousload*Classifier*factories (wrapped as opaque*ClassifierCallbackhandles).Bug found and fixed along the way
While adding test coverage for
StaticSaliencySpectralResidual.Write/Read, the test crashed the process (STATUS_STACK_BUFFER_OVERRUN). Root cause:cv::saliency::Saliencyinheritscv::Algorithmvirtually, so a raw pointer obtained as e.g.StaticSaliencySpectralResidual*is not the same address as thecv::Algorithm*the compiler would produce via the correct virtual-base adjustment. The genericcore_Algorithm_write/readbridge (typed ascv::Algorithm*) was silently given the wrong pointer value for everysaliencyclass that used it. Fixed by givingStaticSaliencySpectralResidual,StaticSaliencyFineGrained,MotionSaliencyBinWangApr2014, andObjectnessBINGtheir ownwrite/readbridges that callwrite()/read()on the pointer at its correct native concrete type.Testing
cmake --build src/build --config Release --target OpenCvSharpExterndotnet build src/OpenCvSharp/OpenCvSharp.csproj -c Releasedotnet test test/OpenCvSharp.Tests/OpenCvSharp.Tests.csproj -c Release(full suite: 1552 total, 1526 passed, 26 skipped, 0 failed — no regressions)ColorCorrectionModelTest,ERFilterTest,OCRDecoderTest, plus additions toXImgProcTest,ShapeContextDistanceExtractorTest,SuperResTest, and the saliencyRead/Writeregression tests for the crash above)Closes #2015
Summary by CodeRabbit
ColorCorrectionModel, plusCv2.GammaCorrection.ERFilter+ callbacks, ER/NM modes, HMM/beam-search/holistic-word decoders, and strongly typed OCR engine/page segmentation options.