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

Dense sift #442

Open
wants to merge 13 commits into
base: 4.x
Choose a base branch
from
Open

Dense sift #442

wants to merge 13 commits into from

Conversation

YuvalNirkin
Copy link

I have added a generalized optical flow algorithm called SIFT flow that can work on images with the same scene characteristics.

Link to the article:
http://people.csail.mit.edu/celiu/SIFTflow/

@PhilLab
Copy link
Contributor

PhilLab commented Nov 16, 2015

thanks for the contribution! Unfortunately there is a merge conflict in modules/optflow/CMakeLists.txt

@StevenPuttemans
Copy link

I am just wondering, since it is a Microsoft Research New England publication, if the proposed approach is actually mergeble under the BSD license?

@YuvalNirkin
Copy link
Author

I will try solving the merge conflict later this week.

I didn't check with them about the license, but because I am using the SIFT descriptor, everything has to be in the nonfree module anyhow.

@cbalint13
Copy link
Contributor

@YuvalNirkin , This is very nice work !

Regarding the license as @StevenPuttemans noticed i am looking at: http://people.csail.mit.edu/celiu/ECCV2008/release.zip original matlab demonstrator, i believe you followed that implementation (but i might be wrong, you might followed the raw paper).

  • Implementation in this PR seems to use opencv's own xfeatures2d::SIFT, so that absolve license.
  • Unfortunate is that in all the original files there is no license (except computeColor.m, BSD).
  • I believe author can be contacted and kindly asked if agree with BSD license for your OpenCV implemetation work here (but i am not a lawyer, usually i did this way for similar opencv PRs).

For citations better would be to add them inside optflow/doc/optflow.bib among all citations present there and just reference them in your top nonfree.hpp header using @cite pragma.


My few +1 (pure opinions, with my best positive intentions here):

  • I wonder how descriptor computation part could be generalized / externalized to allow the very user choose other dense descriptors like AKAZE, SURF or DAISY in that compute() function. But this may be only at some future enhancement, maybe after this PR is passed. Such externalisation would allow to get rid of "nonfree" status in future.
  • I believe some of those += math accumulations within double for loops could be parallelized with opencv's parallel_reduce_ framework, or in other nonaccumulative cases parallel_for_ would do it. But even in this state the code looks great (in my very opinion).

Again very nice work ! Wonder if i or someone could pickup author's next one brilliant co-work on same subject: http://vision.cs.utexas.edu/projects/dsp , had it in my mind myself , and now you really opened the path to it with present SIFTflow.

@cbalint13
Copy link
Contributor

@YuvalNirkin , @StevenPuttemans

  • Got the permission from the author Ce Liu for SIFT_flow, so no more license or ethical problem.
  • @YuvalNirkin , would you mind helping you out by adding me as collaborator for your branch ?

I would try enhance the code in the directions proposed by me.

@YuvalNirkin
Copy link
Author

@StevenPuttemans
I also got permission for Ce Liu.I quote: "There is no official license for SIFT flow. At MIT, we just did something for the common good of humanity, which I think is more important." Good guy :)
@cbalint13
I added you as a collaborator. I'll be happy for some help, I don't have too much time on my hands.
I designed the implementation to be as generic as possible to support different descriptors.
I was planning on adding support for the LATCH descriptor first. Actually the only change needed is to replace the distance function for the data term from L1 to Hamming distance, which is only 1 line of code.

@cbalint13
Copy link
Contributor

@YuvalNirkin ,

Thank You Very Much ! Will do the proposed cosmetic and parallelization. For descriptor part i'll try to an universal api so user just pass a Ptr class with his favorite descriptor. The class can be asked if hamming/euclid type so we can decide upon metric.

Lets see together.

@StevenPuttemans
Copy link

@YuvalNirkin @cbalint13 still considering to fix this and getting it in?

@YuvalNirkin
Copy link
Author

There is a merge conflict that I don't know how to fix in modules/optflow/CMakeLists.txt.
My modules/optflow/CMakeLists.txt:
set(the_description "Optical Flow Algorithms") ocv_define_module(optflow opencv_core opencv_imgproc opencv_video opencv_highgui opencv_ximgproc opencv_features2d opencv_xfeatures2d opencv_calib3d WRAP python)

The master branch's modules/optflow/CMakeLists.txt:
set(the_description "Optical Flow Algorithms") ocv_define_module(optflow opencv_core opencv_imgproc opencv_video opencv_highgui opencv_ximgproc WRAP python)

Maybe somebody can help with that..

@StevenPuttemans
Copy link

Could you try adding

set(the_description "Optical Flow Algorithms") ocv_define_module(optflow opencv_core opencv_imgproc opencv_video opencv_highgui opencv_features2d opencv_calib3d opencv_xfeatures2d opencv_ximgproc WRAP python)

which adds the normal libs first and then the contribs one. I am guessing it has to do with some sort of conflict there.

@StevenPuttemans
Copy link

Or you could create a clean local branch and force push it on top of this with the same name, to make sure it is not a hidden character screwing it up.

@Dikay900
Copy link
Contributor

Dikay900 commented Apr 5, 2016

You have to take the master version of the cmakefile.
With a rebase you can add your changes after merging with master.

@YuvalNirkin
Copy link
Author

@cbalint13, @StevenPuttemans, @Dikay900
Thank you for the tips, I managed to solve the merge problem.

Now on Windows machines I get that 'Eigen/Sparse' header can't be found.
I don't get this error on my Windows machine.
This is crucial for solving the sparse linear system for the implementation of this paper:
http://www.openu.ac.il/home/hassner/projects/scalemaps/TauHassner(TPAMI).pdf

How can i overcome this problem?

@StevenPuttemans
Copy link

My best guess is that the Eigen version of OpenCV is lower then 3.2.8 (https://eigen.tuxfamily.org/dox/group__TutorialSparse.html) and thus it does not contain this yet ... But to be sure we need some admin to confirm this. @alalek can you confirm this might be the case?

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

Successfully merging this pull request may close these issues.

None yet

5 participants