Details

Reviewers

Commits

rLMVee21415a3533: Support multiple distortion models, including a new division model
rLMV615f610df7aa: Support multiple distortion models, including a new division model

Summary

This commit makes it so CameraIntrinsics is no longer hardcoded
to use the traditional polynomial radial distortion model. Currently
the distortion code has generic logic which is shared between
different distortion models, but had no other models until now.

This moves everything specific to the polynomial radial distortion
to a subclass PolynomialDistortionCameraIntrinsics(), and adds a
new division distortion model suitable for cameras such as the
GoPro which have much stronger distortion due to their fisheye lens.

This also cleans up the internal API of CameraIntrinsics to make
it easier to understand and reduces old C-style code.

Diff Detail

Repository: rLMV Libmv
Branch: distortion_models

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Made bundler and keyframe selector use generic CameraIntrinsics in the API

Added virtual destructor to solve underfined behavior of deletion

Some fixes and implemented divisions distortion model

@Keir Mierle (keir), am i right the terminology is that all this models
represents radial distortion and just uses different formulas
for this (meaning, that RadialDistortion is actually a
PolynomialDistortion) ?

Fixed a typo in buffer undistortion

Fix bug in reset lookup grid which lead to read/write to a freed memory

Fixed some issues with bundle adjuster

Just rebased on top of latest devel

Keir Mierle (keir) requested changes to this revision.Feb 28 2014, 10:47 PM

Keir Mierle (keir) added inline comments.

src/libmv/simple_pipeline/bundle.cc
431	use && instead of nesting
src/libmv/simple_pipeline/camera_intrinsics.h
67	Could be its own class.
69	Document this struct; what is it?
72	Could be its own class.
78	Document.
85	I am tempted to suggest the warp grid functionality should be in its own class.
87	"Check" is not a useful name. Perhaps "LookupGridNeedsUpdating"? "LookupGridSynced?" It's actually a bit confusing since the relationship between the grid and the intrinsics is not documented.
111	Document T; what is type T? What is overscan? document.
161	Instead of "Grid" being a struct; perhaps the warping code can go into this object? It seems like focal length instrinsics are needlessly coupled in this object, and a warp grid is a nice object all by itself.
src/libmv/simple_pipeline/camera_intrinsics_impl.h
73	ick this
134	80 columns
src/libmv/simple_pipeline/distortion_models.cc
107	Intended?
142	HAHAHA we are using my old school solver! bwahahahahah. Ceres is much better but I wonder if it is slower due to overhead.

Sergey Sharybin (sergey) added inline comments.Mar 4 2014, 6:04 PM

src/libmv/simple_pipeline/camera_intrinsics.h
69	This is rather weird offset for the lookup grid. Basically (ix, iy) is the integer part of the offset, (fx, fy) is the float part of the offset (well, float offset is actually fx / 255.0). I was never happy with this crap. Originally it was using char for all the components and for sure that sucked a lot because it didn't work for high resolutions (you had overflows). The thing now is -- sizeof(float) is not so much bigger than sizeof9short)+sizeof(char). So i would rather suggest using floats instead of such "optimization". Using floats would also reduce number of int->float conversions (which are rather slow) when applying lookup grid. Thoughts?
87	What it does is `EnsureLookupGridIsUpToDate`. Can you think of shorter name for this functionality? It was always confusing design over here :S
161	Didn't actually understand "focal length instrinsics are needlessly coupled in this object".
src/libmv/simple_pipeline/camera_intrinsics_impl.h
73	Whaaat? :)
134	This is weird code which i guess was tempted to be fast using that offset magic but in fact because of bunch int->float coversions i don't thin it worth being so cryptic here anyway. Read the reply about Offset structure.
src/libmv/simple_pipeline/distortion_models.cc
142	Yes it was slower last time i've checked. But would need to give it another try and tweak some options there. Maybe it'll be same speed or faster, no idea. Need to experiment.

Updates to Keir's review

Forgot to remove TODO line

Keir Mierle (keir) accepted this revision.Apr 1 2014, 5:09 PM

Keir Mierle (keir) added inline comments.

src/libmv/simple_pipeline/bundle.cc
44	Consider adding a comment that this MUST be consecutive integers, since otherwise it'll break Ceres.
171	Alternately, the distortion model could offer int num_distortion_parameters() double* distortion_parameters() and then it would be easy to make this part not use switches based on the model
src/libmv/simple_pipeline/camera_intrinsics.h
43	Pixels? Comment about units.
53	Doesn't intrinsics define the WarpFunction? If so, then why is this templated? I suspect the templated part could be hidden inside the cc.
src/libmv/simple_pipeline/camera_intrinsics_impl.h
79	Spaces around operators.
80	Why compact this to one line? And below.
83	Generally in libmv we don't put stuff on the same line like this.
85	Use braces always; also split across lines.
109	Weird indentation by 4
141	Space around -
src/libmv/simple_pipeline/distortion_models.h
29	DIVISION not DIVISIONS
57	+1 May as well do this in this patch.
81	I wonder if there is any point on doing traits based on T, and in that case special-casing zero for the tangential part. We can't do it for jets since the derivatives are NOT zero, but for doubles we can.
93	division not divisions

Sergey Sharybin (sergey) added inline comments.Apr 2 2014, 4:53 PM

src/libmv/simple_pipeline/bundle.cc
44	Well, it's rather a residue from some experiments from the time when divisions model used lambda1 and lambda2 coefficients (as lambda_1 lambda_2 as they called in some papers), For this i had OFFSET_LAMBDA1 which was the same as OFFSET_K1 (same for LAMBDA2 and K2). But now i'm using k1, k2 name for divisions model so we might skip using explicit values here and review on the status later.
171	That's a good idea.
431	Done.
src/libmv/simple_pipeline/camera_intrinsics.h
43	Done.
53	No, intrinsics doesn't define the warp function. There are actually two warp functions needed -- one for distortion and another one for undistortion. And they both need to work in pixel space. We can add pixel-space equivalents of apply/invert intriniscs to camera intrinisics class, but how to tell lookup grid which callback to use then?
67	Done.
69	Documented for now.
78	Done.
85	Done.
87	Renamed to Update.
111	Done.
161	This is now an own class, but still not sure about that sentence.
src/libmv/simple_pipeline/camera_intrinsics_impl.h
79	Done.
80	Coz of bloody legacy code from Mathias back to 2011..I was never a fan of such an "optimization" actually. I would just go with floats.
83	I know, but i really don't like this code and will rather switch to floats (a bit more mem but much faster execution due to no int<->float conversion). Would make this area and sampling one a way more clear. The Q about using floats is in the comments actually. For until the decision there didn't want to touch this bloody code.
85	See comment above.
109	Fixed.
134	Wrapped into 80 columns.
141	Let's use floats for offset!!! :D
src/libmv/simple_pipeline/distortion_models.h
29	Hrm indeed. Misread it :S Changed all over the code now.
57	Think i'll do it together with `double distortion_coefficients()` thing change. Need to go soon, but stay tuned! :)
81	Yeah, think we can optimize this. Shall we do it now or just mark as TODO for later?
93	Yeah got it. Fixed.

Addressed most of the review

distortion_parameters and renaming ApplyRadialDistortionModel is still to come

Some changes according the feedback:

Use an idea of num_distortion_parameters() and distortion_parameters().
Rename ApplyRadialDistortionModel to ApplyPolynomialDistortionModel and so.

Keir Mierle (keir) requested changes to this revision.Apr 12 2014, 9:14 AM

Keir Mierle (keir) added inline comments.

src/libmv/simple_pipeline/camera_intrinsics.h
218	Do not do this! The compiler is under no obligation to make the distortion coefficients consecutive in memory. Instead, you need to have double parameters[5], then the getters can peek in the array. Same for the others.
222	Not valid; you cannot assume the compiler will put these consecutively.
227	Not valid!
227	by the -> to the
src/libmv/simple_pipeline/camera_intrinsics_impl.h
151	drop blank

Just quick reply for now. Actual code will go later -- too much exhausted during the weekend.

src/libmv/simple_pipeline/camera_intrinsics.h
218	What do you mean not valid, it worked in blender for ages ;) Don't mind changing tho :)

Fixed the compiler alignment issues by using an array for the parameters.

Solved. Mind having a look into the constructors?

src/libmv/simple_pipeline/camera_intrinsics.h
218	done.
227	done.
src/libmv/simple_pipeline/camera_intrinsics_impl.h
151	done.

Ship it!

src/libmv/simple_pipeline/camera_intrinsics.h
222	To be clear: you can assume they will come consecutively, but the compiler is free to add padding.
229	Fun fact: Look at OpenCV 3.0's distortion model: http://docs.opencv.org/trunk/modules/calib3d/doc/camera_calibration_and_3d_reconstruction.html#calibratecamera
238	Clever.

Closed by commit rLMV615f610df7aa (authored by @Sergey Sharybin (sergey)).

Support multiple distortion models, including a new division model
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1081

src/libmv/simple_pipeline/CMakeLists.txt

src/libmv/simple_pipeline/autocalibrate.h

src/libmv/simple_pipeline/autocalibrate.cc

src/libmv/simple_pipeline/bundle.h

src/libmv/simple_pipeline/bundle.cc

src/libmv/simple_pipeline/camera_intrinsics.h

src/libmv/simple_pipeline/camera_intrinsics.cc

src/libmv/simple_pipeline/camera_intrinsics_impl.h

src/libmv/simple_pipeline/camera_intrinsics_test.cc

src/libmv/simple_pipeline/distortion_models.h

src/libmv/simple_pipeline/distortion_models.cc

src/libmv/simple_pipeline/keyframe_selection.h

src/libmv/simple_pipeline/keyframe_selection.cc

src/libmv/simple_pipeline/keyframe_selection_test.cc

src/libmv/simple_pipeline/modal_solver_test.cc

src/libmv/simple_pipeline/uncalibrated_reconstructor.h

src/libmv/simple_pipeline/uncalibrated_reconstructor.cc

src/ui/tracker/calibration.h

src/ui/tracker/calibration.cc

Support multiple distortion models, including a new division modelClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1081

src/libmv/simple_pipeline/CMakeLists.txt

src/libmv/simple_pipeline/autocalibrate.h

src/libmv/simple_pipeline/autocalibrate.cc

src/libmv/simple_pipeline/bundle.h

src/libmv/simple_pipeline/bundle.cc

src/libmv/simple_pipeline/camera_intrinsics.h

src/libmv/simple_pipeline/camera_intrinsics.cc

src/libmv/simple_pipeline/camera_intrinsics_impl.h

src/libmv/simple_pipeline/camera_intrinsics_test.cc

src/libmv/simple_pipeline/distortion_models.h

src/libmv/simple_pipeline/distortion_models.cc

src/libmv/simple_pipeline/keyframe_selection.h

src/libmv/simple_pipeline/keyframe_selection.cc

src/libmv/simple_pipeline/keyframe_selection_test.cc

src/libmv/simple_pipeline/modal_solver_test.cc

src/libmv/simple_pipeline/uncalibrated_reconstructor.h

src/libmv/simple_pipeline/uncalibrated_reconstructor.cc

src/ui/tracker/calibration.h

src/ui/tracker/calibration.cc

Support multiple distortion models, including a new division model
ClosedPublic
Actions

Revision Contents
Changeset List