Invalid camera parameters in gazebo_depthCamera plugin

[jchevrie]

The values of focalLengthX and focalLengthY returned by the plugin (when asking the intrinsic parameters of the camera through rpc via visr get intp) seem to be expressed in meters, while they are expressed in pixel size in the case of hardware components (for example the realsense2Driver in YARP).

This should be fixed so that the simulation provides the same interface as the hardware, or was there a reason behind this behavior?

[traversaro]

Hi @jchevrie , if with focalLengthX you mean the value returned by the yarp::dev::IRGBDSensor::getRgbIntrinsicParam, I think any implementation should respect what is documented in the YARP interface, i.e. in https://github.com/robotology/yarp/blob/47e8ae58889789465b15d90f85e1863aeab4c2c5/src/libYARP_dev/include/yarp/dev/IVisualParams.h#L141, so apparently the value returned should be in mm, so both meters and pixels seems to be wrong.

I think @barbalberto (that originally added in https://github.com/robotology/yarp/commit/9cd5f10e03c4d5bbc7afae1d76b6b9f267555332#diff-59134199dd23edbf48a3af8ff30d60ee the docs) and @Nicogene can also comment on that.

[Nicogene]

Good catch, @traversaro is right, in realsense documentation there is written that fx and fy are represented as a multiple of pixel width and height.

It is a bug inside a realsense2 driver, a conversion should be done in order to have it in mm.

[traversaro]

If in gazebo-yarp-plugins focalLengthX is reported in meters, we need also to fix it there to be consistent with the YARP interface.

[Nicogene]

Issue opened in YARP robotology/yarp#1949

[jchevrie]

I agree that we should follow the documentation if any implementation of yarp::dev::IRgbVisualParams::getRgbIntrinsicParam is currently following it. I'm wondering if it is a bug in the RealSense driver or a mistake in the documentation?

If I'm not mistaken, having the focal length in mm (or meters) does not allow the conversion from 3D metric space to 2D pixel space using only the intrinsic parameters provided by yarp::dev::IRgbVisualParams::getRgbIntrinsicParam (https://github.com/robotology/yarp/blob/47e8ae58889789465b15d90f85e1863aeab4c2c5/src/libYARP_dev/include/yarp/dev/IVisualParams.h#L155). For this a pixel/meter ratio or the physical size of the pixels is missing in the list. Having the focal length directly expressed in pixels (as is the case with the RealSense) made more sense to me since it solves this issue.

[traversaro]

I'm wondering if it is a bug in the RealSense driver or a mistake in the documentation?

That is a good point. If you can find an agreement with the maintainers of the interfaces and of the existing driver implementations (that I guess are @Nicogene and @barbalberto, but probably also @claudiofantacci , @vtikha , @GiuliaVezzani and @xEnVrE may be interested in this) and fix the YARP documentation, I will then be happy to merge the consequentially fix in gazebo-yarp-plugins.

[claudiofantacci]

RealSense driver, just like others, returns the focal lengths and the central point in pixels. To be honest, it is quite strange to me to have the focal lenghts in mm. When @Nicogene wrote the RealSense driver, I did not realize this requirement. Anyway, it might not be trivial to have a formula that converts the focal length from millimiters to pixels that agrees with the values returned by the SDK. The focal length can be converted from millimiters to pixels by knowing the resolution of the image provided by the sensor (e.g. 1920 x 1080) and the size of the sensor in millimiters. Note that the sensor cells can have an aspect ratio different than 1 and, consequently, you have two different focal lengths for the x and y axes. From the data sheet you can find the nominal focal length of the camera in millimiters and one might think that everything is set and done. However, due to (many) uncertainties, it is always good practice to calibrate the cameras. Calibration usually carries out focal length in pixels and the value incapsulate the above-mentioned uncertainties. What you can do is to rescale the focal lengths if you use a different image resolution (e.g. you pass to 640 x 480). What I would do, is to extend the interface to return both focal lengths, in mm (the current ones), and in pixels (useful for projection, deprojections, etc). What do you think about it? At the same time, however, I might missing some points and it would be useful to have some more insights on this issue, cc @barbalberto @randaz81.

[jchevrie]

Indeed, it would be nice to add the values in pixels.

Or even to replace the values from millimeters to pixels in the doc? It seems to be the only place that explicitly mentions mm and, as said, finding the real focal length in millimeters is not always easy nor useful in practice.

I don't know if anyone is using the focal length in mm or even if any implemented driver using the interface from yarp::dev::IRgbVisualParams actually returns a value in mm. Maybe I am also missing something, but as far as I can see from the inheritance diagram, in YARP the implementations for yarp::dev::IRgbVisualParams::getRgbIntrinsicParam are: realsense2Driver: currently returns a value in pixels, fakeDepthCameraDriver: returns 512, which feels more like pixels than mm, TestFrameGrabber: returns meaningless numbers, depthCameraDriver: just reads a config file, so it could be anything, RGBDSensorClient and Implement_RgbVisualParams_Sender: only ask for the parameters through a port, so it could also be anything.

[traversaro]

If there is no recorded use of anyone assuming that focal length is in millimeter, I agree with @jchevrie clarifying/changing the YARP docs is probably the best option.

[claudiofantacci]

Thanks @jchevrie for the detailed analysis. This is somewhat surprising 🙃

If there is no recorded use of anyone assuming that focal length is in millimeter, I agree with @jchevrie clarifying/changing the YARP docs is probably the best option.

I totally agree. We should however have the backing from camera YARP developers as well. It may be worth opening an issue on YARP.

[Nicogene]

I agree too, let's see what @barbalberto thinks about it.

[jchevrie]

After talking with @barbalberto we concluded that a good solution could be to change the interface (and the documentation) as proposed by @claudiofantacci :

• express focalLengthX and focalLengthY in pixels (instead of mm), which is more useful for practical use.
• add a field for the nominal focal length, i.e. one physical value in mm (or maybe meters would be even better to keep standard units).

I will open an issue on YARP to get the feedback from YARP developers. Then I can take care of fixing this in the different places where it is necessary and open some pull requests.

[traversaro]

Thanks a lot @jchevrie .