OpenCV4 compatibility

[hgaiser]

Since OpenCV4 is released a month ago, this package doesn't compile. This PR adds compatibility for OpenCV4.

Regarding the module_opencv4.cpp, it's probably not the most future proof way to do this, but the package already seems to handle opencv2/3 this way so I just amended it.

[gaoethan]

@hgaiser Thanks for your contribution. but it fails to pass the CI, it seems that it requires to enable the rule to use OpenCV4 in the corresponding rosdistro

[jveitchmichaelis]

One fix for this is to remove the version number from the top level CMakeLists.txt file. In any case this is redundant. The source-level CMakeLists file checks which version is installed, so I don't think there's no need for the top level one to force OpenCV3. Might try a separate PR for this to trigger a CI build to see if I can replicate this.

find_package(OpenCV REQUIRED
  COMPONENTS
    opencv_core
    opencv_imgproc
    opencv_imgcodecs
  CONFIG
)

Duplicate the opencv3 module to module_opencv4.cpp, make the adjustments in this commit (i.e. fix the access flags) and add to CMakeLists.txt:

if (OpenCV_VERSION_MAJOR VERSION_EQUAL 4)
add_library(${PROJECT_NAME}_boost module.cpp module_opencv4.cpp)
else(OpenCV_VERSION_MAJOR VERSION_EQUAL 3)
add_library(${PROJECT_NAME}_boost module.cpp module_opencv3.cpp)
else()
add_library(${PROJECT_NAME}_boost module.cpp module_opencv2.cpp)
endif()

[hgaiser]

Updated the PR as @jveitchmichaelis suggested. I can't test it right now, so I'll let Travis test it for me 👍

[edgarriba]

I've been following this thread for a while and I think that by adding a module_opencv*.cpp each we release a new version seems the way to go. However, the only thing I can see and I don't like is that there's a lot of duplicated code in each of this files. I'd suggested to isolate this codes and just put the specific things that happen to break from one version to the other. Maybe @vrabaud have another view about that ?

[jveitchmichaelis]

Needs a slight modification. This now appears to fail because in OpenCV 2 imgcodecs didn't exist. back to square one? The source level file is fine, we need to make the root file more selective.

The existing code is:

find_package(OpenCV 3 REQUIRED
  COMPONENTS
    opencv_core
    opencv_imgproc
    opencv_imgcodecs
  CONFIG
)

So referring to my last post, this suggests we don't care about OpenCV2, but it still seems to matter for the CI... I guess a brute force option would be find_package twice, once to detect the version and once again to select which components are required depending on the library. Unless there's a way of specifying a minimum major version in find_package, which doesn't seem to be the case.

See e.g. http://cmake.3232098.n2.nabble.com/How-do-I-specify-VTK-minimum-version-td7595794.html

A dirty - and untested! - solution:

find_package(OpenCV)

if ( NOT (OpenCV_VERSION_MAJOR VERSION_LESS 3) )
find_package(OpenCV REQUIRED
  COMPONENTS
    opencv_core
    opencv_imgproc
    opencv_imgcodecs
  CONFIG
)
else()
find_package(OpenCV 2 REQUIRED
  COMPONENTS
    opencv_core
    opencv_imgproc
    opencv_highgui
  CONFIG
)
endif()

Or we replace the else case with an error.

Referring to: https://docs.opencv.org/3.4/db/dfa/tutorial_transition_guide.html

[hgaiser]

I changed this PR to ignore OpenCV2.4. This PR is set to merge with the melodic branch, and according to the melodic REP, OpenCV 3.2+ is required. The current Travis tests don't seem to include OpenCV 3.2 so I am ignoring that issue for now.

[v4hn]

It's been half a year and OpenCV4 becomes more and more widely available.
@hgaiser This request still fails CI and I guess it will not get merged before it does.
According to your explanations it looks like opencv2 and 3 are both installed for the check and find_package prefers 2.

How about explicitly requesting v4 and if unavailable v3?

I understand why you did not do this before, but it's a way to solve this dilemma.
The current patch also does not specify a preference between v4 and v3 which can be a big issue for package maintainance.

[hgaiser]

It's been half a year and OpenCV4 becomes more and more widely available.
@hgaiser This request still fails CI and I guess it will not get merged before it does.
According to your explanations it looks like opencv2 and 3 are both installed for the check and find_package prefers 2.

How about explicitly requesting v4 and if unavailable v3?

I understand why you did not do this before, but it's a way to solve this dilemma.
The current patch also does not specify a preference between v4 and v3 which can be a big issue for package maintainance.

Sorry my previous post was a bit unclear. I meant that the current Travis configuration tests with ROS Kinetic and OpenCV 2.4. OpenCV 2.4 is not a target for Melodic, so I removed the parts of this PR that focused on support for OpenCV 2.4 since that complicated things drastically.

[v4hn]

Could you update the travis config in this branch then to check Melodic in the melodic branch instead of kinetic?

[hgaiser]

Considering my time is limited, I would have to decline. Besides, I feel Travis testing with melodic is outside of the scope of this PR.

[hgaiser]

Is there any update on this?

[jwhendy]

Is it accurate to say that because a build bot fails, progress is roadblocked? This seems to be a case where humans are serving a tool vs. the other way around.

The bot is just some environment, no? Could a chroot build + provided logs on n computers not fill its place? Can I build from scratch for you in a bionic chroot?

I realize this isn't such a case, but imagine someone found a vulnerability or safety risk with ROS. We would just let Captain Travis prevent a PR for this long!?

Is there any reasonable alternative for moving forward, or will the team here only accept a CI pass?

[mjcarroll]

@ros-pull-request-builder retest this please

[tjacobs]

Would like to have a working ROS melodic+OpenCV on the NVIDIA TX2 please. JetPack 4.3 comes with OpenCV 4.

[timonegk]

@hgaiser from the PR description, it seems like you initially had the OpenCV4 version in module_opencv4.cpp and then changed it so that all changes are in module_opencv.cpp only, presumably to reduce code duplication.
This breaks OpenCV3 compatibility because the changes (AccessFlag, CV_OVERRIDE, AutoBuffer::data()) are not supported in cv3. I think to avoid the code duplication while maintaining backwards compatibility to cv3 it would be the best way to use preprocessor directives to differentiate the versions, e.g.

#if CV_MAJOR_VERSION > 3
        PyObject* o = PyArray_SimpleNew(dims, _sizes.data(), typenum);
#else
        PyObject* o = PyArray_SimpleNew(dims, _sizes, typenum);
#endif

and similar for the other changes.

[hgaiser]

Ah I see the CI has been updated to build opencv3. I had assumed this code would be backwards compatible to opencv3 but apparently it isn't. @timonegk could you make a PR to merge with this PR? I don't have the time at the moment to look into your suggested fix.

[mjcarroll]

@hgaiser and @timonegk can you take a look at the noetic branch? It should be compatible with both OpenCV3 as well as OpenCV4.

Since I'm not officially a "maintainer" on this package for ROS 1, I'm a bit reluctant to release these changes into melodic, but I don't think that there should be any reason that this won't build from source in your configuration.

[timonegk]

@hgaiser I opened a pull request at fizyr-forks#1 and tested for both cv versions

[timonegk]

@mjcarroll I am pretty sure that https://github.com/ros-perception/vision_opencv/blob/55bf08947a302aec8c83bec1883be1880d56cff5/cv_bridge/src/module_opencv4.cpp (current noetic branch) will not build on bionic (opencv3.2):

• CV_OVERRIDE was added in 3.4.2 (opencv/opencv@8498074)
• AccessFlag was introduced in 4.0.0 (opencv/opencv@ef5579d#diff-bc1d784738cd852f5b1e95ce10a56d06R64)

[kangkelvin]

Hello,

Does the fix work? If yes, then can it be merged?

Thanks

[Ajun11]

Since OpenCV4 is released a month ago, this package doesn't compile. This PR adds compatibility for OpenCV4.

Regarding the module_opencv4.cpp, it's probably not the most future proof way to do this, but the package already seems to handle opencv2/3 this way so I just amended it.

sincerely thanks