8000 dnn: fix HAVE_TIMVX macro definition in dnn test by fengyuentau · Pull Request #24425 · opencv/opencv · GitHub
[go: up one dir, main page]
Skip to content

dnn: fix HAVE_TIMVX macro definition in dnn test #24425

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 20, 2023
Merged

dnn: fix HAVE_TIMVX macro definition in dnn test #24425

merged 3 commits into from
Oct 20, 2023

Conversation

fengyuentau
Copy link
Member

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

@fengyuentau fengyuentau added category: dnn category:dnn_timvx TIM-VX related issues in DNN module labels Oct 18, 2023
@fengyuentau fengyuentau added this to the 4.9.0 milestone Oct 18, 2023
asmorkalov
asmorkalov reviewed Oct 18, 2023
View reviewed changes
modules/dnn/CMakeLists.txt Outdated
ocv_option(OPENCV_TEST_DNN_TIMVX "Build test with TIM-VX" (TARGET ocv.3rdparty.timvx))
if(TARGET ocv.3rdparty.timvx AND OPENCV_TEST_DNN_TIMVX)
if(TARGET opencv_test_dnn)
ocv_target_link_libraries(opencv_test_dnn ocv.3rdparty.timvx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need it? It's internal detail of implementation and user code should not depend on it, including tests. libopencv_dnn should have dependency from TIMVX.

Copy link
Member Author

Choose a reason for hiding this comment

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

Test does not work with missing HAVE_TIMVX macro:

opencv/modules/dnn/test/test_int8_layers.cpp

Lines 11 to 22 in 240b245

testing::internal::ParamGenerator< tuple<Backend, Target> > dnnBackendsAndTargetsInt8()
{
std::vector< tuple<Backend, Target> > targets;
targets.push_back(make_tuple(DNN_BACKEND_OPENCV, DNN_TARGET_CPU));
#ifdef HAVE_TIMVX
targets.push_back(make_tuple(DNN_BACKEND_TIMVX, DNN_TARGET_NPU));
#endif
#ifdef HAVE_INF_ENGINE
targets.push_back(make_tuple(DNN_BACKEND_INFERENCE_ENGINE_NGRAPH, DNN_TARGET_CPU));
#endif
return testing::ValuesIn(targets);
}

Basically I need HAVE_TIMVX=1.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need that.

There is registry API:

opencv/modules/dnn/include/opencv2/dnn/dnn.hpp

Lines 125 to 126 in 240b245

CV_EXPORTS std::vector< std::pair<Backend, Target> > getAvailableBackends();
CV_EXPORTS_W std::vector<Target> getAvailableTargets(dnn::Backend be);

And usage in tests:

opencv/modules/dnn/test/test_common.impl.hpp

Lines 291 to 296 in 240b245

if (withVkCom)
{
available = getAvailableTargets(DNN_BACKEND_VKCOM);
for (std::vector< Target >::const_iterator i = available.begin(); i != available.end(); ++i)
targets.push_back(make_tuple(DNN_BACKEND_VKCOM, *i));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

DNN_BACKEND_TIMVX is not in opencv/modules/dnn/test/test_common.impl.hpp.

@asmorkalov
Copy link
Contributor

The mentioned macro is used in tests in 2 places and both of them may be replaced with runtime check like Alexander suggested.

@fengyuentau
Copy link
Member Author

So why these happen? See

opencv/modules/dnn/CMakeLists.txt

Lines 290 to 295 in 6e4280e

ocv_option(OPENCV_TEST_DNN_OPENVINO "Build test with OpenVINO code" (TARGET ocv.3rdparty.openvino))
if(TARGET ocv.3rdparty.openvino AND OPENCV_TEST_DNN_OPENVINO)
if(TARGET opencv_test_dnn)
ocv_target_link_libraries(opencv_test_dnn ocv.3rdparty.openvino)
endif()
endif()

and

opencv/modules/dnn/test/test_int8_layers.cpp

Lines 11 to 22 in 6e4280e

testing::internal::ParamGenerator< tuple<Backend, Target> > dnnBackendsAndTargetsInt8()
{
std::vector< tuple<Backend, Target> > targets;
targets.push_back(make_tuple(DNN_BACKEND_OPENCV, DNN_TARGET_CPU));
#ifdef HAVE_TIMVX
targets.push_back(make_tuple(DNN_BACKEND_TIMVX, DNN_TARGET_NPU));
#endif
#ifdef HAVE_INF_ENGINE
targets.push_back(make_tuple(DNN_BACKEND_INFERENCE_ENGINE_NGRAPH, DNN_TARGET_CPU));
#endif
return testing::ValuesIn(targets);
}

(see line 18).

The TIM-VX backend is only for int8-quantized models. dnnBackendsAndTargetsInt8 is only defined and used in modules/dnn/test/test_int8_layers.cpp.

@fengyuentau
Copy link
Member Author

Also @asmorkalov could you provide a key via email for me to set up a GitHub Actions connection with my Khadas VIM3?

@opencv-alalek
Copy link
Contributor

So why these happen? See

Because tests use OpenVINO API directly to test IR models (as a reference result).

opencv/modules/dnn/test/test_ie_models.cpp

Lines 185 to 210 in 6e4280e

void runIE(Target target, const std::string& xmlPath, const std::string& binPath,
std::map<std::string, cv::Mat>& inputsMap, std::map<std::string, cv::Mat>& outputsMap)
{
SCOPED_TRACE("runIE");
std::string device_name;
#if defined(INF_ENGINE_RELEASE) && INF_ENGINE_VER_MAJOR_GT(2019010000)
Core ie;
#else
InferenceEnginePluginPtr enginePtr;
InferencePlugin plugin;
#endif
#if defined(INF_ENGINE_RELEASE) && INF_ENGINE_VER_MAJOR_GT(2019030000)
CNNNetwork net = ie.ReadNetwork(xmlPath, binPath);
#else
CNNNetReader reader;
reader.ReadNetwork(xmlPath);
reader.ReadWeights(binPath);
CNNNetwork net = reader.getNetwork();
#endif
ExecutableNetwork netExec;
InferRequest infRequest;

Tests doesn't use special TIM-VX API

opencv-alalek
opencv-alalek approved these changes Oct 20, 2023
View reviewed changes
modules/dnn/CMakeLists.txt Outdated
ocv_option(OPENCV_TEST_DNN_TIMVX "Build test with TIM-VX" (HAVE_TIMVX))
if(OPENCV_TEST_DNN_TIMVX)
if(TARGET opencv_test_dnn)
ocv_target_compile_definitions(opencv_test_dnn PRIVATE "HAVE_TIMVX=1")
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for finding this! Meanwhile please hold merging this PR until the hardware is in the CI running the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved.

@fengyuentau
Copy link
Member Author

The log shows tim-vx backend works fine with this patch, https://github.com/opencv/opencv/actions/runs/6586141510/job/17893901244?pr=24425

Ready to be merged once all tests are done.

@asmorkalov
Copy link
Contributor

@fengyuentau I see VIM3 build passed on CI. Is it ready? May I merge the pr?

asmorkalov
asmorkalov approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor
@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

👍

@fengyuentau
Copy link
Member Author

@fengyuentau I see VIM3 build passed on CI. Is it ready? May I merge the pr?

Yes, it is ready and you can merge it.

@asmorkalov asmorkalov merged commit 996b6c3 into opencv:4.x Oct 20, 2023
@fengyuentau fengyuentau deleted the fix_timvx_test branch October 20, 2023 15:29
@asmorkalov asmorkalov mentioned this pull request Nov 3, 2023
Merged
IskXCr pushed a commit to Haosonn/opencv that referenced this pull request Dec 20, 2023
@fengyuentau @IskXCr
Merge pull request opencv#24425 from fengyuentau:fix_timvx_test
d01271e 
dnn: fix HAVE_TIMVX macro definition in dnn test opencv#24425

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
@fengyuentau @thewoz
Merge pull request opencv#24425 from fengyuentau:fix_timvx_test
77056d3 
dnn: fix HAVE_TIMVX macro definition in dnn test opencv#24425

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
thewoz pushed a commit to thewoz/opencv that referenced this pull request May 29, 2024
@fengyuentau @thewoz
Merge pull request opencv#24425 from fengyuentau:fix_timvx_test
98cf2d3 
dnn: fix HAVE_TIMVX macro definition in dnn test opencv#24425

### Pull Request Readiness Checklist

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

- [x] I agree to contribute to the project under Apache 2 License.
- [x] 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
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

@opencv-alalek opencv-alalek opencv-alalek approved these changes

Assignees

@asmorkalov asmorkalov

Labels
category:dnn_timvx TIM-VX related issues in DNN module category: dnn
Projects
None yet
Milestone
4.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@fengyuentau @asmorkalov @opencv-alalek
0