[build][option] Introduce `OPENCV_DISABLE_THREAD_SUPPORT` option. by fpetrogalli · Pull Request #19985 · opencv/opencv

fpetrogalli · 2021-04-26T17:09:10Z

The option forces the library to build without the use of #include <thread>. The modules for which this is not possible has been
conditionally disabled when the option is active.

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 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=linux,docs,ARMv8,ARMv7,Custom
build_image:Custom=javascript
buildworker:Custom=linux-4,linux-6

fpetrogalli · 2021-04-26T17:10:57Z

@alalek, @vpisarev - I have factored out the option CV_DISABLE_MULTI_THREAD as suggested from #19921

fpetrogalli · 2021-04-27T09:03:37Z

HI @alalek - I am taking the comment at #19921 (comment) here as it seems specific to this PR.

I believe stubs should be created in the #else branch to handle these unsupported cases (which throws CV_Error(Error::StsNotImplemented, ...))

Are you saying that code that is depending on #include <thread> should be modified as follows?

#ifndef CV_DISABLE_MULTI_THREAD
#include <thread>
#else 
// Stub implementation of whatever is needed from pthread, that raises errors for unimplemented methods:
namespace std {
  struct thread {
    // ...
    void join() {
      CV_Error(Error::StsNotImplemented, ...);
    }
  };
}
#endif

If that's the case, I think it would make sense to merge the stubs in the #else branch in a common header file. What would be the best place to store such file for automatic inclusion in all the project? Probably the main include of the code module?

alalek · 2021-04-27T17:58:29Z

That comment is about stubs for OpenCV types, like cv::AsyncArray, cv::Mutex (exception for lock()/unlock() only), cv::AutoLock (no-op instead of exception).
Creating stubs for std::thread looks dangerous.

fpetrogalli · 2021-04-28T05:39:28Z

That comment is about stubs for OpenCV types, like cv::AsyncArray, cv::Mutex (exception for lock()/unlock() only), cv::AutoLock (no-op instead of exception).

I see - thank you for explaining @alalek . Are there any other class I should be aware of that I should conditionally redefined based on the macro CV_DISABLE_MULTI_THREAD?

Creating stubs for std::thread looks dangerous.

Yep, but I thought that was wanted me to do with the "port" file that you wanted me to create in #19921 I didn't get as far as implementing the rest of the struct thread there because many things are disabled automatically when using a baremetal toolchain:

-- To be built: core features2d highgui imgcodecs imgproc ml photo video
-- Disabled: flann ts videoio world
-- Disabled by dependency: calib3d objdetect stitching
-- Unavailable: dnn gapi java python2 python3

If we want the option CV_DISABLE_MULTI_THREAD to be an option for the whole library and (understandably) we don't want to create stubs for std::thread, I think I'll have to disable all the modules that do #include <thread> (either directly or indirectly via the use of <future> or other things). As far as I can tell, these should be at least the ones listed in the Unavailable field.

fpetrogalli · 2021-04-29T13:42:01Z

@vpisarev - is the arm64 builder at https://build.opencv.org.cn/job/precommit/job/ubuntu-20.04-arm64/ broken? Or is the failure of the latest commit a genuine failure caused by my commit?

fpetrogalli · 2021-05-06T09:21:19Z

@vpisarev @alalek - gentle ping on this patch too. The OpenCV CN CI failures seem to be unrelated too as for #20010

fpetrogalli · 2021-05-10T10:38:37Z

@alalek @vpisarev - may I kindly ask some feedback on this patch? I believe I addressed all concerns raised up to this point.

Kind regards,

Francesco

CMakeLists.txt

3rdparty/libwebp/CMakeLists.txt

modules/core/CMakeLists.txt

modules/core/src/utils/logtagmanager.hpp

fpetrogalli · 2021-05-17T16:39:29Z

Thank you for the review, @alalek !

modules/core/include/opencv2/core/utility.hpp

CMakeLists.txt

alalek · 2021-05-21T04:13:12Z

modules/core/src/async.cpp

+#if defined(CV_CXX11) && !defined(OPENCV_DISABLE_THREAD_SUPPORT)
        std::unique_lock<std::mutex> lock(mtx);
 #else
        cv::AutoLock lock(mtx);


"#else" non C++11 branch is candidate for removal.
This patch forces to keep this.

IMHO, we need dedicated implementation for AsyncArray for case without threads (or even forbid using of this).

I am happy to provide an implementation of AsyncArray that does not use threads. I'll give it a go, but I might need some guidance on this.

Just to make sure I am going in the right direction. You want me to have a big #ifundef / #else / #endif switch where the #else branch exposes the curret implementation (threaded) of AsyncArray, while the #ifundef OPENCV_DISABLE_THREAD_SUPPORT will implement AsyncArray without using threads (ro shoudl it be just stubs?).

Also, what the alternative...

even forbid using of this

... consist of? Disabling the use of AsyncArray in all places where it is used?

Stub with exceptions should work for now (there are no many places which uses AsyncArray)

I started providing stubs for the API of cv::AsyncArray, and it turned out I also need to provide stubs for cv::AsyncPromise. The approach I followed was to remove the AsyncArray::Impl definition and uses. This however caused writing many #ifundef / #else / #endif cases, which doesn't seem to fit very well with the goal of miniming such cases.

Would it be OK if instead of providing stubs for cv::AsyncArray and cv::AsyncPromise, I would be providing a stub class AsyncArray::Impl that would expose stubs only for the methods used by cv::AsyncArray and cv::AsyncPromise? This solution would still provide useful exception message, and would require just a couple of #ifundef / #else / #endif cases.

modules/core/src/system.cpp

fpetrogalli · 2021-05-21T08:05:11Z

Thank you for the review @alalek !

fpetrogalli · 2021-05-21T12:59:02Z

@alalek - thank you for the review. The baremetal builds are still running correctly even after the changes you requested, this is very nice.

As a matter of fact, I need more feedback from you about #19985 (comment), as I am not 100% sure what I have done is the right thing.

Thank you again,

Francesco

fpetrogalli · 2021-05-24T07:53:33Z

@alalek - please note that, once you accept this patch, I think that the commits in this patch should be squashed before merging it into master.

CMakeLists.txt

cmake/OpenCVFindLibsGUI.cmake

asmorkalov · 2021-06-04T10:00:16Z

CMakeLists.txt

 endif()

+if(OPENCV_DISABLE_THREAD_SUPPORT)
+  add_definitions(-DOPENCV_DISABLE_THREAD_SUPPORT)


The option is not exported to OpenCV-config.cmake and other build configs. All methods in header files will use another pre-processor branch and break user code.

I am happy to do the export of the variable but I am not sure I understand the issue.

Also, how can I export OPENCV_DISABLE_THREAD_SUPPORT in OpenCV-config.cmake? I have looked a bit into cmake/OpenCVGenConfig.cmake but I am missing the mechanism that does this export.

modules/core/include/opencv2/core/async.hpp

modules/core/include/opencv2/core/utility.hpp

modules/core/src/async.cpp

modules/ts/src/ts_gtest.cpp

modules/ts/src/ts_perf.cpp

fpetrogalli · 2021-06-14T10:37:36Z

@alalek @asmorkalov - Thank you for the review. Apologies for the delay in getting back to you but I was OoO.

Francesco

fpetrogalli · 2021-06-21T07:55:35Z

Hi @alalek - thank you for the patch that you have added on top of my changes. Extracting the cmake commands in a separate file and reducing the number of #ifdef conditions make totally sense. The code looks much better now!

Unfortunately, the changes are breaking the build for the aarch64-baremetal branch at #19921 . I'll add my own commits on top of your to get #19921 to build with the changes on top of #19985 .

While I am at that, I will also merge all the commits of #19985 marked with "to squash" into a single commit (I won't include your commit in the squash).

Francesco

fpetrogalli · 2021-06-24T15:43:23Z

@alalek @asmorkalov - gentle ping!

Francesco

alalek · 2021-06-30T12:32:26Z

reverted commit with ocv_force_update.
updated patch to handle mode variable change from toolchain too.

I'm unable to check #19921 (proposed toolchain requires AARCH64 host - I don't have such configured setup for development), please take a look on your side.

alalek · 2021-07-07T19:11:38Z

Patch is rebased to resolve conflict in cmake/OpenCVCompilerOptions.cmake (near add_extra_compiler_option(-pthread) condition)

The option forces the library to build without thread support.

- reduce amount of #if conditions

fpetrogalli · 2021-07-08T10:16:26Z

@alalek , thank you for looking into this code again.

I'm unable to check #19921 (proposed toolchain requires AARCH64 host - I don't have such configured setup for development), please take a look on your side.

#19921 is still to considered a WIP. There is no need to review it for now. I'd rather have this PR (#19985 ) done and dusted before asking you to look in the baremetal one. We have CI that depends on #19921 , for now I just keep updating it to make our CI happy again when things start failing because of updates or merges.

alalek · 2021-07-08T10:18:56Z

CMakeLists.txt


+if (OPENCV_DISABLE_THREAD_SUPPORT)
+  set(WITH_GSTREAMER OFF)
+endif()


Why do we need this?

It should be handled in cmake/vars/OPENCV_DISABLE_THREAD_SUPPORT.cmake: https://github.com/opencv/opencv/pull/19985/files#diff-87b6de92e07d14f041e4b331f8936a5d5a4f2f8e499aa71bc3967de40cff9960

Please avoid any specific changes in root CMakeLists.txt - it should not manage details of 3rdparty components (otherwise we would get unsupportable huge mess)

Ensure that you have clean CMake cache (after code update)

It should be handled in cmake/vars/OPENCV_DISABLE_THREAD_SUPPORT.cmake
I agree, sorry, I keep forgetting about this file and I end up running in the same issue everytime I was recompiling after changing branch because...

Ensure that you have clean CMake cache (after code update)
... I was not aware of this trick! Thank you. I am going to remove the last commit with this change.

alalek

Thank you for contribution 👍

fpetrogalli · 2021-07-09T08:13:00Z

Thank you @alalek ! Now I'll clean up #19921 and get it ready for review!

Francesco

* [build][option] Introduce `OPENCV_DISABLE_THREAD_SUPPORT` option. The option forces the library to build without thread support. * update handling of OPENCV_DISABLE_THREAD_SUPPORT - reduce amount of #if conditions * [to squash] cmake: apply mode vars in toolchains too Co-authored-by: Alexander Alekhin <alexander.a.alekhin@gmail.com>

fpetrogalli mentioned this pull request Apr 28, 2021

[baremetal][aarch64] Build a baremetal OpenCV for AArch64. #19921

Closed

6 tasks

fpetrogalli force-pushed the disable_threads branch 2 times, most recently from 9088150 to a98c5cd Compare May 4, 2021 15:54

fpetrogalli force-pushed the disable_threads branch from a98c5cd to 47d8b59 Compare May 6, 2021 17:40

fpetrogalli force-pushed the disable_threads branch 3 times, most recently from 5039985 to efa162f Compare May 12, 2021 08:17

alalek reviewed May 17, 2021

View reviewed changes

fpetrogalli force-pushed the disable_threads branch from efa162f to 9a875e2 Compare May 17, 2021 16:38

fpetrogalli changed the title ~~[build][option] Introduce CV_DISABLE_MULTI_THREAD option.~~ [build][option] Introduce OPENCV_DISABLE_THREAD_SUPPORT option. May 17, 2021

fpetrogalli commented May 18, 2021

View reviewed changes

modules/core/include/opencv2/core/utility.hpp Show resolved Hide resolved

alalek reviewed May 21, 2021

View reviewed changes

fpetrogalli force-pushed the disable_threads branch from 9a875e2 to 6c73396 Compare May 21, 2021 12:48

fpetrogalli mentioned this pull request May 21, 2021

[build] Move OPENCV_DISABLE_FILESYSTEM_SUPPORT. [NFC] #20137

Merged

6 tasks

fpetrogalli force-pushed the disable_threads branch from 6c73396 to 323b07d Compare May 24, 2021 07:33

vpisarev requested a review from alalek May 27, 2021 14:24

fpetrogalli force-pushed the disable_threads branch from 323b07d to 9b988c3 Compare May 27, 2021 17:03

asmorkalov reviewed Jun 1, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

cmake/OpenCVFindLibsGUI.cmake Outdated Show resolved Hide resolved

asmorkalov requested changes Jun 4, 2021

View reviewed changes

alalek reviewed Jun 6, 2021

View reviewed changes

fpetrogalli force-pushed the disable_threads branch 2 times, most recently from a6f0039 to 608fec0 Compare June 18, 2021 08:44

fpetrogalli force-pushed the disable_threads branch from 1b8cbc4 to bb90145 Compare June 21, 2021 10:50

Francesco Petrogalli and others added 3 commits July 8, 2021 10:07

[build][option] Introduce OPENCV_DISABLE_THREAD_SUPPORT option.

01049be

The option forces the library to build without thread support.

update handling of OPENCV_DISABLE_THREAD_SUPPORT

8630492

- reduce amount of #if conditions

[to squash] cmake: apply mode vars in toolchains too

0d819b1

fpetrogalli force-pushed the disable_threads branch from 1ef79f6 to 30a81fd Compare July 8, 2021 10:09

alalek reviewed Jul 8, 2021

View reviewed changes

fpetrogalli force-pushed the disable_threads branch from 30a81fd to 0d819b1 Compare July 8, 2021 14:06

alalek approved these changes Jul 8, 2021

View reviewed changes

alalek merged commit b928ebd into opencv:master Jul 8, 2021

fpetrogalli deleted the disable_threads branch July 9, 2021 08:13

alalek mentioned this pull request Jul 13, 2021

cmake: use relative path for mode vars #20400

Merged

alalek mentioned this pull request Aug 4, 2021

Improves FLANN's heap allocations by a memory pool #20460

Merged

5 tasks

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged

alalek mentioned this pull request Feb 16, 2022

New Volume pipeline #21189

Merged

6 tasks

Uh oh!

Conversation

fpetrogalli commented Apr 26, 2021 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

fpetrogalli commented Apr 26, 2021

Uh oh!

fpetrogalli commented Apr 27, 2021

Uh oh!

alalek commented Apr 27, 2021

Uh oh!

fpetrogalli commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fpetrogalli commented Apr 29, 2021

Uh oh!

fpetrogalli commented May 6, 2021

Uh oh!

fpetrogalli commented May 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fpetrogalli commented May 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alalek May 21, 2021

Choose a reason for hiding this comment

Uh oh!

fpetrogalli May 21, 2021

Choose a reason for hiding this comment

Uh oh!

alalek May 21, 2021

Choose a reason for hiding this comment

Uh oh!

fpetrogalli May 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fpetrogalli commented May 21, 2021

Uh oh!

fpetrogalli commented May 21, 2021

Uh oh!

fpetrogalli commented May 24, 2021

Uh oh!

Uh oh!

Uh oh!

asmorkalov Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

fpetrogalli Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fpetrogalli commented Jun 14, 2021

Uh oh!

fpetrogalli commented Jun 21, 2021

Uh oh!

fpetrogalli commented Jun 24, 2021

Uh oh!

alalek commented Jun 30, 2021

Uh oh!

alalek commented Jul 7, 2021

Uh oh!

fpetrogalli commented Jul 8, 2021

Uh oh!

alalek Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

fpetrogalli commented Apr 26, 2021 •

edited by alalek

Loading

fpetrogalli commented Apr 28, 2021 •

edited

Loading

alalek Jul 8, 2021 •

edited

Loading