Polish ConnectedComponents wrappers (doc fix + eager-copy / round-trip efficiency) - #1999
Merged
Merged
Conversation
Fixes issue #1985: - Fix LabelCount XML doc (it is the full label count including background, not "count - 1"). - Document that Blobs[0] is the background blob. - ConnectedComponentsEx now keeps the native label Mat and defers materializing the managed Labels array until it is actually accessed, instead of always eagerly copying it. - FilterByLabels/GetLabelMask compare directly against the native label Mat, removing the managed round-trip through int[,] and Mat.FromPixelData. - RenderBlobs iterates via Mat.AsRows instead of the generic indexer, avoiding per-pixel indexer overhead. - Add an optional ltype parameter to ConnectedComponentsEx so callers can opt into CV_16U labels to halve the native label image's memory footprint. ConnectedComponents now implements IDisposable to release the retained native label image. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughConnectedComponentsEx now accepts an optional label output type, including CV_16U, and converts labels through a widening helper. ConnectedComponents updates label-based masking and blob rendering to use the existing label buffer more directly. Tests add CV_16U coverage. ChangesConnectedComponents label type and buffer handling
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Cv2
participant OpenCvNative
participant Helper
Caller->>Cv2: ConnectedComponentsEx(image, connectivity, ccltype, ltype)
Cv2->>Cv2: validate label type
Cv2->>OpenCvNative: ConnectedComponentsWithStatsWithAlgorithm(...)
OpenCvNative-->>Cv2: labelsMat, stats, centroids
Cv2->>Helper: ToInt32RectangularArray(labelsMat, labelType)
Helper-->>Cv2: int[,]
Cv2-->>Caller: ConnectedComponents
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)
test/OpenCvSharp.Tests/imgproc/ConnectedComponentsTest.cs (1)
31-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: missing buffer non-emptiness check.
Unlike the
Runtest (Line 22), this test doesn't assertcc.Labels.GetBuffer()is non-empty for the CV_16U path. Not a blocker, but keeping parity would strengthen coverage for the new lazy-materialization path.🤖 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/imgproc/ConnectedComponentsTest.cs` around lines 31 - 49, Add the missing non-empty buffer assertion in the ConnectedComponentsEx CV_16U test to match the existing Run test coverage. In RunWithCv16ULabelType, after verifying cc.Labels dimensions, also assert that cc.Labels.GetBuffer() is not empty so the lazy-materialization path is covered consistently. Keep the change focused on the ConnectedComponentsTest and the cc.Labels access in the new test.
🤖 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/imgproc/ConnectedComponent.cs`:
- Line 24: Add a disposal guard to the native-backed accessors in
ConnectedComponent so they fail fast with ObjectDisposedException instead of
reading freed Mat memory. Update Labels and the other entry points mentioned in
this class, including RenderBlobs, GetLabelMask, and the FilterBy* methods, to
call a shared ThrowIfDisposed-style check before touching labelsMat or any
native-backed state. Use the ConnectedComponent members to locate the affected
paths and ensure the guard runs regardless of whether labelsCache has already
been populated.
---
Nitpick comments:
In `@test/OpenCvSharp.Tests/imgproc/ConnectedComponentsTest.cs`:
- Around line 31-49: Add the missing non-empty buffer assertion in the
ConnectedComponentsEx CV_16U test to match the existing Run test coverage. In
RunWithCv16ULabelType, after verifying cc.Labels dimensions, also assert that
cc.Labels.GetBuffer() is not empty so the lazy-materialization path is covered
consistently. Keep the change focused on the ConnectedComponentsTest and the
cc.Labels access in the new test.
🪄 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: a68fc6ab-bb0c-4130-8e83-4ab9e567cc33
📒 Files selected for processing (3)
src/OpenCvSharp/Cv2/Cv2_imgproc.cssrc/OpenCvSharp/Modules/imgproc/ConnectedComponent.cstest/OpenCvSharp.Tests/imgproc/ConnectedComponentsTest.cs
Retaining the native label Mat for lazy materialization forced ConnectedComponents to become IDisposable, which regressed the convenience API's ergonomics (every caller now had to remember to dispose). Revert to eager conversion of the label image to a plain managed int[,] in ConnectedComponentsEx, as before. FilterByLabels/GetLabelMask still avoid the previous full-size scalar-filled comparison Mat: they wrap the managed Labels buffer via the existing zero-copy Mat.FromPixelData and compare directly against a Scalar. RenderBlobs still avoids the generic indexer, reading straight from the int[,] buffer and writing via Mat.AsRows. The CV_16U ltype option is preserved, widening to int[,] once during construction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/OpenCvSharp/Cv2/Cv2_imgproc.cs (1)
2239-2241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the invalid-
ltypevalidation path.The new
ArgumentExceptionguard forltype(must beCV_32S/CV_16U) has no corresponding test verifying it throws for an unsupported type (e.g.CV_8U).🤖 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/Cv2/Cv2_imgproc.cs` around lines 2239 - 2241, Add test coverage for the new invalid ltype validation in Cv2_imgproc.Cv2 connected to the labelType guard: add a test that calls the relevant imgproc API with an unsupported ltype such as CV_8U and asserts an ArgumentException is thrown with the ltype parameter name. Place the test alongside the existing Cv2 imgproc tests that cover label-type behavior so the new CV_32S/CV_16U check is verified for failure cases.
🤖 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.
Nitpick comments:
In `@src/OpenCvSharp/Cv2/Cv2_imgproc.cs`:
- Around line 2239-2241: Add test coverage for the new invalid ltype validation
in Cv2_imgproc.Cv2 connected to the labelType guard: add a test that calls the
relevant imgproc API with an unsupported ltype such as CV_8U and asserts an
ArgumentException is thrown with the ltype parameter name. Place the test
alongside the existing Cv2 imgproc tests that cover label-type behavior so the
new CV_32S/CV_16U check is verified for failure cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27e04dda-9b83-4eb9-8467-b9a286eba202
📒 Files selected for processing (3)
src/OpenCvSharp/Cv2/Cv2_imgproc.cssrc/OpenCvSharp/Modules/imgproc/ConnectedComponent.cstest/OpenCvSharp.Tests/imgproc/ConnectedComponentsTest.cs
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
Addresses the polish items from #1985 for the
ConnectedComponentsfamily wrappers.LabelCountXML doc: it is the full label count including the background label (N), not "the number of labels - 1".FilterByLabel(s)/GetLabelMasknow wrap the managedLabelsbuffer via the existing zero-copyMat.FromPixelDataand compare directly against aScalar(Cv2.Compare(labelsView, new Scalar(label), ...)), removing the extra full-size scalar-filled comparisonMatthe old code allocated.RenderBlobsnow reads straight from the plainint[,]buffer and writes viaMat.AsRows<T>(zero-P/Invoke-per-element row access) instead of going through the genericMat<Vec3b>indexer, avoiding per-pixel indexer overhead.Blobs[0]is the background blob.ltypeparameter toConnectedComponentsExso callers can opt intoCV_16Ulabels (halves the native label image's memory footprint during the native call, at the cost of a 65535 label limit), matching what the native API already supports. The result is still always widened to a plainint[,]forLabels.ConnectedComponentsintentionally stays a plain (non-IDisposable) type: an earlier version of this PR made it retain the native labelMatfor lazy materialization, but that forcedIDisposableonto every caller for a convenience API that previously never needed disposal.Labelsis still eagerly materialized to a managedint[,]inConnectedComponentsEx, same as before.Closes #1985.
Test plan
dotnet buildforOpenCvSharpandOpenCvSharp.Tests— no warnings/errors.dotnet test --filter FullyQualifiedName~ConnectedComponents— all pass, including a new test covering theCV_16Ultypeoption.dotnet test --filter FullyQualifiedName~ImgProc— full ImgProc suite (243 tests) passes.🤖 Generated with Claude Code
Summary by CodeRabbit
ConnectedComponentsEx.RunWithCv16ULabelType) and validating label count and dimensions.