Yarp: Side effects of copying an empty/invalid yarp::sig::Image

[PeterBowman]

I have observed neither base yarp::sig::Image nor its derived yarp::sig::FlexImage can be safely copied prior to populating them with pixels. In short, all boils down to:

yarp::sig::FlexImage image1, image2;
image2 = image1; // prints "*** Trying to allocate an invalid pixel type image" and quits

This is caused by the following lines (note the somewhat draconian std::exit call) and the fact that getPixelCode equals VOCAB_PIXEL_INVALID:

https://github.com/robotology/yarp/blob/b65ceeb909cd48ca7e9b68b10652c9ac827c1388/src/libYARP_sig/src/yarp/sig/Image.cpp#L349-L353

In a more realistic scenario, this bug exploded in my face while attempting a remote connection to an RGBD sensor. Steps:

1. Launch a RGBDSensorWrapper with a wrapped subdevice of your choice (e.g. depthCamera or realsense2).
   
   
   
   • Don't use fakeDepthCamera as it sets VOCAB_PIXEL_RGB internally (ref).
   
   
   • Please note RGD frames are represented by yarp::sig::FlexImage, whereas depth frames use yarp::sig::ImageOf<yarp::sig::PixelFloat>.
   
   
2. Start a local RGBDSensorClient, configure it so that it connects to the server's RGB and depth ports.
3. Retrieve both RGB and depth frames using yarp::dev::IRGBDSensor's getRgbImage and getDepthImage, respectively.

Unless you place a small delay between steps 2. and 3., getRgbImage will force the application to quit, showing that same invalid pixel type error. On the other hand, getDepthImage is fine. This is caused by FlexImage not having its internal pixel code initialized, as demonstrated earlier. Conversely, ImageOf<T> always knows how to interpret its T pixel type (ref), no matter if it is storing a frame or not yet.

In view of this, all RGBDSensorClient does is to periodically receive streamed RGB+D frames from the server device, store them locally in their respective class members, and make them available on request via getRgbImage and getDepthImage. In this last stage, the copy assignment operator is invoked (same as in my first snippet, see above). Before the first RGB frame arrives, last_rgb does not know what pixel type to expect and getPixelCode() returns VOCAB_PIXEL_UNKNOWN, causing the early exit. Ref.: RGBDSensorClient_StreamingMsgParser.cpp

I propose to drop the draconian std::exit call and allow cloning empty/invalid FlexImage instances. This is not a problem with ImageOf<T> just because of the pixel code being known beforehand. Perhaps a more thorough review is to be considered due to:

https://github.com/robotology/yarp/blob/b65ceeb909cd48ca7e9b68b10652c9ac827c1388/src/libYARP_sig/src/yarp/sig/Image.cpp#L11

Config: Ubuntu 16.04/18.04, GCC 7.5.0, YARP 3.4 (3.4.0+8-20200715.210+gitb14899f).

[drdanz]

That exit should definitely not be there...

This file is in a pretty hacky state. Sorry!

The whole sig::Image stuff used to be in a pretty hacky state, but it is currently a complete mess... this had a very complicated extra layer to support IPL images which are no longer supported anywhere, therefore all the IPL image support should be removed.

We should schedule a big rewriting of this part...

[PeterBowman]

That exit should definitely not be there...

There is another one here:

https://github.com/robotology/yarp/blob/b65ceeb909cd48ca7e9b68b10652c9ac827c1388/src/libYARP_sig/src/yarp/sig/Image.copyPixels.cpp#L1459-L1462

This is why manually setting the pixel code in the target image does not help (in case anyone thinks of calling setPixelCode right before getRgbImage in my above test scenario):

yarp::sig::FlexImage image1, image2;
image2.setPixelCode(VOCAB_PIXEL_RGB);
image2 = image1; // prints "*** Tried to copy type 0 to 6449010" and quits

[Nicogene]

extra layer to support IPL images which are no longer supported anywhere, therefore all the IPL image support should be removed.

This https://github.com/robotology/yarp/pull/1932 deprecates getIplImage and wrapIplImage, maybe at some point we can get rid of that part

[drdanz]

We had an internal discussion, and, since the interface is supposed to return false when the image is not available, we will fix the client to return false if the image is not ready yet.

Later, we will investigate how to properly handle this in Image:

• The interface is lacking an isValid or isNull method, to be able to check if the image is actually a valid image
• The copy of an invalid image should create an invalid image, without calling exit

[drdanz]

Opened #2369 and #2370 about proper handling in yarp::sig::Image