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

Add opencv compatibility library #1932

Merged
merged 6 commits into from
Dec 28, 2018
Merged

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented Dec 7, 2018

In late 2018 we have finally a easy way that allow to get a cv::Mat from a yarp::sig::ImageOf 🎉

Since there is no way to retrieve the Pixel code from the cv::Mat and the default coding is BGR, this library assumes that when converting to yarp image, the cv::Mat has to be BGR, and the cv::Mat obtained from the reversed conversion is BGR.

Converting from OpenCV to Yarp no copies are involved.
In the other direction is quite tricky to understand when OpenCV performs a copy and when not 😕

@drdanz and me found a sort of rule behind their choice to copy or not, but maybe it is not important.
The argument of toCvMat() is a && a.k.a a right-reference, this means that after this function call the integrity of the data of the input yarp image is no longer guaranteed. Since we can't actually guarantee it 😅

It can be used easily in cmake projects as cv component:

find_package(YARP REQUIRED
                  COMPONENTS OS
                             conf
                             sig
                             dev
                             cv)

Example of usage:

#include <yarp/cv/Cv.h>
#include <utility>    // for std::move()

yarp::sig::ImageOf<yarp::sig::PixelRgb> inImg;
// Read it from port
cv::Mat cvImg = yarp::cv::toCvMat(std::move(inImg));
// Some OpenCV processing
auto yarpImg = yarp::cv::fromCvMat<yarp::sig::PixelRgb>(std::move(cvImg));
//  Write on a port

Lastly, this PR deprecates getIplImage and wrapIplImage since they are now obsolete.

It is super WIP and all the suggestions of the CV guys are more than welcome.

It fixes #1672.

It has been tested with different pixel codes (PixelRgb, PixelRgba, PixelMono, PixelBgr, PixelBgra), and it works 😎

Please review code

@Nicogene Nicogene added PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Component: Library - YARP_sig PR Status: Changelog - OK This PR has a proper changelog entry Target: YARP v3.2.0 labels Dec 7, 2018
@traversaro
Copy link
Member

The argument of toCvMat() is a && a.k.a a right-reference, this means that after this function call is not guarantee the integrity of the data of the input yarp image. Since we can't actually guarantee it 😅

I guess this is part of what && means, but given that is a relatively unknown constructor among YARP users (that sometimes are not exactly C++ super-experts) I think this is a comment that it may be worth make in the function docs.

@drdanz
Copy link
Member

drdanz commented Dec 7, 2018

Regarding the BGR/BGRA discussion we had earlier, I think that, in order to avoid data loss, the user should receive a Mat that is of the same type as the default for that pixel type, i.e. BGR for types mapping CV_8UC3 (RGB, BGR), BGRA  for types mapping CV_8UC4 (RGBA, BGRA), and whatever is the default for all the other types.

The user can eventually perform the conversion he needs of the types is not the one he needs.

The same is valid for the CV to yarp conversion, i.e. we can assume that the type is the default for the type of the Mat (i.e. BGR for CV_8UC3, BGRA for CV_8UC4)

@drdanz
Copy link
Member

drdanz commented Dec 7, 2018

The argument of toCvMat() is a && a.k.a a right-reference, this means that after this function call is not guarantee the integrity of the data of the input yarp image. Since we can't actually guarantee it 😅

The same should be valid for the fromCvMat

@drdanz
Copy link
Member

drdanz commented Dec 7, 2018

Actually, I think this is a good pattern for all the compatibility libraries, we should add the right reference overload everywhere, and deprecate the const reference ones. Also because the getRawImage() const method should return a const...

Copy link
Collaborator

@claudiofantacci claudiofantacci left a comment

Choose a reason for hiding this comment

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

Just some minor comments and suggestions!

Thanks! 🎉

@claudiofantacci
Copy link
Collaborator

Travis builds with TRAVIS_NO_DEPRECATED = TRUE fail compiling YARP Image implementation.

@pattacini
Copy link
Member

pattacini commented Dec 15, 2018

I believe it would be convenient to merge this PR before the holidays so that we (at least I) could employ the Christmas break to update the user code.

@Nicogene
Copy link
Member Author

Nicogene commented Dec 17, 2018

Unfortunately we reached a sort of API impasse, maybe someone can help us to overcome it 😉

Here is the problem.

Since no copy is involved, the memory is shared between cv::Mat and the yarp Image, and conversions(data modification) can be involved.
For both conversion functions:

  • ❌ The argument can't be const &. The compiler doesn't complain but if I pass a const & I expect that the data is not changed by the function, but it is actually changed.
  • ❌ Pass by && has the problem that we don't have move constructor/operator for yarp::sig::ImageOf, and honestly I don't know if it is implemented in opencv(probably yes).
  • ✔️ pass by copy is inefficient, but it has no pitfalls .

What do you think about it?

@drdanz @traversaro @claudiofantacci @pattacini @vtikha

The PR is ready once we solve this API issue!

@Nicogene Nicogene changed the title [WIP] Add opencv compatibility library Add opencv compatibility library Dec 17, 2018
@Nicogene
Copy link
Member Author

Discussing with @claudiofantacci and @aerydna with decided to use & not const as argument.

@claudiofantacci
Copy link
Collaborator

@Nicogene thanks for the changes. I think they are ok, however test are failing because there are some things to be fixed in YARP. Are you also fixing them (like the yarpdataplayer)?

@Nicogene
Copy link
Member Author

My hope was to port yarpdataplayer to the new functions in a second iteration, but unfortunately the build YARP_NO_DEPRECATED are broken.
I will port the code as soon as possible 👍

@Nicogene
Copy link
Member Author

yarpdataplayer ported and tested 💪

image

YARP_NO_DEPRECATED builds fixed.

As soon as the tests passes we can merge from my side.

@traversaro
Copy link
Member

It is always good to see good old @tanismar , even if just in a dataset. : )

@Nicogene
Copy link
Member Author

Tests are now passed, we can merge it as soon as this PR is approved

@pattacini
Copy link
Member

@Nicogene there's also yarpdatadumper that needs to be updated.

@Nicogene
Copy link
Member Author

For the yarpdatadumper there is this 2e2e40d commit, but I didn't tested it actually. Is it enough that commit in your opinion?

@pattacini
Copy link
Member

pattacini commented Dec 21, 2018

Actually, there are references to IplImage* that I expect need to be addressed.

@pattacini
Copy link
Member

pattacini commented Dec 21, 2018

Also, I would test the functionality carefully before merging.

@Nicogene
Copy link
Member Author

I successfully dumped with yarpdatadumper the images streamed by the yarpdataplayer.

Copy link
Collaborator

@claudiofantacci claudiofantacci left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @Nicogene 🎉

@drdanz drdanz merged commit 54c7262 into robotology:devel Dec 28, 2018
@pattacini
Copy link
Member

pattacini commented Dec 28, 2018

⚠️ Caveat

This PR warns the user that wrapIplImage() is deprecated in favor of fromCvMat(). That's not strictly true since the former function works on IplImage* and not on cv::Mat.

Then, the proper conversion to help get around this should be:

IplImage * ipl = ...;
cv::Mat m = cv::cvarrToMat(ipl); 

As I've heard cvarrToMat() is going to disappear in latest OpenCV releases, we would definitely need to take our time to upgrade the user code (still functional) relying on those legacy OpenCV C-like routines producing/handling IplImage* objects (e.g. cvCreateImage(), cvReleaseImage()...) in favor of their modern C++-like counterparts that instead deal directly with cv::Mat objects.

The sore point here is that this upgrade can be quite cumbersome, requiring a thorough testing stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Library - YARP_sig PR Status: Changelog - OK This PR has a proper changelog entry PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Resolution: Merged Target: YARP v3.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants