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

adding option to independently calibrate mono cameras before a stereo cal #446

Open
wants to merge 1 commit into
base: melodic
Choose a base branch
from

Conversation

phil0stine
Copy link

Adds an optional flag to allow user to do a good monocular calibration of each camera of a stereo pair independently, then fix those values for the stereo calibration between them, with the --mono-before-stereo flag.

Currently, each camera is independently calibrated, using only the corners that were valid for both cameras during the stereo data collection. For wider baselines, this means that the non-overlapping portions of each camera's image will have no valid data but will still be used to overwrite any existing monocular calibration.

One option to address this would be to also listen for camera_info messages and provide a flag to use that intrinsic information for the stereo calibration.

This PR instead combines the 3 calibration processes into one; first, data for the left mono camera is collected, then the right mono camera, and finally the stereo pair. The UI updates for each step; and the monocular calibrations are deferred until the end to save the user lots of time waiting for each mono to complete.

I have tested this primarily on version 1.12.23 with ROS melodic, and have seen some lagginess issues using the --approximate flag on version 1.13.0, but I don't see how it would be related to these changes.

@valgur
Copy link
Contributor

valgur commented Aug 21, 2019

This looks like a fantastic improvement for both the stereo calibration accuracy and convenience of the calibration workflow. Thank you!

The new logic does not seem to share data collected in one step (e.g. left) with other ones (right and stereo), though. Have you considered adding this? I think this could make the calibration process much faster.

One option to address this would be to also listen for camera_info messages and provide a flag to use that intrinsic information for the stereo calibration.

This would be a very useful option to have as well.

@valgur
Copy link
Contributor

valgur commented Aug 22, 2019

Also, if the monocular calibrations are available, then the self.c.goodenough / self.c.compute_goodenough() could be relaxed considerably. Even a single checkerboard image pair is enough to calculate stereo extrinsics for calibrated cameras, at least theoretically. This would be particularly useful for cameras that have very little overlap, where the existing monocular calibration criteria would not allow the calibration to become enabled.

@phil0stine
Copy link
Author

The new logic does not seem to share data collected in one step (e.g. left) with other ones (right and stereo), though. Have you considered adding this? I think this could make the calibration process much faster.

I am not completely sure how I would go about this, although I do agree that anything we can do to reduce the optimization time would be very helpful. Currently, the rcal and lcal objects are independent, as you say; which is analogous to doing each separately and then using the existing CameraInfo messages to fix the intrinsics. If you have a preferred way to improve the implementation I can get it done.

Also, if the monocular calibrations are available, then the self.c.goodenough / self.c.compute_goodenough() could be relaxed considerably

Good point, the major difficulty with running the stereo calibration tool for minimal overlapping FOV is simply getting self.c.goodenough to report true, so we can even execute a calibration. Assuming horizontal camera pairs, it would seem to require at least lowering the goodenough threshold for variation in X.

@JWhitleyWork
Copy link
Collaborator

JWhitleyWork commented Dec 2, 2019

@phil0stine - Please rebase on melodic.

@phil0stine
Copy link
Author

@JWhitleyAStuff Rebased. I had to resolve some conflicts and haven't had a chance to test again, but I think it's ready to go.

Copy link
Collaborator

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

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

LGTM. Will probably hold merge until another test is complete.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Yeah, I'd like an independent person to verify

@JWhitleyWork
Copy link
Collaborator

Been holding for testing for a couple of years now. @phil0stine Are you able to test this and/or still want it merged?

@phil0stine
Copy link
Author

@JWhitleyWork Yes this would still be useful to merge. I will update the PR, re-test and let you know when it's ready

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

Successfully merging this pull request may close these issues.

5 participants