Drop params from Mat/UMat ROI constructors; fix related silent-empty-array bugs - #2063
Merged
Conversation
…array bugs new Mat(m, params Range[] ranges) let a plain new Mat(m) - meant as a copy/view - compile fine and bind to the ROI constructor with an empty ranges array, throwing "empty ranges" at runtime instead of failing to compile (#2062). Drop params from the Mat/UMat/Mat<TElem> ROI constructors so callers must pass the array explicitly. Fixing the build surfaced a live instance of exactly this bug: MatMemoryManager's new Mat(mat) (isDataOwner: false path) was silently resolving to the ROI constructor because the intended copy constructor, Mat(Mat m), was protected and inaccessible from there. Changed it to internal so the correct constructor binds. Auditing the rest of the params-array surface for the same shape found further cases where an empty array compiles but is either invalid or unsafe: - Mat.SubMat()/UMat.SubMat() with zero ranges hit undefined behavior in the native glue (&rangesVec[0] on an empty vector). Now guarded. - Mat.Ptr() with too few indices let cv::Mat::ptr(const int*) read Dims many ints from a shorter native buffer - an out-of-bounds read. Guarded, but only in DEBUG: Dims is itself a P/Invoke call, and Ptr sits on the per-pixel-access hot path, so the check (and its test) only exist in DEBUG builds, matching cv::Mat::ptr's own CV_DbgAssert-only checking. - SparseMat.Create had a pre-existing typo (sizes.Length == 1 instead of == 0) that rejected valid 1-D sparse mats while letting truly empty sizes through; fixed, and the same guard was added to the SparseMat<T>(params int[] dimensions) constructor. 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMatrix ROI, reshape, pointer, and encoding APIs now require explicit arrays or typed parameters, with added argument validation. File path and SparseMat validation are updated, and tests cover these changes plus MatMemoryManager ownership behavior. ChangesMatrix API contracts and validation
SparseMat dimension validation
Encoding and file-path signatures
MatMemoryManager coverage
Estimated code review effort: 3 (Moderate) | ~30 minutes 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.
🧹 Nitpick comments (1)
src/OpenCvSharp/Modules/core/SparseMat.cs (1)
215-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProvide the parameter name to
ArgumentException.Consider adding
nameof(sizes)to theArgumentExceptionto match the behavior inValidateDimensionsbelow and to improve error clarity.♻️ Proposed refactor
- if (sizes.Length == 0) - throw new ArgumentException("sizes is empty"); + if (sizes.Length == 0) + throw new ArgumentException("sizes is empty", nameof(sizes));🤖 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/core/SparseMat.cs` around lines 215 - 216, Update the ArgumentException thrown by the sizes.Length validation in SparseMat to include nameof(sizes) as its parameter name, matching the existing ValidateDimensions convention while preserving the current message and validation 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.
Nitpick comments:
In `@src/OpenCvSharp/Modules/core/SparseMat.cs`:
- Around line 215-216: Update the ArgumentException thrown by the sizes.Length
validation in SparseMat to include nameof(sizes) as its parameter name, matching
the existing ValidateDimensions convention while preserving the current message
and validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3754dce-0fa3-4929-bc74-e4832c930702
📒 Files selected for processing (8)
src/OpenCvSharp/Modules/core/Mat/Mat.cssrc/OpenCvSharp/Modules/core/Mat/MatOfT.cssrc/OpenCvSharp/Modules/core/Mat/UMat.cssrc/OpenCvSharp/Modules/core/SparseMat.cstest/OpenCvSharp.Tests/core/MatMemoryManagerTest.cstest/OpenCvSharp.Tests/core/MatTest.cstest/OpenCvSharp.Tests/core/SparseMatTest.cstest/OpenCvSharp.Tests/core/UMatTest.cs
Same rationale as the ROI constructors: mat.SubMat() with zero ranges should fail to compile, not throw (or previously, hit undefined behavior in the native glue). This is strictly better than the runtime guard added earlier - no runtime cost at all, in any configuration - and needs no companion analyzer rule. The two/three-argument Range call sites are unaffected: Mat/UMat also expose a dedicated SubMat(System.Range, System.Range) overload that mat.SubMat(0..2, 1..4)-style calls already resolved to, independent of the params array overload. Only the genuinely N-ary (3+) explicit-array call and the empty-array defense-in-depth tests needed updating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…/ToBytes onto ImageEncodingParam[]
Continues the same-shaped cleanup from the previous commits, applied to
overloads that were reviewed and found safe to leave alone at the time,
but turned out to have no real ergonomic reason to keep params either:
- FileNode/FileStorage.GetPath(params object[] path): already guarded
path.Length == 0 at runtime with a clear message - the same "should
have been a compile error" situation as SubMat. No competing overload
exists at the same arity, so there is no ambiguity risk either.
- Mat/UMat.Reshape(int cn, params int[] newDims): dropping params here
is a readability call, not a safety one. A run of bare ints after cn
reads as an undifferentiated list ("what do these numbers mean?"),
whereas newDims: [5, 10, 15] visually groups them as one shape value.
Reshape(int cn, int rows = 0) is untouched and still covers the
common 0/1-argument case in normal form.
- Mat.SetArray<T>(params T[] data): no bug either, but the only two
call sites that relied on the spread form were trivial to convert to
an explicit array, so there was no reason to keep the params surface.
Cv2.ImWrite/ImEncode/Mat.ToBytes previously carried two overloads each:
a raw int[]-pairs form (kept as an escape hatch for encoding flags not
covered by the ImageEncodingParam/ImwriteFlags enum) and a
params ImageEncodingParam[] form. Since ImwriteFlags is a plain enum,
any flag value - including ones without a named member - can already
be expressed as (ImwriteFlags)rawValue, so the raw int[] form was not
actually buying any coverage the typed form couldn't already reach.
Removed the raw overloads and merged everything onto a single
ImageEncodingParam[]? prms = null parameter per method (ToMemoryStream/
WriteToStream already had no int[] sibling and gain the same default).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/OpenCvSharp/Cv2/Cv2_imgcodecs.cs (1)
285-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer BCL argument-validation helpers for string checks.
The manual
string.IsNullOrEmptychecks violate the coding guidelines, which require using BCL argument-validation helpers instead of manual checks. You can use.NET 7+ArgumentException.ThrowIfNullOrEmptyto simplify these guards natively.
src/OpenCvSharp/Cv2/Cv2_imgcodecs.cs#L285-L286: Replace the manualstring.IsNullOrEmptycheck withArgumentException.ThrowIfNullOrEmpty(fileName);.src/OpenCvSharp/Cv2/Cv2_imgcodecs.cs#L306-L307: Replace the manualstring.IsNullOrEmptycheck withArgumentException.ThrowIfNullOrEmpty(fileName);.src/OpenCvSharp/Cv2/Cv2_imgcodecs.cs#L450-L451: Replace the manualstring.IsNullOrEmptycheck withArgumentException.ThrowIfNullOrEmpty(ext);.♻️ Proposed refactor
- if (string.IsNullOrEmpty(fileName)) - throw new ArgumentNullException(nameof(fileName)); + ArgumentException.ThrowIfNullOrEmpty(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 `@src/OpenCvSharp/Cv2/Cv2_imgcodecs.cs` around lines 285 - 286, Replace the manual string.IsNullOrEmpty guards with ArgumentException.ThrowIfNullOrEmpty in Cv2_imgcodecs.cs at lines 285-286 and 306-307 for fileName, and lines 450-451 for ext. Preserve the existing argument-validation behavior while using the BCL helper.Source: Coding guidelines
🤖 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_imgcodecs.cs`:
- Around line 285-286: Replace the manual string.IsNullOrEmpty guards with
ArgumentException.ThrowIfNullOrEmpty in Cv2_imgcodecs.cs at lines 285-286 and
306-307 for fileName, and lines 450-451 for ext. Preserve the existing
argument-validation behavior while using the BCL helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: acf82797-23f0-4e40-98e8-a94ee26d349e
📒 Files selected for processing (10)
src/OpenCvSharp/Cv2/Cv2_imgcodecs.cssrc/OpenCvSharp/Modules/core/FileNode.cssrc/OpenCvSharp/Modules/core/FileStorage.cssrc/OpenCvSharp/Modules/core/Mat/Mat.cssrc/OpenCvSharp/Modules/core/Mat/MatOfT.cssrc/OpenCvSharp/Modules/core/Mat/UMat.cstest/OpenCvSharp.Tests/core/FileStorageTest.cstest/OpenCvSharp.Tests/core/MatExprBehaviorTest.cstest/OpenCvSharp.Tests/core/MatTest.cstest/OpenCvSharp.Tests/imgcodecs/ImgCodecsTest.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/OpenCvSharp/Modules/core/Mat/MatOfT.cs
- test/OpenCvSharp.Tests/core/MatTest.cs
Matches the ValidateDimensions convention added alongside it. 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
Mat(Mat m, params Range[] ranges)(and the same shape onUMat/Mat<TElem>) let a plainnew Mat(m)- meant as a copy/view - compile fine, silently binding to the ROI constructor with an emptyrangesarray and throwingArgumentException: empty rangesat runtime instead of failing to compile. Droppedparamsfrom the Mat/UMat/Mat<TElem>ROI constructors so callers must pass the array explicitly (e.g.new Mat(m, [r0, r1])).MatMemoryManager'snew Mat(mat)(theisDataOwner: falsepath) was silently resolving to the ROI constructor, because the constructor it actually meant to call,Mat(Mat m), wasprotectedand inaccessible from there. Changed it tointernalso the correct constructor binds.params-array surface for the same shape (an empty array compiles fine but is invalid, unsafe, or just hard to read) and made the following changes:Mat.SubMat()/UMat.SubMat()with zero ranges hit undefined behavior in the native glue code (&rangesVec[0]on an emptystd::vector). Droppedparamshere too, so this is a compile error rather than a runtime guard. The two-argumentRangecall sites (e.g.mat.SubMat(0..2, 1..4)) are unaffected, since those resolve to the separateSubMat(System.Range, System.Range)overload.Mat.Ptr()with too few indices letcv::Mat::ptr(const int*)readDimsmany ints from a shorter native buffer - an out-of-bounds read. This one can't dropparamsthe same way (the N-DPtr(int[] idx)overload has legitimate 1/2/3/4+-argument uses that only work viaparams), so it's guarded instead, butDEBUG-only:Dimsis itself a P/Invoke call, andPtrsits on the per-pixel-access hot path, so this check (and its test) exist only inDEBUGbuilds, matchingcv::Mat::ptr's ownCV_DbgAssert-only bounds checking and keeping the Release fast path untouched.SparseMat.Createhad a pre-existing typo (sizes.Length == 1instead of== 0) that rejected valid 1-D sparse mats while letting truly emptysizesthrough. Fixed, and the same guard was added toSparseMat<T>(params int[] dimensions).FileNode/FileStorage.GetPath(params object[] path)already guardedpath.Length == 0at runtime with a clear message - the same "should have been a compile error" situation asSubMat. Droppedparamshere too.Mat/UMat.Reshape(int cn, params int[] newDims): this one is a readability call rather than a safety one - a run of bare ints aftercnreads as an undifferentiated list, whereasnewDims: [5, 10, 15]visually groups them as one shape value.Reshape(int cn, int rows = 0)is untouched and still covers the common 0/1-argument case.Mat.SetArray<T>(params T[] data): no bug either, but the only two call sites relying on the spread form were trivial to convert to an explicit array, so there was no reason to keep theparamssurface.Cv2.ImWrite/ImEncode/Mat.ToBytespreviously carried two overloads each: a rawint[]-pairs form (originally kept as an escape hatch for encoding flags not covered byImageEncodingParam/ImwriteFlags) and aparams ImageEncodingParam[]form. SinceImwriteFlagsis a plain enum, any flag value - including ones without a named member - can already be expressed as(ImwriteFlags)rawValue, so the rawint[]form wasn't buying any coverage the typed form couldn't already reach. Removed the raw overloads and merged everything onto a singleImageEncodingParam[]? prms = nullparameter per method (ToMemoryStream/WriteToStreamalready had noint[]sibling and gain the same default).Window.ShowImages,MatShape, andImWrite/ImEncode/ToBytes's remainingImageEncodingParam[]convenience alone: each has real call sites that rely on the bare-argument spread form (Window.ShowImages(a, b, c)across 30+ call sites;new MatShape(2, 3, 4)as the primary construction idiom with an explicitly-documented 0-argument scalar-shape meaning), and none has a live safety issue motivating the change.Closes #2062
Testing
dotnet build OpenCvSharp.sln -c Debug/dotnet build test\OpenCvSharp.Tests\OpenCvSharp.Tests.csproj -c Release(the solution-wide Release build fails locally on the nativeOpenCvSharpExtern.vcxprojneeding full MSBuild/VS C++ tooling, unrelated to this change; the C# projects build fine standalone in both configurations)dotnet test test\OpenCvSharp.Tests\OpenCvSharp.Tests.csproj -c Debug --filter "FullyQualifiedName~OpenCvSharp.Tests.Core|FullyQualifiedName~OpenCvSharp.Tests.Imgcodecs"(503 passed, 3 skipped)dotnet test test\OpenCvSharp.Tests\OpenCvSharp.Tests.csproj -c Release --filter "FullyQualifiedName~OpenCvSharp.Tests.Core|FullyQualifiedName~OpenCvSharp.Tests.Imgcodecs"(502 passed, 3 skipped - the DEBUG-onlyPtrguard test is excluded from Release, matching CI's-c Releasetest runs)🤖 Generated with Claude Code
Summary by CodeRabbit