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

Adding basic types for OpenCV #53

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

mtourne
Copy link

@mtourne mtourne commented Feb 14, 2014

Some of the basic types for OpenCV
The mat type is binary and won't archive well with non-binary archivers

OpenCV's FileStorage api still might be a bit more convenient to use (supplying matrixes by hand)
But this can be useful.

@randvoorhies
Copy link
Contributor

Looks nice! We do need to be a little careful about not adding in serialization functions for every library out there, but I think OpenCV is of wide enough interest that it's probably worth having this support included in cereal. If we do accept the pull, then someone will need to go through and do a more thorough sweep of OpenCV to add in as much useful stuff as possible (e.g. cv::RotatedRect_, cv::Scalar_, cv::Range, etc, etc, etc).

@AzothAmmo
Copy link
Contributor

What about having some kind of easily accessible modules, such as OpenCV, Boost, etc, that people could download and plug into cereal? Things like this, even if we keep the number small, add more clutter if someone doesn't need it. We could potentially split things like that off to other repositories since I'm not sure if there is a great way to do this with git (but I'm no git guru).

As far as the code goes, it's a good start but we need unit tests and fixed size types for any serialization, and definitely can't have any debug statements within the code by the time it reaches us. Basically it needs to look like one of our existing std type files.

@mtourne
Copy link
Author

mtourne commented Feb 14, 2014

I guess I was a bit quick on the pull request button, this debug statement shouldn't be in there.

Fixed.

Some of the basic types for OpenCV

The mat type is binary and won't archive well with non-binary archivers
@Devacor
Copy link

Devacor commented Feb 19, 2014

I prefer modules personally. I don't plan on using OpenCV and haven't even heard of it until just now. Seems like cruft to add it directly to the project, but it could be exceptionally useful to people who use it if made available in a more independent way.

But, cool to see development from others! It's exciting.

@AzothAmmo AzothAmmo added this to the v1.1.0 milestone Mar 9, 2014
@AzothAmmo AzothAmmo modified the milestones: v1.2.0, v1.1.0 Apr 18, 2014
@AzothAmmo
Copy link
Contributor

Pushing this to a later release than 1.1 - this isn't something that'd be part of cereal anyway, but rather a module that would be compatible with cereal. My idea here is either to have a collection of repositories or a single repositories with the various modules available in it.

@mattyclarkson
Copy link
Contributor

I'd prefer modules, we use OpenCV here but I could then easily contribute and maintain my own open source bindings separate to the project.

@patrikhuber
Copy link
Contributor

I also wrote cereal serialisation functions for OpenCV a few months ago and I have a few remarks regarding this PR:

This pulls in all kinds of headers from OpenCV for cv::DMatch and cv::KeyPoint. These should be separated from what is in OpenCV core (i.e. cv::Mat, cv::Vec, etc.). Ideally we'd have a cereal module for each of the OpenCV modules that require serialisation, but I think it's enough to just serialise the types in core for a start. Certainly we shouldn't require the user to pull in modules others than core, so these additional types should be removed from the PR.

OpenCV has a proxy type for its data structures, OutputArray and InputArray. I'm not sure whether they can or should be used, but we should think about it. (one reason against it would be that for example std::vector<cv::Vec3f> is an InputArray as well, but in this case we wouldn't want OpenCV to serialise it but rather use cereal/types/vector.hpp to serialise the std::vector and our OpenCV serialisation to serialise the cv::Vec3f's).

The serialisation for cv::Mat in this PR is not as simple as it could be - it's sufficient to serialise mat.rows, mat.cols, mat.type() and mat.isContinuous(). You can find my implementation in my blog: http://www.patrikhuber.ch/blog/6-serialising-opencv-matrices-using-boost-and-cereal which in my opinion is much simpler.

Seems to me like you shouldn't use CV_IS_MAT_CONT (deprecated C-API macro).

Also, in my opinion, supporting XML serialisation for cv::Mat doesn't make sense, because the files get very large very quickly, and if you look at them, each matrix entry will be inside its own XML attribute, so it's not really "human-readable" anyway. So I would remove _CEREAL_NVP. (But in case you want to keep it, I think you should use cereal::make_nvp instead).

Interestingly, I believe in your implementation if you save a non-continuous matrix, it will have continuous = true after loading it. We should think about whether we want this behaviour.

@AzothAmmo AzothAmmo modified the milestones: v1.2.0, v1.3.0 Sep 28, 2015
}
}

#endif // !CEREAL_TYPOS_OPENCV_HPP_
Copy link

Choose a reason for hiding this comment

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

I assume TYPOS should be TYPES?

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

Successfully merging this pull request may close these issues.

7 participants