Rework detector API and implement Harris detector
ClosedPublic
Actions

Authored by Sergey Sharybin (sergey) on Jan 24 2014, 2:58 PM.

Details

Reviewers

Keir Mierle (keir)

Commits

rLMVca15262cf07a: Rework detector API and implement Harris detector

Summary

Switch the detector API to a single function which accepts
a float image and detector options. This makes usage of
feature detection more unified across different algorithms.

Options structure is pretty much straightforward and contains
detector to be used and all the detector-specific settings.

Also implemented Harris feature detection algorithm which
is not as fast as FAST one but is expected to detect more
robust feature points.

Diff Detail

Branch: harris_detector

Event Timeline

Sergey Sharybin (sergey) added a reviewer: Keir Mierle (keir).Jan 24 2014, 2:58 PM

Nice! This will take a bit to review.

Keir Mierle (keir) requested changes to this revision.Jan 24 2014, 7:35 PM

Keir Mierle (keir) added inline comments.

src/libmv/simple_pipeline/detect.cc
51	This belongs in image/somewher.h Also doesn't this function exist already?
53	Line continuatios indent 2 more
67	Perhaps document that this is O(n^2) for now. Maybe consider doing a grid if speed is an issue.
83–104	Why do this? Why not just loop over all features? I think this mehod is slower and is harder to understand since it is not obvious why a priority features array is needed. Maybe you just want to sort? Or at least comment about this.
187	indentation
197	"data"?? Surely you can make a better name.
src/libmv/simple_pipeline/detect.h
55–56	Dind? Add TODO for documentation; it's not clear what the contents of this array is. Is it a null-terminated C string? Prefer to use std::string instead in that case. O
56	moravec_pattern?
58	harris_threshold? I'd like to have different names for the thresholds if they are in different units. e.g. the FAST threshold != harris threshold
59	fast_min_trackness?
61	Why is this inside the options? That's a bit confusing. Why not make this an inner struct in Detector like we do in Ceres?
src/ui/tracker/main.cc
315–323	Wow this old thing compiles!

Address code review from Keir

src/libmv/simple_pipeline/detect.cc
51	Yes, think better place is image/image_converter.h (which does exist already). And no, didn't see such function actually. Could be blind tho, wouldn't mind if you doublecheck on this..
53	Done.
67	Documented. Speed wasn't an issue so far. Not sure which grid you're talking about. We could do some KD-tree or AABB-tree to speed things up for sure.
83–104	This is to sort the features. Didn't want to sort them in a vector passed to this function so callee function could use all_features after this call. And since i'm doing a copy anyway didn't see a reason why not to use priority queue for such a thing. If implementation doesn't suck, it'll be the same asymptotic as sorting the vector. You've got more experience in c++ and could think of better things to do here. I'm all ears :) P.S. Added a comment here.
187	Done.
197	Made it `byte_image`. Is it good enough now?
src/libmv/simple_pipeline/detect.h
55–56	`Find` Added some info about what a hell this pattern is. IMO, moravec detector is the less thought thing here, but would rather leave refactoring it to another day. It wasn't so much useful on practice. Even FAST used to do detect much better features...
56	Guess it is to be `moravec_max_count`?
56	Done.
58	Sure. Good point. Absolutely agreed :)
59	Done.
61	Not sure what exactly this means.
src/ui/tracker/main.cc
315–323	Yes, trying to keep it working.. Wouldn't mind doing bigger house cleanup here tho.

@Keir Mierle (keir), Thanks for the review. Think addressed all the current notes from you.

Could you see any issues in the code of new Harris detector?

Keir Mierle (keir) accepted this revision.Jan 28 2014, 9:06 AM

Keir Mierle (keir) added inline comments.

src/libmv/simple_pipeline/detect.cc
51	image_converter.h works for me. You can move it in a followup diff if you like.
197	Sounds good.
256	Consider using a scoped_array for this and scores. Also perhaps just use a vector...
src/libmv/simple_pipeline/detect.h
60	Maybe Type? or DetectorType?