3.4-tengine by liqi-c · Pull Request #16724 · opencv/opencv

liqi-c · 2020-03-03T12:46:13Z

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 OpenCV (BSD) License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to 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

force_builders=ARMv7,ARMv8
Xbuild_image:Android armeabi-v7a=android-gradle
Xbuildworker:Android armeabi-v7a=linux-4

#build_image:Android pack=android-gradle
#android_pack_config:Android pack=ndk-16.config.py

vpisarev · 2020-03-03T16:34:53Z

@alalek, I wonder if it's possible to merge this patch into 3.4 (and then master) before the code freeze?

alalek · 2020-03-03T17:02:37Z

As this is optimization patch, it would be useful to see performance benefits from this library.

alalek

Thank you for contribution!

Please measure performance gain on some public ARM platforms against current OpenCV code (use opencv_perf_dnn).

CMakeLists.txt

alalek · 2020-03-03T17:06:05Z

CMakeLists.txt

 include(cmake/OpenCVFindLAPACK.cmake)
 include(cmake/OpenCVFindProtobuf.cmake)
+if(WITH_TENGINE)
+  if (X86_64 OR X86 OR ARM OR AARCH64 OR CMAKE_CROSSCOMPILING)


Move this condition to corresponding OCV_OPTION.

alalek · 2020-03-03T17:07:56Z

cmake/OpenCVFindTengine.cmake

+#
+
+# Default tengine source store directory .
+SET(DEFAULT_OPENCV_TENGINE_SOURCE_PATH ${OpenCV_BINARY_DIR}/3rdparty/libtengine/Tengine-1.12.0)


1.12.0

Avoid hardcoded versions in strings. Especially if used in multiple places.
Version should be dumped in CMake build summary.

cmake/OpenCVFindTengine.cmake

cmake/templates/cvconfig.h.in

alalek · 2020-03-03T17:19:34Z

CMakeLists.txt

 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


OFF

Please add temporary "test" commit with enabling this for ARMv7/ARMv8 builders (building is checked only, dnn tests are not configured for ARM).

BUG1989 · 2020-03-04T01:25:55Z

The performance improvement is obvious.

dkurt · 2020-03-04T06:36:27Z

modules/dnn/src/layers/convolution_layer.cpp

        }

-        int nstripes = std::max(getNumThreads(), 1);
+#ifdef HAVE_TENGINE


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.

@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

Got it. So maybe let's create an internal forward_tengine inside the layer?

dkurt · 2020-03-04T06:38:11Z

modules/dnn/src/tengine4dnn/src/tengine_graph_convolution.cpp

+void tengine_set_Winograd(bool flag)
+{
+    // Winograd on/off .
+    setenv("NO_WINO", "1", (int)!flag);


Can we avoid changing environment from code?

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.

We've made some changes and removed setenv() call.

dkurt · 2020-03-04T06:39:21Z

modules/dnn/src/tengine4dnn/src/tengine_graph_convolution.cpp

+        }
+
+        postrun_graph(graph);
+        destroy_graph(graph);


Is that optimal to create and destroy graph every run?

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

alalek · 2020-03-04T12:12:32Z

cmake/OpenCVFindTengine.cmake

+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.")


This condition doesn't look logical (irrational).
Also use whitelist principle instead of blacklist - OpenCV is cross-platform library.

modules/dnn/CMakeLists.txt

alalek · 2020-03-04T12:15:13Z

modules/dnn/CMakeLists.txt

 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)


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.

modules/dnn/src/tengine4dnn/include/tengine_graph_convolution.hpp

alalek · 2020-03-04T12:16:46Z

modules/dnn/src/tengine4dnn/src/tengine_graph_convolution.cpp

+ * Author: qtang@openailab.com
+ */
+
+#include <iostream>


include "../../precomp.hpp" should be first.

3rdparty/libtengine/tengine.cmake

liqi-c · 2020-03-05T07:49:39Z

@alalek hello , Can we specify NDK version in OpenCV CI system? We only support versions above ndk14 . So how do we get through CI ？

alalek · 2020-03-05T12:23:49Z

above ndk14

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).

Please ignore warnings from cmake_checks.cmake
also please adjust the "test" commit and disable BUILD_ANDROID_PROJECTS variable.

alalek · 2020-03-05T19:11:49Z

modules/dnn/CMakeLists.txt


+if(HAVE_TENGINE)
+  list(APPEND include_dirs ${TENGINE_INCLUDE_DIRS})
+  list(APPEND libs ${TENGINE_LIBRARIES})


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.

vpisarev · 2020-03-06T14:22:34Z

@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.

alalek · 2020-03-06T16:10:02Z

CMakeLists.txt

  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


ON

NOT ANDROID

@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

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.

liqi-c · 2020-03-07T11:03:25Z

above ndk14

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).

Please ignore warnings from cmake_checks.cmake

also please adjust the "test" commit and disable BUILD_ANDROID_PROJECTS variable.

@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 .

alalek · 2020-03-07T15:20:16Z

ANDROID_NDK_REVISION

It is modern stuff only. For example, it is not available in current NDK10e builders for OpenCV 3.4.
But ANDROID_NDK_REVISION value is available on master branch.
Add DEFINED check for that.

LESS

Wrong. You should use VERSION_LESS instead.

if(${ANDROID_NDK_REVISION} ...

Value unpacking is wrong too in modern CMake.

vpisarev · 2020-03-07T17:31:54Z

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

liqi-c and others added 9 commits March 2, 2020 11:53

Add Tengine support .

a70853a

Modify printf to CV_LOG_WARNING

48ba4d3

a few minor fixes in the code

f46f6f7

Renew Tengine version

2ee5175

Add header file for CV_LOG_WARNING

85a4fb4

Add #ifdef HAVE_TENGINE in tengine_graph_convolution.cpp

0e5110d

remove trailing whitespace

b681e2b

Remove trailing whitespace

bb5d866

Modify for compile problem

e042357

vpisarev requested a review from alalek March 3, 2020 16:34

alalek reviewed Mar 3, 2020

View reviewed changes

dkurt reviewed Mar 4, 2020

View reviewed changes

liqi-c added 4 commits March 4, 2020 17:41

Modify some code style error

fbfd088

remove whitespace

c9231f4

Move some code style problem

8b6307e

test

c52451d

alalek reviewed Mar 4, 2020

View reviewed changes

liqi-c added 4 commits March 4, 2020 21:19

add ios limit and build problem

e851a65

Modified as alalek suggested

71e6181

Add cmake 2.8 support

e24b3fa

modify cmake 3.5.1 problem

7aed316

test and set BUILD_ANDROID_PROJECTS OFF

8f3d390

alalek reviewed Mar 5, 2020

View reviewed changes

liqi-c added 2 commits March 6, 2020 16:12

remove some compile error

19899d3

remove some extra code in tengine

4974fb8

close test.

557a07b

alalek assigned vpisarev Mar 6, 2020

Test again

830b446

liqi-c added 2 commits March 6, 2020 22:27

disable android.

97ba3c5

delete ndk version judgement

57bc4a0

alalek reviewed Mar 6, 2020

View reviewed changes

liqi-c added 2 commits March 7, 2020 16:03

Remove setenv() call . and add License information

fa37e3d

Set tengine default OFF. Close test .

d28ed7c

alalek merged commit 0bcdf7d into opencv:3.4 Mar 9, 2020

alalek mentioned this pull request Mar 9, 2020

Merge 3.4 #16764

Merged

alalek mentioned this pull request Nov 4, 2021

dnn(cmake): don't hijack OpenCL options with Tengine #21007

Merged

fengyuentau mentioned this pull request Aug 8, 2023

dnn: cleanup of tengine backend #24122

Merged

6 tasks

		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.")

Uh oh!

Conversation

liqi-c commented Mar 3, 2020 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

vpisarev commented Mar 3, 2020

Uh oh!

alalek commented Mar 3, 2020

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BUG1989 commented Mar 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpisarev Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpisarev Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liqi-c commented Mar 5, 2020

Uh oh!

alalek commented Mar 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpisarev commented Mar 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpisarev Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

liqi-c commented Mar 3, 2020 •

edited by alalek

Loading

vpisarev Mar 4, 2020 •

edited

Loading

vpisarev Mar 4, 2020 •

edited

Loading

vpisarev Mar 7, 2020 •

edited

Loading