Fix MergeDebevec/MergeMertens segfault caused by C++/C# calling convention mismatch - #1878
Merged
Merged
Conversation
…s pattern Same calling-convention mismatch as #1876: photo_Ptr_MergeDebevec_get/delete and photo_Ptr_MergeMertens_get/delete were left in the old direct-return style when #1846 updated the C# P/Invoke declarations to ExceptionStatus + out-param. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
These classes had no test coverage, which is why the calling-convention mismatch introduced in #1846 went undetected until now. Co-Authored-By: Claude Sonnet 4.6 <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 (2)
📝 WalkthroughWalkthroughC API wrapper functions for MergeDebevec and MergeMertens are standardized to return ExceptionStatus with out-parameters, replacing direct pointer/void returns and wrapping operations in exception-handling macros. Comprehensive unit tests for both merge algorithms verify successful creation, exercise Process overloads with test data, and validate output properties. ChangesPhoto HDR API Exception Safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughC wrappers for MergeDebevec/MergeMertens and their process functions now return ChangesPhoto HDR API Exception Safety
Sequence Diagram(s): sequenceDiagram
participant MergeExposuresProc as MergeExposures.Process (managed)
participant MergeMertensProc as MergeMertens.Process (managed)
participant NativeMethods as NativeMethods (P/Invoke)
participant ExternWrappers as photo_HDR (C wrappers)
participant OpenCV as cv::Merge* (C++)
MergeExposuresProc->>NativeMethods: HandleException(photo_MergeExposures_process(...))
MergeMertensProc->>NativeMethods: HandleException(photo_MergeMertens_process(...))
NativeMethods->>ExternWrappers: call exported function returning ExceptionStatus
ExternWrappers->>OpenCV: obj->process(...) / create/get/delete (BEGIN_WRAP/END_WRAP)
ExternWrappers-->>NativeMethods: ExceptionStatus (ok or error)
NativeMethods-->>MergeExposuresProc: translate via HandleException (throw on error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…y ExceptionStatus pattern - photo_createMergeDebevec/MergeMertens were still returning pointer directly while C# side expected ExceptionStatus + out param, causing null ptr crash - photo_MergeExposures_process and photo_MergeMertens_process: void→ExceptionStatus - Wrap all call sites with NativeMethods.HandleException Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Callers dispose the returned Mats in finally blocks; the warning is a false positive. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OpenCvSharp/Modules/photo/MergeMertens.cs (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSave this file with a UTF-8 BOM.
This file appears to be committed without the BOM required for
.cssources in this repo.As per coding guidelines,
**/*.{cs,csproj,yml,md,json}: All source files must be saved as UTF-8 with BOM (EF BB BF)🤖 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/photo/MergeMertens.cs` at line 1, The file MergeMertens.cs (contains using OpenCvSharp.Internal and the MergeMertens class) must be saved with a UTF-8 BOM; reopen the file in your editor or commit tool and resave/encode it as UTF-8 with BOM (EF BB BF) so it conforms to the repository's encoding rule for .cs files, then recommit the updated file.
🤖 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/photo/MergeExposuresTest.cs`:
- Line 1: The file MergeExposuresTest.cs is missing the required UTF-8 BOM;
reopen the file and re-save it with UTF-8 with BOM (EF BB BF) encoding so it
complies with the repo guideline for C# sources (ensure your editor or git
config writes the BOM), then commit the updated file (referenced symbol: the
test file MergeExposuresTest.cs).
---
Outside diff comments:
In `@src/OpenCvSharp/Modules/photo/MergeMertens.cs`:
- Line 1: The file MergeMertens.cs (contains using OpenCvSharp.Internal and the
MergeMertens class) must be saved with a UTF-8 BOM; reopen the file in your
editor or commit tool and resave/encode it as UTF-8 with BOM (EF BB BF) so it
conforms to the repository's encoding rule for .cs files, then recommit the
updated file.
🪄 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: d4ebaa05-54b7-4b8f-a47f-870951f44442
📒 Files selected for processing (5)
src/OpenCvSharp/Internal/PInvoke/NativeMethods/photo/NativeMethods_photo_HDR.cssrc/OpenCvSharp/Modules/photo/MergeExposures.cssrc/OpenCvSharp/Modules/photo/MergeMertens.cssrc/OpenCvSharpExtern/photo_HDR.htest/OpenCvSharp.Tests/photo/MergeExposuresTest.cs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
This was referenced Jun 22, 2026
This was referenced Jul 6, 2026
This was referenced Jul 25, 2026
This was referenced Aug 10, 2026
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
Same class of bug as #1876.
MergeDebevecandMergeMertensare affected by the same calling-convention mismatch introduced in #1846: the C# P/Invoke declarations forphoto_Ptr_MergeDebevec_get/deleteandphoto_Ptr_MergeMertens_get/deletewere updated to the new
ExceptionStatus+ out-parameter convention, but the C++ implementations inphoto_HDR.hwere left in the old direct-return style.This causes
RawPtrto be populated with garbage/null on construction, leading to a segfault when any method is called on these objects.Fix
Update the four C++ functions to the convention already used by the surrounding code in the same file (
CalibrateDebevec,CalibrateRobertson, etc.): returnExceptionStatus, wrap withBEGIN_WRAP/END_WRAP, and pass the result via an output pointer parameter.Summary by CodeRabbit