-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Add FastCV DSP Initialization, QcAllocator and FastCV DSP Extension APIs #3931
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
base: 4.x
Are you sure you want to change the base?
Conversation
* @param apertureSize The Sobel kernel size for calculating gradient. Supported sizes are 3, 5 and 7. | ||
* @param L2gradient L2 Gradient or L1 Gradient | ||
*/ | ||
CV_EXPORTS_W void canny(InputArray _src, OutputArray _dst, int lowThreshold, int highThreshold, int apertureSize = 3, bool L2gradient = false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canny is name. Please use capital letter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will update
if(data0) | ||
u->flags |= cv::UMatData::USER_ALLOCATED; | ||
|
||
u->userdata = new std::string("QCOM"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution leads to memory leak. Something like u->userdata = "QCOM";
should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will compare Mat::allocator field with DSP allocator address, I will remove this
if (u->userdata) | ||
{ | ||
delete static_cast<std::string*>(u->userdata); | ||
u->userdata = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String is not freed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will compare Mat::allocator field with DSP allocator address, I will remove this as it won't be needed
CV_Assert(CV_MAT_DEPTH(kernel.type()) == CV_8S); | ||
parallel_for_(Range(0, src.rows), FcvFilter2DLoop_Invoker(src, dst, kernel), nStripes); | ||
break; | ||
} | ||
case CV_32F: | ||
{ | ||
CV_Assert(CV_MAT_DEPTH(kernel.type()) == CV_32F); | ||
parallel_for_(Range(0, src.rows), FcvFilter2DLoop_Invoker(src, dst, kernel), nStripes); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DSP is a shared resource. Are you sure that parallel_for_ makes sense here? Also I would say that we need parallel scheme according to amount of DSP cores/units rather than CPU cores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the parallel_for_ for now, we have parallelization within DSP API.
#define IS_FASTCV_ALLOCATED(mat) \ | ||
((mat.u && mat.u->userdata && \ | ||
*static_cast<std::string*>(mat.u->userdata) == "QCOM") ? true : \ | ||
(CV_Error(cv::Error::StsBadArg, cv::format("Matrix '%s' not allocated with FastCV allocator. " \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just compare Mat::allocator field with DSP allocator address. User data is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, thanks for the input
// Check DSP initialization status and initialize if needed | ||
FASTCV_CHECK_DSP_INIT(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say, that it should be at the very beginning of the function. If IS_FASTCV_ALLOCATED returned true, then DSP is already initialized, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case the user fails to allocate the buffer using QcAllocator then we do not need to Check or Initialize the DSP. IS_FASTCV_ALLOCATED does not mean that the DSP is initialized. DSP will be initialized only when fcvdspinit is called or during FASTCV_CHECK_DSP_INIT();
Java bindings generator does not handle namespaces well:
If I understand correctly, DSP part is not usable from Java code (no way to manage allocators in Java). I propose to replace |
sure @asmorkalov , will change CV_EXPORTS_W with CV_EXPORTS |
Detailed Description
This PR introduces FastCV DSP Extension APIs within the 'cv::fastcv::dsp' namespace.
The following APIs have been added:
The QcAllocator has been added to manage memory allocations on Qualcomm's Chipsets. This allocator ensures that matrices are allocated using the Qualcomm hardware memory allocator, providing efficient DSP operations.
Requires binary from opencv/opencv_3rdparty#95
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.