Skip to content

Add assert to ensure using non-overlapping memory regions #3983

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: 4.x
Choose a base branch
from

Conversation

mitresthen
Copy link

@mitresthen mitresthen commented Jul 31, 2025

This pr addresses this issue: opencv/opencv#27429
where the user did not realize that the warpaffine function requires non-overlapping src and dst memory regions.
The code now compares the input memory regions and asserts that they do not overlap. There is also a test for this functionality.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@cudawarped
Copy link
Contributor

cudawarped commented Aug 1, 2025

@asmorkalov Should this be a documentation issue? e.g. This function does not support in place operations.

I've been looking through to find a convention for dealing with this. I've found functions which

  1. explicitly document they support in place
  2. explicitly document they don't support in place but don't assert on it
  3. don't document in place operations
  4. clone the source if the source and destination memory locations are equal. e.g. cv::warpPerspective
  5. use a buffer if the source and destination memory locations are equal. e.g cv::cuda::SeparableLinearFilter::apply

But I haven't found the consistent approach that the original issue implies

If it does not support inplace operations an error should appear similar to what happens with other functions.

Would it be better in this PR to document that the function does not support in place operations with a simple assert

CV_Assert( src.data == dst.data );

@mitresthen
Copy link
Author

I think the suggestion to document that this function is not meant to be used inplace and combining it with an assertion is the best way forward.
That way it will be very clear how the function should be used, even those who don't read the function docs will be made aware of the correct usage. The assert also serves as a contract so the function can assume src.data != dst.data.
Using source clone or buffering (options 4 and 5) adds unnecessary overhead and hidden complexity to the function. For a low-level library like OpenCV which is often used in realtime applications, this is probably not the best approach.

@cudawarped
Copy link
Contributor

For a low-level library like OpenCV which is often used in realtime applications, this is probably not the best approach.

I agree but I would take it one step futher. I don't think we should be checking to see if the source and destination are overlapping just that they are not the same for the reason you mentioned

even those who don't read the function docs will be made aware of the correct usage.

If a user has a source and destination which are not equal but are overlapping then they likely have far bigger issues with their program and an assert on overlap is not going to help them out.

@mitresthen
Copy link
Author

Fair point! I've simplified it to be an equality check now, and removed all the overlap logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants