magazino / pylon_camera

ROS-Driver for Basler Cameras
BSD 3-Clause "New" or "Revised" License
78 stars 108 forks source link

Added camera_info publishing synced with image publishing. #18

Open basti35 opened 7 years ago

basti35 commented 7 years ago

I added also a default calibration file (for a Basler acA1600-20uc with a 8mm lens from computar). The camera_info msg is very cheap, but sometimes is necessary.

jonbinney commented 7 years ago

It looked like this was already being done for the image_raw topic and you're adding it for the image_rect topic, right? If there are subscribers to both image_raw and image_rect, won't this publish camera info too often?

Actually, why does pylon_camera do its own image rectification anyway? Wouldn't it be simpler for pylon_camera to just publish image_raw and camera_info, and to add image_proc to do the launchfiles to do the rectification?

Also, since the calibration is going to be different for every camera and every lens, I think it probably doesn't make sense to include a default here. If the camera hasn't been calibrated, there shouldn't be any rectified images published.

marc-up commented 7 years ago

Thanks @basti35 for your contribution, but I think we won't merge you're PR. As @jonbinney pointed out, the calibration of a camera is highly device and lens specific, so the calibration you added should not be part of this pkg. It will only generate valid rectificated images for your individual case but not in general. In case no calibration is given, only raw_images should be provided, as it is done up to now. Furthermore, I didn't get the point, why you want to add a second camera_info publisher. The CameraInfo message is always sent in case someone listens to the topic. So what do you mean by 'camera_info publishing synced with image publishing'? Can you describe the situation when they are not synced? img_raw_pub_.getNumSubscribers() > 0 is true in case s.o. listens to either camera_info or image_raw as you can see here, so you should always receive the CameraInfo message, in case you want it.

basti35 commented 7 years ago

Sorry for the delay of my reply. The reason of my PR is that sometimes nodes use image_transport for subscribing. It waits for both the messages to spin the callback. Because the publisher of img_raw is called only if there are subscribers, it could happen that the camera_info is not published concurrently with the image_rect. e.g. I am using apriltag_ros , and it use a image_transport subscriber. Thus, it requires camera_info. It use this msg for obtaining the center of the image.

marc-up commented 7 years ago

Ok, I got the point. But is that the standard way this is done? Publishing the camera_info msg that is static in most situations with every new acquired image does not sound like the perfect solution for me. The size of the CameraInfo-msg is on the other hand not that big... Does anyone have another idea how this issue could be resolved?

jonbinney commented 7 years ago

Publishing exactly one CameraInfo for every image, although it seems a bit odd, actually works quite well (in my experience). ROS nodes that use calibrated images know to read both the camera_info topic and the images topic, and look for exact timestamp matches. Using this approach works fine for static calibrations, but also handles the case where the camera is periodically recalibrated, or even calibrated on the fly. Most importantly, it is a common practice and so other nodes/users expect it and know how to deal with it. For example, there is a message_filters package with a time synchronizer filter that will subscribe to two topics and call your callback with matched pairs of image+info: http://wiki.ros.org/message_filters

mgrrx commented 6 years ago

@basti35 thank you again for providing this PR. In order to get this merged can you please:

If you are no longer interested in this PR please consider to give the maintainers access to your branch.