CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
3.4-tengine #16724
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
3.4-tengine #16724
Conversation
@alalek, I wonder if it's possible to merge this patch into 3.4 (and then master) before the code freeze? |
As this is optimization patch, it would be useful to see performance benefits from this library. |
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.
Thank you for contribution!
Please measure performance gain on some public ARM platforms against current OpenCV code (use opencv_perf_dnn).
CMakeLists.txt
Outdated
@@ -687,6 +694,11 @@ include(cmake/OpenCVFindLibsVideo.cmake) | |||
include(cmake/OpenCVFindLibsPerf.cmake) | |||
include(cmake/OpenCVFindLAPACK.cmake) | |||
include(cmake/OpenCVFindProtobuf.cmake) | |||
if(WITH_TENGINE) | |||
if (X86_64 OR X86 OR ARM OR AARCH64 OR CMAKE_CROSSCOMPILING) |
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.
Move this condition to corresponding OCV_OPTION
.
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.
done
cmake/OpenCVFindTengine.cmake
Outdated
# | ||
|
||
# Default tengine source store directory . | ||
SET(DEFAULT_OPENCV_TENGINE_SOURCE_PATH ${OpenCV_BINARY_DIR}/3rdparty/libtengine/Tengine-1.12.0) |
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.
1.12.0
Avoid hardcoded versions in strings. Especially if used in multiple places.
Version should be dumped in CMake build summary.
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.
done
@@ -432,6 +432,9 @@ OCV_OPTION(WITH_IMGCODEC_PXM "Include PNM (PBM,PGM,PPM) and PAM formats support" | |||
OCV_OPTION(WITH_QUIRC "Include library QR-code decoding" ON | |||
VISIBLE_IF TRUE | |||
VERIFY HAVE_QUIRC) | |||
OCV_OPTION(WITH_TENGINE "Include Arm Inference Tengine support" OFF |
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.
OFF
Please add temporary "test" commit with enabling this for ARMv7/ARMv8 builders (building is checked only, dnn tests are not configured for ARM).
@@ -1272,10 +1275,43 @@ class ConvolutionLayerImpl CV_FINAL : public BaseConvolutionLayerImpl | |||
} | |||
} | |||
|
|||
int nstripes = std::max(getNumThreads(), 1); | |||
#ifdef HAVE_TENGINE |
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.
Maybe it's better to have something like DNN_BACKEND_TENGINE and separate forward? Current layers implementation is not perfect on terms of backends but if TEngine uses host Mat data and there is no need for wrappers - it must be easy to manage.
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.
@dkurt, that was the original idea, but implementing a dedicated backend takes much more efforts. To get some immediate performance improvement, only convolution layer is accelerated now. Later on, the backend will be created, I think
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.
Got it. So maybe let's create an internal forward_tengine inside the layer?
void tengine_set_Winograd(bool flag) | ||
{ | ||
// Winograd on/off . | ||
setenv("NO_WINO", "1", (int)!flag); |
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.
Can we avoid changing environment from code?
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.
that's what I told too. For now, there is no appropriate function for it in the Tengine itself, and without this setting the unit tests fail. I think, in some next version of Tengine the function to configure Winograd branch will be added and that function will be called instead of setting environment variable. Since "setenv()" affects only the current process, it's not a showstopper, just a question of style and extra overhead.
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've made some changes and removed setenv() call.
} | ||
|
||
postrun_graph(graph); | ||
destroy_graph(graph); |
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.
Is that optimal to create and destroy graph every run?
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.
Yes, it's not very optimal. I guess, each graph consumes some memory and there is no way currently to make it to use external buffers. If the graph is constructed once, the Tengine-accelerated OpenCV DNN will consume much more memory. For now the performance increase because of using Tengine just for convolution outweights the overhead of the graph construction. I believe, some better solution will be developed later
cmake/OpenCVFindTengine.cmake
Outdated
IF( WITH_CUDA OR WITH_OPENCL OR WITH_CUDNN OR WITH_VULKAN OR X86 OR X86_64) | ||
MESSAGE(STATUS "TENGINE:-- Not support . Turning Tengine_FOUND off.") |
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 condition doesn't look logical (irrational).
Also use whitelist principle instead of blacklist - OpenCV is cross-platform library.
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.
done
modules/dnn/CMakeLists.txt
Outdated
@@ -13,6 +13,10 @@ ocv_add_dispatched_file_force_all("layers/layers_common" AVX AVX2 AVX512_SKX) | |||
ocv_add_module(dnn opencv_core opencv_imgproc WRAP python java js) | |||
|
|||
ocv_option(OPENCV_DNN_OPENCL "Build with OpenCL support" HAVE_OPENCL AND NOT APPLE) | |||
ocv_option(OPENCV_DNN_TENGINE "Build with Tengine support" HAVE_TENGINE) |
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.
OPENCV_DNN_TENGINE
Probably not needed at all.
OpenCL is OpenCV-wide stuff (so we want to control this through OPENCV_DNN_OPENCL
), but tengine is not.
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.
done
modules/dnn/src/tengine4dnn/include/tengine_graph_convolution.hpp
Outdated
Show resolved
Hide resolved
* Author: qtang@openailab.com | ||
*/ | ||
|
||
#include <iostream> |
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.
include "../../precomp.hpp" should be first.
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.
done
@alalek hello , Can we specify NDK version in OpenCV CI system? We only support versions above ndk14 . So how do we get through CI οΌ |
It would be nice to add this check into CMake scripts somewhere. I have switched on gradle-based Android build environment (but it is not well supported on 3.4 branch).
|
modules/dnn/CMakeLists.txt
Outdated
@@ -83,6 +86,10 @@ else() | |||
set(sources_options EXCLUDE_OPENCL) | |||
endif() | |||
|
|||
if(HAVE_TENGINE) | |||
list(APPEND include_dirs ${TENGINE_INCLUDE_DIRS}) | |||
list(APPEND libs ${TENGINE_LIBRARIES}) |
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.
No rule to make target
lib/libtengine.a
In case of "build" mode CMake should get here the target name instead of library path. It is necessary for creating dependencies.
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.
done
@liqi-c, maybe it makes sense to temporarily disable Tengine on Android, in order to make all the builds pass? Then we can merge it in before the code freeze. Later on, you can re-enable it. |
@@ -432,13 +432,16 @@ OCV_OPTION(WITH_IMGCODEC_PXM "Include PNM (PBM,PGM,PPM) and PAM formats support" | |||
OCV_OPTION(WITH_QUIRC "Include library QR-code decoding" ON | |||
VISIBLE_IF TRUE | |||
VERIFY HAVE_QUIRC) | |||
OCV_OPTION(WITH_TENGINE "Include Arm Inference Tengine support" ON | |||
VISIBLE_IF (ARM OR AARCH64) AND UNIX AND NOT ANDROID AND NOT IOS |
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.
ON
NOT ANDROID
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.
@alalek, can you please decrypt this comment? :) the decision is to temporarily disable Tengine on Android, because the builds on Android fail now. Let's repair the Android case later
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.
VISIBLE_IF
is for "complete" filtering of platforms. This filter "hides" the option.
Android can be use used, but should be configured for proper version.
So right place for "NOT ANDROID" "by default" is ON/OFF condition.
@alalek How to add NDK version check ? we add like this " if(${ANDROID_NDK_REVISION} LESS 14) " but it's CI test failed when ndk version is r10 . |
It is modern stuff only. For example, it is not available in current NDK10e builders for OpenCV 3.4.
Wrong. You should use
Value unpacking is wrong too in modern CMake. |
I think, it's ready to merge π Android support could be added later |
dnn: cleanup of tengine backend #24122 π Cleanup for OpenCV 5.0. Tengine backend is added for convolution layer speedup on ARM CPUs, but it is not maintained and the convolution layer on our default backend has reached similar performance to that of Tengine. Tengine backend related PRs: - #16724 - #18323 ### 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
dnn: cleanup of tengine backend opencv#24122 π Cleanup for OpenCV 5.0. Tengine backend is added for convolution layer speedup on ARM CPUs, but it is not maintained and the convolution layer on our default backend has reached similar performance to that of Tengine. Tengine backend related PRs: - opencv#16724 - opencv#18323 ### 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
dnn: cleanup of tengine backend opencv#24122 π Cleanup for OpenCV 5.0. Tengine backend is added for convolution layer speedup on ARM CPUs, but it is not maintained and the convolution layer on our default backend has reached similar performance to that of Tengine. Tengine backend related PRs: - opencv#16724 - opencv#18323 ### 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
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.