-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
FastCV Extension code for Opencv APIs #3811
base: 4.x
Are you sure you want to change the base?
Conversation
Change-Id: I721b810181dd9127221b6e2182d7aad5ac679b68
Added FastCV Extension code for Opencv APIs
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 rest is OK, waiting for other wrapped functions to be added.
The tests can be merged as-is, without fixing all my comments. But I still have to leave these comments so that they can be fixed in the future.
|
||
# here you can change the library to a specific one | ||
set(FASTCV_LIB "/libs/Android/lib64/libfastcvopt.so") | ||
|
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.
Extra line
|
||
This module provides wrappers for several FastCV functions not covered by the corresponding HAL in OpenCV. | ||
Please note that: | ||
1. This module supports ARM architecture only. This means that CMake script aborts configuration under x86 platform even if you don't want to build binaries for your machine and just want to build docs or enable code analysis in your IDE. In that case you should fix CMakeLists.txt file as told inside it. |
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.
This behavior was changed, please change this part of readme.
Now CMake will just emit a warning if the platform is not ARM but won't stop configuration. Any attempts to link under any platform but ARM would lead to a failure. The reason is to let various IDEs run their code analysis tools that require the code to be compiled at least.
* @param src2 Second source matrix of type CV_8S | ||
* @param dst Resulting matrix of type CV_32S | ||
*/ | ||
CV_EXPORTS_W void matmuls8s32(InputArray src1, _InputArray src2, OutputArray dst); |
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.
_InputArray src2
-> InputArray src2
|
||
/** | ||
* @brief Applies Bilateral filter to an image considering d-pixel diameter of each pixel's neighborhood. | ||
This filter does not work inplace. |
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.
This should be checked in the code with something like CV_Assert(src.data != dst.data);
* @brief Calculates all of the moments up to the third order of the image pixels' intensities | ||
The results are returned in the structure cv::Moments. | ||
* @param _src Input image with type CV_8UC1, CV_32SC1, CV_32FC1 | ||
* @param binary If 1, binary image (0x00-black, oxff-white); if 0, grayscale image |
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.
-> If true, assumes the image to be binary (0x00 for black, 0xff for white), otherwise assumes the image to be grayscale
cv::minMaxLoc(diffImage, nullptr, &maxVal); | ||
|
||
// Assert if the difference is acceptable (max difference should be less than 10) | ||
CV_Assert(maxVal < 10 && "Difference between images is too high!"); |
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 same about L_inf norm
cv::minMaxLoc(diffImage, nullptr, &maxVal); | ||
|
||
// Assert if the difference is acceptable (max difference should be less than 10) | ||
CV_Assert(maxVal < 10 && "Difference between images is too high!"); |
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 same about L_inf norm
cv::minMaxLoc(diffImage, nullptr, &maxVal); | ||
|
||
// Assert if the difference is acceptable (max difference should be less than 10) | ||
CV_Assert(maxVal < 10 && "Difference between images is too high!"); |
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 same about L_inf norm
cv::fastcv::resizeDownBy2(inputImage, resized_image); | ||
|
||
// Check if the output size is correct | ||
EXPECT_EQ(resized_image.size().width, size.width * 0.5); |
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.
These checks can be moved to the accuracy
test
cv::fastcv::resizeDownBy4(inputImage, resized_image); | ||
|
||
// Check if the output size is correct | ||
EXPECT_EQ(resized_image.size().width, size.width * 0.25); |
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.
These checks can be moved to the accuracy
test
cleanup and added make rules to download the fastcv bins from the 3rdparty repo Change-Id: I9a9db68a310263658ec93ea299ea37860d07c507
Fastcv ext changes for Opencv Acceleration
if((ARM OR AARCH64) AND (ANDROID OR (UNIX AND NOT APPLE AND NOT IOS AND NOT XROS))) | ||
set(the_description "FastCV extension module") | ||
set(FCV_MODULE_HEADER_DIR "${CMAKE_CURRENT_BINARY_DIR}/inc") | ||
set(FCV_MODULE_LIB_DIR "${CMAKE_CURRENT_BINARY_DIR}/libs") | ||
|
||
# here you can change the library to a specific one | ||
set(FASTCV_LIB "${FCV_MODULE_LIB_DIR}/libfastcvopt.so") |
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.
There is the same code in main repo. I propose to remove duplicates.
if((NOT EXISTS ${FCV_MODULE_HEADER_DIR}) OR (NOT EXISTS ${FCV_MODULE_LIB_DIR})) | ||
include("${CMAKE_CURRENT_SOURCE_DIR}/fastcv.cmake") | ||
download_fastcv("${CMAKE_CURRENT_BINARY_DIR}") | ||
endif() |
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.
Let it be called from the root cmake in main repo.
FastCV extension for OpenCV | ||
=========================== | ||
|
||
This module provides wrappers for several FastCV functions not covered by the corresponding HAL in OpenCV. |
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.
or have implementation incompatible with OpenCV.
@@ -0,0 +1,35 @@ | |||
function(download_fastcv root_dir) |
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.
It should be in main CMake.
* @param _src Input image with type CV_8UC1, CV_32SC1, CV_32FC1 | ||
* @param binary If 1, binary image (0x00-black, oxff-white); if 0, grayscale image | ||
*/ | ||
CV_EXPORTS_W cv::Moments moments(InputArray _src, bool binary); |
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.
Moments are part of HAL and already wrapped, right?
double normL2 = cv::norm(diffs, NORM_L2) / nClusters; | ||
|
||
EXPECT_LT(normL2, 0.392); |
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.
Average distance hides corner cases, e.g. all points, besides one are the same, but the last one is significant outlier. The check should be reworked.
double normH = cv::norm(bindings8u, trueBindings8u, NORM_HAMMING) / nPts; | ||
EXPECT_LT(normH, 0.658); |
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 same
int border = std::get<2>(p); | ||
bool nmsEnabled = std::get<3>(p); | ||
|
||
cv::Mat src = imread(cvtest::findDataFile("cv/shared/lena.png"), cv::IMREAD_GRAYSCALE); |
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.
Lena note.
|
||
RNG& rng = cv::theRNG(); | ||
Mat src(size, CV_8UC1); | ||
cvtest::randUni(rng, src, Scalar::all(0), Scalar::all(256)); |
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.
256 - overflow.
namespace cv { | ||
namespace fastcv { | ||
|
||
bool isInitialized = 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.
The same initialization issue as in main repo.
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 add singleton similar to main
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.