API review

Proposer: Jack O'Quin

Reviewers:

• Blaise Gassend
• Michael Quinlan
• Bill Morris
• Trevor Jay
• Eric Perko
• Ken Tossell

Question / concerns / comments

This package is proposed for inclusion in camera_drivers for the C-Turtle release. It only has a single CameraInfoManager C++ class interface.

• (Blaise) Should we add support for the flash:///1 URL format to this release?

  • (Jack) Blaise and I discussed this just before the review started. None of the ROS camera_drivers that support flash devices are using camera_info_manager in this release. We decided not to add the feature until there is a user. If anyone has a camera driver that needs this feature in the C-turtle timeframe, we can add it. Otherwise, it should wait until D-turtle.

• (Jack) The dependency on yaml_cpp should not be needed for latest, it was needed on Box Turtle to work around a camera_calibration_parser bug #3922. Remove the dependency and the code to catch YAML exceptions, then re-test with latest. Done, committed to SVN.

• (Blaise) Should this package be somewhere other than the camera_drivers stack? (enhancement #3921)

  • (Jack) the ticket explains the details, but the consensus answer seems to be: "yes, but not until D-turtle", since camera_common is already frozen for C-turtle.

• (Eric) File naming: It seems weird to me that the source and header for a camera_info_manager would be called just camera_info... I would expect that a file named camera_info.h would provide me with some camera_info data structures and not a manager class. Why not rename this to something more descriptive like manager or something so that the include is <camera_info_manager/manager.h> or <camera_info_manager/camera_info_manager.h> and not <camera_info_manager/camera_info.h>. The latter just seems confusing in my opinion if I were looking at code that used this class and trying to figure out exactly where the manager was declared.

  • (Michael) I agree, I would lean towards <camera_info_manager/camera_info_manager.h> , it's wordy but descriptive.

• (Eric) I see no clear mention about what formats this manager actually supports. I can see that it is based on how the camera_calibration_parsers work, but the camera_info_manager doesn't actually mention that it only supports those formats besides the fact that it has a dependency on the camera_calibration_parsers.

• (Eric) I don't see anywhere that talks about the file format choice of the manager based on what URL I set. I know that if I set a file:///cal.yaml file, I get a yaml output of the calibration data. What happens if I set a file:///cal.ini file as the URL? Do I actually get a Videre INI format file? What about setting file:///cal.data as the URL? Do we assume that all flash:/// style URLs want INI format and not possibly some other format (maybe totally custom)? Depending on this choice, we may need some sort of separate format specifier to be added to the API.

  • (Jack) I believe format information could be encoded in the URL, perhaps using similar extensions to the file names, e.g. flash:///1.ini.

• (Ken) I think a bool isConfigured()-like method would be helpful. Currently there's no certain way to tell whether the camera has been calibrated (aside from testing that every value is zero). Two situations where this could come up: 1) Program instantiates CIM with a non-empty URL parameter. 2) Program instantiates with some possibly-empty URL. It later changes to URL x, setURL(x)=false, and then the camera gets calibrated using the service. It might be beneficial to have some programs publish CameraInfo only when calibration information is available. Adding such a method would allow that. I'm sure there are other reasons why a program would want to know calibration status.

  • (Blaise) I believe that the convention is that K[0]==0 is the convention for verifying whether your camera is calibrated. However, I did not find that in the message documentation so I added #4105 to get it documented.

  • (Blaise) It isn't clear to me what the semantics of isConfigured would be. Does it indicate if the camera is uncalibrated (redundant with K[0]==0)? Does it indicate whether the current URL was successfully loaded? Does it indicate whether a URL has ever been successfully loaded? Dose it indicate that the current URL has correct syntax? It is a bit strange that setURL returns a bool, but that there is no way of checking if the URL passed to the constructor was OK. From the driver writer point of view I wouldn't ever expect to use the output of setURL or isConfigured. The URL will come straight from a parameter (which I shouldn't be tampering with), and I will publish the camera info straight to the camera_info topic (which I have to do no matter what).

    • (Ken) I had been thinking of a method that would return true iff the manager had successfully loaded a calibration or received a calibration over the service. The name Michael's suggesting below (`isCalibrated()') would be better.

  • (Michael) isConfigured() is confusing. isCalibrated() or isCameraCalibrated() would be better names. I echo the notion that knowing this information is useful for many tasks. Having such a method may be redundant (returns K[0]==0), but it's definitely more informative and provides a clearer API.

    • (Trevor) I concur. An isCalibrated() function may be redundant, but if (camera_manager.isCalibrated() {}; is going to be much more concise than if (camera_manager.getCameraInfo() ... == 0).

• (Ken) I would expect setURL() to zero out cam_info_ (and set isConfigured=0) by default if it can't load any calibration from the new URL.

  • (Jack) Seems reasonable sometimes, but if set_camera_info has already been called and the user is trying to configure the URL for a new file to store it in, the calibrated data would be lost. We could keep track of whether set_camera_info has been called and behave differently, but that starts getting complicated to explain. I took the approach that the private camera_info should never be disturbed, regardless of URL accessibility. Maybe there's a better way.

    • (Eric) To address whether or not a URL was meant for saving or loading calibration data (thus intending to change the private camera_info), perhaps we should add a reload_camera_info service call. This would allow the user to specify what we should be doing with the current URL the manager has, i.e. if I call set_camera_info I want to save to that URL but if I call reload_camera_info we attempt to load a CameraInfo from that URL.

    • (Michael) I made a similar note when reading over the API. To me the confusing part was that setURL does more then simply set the URL. I would strongly prefer that a set method only sets the url_ variable and then a second method (such as reloadCameraInfo) called to load the data. If we expect that these two are often done in conjunction and we want a single call then create something like setURLReloadCameraInfo.

    • (Michael) Related note, would we expect that getCalibration(const std::string &url) sets url_ ?

• (Ken) It might be nice to let the calling program supply a new CameraInfo.

  • (Trevor) I agree that it seems natural to have a version of the constructor that accepts a CameraInfo. Additionally, I think a version of the constructor that takes only a name and node-handler would be a good idea (i.e. no URL, it just waits for a set message). This is meant to a labor saving class afterall.

    • (Jack) Note that the current constructor already allows omitting the URL.

• (Ken) Since you can change the URL, maybe let the caller change the camera's name, too? I can't think of any good reason to use these last two methods, but they seem potentially useful ...

  • (Jack) I thought about that, too. But, I hate to add support for an interface with no users. When we have a use case, we should add it.

  • (Jack) I am also a little unclear exactly what the camera name means. It is mainly present because camera_calibration_parsers store it in the file and return it on reads.

• (Ken) Should the node we pass to the constructor be a private one, or will CIM derive a private namespace from a supplied public node?

  • (Jack) Normally, it should reference the driver's streaming namespace, typically NodeHandle("camera"). The private namespace is not appropriate for topics or services. See: the Constructor interface.

• (Blaise) One thing I would like to see in here is the ability to store information about the mode the camera was in when the calibration was done so that the validity of the calibration for the current mode can be verified. Width and height aren't sufficient in general (region of interest, binning, mirroring come to mind from wge100). This is not a C-turtle problem, though.

• (Blaise) Would it make sense to check that the width and height in the camera info match the width and height that the camera is currently set to, for example by having the camera tell the camera_info_manager what the width and height are? Is it better to leave that out until a more general validity framework is laid out? Would a callback that gets called when camera_info changes be able to fill this need?

• (Blaise) What happens if you getCameraInfo from one thread while the camera_info is being written from another thread? Should a mutex be used to synchronize concurrent access, or will that create a deadlock risk?

  • (Ken) setCameraInfo(req, rsp) { cam_info_ = req.camera_info; ... }' (same in getCalibrationFile'), so the built-in multithreading is very unlikely to cause data corruption, but I don't think it would hurt to make CIM thread-safe -- it should be safe from deadlock, though a too-long lock could come up if somehow the load-from-file operation blocks (probably `getCalibrationFile' should only grab the lock after reading).

• (Blaise) Should getCameraInfo return a const sensor_msgs::CameraInfo & instead of a sensor_msgs::CameraInfo?

• (Michael) Should getCalibration and getCalibrationFile be called loadCalibration and loadCalibrationFile ? (Since they override cam_info_)

• (Bill) Overall I would like to have this package included asap, even if it is not perfect.

• (Bill) isCameraCalibrated() seems clearer then checking K[0]==0.

• (Bill) setURL() is confusing, I would personally rename it to setCalibration() or separate it to setURL() and loadCalibration(). Separating the calls doesn't make a lot of sense to me since I can't see why you would set the URL without wanting to reload the config. The only reason I can see to separate the calls is if one file has multiple calibrations as explained below.

• (Bill) The one thing I dislike about the current interface is that it seems to imply that there is only one calibration. I briefly spent some time looking at implementing a calibration lookup table for a webcam that has autofocus and I am looking at some cameras that can adjust between wide angle and telephoto. Once multiple resolutions are taken into account, I think there should be an explicit assumption that there are multiple calibrations. If all the calibrations were stored in a single file it would simplify things. Something like this might work for access.

setCalibration("file:///tmp/calibration_camera.yaml","Resolution","640x480");
or the other way
setURL("file:///tmp/calibration_camera.yaml");
setMode("Resolution","640x480");
setMode("Autofocus","2");
loadCalibration();
or something like this
setURL("file:///tmp/calibration_camera.yaml");
setResolution(640,480);
setFocus("2"); // The logitech quickcam I looked at using returned a
value from 1 to 20 for the current focus setting.
// This should probably be the focal distance in m with 0 == inf
// Alternatively it may be better to use depth of field.
setFOV("45"); // Field of View Telephoto vs Macro Zoom
// Focal length may be more appropriate
loadCalibration();

• Having all of the calibrations in one file should allow the data to be cached in memory which is useful if the zoon or focus is constantly changing. The driver should be able to provide a warning if the current resolution does not match the resolution of the calibration. Is there a way to reasonably estimate the correct calibration for another resolution? In some cases a poor estimate may be better than no calibration.

  • (Eric) I definitely prefer having each file be smaller and just having lots of files. In this situation, I would prefer to be able to have the node doing the zoom/focus changing to do a setURL/load_camera_info call pair instead of the driver autohunting for calibration. What does the info manager do when it can't find a valid calibration? If some driver somewhere is supposed to explicitly manage the zoom/focus, it would be reasonable to send it an exception when we cannot find a valid calibration file; if there is no explicit manager, who is responsible? If we want to cache the files, we could have semantics for that, e.g. a method to set a bunch of URLs and then maintain a lookup table or something. I think the line between what a driver should handle and what an "info_manager" should handle is getting a little blurry here.

  • (Jack) my understanding of camera_info_manager is that it handles the CameraInfo currently being streamed by the driver. The URL syntax is sufficiently flexible to provide a hierarchy cameras, lenses and video modes, where needed, and setURL(); loadCameraInfo(); provides a means to change it. We need to decide if that is good enough for this release while leaving an opening for future enhancements.

  • (Jack) Note that multiple streams from the same camera driver are handled by creating multiple CameraInfoManager class instances (e.g. for a stereo device).

• (Bill) Calibration Statistics: It would be nice to consider some way of providing an option of uploading calibration parameters to a webserver via http. This could then be used to generate a statistical model for each camera so that in the future a warning could be generated if camera calibration process generates parameters that deviate significantly from the most common calibration. I suppose this should be done in the calibration tool.

• (Bill) Color Calibration: Since the driver is called Camera_Info_Manager I would think that color calibration information should be integrated and eventually this should be provided via camera_info. http://www.ros.org/wiki/color_calib

• (Bill) Camera Controls: It would be nice to have some sort of unified interface for controlling the camera's contrast and brightness, somewhat independent of which camera driver is being used.

  • (Eric) What about dynamic_reconfigure?

  • (Jack) I am a big fan of dynamic_reconfigure for drivers. In its current form, it does not support configuration of subordinate C++ classes well. Blaise has mentioned possible future work to do that better.

    • (Bill) It can probably wait until dynamic_reconfigure works better, but the common parameter names should be standardized across camera drivers for compatibility.

• (Bill) nodelets: It looks to me like the best approach is going to be to release camera_info_manager as soon as possible so that each camera driver does not need to be modified to provide camera_info to support the calibration process. However I think the interface should be deprecated in the future in favor of a nodelets approach.

Related Nodelets
 camera driver        Hardware Driver (usb_cam, wge100_camera, etc)
 *color_proc          Color Correction
 image_proc           Image Rectification
 image_transport      Compression/Decompression
 camera_info_manager  Camera Information Manager
 *camera_control      Unified Camera Control (brightness/contrast/etc)
 user_application     Custom OpenCV code.
* = Does not exist yet

• This way the zoom (telephoto vs wideangle) could be changed with camera_control which would then pass the message to the camera hardware and load the correct calibration in the camera_info_manager. If the camera implements autofocus in hardware the hardware driver should constantly report the current focus to camera_info_manager so that each frame is rectified correctly. Grouping the most common image related tasks as nodlets should provide a significant boost to performance without complicating matters too much if the tasks are split between computers. This assumes that nodelets work the way I think they do, since I have not had a chance to use them yet.

* (Trevor) Calibration Changes: To fully replace responding to the appropriate "manually", this utility class needs to support registering a callback for when the calibration changes.

• (Bill) A callback interface seems like a good idea.

* (Trevor) Param Storage: If the URL abstraction is already going to be used. It would be nice to support some kind of custom rosparam:// or param:// resource-type that would allow storage of the CameraInfo data within the RPC datastore.

Meeting agenda

(Jack) Since this review is being held on-line via e-mail and wiki updates, there will be no face-to-face meeting. Instead, my intention is to pull together action items from the questions and comments section for approval by the reviewers.

The "meeting" is scheduled for this afternoon, 2010-05-20.

(Suggestions for improving or streamlining this process are welcome.)

(Eric) Suggestion: with no face-to-face meeting, should we attempt to use IRC room or Google Wave (or other more real-time communication) for this? At least, so that during the meeting time this wiki page doesn't explode with tons of simultaneous comments.

(Jack) That's a good idea. The wiki is not good for simultaneous updating. Unfortunately, I now need to do some testing with students this afternoon at the advertised review time (4:00PM CDT, 2:00PM PDT). To improve future reviews (e.g. camera1394), I'll ask everyone for feedback on the process. We can discuss this and other ideas.

Today, I plan to update the Conclusion section (below) and ask for reviewers' comments and sign-off via e-mail. If there is no general agreement, I'll schedule a follow-up review.

Conclusion

Actions for this release:

• Change the include name to <camera_info_manager/camera_info_manager.h>

• Document the file: formats. Since these all depend on camera_calibration_parsers, that may involve a level of indirection.

• Add public bool isCalibrated() in-line method that returns true if K[0] != 0.0.

• Rename bool setURL() method to bool loadCameraInfo(), with same parameters and semantics.

• Rename private getCalibration*() methods to loadCalibration*().

• Add a mutex lock to protect class private data if called from multiple threads. The lock should not be held when calling anything that might wait.

• Add a public bool validateURL(const std::string &url) method that returns true if the URL syntax is valid (regardless of its existence).

• Add a public bool setCameraName(const std::string &name) method that returns true if the name syntax is valid.

• Log a warning if the camera name loaded from a URL differs from that set by the driver. Always write the name provided by the driver.

Items for future releases:

• Provide a subscribe() method that defines a callback used when the set_camera_info service is invoked. This callback needs to be able to reject calibration information that is not compatible with the current driver mode. Exact interface TBD.

• Possible URL extensions, to be added when needed:

  • flash:

  • http:

  • rosparam:

• Improve handling of multiple calibrations. Currently, each calibration has its own URL, camera_info_manager only keeps track of the active one.

  • For this release, that means each calibration must be stored in a separate file.

  • Future extensions to the file: URL syntax might allow multiple calibrations in a single file.

• Should this package handle color calibration?

  • camera_info_manager handles sensor_msgs::CameraInfo message data. If color calibration is added, it should support that.