alicevision / CCTag

Detection of CCTag markers made up of concentric circles.
https://cctag.readthedocs.io
Mozilla Public License 2.0
358 stars 89 forks source link

TBB:2021 oneAPI compatibility #178

Closed bartoszek closed 2 years ago

bartoszek commented 2 years ago

Description: TBB:2021 compatibility fix

Implementation remarks

Fix #177

simogasp commented 2 years ago

Thanks for this! We may need to better handle the c++17 flag because that also affects the cuda compiler and the minimum cuda version that is supported (11+). We have a CCTAG_CXX_STANDARD that we can use to determine the standard to use. https://github.com/alicevision/CCTag/blob/a0f4630d018ac9026299e3443ff3cde64f850dc8/CMakeLists.txt?_pjax=%23js-repo-pjax-container%2C%20div%5Bitemtype%3D%22http%3A%2F%2Fschema.org%2FSoftwareSourceCode%22%5D%20main%2C%20%5Bdata-pjax-container%5D#L68-L72

So maybe we need to move the code around in the CMakeLists to first determine if it is c++17 or c++14 and set the value.

bartoszek commented 2 years ago

Yea, I saw this, just tried the minimum required changes to allow tbb:2021 support, haven't thought about aberrations form cuda compiler.

p12tic commented 2 years ago

@simogasp Could we maybe just use std::mutex and std::lock_guard to avoid conditional compilation? The project uses C++14 as a minimum, so std::mutex should perform just as well.

simogasp commented 2 years ago

@simogasp Could we maybe just use std::mutex and std::lock_guard to avoid conditional compilation? The project uses C++14 as a minimum, so std::mutex should perform just as well.

yes that's what they just did on vcpkg a couple of days ago when they updated TBB version https://github.com/microsoft/vcpkg/commit/5f6dfcb4d7a240309c79ad725007ad1a7c9e4157#diff-8a60cbd7865e2b41380671bdb5e4dcde34d4a1b7d51463019e270ff7bb8d6f84 I will rework this PR to make it more clean, I think at this point we can require a more recent version of TBB as managing cmake config and cmake old style is painful. We need to update the docker images for the CI though. @fabiencastan

simogasp commented 2 years ago

Thanks for initiating this PR! The issue is now addressed in #200 so I'm closing this one.