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 RIC method for sparse match interpolation #2367

Merged

Conversation

tsenst
Copy link
Contributor

@tsenst tsenst commented Nov 29, 2019

  • commit implementation of RIC interpolation method (from "Robust Interpolation of Correspondences for Large Displacement Optical Flow" Yinlin et. al 2017)
  • integrate RIC interpolation to DenseRLOFOpticalFlow class and functions
  • add RLOF_EPIC and RLOF_RIC to (sample)optical_flow_estimation project
  • implement interface to allow appling better edge estimators (e.g. SED) for edge-awar cost map computation that is used in the EdgeAwareInterpolator and RICInterpolator

tsenst and others added 22 commits July 9, 2019 09:46
…n of the maximal number of matches to be interpolated from SHORT_MAX to INT_MAX by substituting short by int types
sparse_match_interpolators EdgeAwareInterpolation - enhance limitatio…
* add test for RIC
* add ASSERTION for curr image
* fix warnings
* fix warnings
Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for contribution!

@@ -1,2 +1,2 @@
set(the_description "Extended image processing module. It includes edge-aware filters and etc.")
ocv_define_module(ximgproc opencv_core opencv_imgproc opencv_calib3d opencv_imgcodecs WRAP python java)
ocv_define_module(ximgproc opencv_core opencv_imgproc opencv_calib3d opencv_imgcodecs opencv_tracking WRAP python java)
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to keep dependency list minimal - adding tracking here, this would block adding ximgproc as dependency into tracing module.

Do we really need this dependency?

Copy link
Contributor Author

@tsenst tsenst Dec 2, 2019

Choose a reason for hiding this comment

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

Thank you for that comment. In fact tracking was not needed anymore but the video module which has been changed

modules/ximgproc/src/sparse_match_interpolators.cpp Outdated Show resolved Hide resolved
/** @copybrief setK
* @see setK
*/
CV_WRAP virtual int getK() const = 0;
Copy link
Member

Choose a reason for hiding this comment

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

please remove extra double spaces here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double spaces have been removed

@@ -201,13 +207,13 @@ void EdgeAwareInterpolatorImpl::interpolate(InputArray from_image, InputArray fr
if(use_post_proc)
fastGlobalSmootherFilter(src,dst,dst,fgs_lambda,fgs_sigma);

costMap = Mat();
Copy link
Member

Choose a reason for hiding this comment

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

costMap.release(); should do the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry. Got the wrong line of code in line 1306 was the same issue

Copy link
Member

Choose a reason for hiding this comment

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

There are two extra cases:
= Mat(); => .release();

modules/ximgproc/src/sparse_match_interpolators.cpp Outdated Show resolved Hide resolved
Comment on lines 1228 to 1230
CV_Assert(!from_points.empty() && from_points.isVector() &&
!to_points.empty() && to_points.isVector() &&
from_points.sameSize(to_points));
Copy link
Member

Choose a reason for hiding this comment

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

Avoid using of complex assertion statements - they emits over-complicated unclear messages to Users.

CV_Assert(!from_points.empty() && from_points.isVector());
CV_Assert(!to_points.empty() && to_points.isVector());
CV_Assert(from_points.sameSize(to_points));

Their messages helping to localize parameters mistakes much better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

private:
inline void swap(float & a, float & b)
Copy link
Member

Choose a reason for hiding this comment

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

Why is not std::swap()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

swap function has been removed

modules/ximgproc/src/sparse_match_interpolators.cpp Outdated Show resolved Hide resolved
tsenst added 4 commits December 3, 2019 14:22
* underscore from parameters
* substitute parallel_for_() classes with lambda functions
* remove complex assertion statements
* remove dead code
* substitute swap function with std::swap()
* ocv_define_module now contains video instead of tracking module
Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you for update!

@@ -201,13 +207,13 @@ void EdgeAwareInterpolatorImpl::interpolate(InputArray from_image, InputArray fr
if(use_post_proc)
fastGlobalSmootherFilter(src,dst,dst,fgs_lambda,fgs_sigma);

costMap = Mat();
Copy link
Member

Choose a reason for hiding this comment

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

where?

int end = std::min(range.end * stripe_sz, match_num);
nodeHeap q((int)match_num);
int num_expanded_vertices;
unsigned char* expanded_flag = new unsigned char[match_num];
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid manual memory management. Use cv::AutoBuffer<unsigned char> or std::vector instead.


geodesicDistanceTransform(matDistanceMap, costMap);

g = new vector<node>[match_num];
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid manual memory management.

}
});

//parallel_for_(Range(0, iterCnt), GenerateHypothesis_ParBody(*this, &bestCost[0], outModels, supportCnt, spCnt, spNN, &supportMatchIds[0], &supportMatchDis[0], inputMatches, inLierFlag));
Copy link
Member

Choose a reason for hiding this comment

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

dead code?

exit()
#if len(sys.argv) != 2:
# print('Input video name is missing')
# exit()
Copy link
Member

Choose a reason for hiding this comment

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

please rollback unrelated changes here and below

/** @copybrief setSuperpixelRuler
* @see setSuperpixelRuler
*/
CV_WRAP virtual float getSuperpixelRuler() const = 0;
Copy link
Member

Choose a reason for hiding this comment

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

please remove double space

modules/ximgproc/src/sparse_match_interpolators.cpp Outdated Show resolved Hide resolved
{
RICInterpolatorImpl *eai = new RICInterpolatorImpl();
eai->init();
return Ptr<RICInterpolatorImpl>(eai);
Copy link
Member

Choose a reason for hiding this comment

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

Memory leak is possible in case of exception in "init".
Don't use raw pointers:

auto eai = makePtr<RICInterpolatorImpl>();
eai->init()
return eai;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for that hint!

tsenst added 2 commits December 4, 2019 22:59
*  remove double space
* use static for inner functions
* update create function
* modify mem init. to avoid manual memory management
tsenst added 2 commits December 5, 2019 10:39
if (m_validSize >= m_size)
{
CV_Error(CV_StsOutOfRange, " m_validSize >= m_size this problem can be resolved my decreasig k parameter");
};
Copy link
Member

Choose a reason for hiding this comment

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

; is not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has been removed

int *label_row;
int *label_row_prev;

#define CHECK(cur_dist,cur_label,cur_cost,prev_dist,prev_label,prev_cost,coef)\
Copy link
Member

Choose a reason for hiding this comment

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

CHECK

Please avoid using common names for macros (see this issue about lowercase check).
Consider adding some prefix/suffix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change CHECK to CHK_GD

Comment on lines 1492 to 1493
//
vector<int> diffPairs(labels.cols*_labels.rows * 4, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove empty comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -201,13 +207,13 @@ void EdgeAwareInterpolatorImpl::interpolate(InputArray from_image, InputArray fr
if(use_post_proc)
fastGlobalSmootherFilter(src,dst,dst,fgs_lambda,fgs_sigma);

costMap = Mat();
Copy link
Member

Choose a reason for hiding this comment

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

There are two extra cases:
= Mat(); => .release();

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Well done 👍

tsenst added 2 commits December 8, 2019 13:43
* change CHECK to CHK_GD
* remove not necessary ;
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.

None yet

2 participants