Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YARP does not compile with OpenCV 4 #1672

Closed
Nicogene opened this issue May 3, 2018 · 30 comments
Closed

YARP does not compile with OpenCV 4 #1672

Nicogene opened this issue May 3, 2018 · 30 comments

Comments

@Nicogene
Copy link
Member

Nicogene commented May 3, 2018

I pulled opencv and yarpdataplayer and yarpdatadumper doesn't compile:

/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:418:30: error: use of undeclared identifier 'IplImage'
                int frameW=((IplImage*)itemEnd.obj->getPtr())->width;
                             ^
/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:418:39: error: expected expression
                int frameW=((IplImage*)itemEnd.obj->getPtr())->width;
                                      ^
/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:419:30: error: use of undeclared identifier 'IplImage'
                int frameH=((IplImage*)itemEnd.obj->getPtr())->height;
                             ^
/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:419:39: error: expected expression
                int frameH=((IplImage*)itemEnd.obj->getPtr())->height;
                                      ^
/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:428:52: error: use of undeclared identifier 'CV_FOURCC'
                videoWriter.open(videoFile.c_str(),CV_FOURCC('H','F','Y','U'),
                                                   ^
/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:429:38: error: use of undeclared identifier 'cvSize'
                                 fps,cvSize(frameW,frameH),true);
                                     ^
/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:457:49: error: use of undeclared identifier 'IplImage'
                    cv::Mat img=cv::cvarrToMat((IplImage*)item.obj->getPtr());
                                                ^
/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:457:58: error: expected expression
                    cv::Mat img=cv::cvarrToMat((IplImage*)item.obj->getPtr());
                                                         ^
/home/ngenesio/yarp/src/yarpdatadumper/main.cpp:457:37: error: no member named 'cvarrToMat' in namespace 'cv'
                    cv::Mat img=cv::cvarrToMat((IplImage*)item.obj->getPtr());

my opencv_version is 4.0.0-pre.
I'm afraid that in opencv 4 they will remove IplImage and cvarrToMat.
We should make yarp::sig::Image compatible with cv::Mat without taking account of IplImage.

@drdanz
Copy link
Member

drdanz commented May 3, 2018

It could be a good opportunity to get rid of the opencv integration inside YARP_sig, and to add a separate library YARP_opencv

@Nicogene
Copy link
Member Author

Nicogene commented May 3, 2018

They moved these classes in different headers or removed the inclusions from the "principal" headers.
Adding :

    #include <opencv2/core/types_c.h>
    #include <opencv2/videoio/videoio_c.h>

compiles 👍
Probably they did these changes to push people to move to cv:Mat, by the way let's wait the official release of OpenCV 4.0.0 scheduled for July 2018

@claudiofantacci
Copy link
Collaborator

Yes, they are getting rid/transferring C code in dedicated headers.
Anyway, I like the idea proposed by @drdanz and a clean way to integrate our image with opencv which I find somewhat convoluted.

@traversaro
Copy link
Member

traversaro commented Jan 29, 2019

@Nicogene can we change the name of the issue to "YARP does not compile with OpenCV 4" so it is clear what this issue is tracking? Thanks!

@traversaro
Copy link
Member

If the solution is simply add those includes if OpenCV 4 is used, probably it could make sense to add those lines to solve problems such as #1948?

@claudiofantacci
Copy link
Collaborator

If the solution is simply add those includes if OpenCV 4 is used, probably it could make sense to add those lines to solve problems such as #1948?

BTW, brew formula opencv@3 is out, we should update our macOS builds accordingly.

@Nicogene Nicogene changed the title OpenCV troubles YARP does not compile with OpenCV 4 Jan 29, 2019
@traversaro
Copy link
Member

cc @nunoguedelha

@Nicogene
Copy link
Member Author

Nicogene commented Jan 29, 2019

BTW, brew formula opencv@3 is out, we should update our macOS builds accordingly.

I agree to use opencv@3 formula, btw in #1948 there is an issue, the formula is keg-only, then actually in macOs builds opencv is not found and then not used.
@claudiofantacci if you do the PR to change the formula could you please also fix this issue?

@Nicogene
Copy link
Member Author

If the solution is simply add those includes if OpenCV 4 is used

Unfortunately it is not so simple, because what fixes the builds with opencv 4 breaks the builds with opencv 2, and unfortunately both in appveyor and travis(linux) there are old versions of opencv.

We should use a lot of #ifdef or decide to update the CI infrastructure and drop the support for opencv2

@claudiofantacci
Copy link
Collaborator

Unfortunately it is not so simple, because what fixes the builds with opencv 4 breaks the builds with opencv 2, and unfortunately both in appveyor and travis(linux) there are old versions of opencv.

We should use a lot of #ifdef or decide to update the CI infrastructure and drop the support for opencv2

Wait a moment, it breaks opencv3. We are not supporting code with opencv2, right?

@traversaro
Copy link
Member

I agree to use opencv@3 formula, btw in #1948 there is an issue, the formula is keg-only, then actually in macOs builds opencv is not found and then not used.

If a formula is keg-only, we should just properly define the env variables so it can be found, see https:/robotology/robotology-superbuild#system-dependencies-1 for Qt5 for example.

@claudiofantacci
Copy link
Collaborator

claudiofantacci commented Jan 29, 2019

It is keg-only because opencv (i.e. opencv4) is the most updated formula. If we install opencv@3 only, we can just force-link it with brew install --force opencv@3.

@Nicogene
Copy link
Member Author

Nicogene commented Jan 29, 2019

We are not supporting code with opencv2, right?

In theory no, in practice yes 😅 we have trusty on travis.
See : https://packages.ubuntu.com/source/trusty/opencv

@claudiofantacci
Copy link
Collaborator

In theory yes, in practice no 😅 we have trusty on travis.
See : https://packages.ubuntu.com/source/trusty/opencv

Travis now has xenial and by my understanding we no longer support trusty.
We could just update everything.

@nunoguedelha
Copy link
Contributor

It is keg-only because opencv (i.e. opencv4) is the most updated formula. If we install opencv@3 only, we can just force-link it with brew install --force opencv@3.

I guess you meant brew link --force opencv@3...

As a side note, the link to the files in "Cellar" folder is created as /usr/local/share/OpenCV and not /usr/local/lib/cmake/opencv4 as for the normal formula.
Anyway, CMake had no trouble finding it.

For me it's building just fine now.

@claudiofantacci
Copy link
Collaborator

claudiofantacci commented Jan 29, 2019

I guess you meant brew link --force opencv@3

There is a --force option for the install command as well that should do it at install time (we were talking about testing on Travis). I can be wrong, I can't test it atm.

@nunoguedelha
Copy link
Contributor

Actually I've tested. Even after uninstalling the opencv4, installing the opencv@3 as you suggested, it would not symlink it. You can always try, better two independent tests than one.

@claudiofantacci
Copy link
Collaborator

Ok, thanks! Good to keep in mind!

@lrapetti
Copy link
Member

lrapetti commented Apr 3, 2019

Actually I've tested. Even after uninstalling the opencv4, installing the opencv@3 as you suggested, it would not symlink it. You can always try, better two independent tests than one.

@nunoguedelha @claudiofantacci
I confirm brew install --force opencv@3 was not enough. I had to do brew unlink opencv, and brew link --force opencv@3 (without the unlink I was not able to link opencv@3 even if using --force).

@traversaro
Copy link
Member

Status update: the default opencv vcpkg port now builds opencv4 (see microsoft/vcpkg#5169 (comment)). While an opencv3 port is also still available, I will expect that the availability of opencv4 in vcpkg will mean more user will be impacted by this issue.

@drdanz
Copy link
Member

drdanz commented Aug 14, 2019

The main problem here is that debian/ubuntu don't have opencv4 yet (it is currently in debian experimental) therefore we are stuck with supporting opencv3 for a long time, but we can drop opencv2 as long as we drop support for debian stretch (oldstable)
It would be great if we could support opencv4 using only features that are also available in opencv3

@drdanz
Copy link
Member

drdanz commented Nov 14, 2019

OpenCV 4.1.3 landed in debian testing. This means that it will also be in the next Ubuntu LTS (20.04).
Therefore supporting OpenCV 4 is now a major issue. @Nicogene if I remember correctly, some time ago you started working on this, did you come to any conclusion?

@drdanz
Copy link
Member

drdanz commented Nov 14, 2019

OpenCV 4.1.3 landed in debian testing. This means that it will also be in the next Ubuntu LTS (20.04).

Just for the records, this means that OpenCV 3 will no longer be available on debian/ubuntu

@Nicogene
Copy link
Member Author

I started working on it some time ago, support both version 3 and 4 was a mess of #ifdef . We have to decide if we want to support only version 4 or both

@drdanz
Copy link
Member

drdanz commented Nov 14, 2019

I think we can drop OpenCV 2 since we have the new icub head image with debian buster, and therefore we can drop support for debian stretch, but we will need to support both OpenCV 3 and 4 for at least 2 more years

@drdanz
Copy link
Member

drdanz commented Nov 14, 2019

I disabled building YARP with OpenCV 4, since this will only cause compilation issues.

@Nicogene
Copy link
Member Author

With #2162 it compiles fine with both OpenCV 3 and 4.

The question is, we want to migrate to the c++ api and leave the old OpenCV 2 api?

cc @pattacini @vtikha @traversaro @drdanz @randaz81 @lornat75 @maggia80

@traversaro
Copy link
Member

The question is, we want to migrate to the c++ api and leave the old OpenCV 2 api?

cc @pattacini @vtikha @traversaro @drdanz @randaz81 @lornat75 @maggia80

I think we can merge #2162, and afterwards we can discuss if there is any advantage migrating to the new API.

@Nicogene Nicogene self-assigned this Dec 20, 2019
@pattacini
Copy link
Member

Agree with @traversaro
Anyway, my opinion is to get rid of the old C API. When this needs to be done is up to you.

@Nicogene
Copy link
Member Author

Nicogene commented Jan 7, 2020

Fixed by #2162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment