Skip to content

Prevent cudacodec libs locations being reset when cmake is re-run#22880

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
cudawarped:remember_cudacodec_lib_locations
Dec 16, 2022
Merged

Prevent cudacodec libs locations being reset when cmake is re-run#22880
asmorkalov merged 1 commit intoopencv:4.xfrom
cudawarped:remember_cudacodec_lib_locations

Conversation

@cudawarped
Copy link
Copy Markdown
Contributor

@cudawarped cudawarped commented Nov 29, 2022 •
edited by alalek
Loading

If cmake is run a second time then the location of the Nvidia Video Codec SDK libs are reset due to the implementation of FindCUDA.cmake. This was observerd in opencv/opencv_contrib#3195 (comment).

This PR also forces WITH_NVCUVID and WITH_NVCUVENC to default to ON so that if the libs are present decoding and/or encoding support is added to cudacodec without the user having to manually specify it. This mirrors the way in which the DNN module is built with cuDNN if the cuDNN libs are present without the user having to specify WITH_CUDNN=ON.

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
force_builders=Custom
build_image:Custom=ubuntu-cuda:16.04
buildworker:Custom=linux-1

@asmorkalov asmorkalov self-assigned this Nov 29, 2022
@asmorkalov asmorkalov self-requested a review November 29, 2022 12:11
@asmorkalov asmorkalov added the category: gpu/cuda (contrib) OpenCV 4.0+: moved to opencv_contrib label Nov 29, 2022
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 13, 2022

@cudawarped Thanks for the contribution. I suspect that you just need to remove WIN32 condition here: https://github.com/opencv/opencv/blob/4.x/cmake/FindCUDA.cmake#L792 without extra changes in cmake. find_cuda_helper_libs calls find_library at the end. find_library stores results in CMake cache and you should get the expected behavior.

@cudawarped
Copy link
Copy Markdown
Contributor Author

cudawarped commented Dec 13, 2022 •
edited
Loading

@asmorkalov Unless an old version of Cmake is used or the behaviour is overidden OpenCV use CMake's internal version of FindCUDA which we can't alter.

Inside CMake's version the problem appears to be a result of CUDA_TOOLKIT_TARGET_DIR not being set in the cache resulting in cuda_unset_include_and_libraries() beng called on every subsequent invocation of cmake here

if(DEFINED CUDA_TOOLKIT_TARGET_DIR
AND NOT "${CUDA_TOOLKIT_TARGET_DIR}" STREQUAL "${CUDA_TOOLKIT_TARGET_DIR_INTERNAL}")
cuda_unset_include_and_libraries()
endif()

This is not an issue if the libraries are in a location which FindCUDA searches as they can be picked up again but it is a issue if they were supplied at the command line (-DCUDA_nvcuvid_LIBRARY= or -DCUDA_nvencodeapi_LIBRARY=).

My initial fix was to set CUDA_TOOLKIT_TARGET_DIR before calling FindCUDA but because changes to FindCUDA are out of our control I thought it would be better to store the current locations of the encoding/decoding libs instead.

Should we instead make changes to the OpenCV version of FindCUDA and force it to be used here

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 14, 2022

@alalek What is your opinion? The proposed solution with _internal_ variables works, but looks ugly a bit.

alalek
alalek reviewed Dec 14, 2022
View reviewed changes
cmake/OpenCVDetectCUDA.cmake Outdated
ocv_cuda_SEARCH_NVCUVID_HEADER("nvcuvid.h" HAVE_NVCUVID_HEADER)
ocv_cuda_SEARCH_NVCUVID_HEADER("dynlink_nvcuvid.h" HAVE_DYNLINK_NVCUVID_HEADER)
if(DEFINED CUDA_nvcuvid_LIBRARY_INTERNAL)
set(CUDA_nvcuvid_LIBRARY ${CUDA_nvcuvid_LIBRARY_INTERNAL} CACHE FILEPATH "\"nvcuvid\" library" FORCE)
Copy link
Copy Markdown
Member

@alalek alalek Dec 14, 2022

Choose a reason for hiding this comment

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

calls to FindCUDA can reset

Should we check for the empty value then?

Copy link
Copy Markdown
Contributor Author

@cudawarped cudawarped Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@alalek I agree but I've just noticed this issue was fixed in CMake 6 months ago https://gitlab.kitware.com/cmake/cmake/-/commit/a7758394afc2717bb31a47826e62c1ba86a63c41. I suggest for compatibility with older versions of CMake we could apply the same fix as mentioned there.

@cudawarped
Enable cudacodec libs by default, prevent their locations being lost …
b1288da 
…when cmake is re-run and add log info when the libs/headers cannot be found.
@cudawarped cudawarped force-pushed the remember_cudacodec_lib_locations branch from 0bd756f to b1288da Compare December 15, 2022 13:02
@asmorkalov asmorkalov merged commit 6db9fcb into opencv:4.x Dec 16, 2022
@alalek alalek mentioned this pull request Jan 8, 2023
Merged
@asmorkalov asmorkalov added this to the 4.7.0 milestone Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek left review comments

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@asmorkalov asmorkalov

Labels

category: build/install category: gpu/cuda (contrib) OpenCV 4.0+: moved to opencv_contrib

Projects

None yet

Milestone

4.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@cudawarped @asmorkalov @alalek