-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Implementation of Quasi Dense Stereo algorithm. #1941
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this algorithm should be added into stereo module instead of creation of new one.
CV_PROP_RW cv::Point2i p1; | ||
CV_PROP_RW float corr; | ||
|
||
CV_WRAP bool operator < (const Match & rhs) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ operators can't be wrapped directly.
Use CV_WRAP_AS(less)
to wrap under different name.
Yes, I can add it there, just tell me how do you want me to integrate it. For now, will try and remove the warnings that your build-bot exports. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migration to "stereo" module:
Extract public API part (which is going to be used by samples or high-level tests) from all headers and put it all into (opencv2/stereo/quasiDenseStereo.hpp). Merge documentation (documentation is generated from "public" headers (include/) only).
BTW, It is better to avoid case in filenames - quasi_dense_stereo.hpp
Include "opencv2/stereo/quasiDenseStereo.hpp" into "opencv2/stereo.hpp". Like this (for ximgproc module)
-
Merge .bib files and move tutorials
Update dependencies in stereo/CMakeLists.txt
opencv_imgcodecs - should not be used. Algorithms should accept cv::Mat, instead of file names. Most part of image data comes from cameras.
Avoid entries duplication in OPTIONAL
section.
-
Bindings integration can be done later (current state probably doesn't work well and there is Python sample). Just don't use
WRAP
or_W
variants of macros.CV_EXPORTS
is enough. -
It would be nice to add some minimal test for primary API.
|
||
PropagationParameters Param; | ||
|
||
protected: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split interface (include/) and implementation (src/).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand how you want me to change the structure. I think that this version of the class is already split. Could you please provide me with more information on how you want me to split the class?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of algorithms use following scheme:
// hpp
class Algo {
public:
virtual void doSomething() = 0;
virtual void setSomeProperty() = 0;
virtual float getSomeProperty() = 0;
...
// no protected or private members
// no class fields
};
// can be defined inside the class - "static Ptr<Algo> Algo::create(...)"
static Ptr<Algo> createAlgo(...);
// cpp
class AlgoImpl : public Algo {
// implement all methods
};
Ptr<Algo> createAlgo(...)
{
return makePtr<AlgoImpl>(...);
}
@@ -0,0 +1,18 @@ | |||
#ifdef HAVE_OPENCV_QDS | |||
#include "opencv2/core/saturate.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why saturate header is needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, I had trouble wrapping for python and I copied a snippet from pyopencv_linemod.hpp without checking if the header is needed.
Thank you for your input. I made changes to qds module. If your build-bot won't output any errors/warnings, I will start the integration to stereo module the way you described it. |
* in case of video processing. | ||
* @sa loadParameters | ||
*/ | ||
CV_EXPORTS virtual int loadParameters(cv::String filepath="") = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove CV_EXPORTS from members. It is already applied for whole class.
I will fix the errors within the next hour and I will upload it again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
Please take a look on comments below.
modules/stereo/doc/stereo.bib
Outdated
isbn="978-3-642-15705-9" | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove extra space before "}" and extra empty lines
|
||
}; | ||
|
||
typedef std::priority_queue<Match, std::vector<Match>, std::less<Match> > t_matchPriorityQueue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used in public API. Move to "src/" private headers.
Move #include <queue>
too.
* in case of video processing. | ||
* @sa loadParameters | ||
*/ | ||
virtual int loadParameters(cv::String filepath="") = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=""
default value is not needed here
* @note This method can be used to generate a template file for tuning the class. | ||
* @sa loadParameters | ||
*/ | ||
virtual int saveParameters(cv::String filepath="./qds_parameters.yaml") = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove default value too
virtual cv::Mat getDisparity(uint8_t disparityLvls=50) = 0; | ||
|
||
|
||
static cv::Ptr<QuasiDenseStereo> create(cv::Size monoImgSize, cv::String paramFilepath =""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= cv::String()
@@ -0,0 +1,234 @@ | |||
/* | |||
By downloading, copying, installing or using the software you agree to this license. | |||
If you do not agree to this license, do not download, install, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use short OpenCV license header:
// This file is part of OpenCV project.
// It is subject to the license terms in the LICENSE file found in the top-level directory
// of this distribution and at http://opencv.org/license.html.
https://github.com/opencv/opencv/wiki/Coding_Style_Guide
You may add "author" information after these 3 lines.
- Compute dense Stereo correspondences. | ||
|
||
----------- | ||
[Source Code](../samples/dense_disparity.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link is broken: http://pullrequest.opencv.org/buildbot/export/pr_contrib/1941/docs/d3/d7d/quasi_dense_stereo.html
Use @include or @snippet or GitHub link (opencv/opencv master branch)
``` | ||
Mat rightImg, leftImg; | ||
leftImg = imread("./imgLeft.png", IMREAD_COLOR); | ||
rightImg = imread("./imgRight.png", IMREAD_COLOR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use @snippet
instead of code duplication.
{ | ||
Match tmpMatch; | ||
dMatches.clear(); | ||
// dMatches.resize(dMatchesLen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dMatches.reserve
… tutorials in new directories. Modify samples and tutorials to use snippet and include Doxygen commands.
…DMatch build-in type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
/* | ||
By downloading, copying, installing or using the software you agree to this license. | ||
If you do not agree to this license, do not download, install, | ||
copy or use the software. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the short license header here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
@dimitrisPs |
@ZhuLingfeng1993 |
@dimitrisPs |
@ZhuLingfeng1993 |
I have implemented a class that establishes dense pairwise correspondences from a stereo image pair, and I wish to contribute it to the official repository. The class extracts a sparse set of correspondences from a stereo image pair using pyramidal Lucas-Kanade Flow algorithm based on Shi and Tomasi features, this way the class can process non-rectified images. To interpolate a dense set of correspondences the class uses the Quasi Dense Stereo algorithm proposed by M. Lhuillier and Long Quan et. al. [https://ieeexplore.ieee.org/document/905620].
Please let me know if you have any further questions or comments.
Thank You