Skip to content

Conversation

incubus-ank
Copy link
Contributor

I found missing memory cudaFree in cv::cuda::FAST_Impl::detectAsync()

I created an issue, but did not receive a response.
#3994

Pull Request Readiness Checklist

  • 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

@incubus-ank Would it be more performant to move the allocation into the constructor and free into the destructor of FAST_Impl to save the allocation on every invocatoin of FAST_Impl::detectAsync?

@incubus-ank
Copy link
Contributor Author

@incubus-ank Would it be more performant to move the allocation into the constructor and free into the destructor of FAST_Impl to save the allocation on every invocatoin of FAST_Impl::detectAsync?

Yes, I think it's a good idea.

I'm not a pro at working with CUDA. Should I define/remove copy and move constructors to avoid funny situations and create a better interface?

@cudawarped
Copy link
Contributor

I would say no because this class can only be created with the factory method cv::cuda::FastFeatureDetector::create and accessed through the resulting smart pointer, @asmorkalov keep me honest here?

Ideally I would suggest using a GpuMat member variable for d_counter which would handle the memory management for you but that would involve altering the type from unsigned int to int and I not sure if this has any implications.

Copy link
Contributor

@cudawarped cudawarped left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@asmorkalov asmorkalov merged commit b01e6a7 into opencv:4.x Sep 12, 2025
12 of 13 checks passed
@asmorkalov asmorkalov self-assigned this Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants