Breaking changes in YARP 3.5 for the Image/Camera interfaces, serialization and network protocols

[drdanz]

Hello all,

while writing the NWC/NWS for the cameras, we hit a few issues, and we are now considering a few breaking changes in the Image/Camera interfaces and I'd like to discuss this before we do the changes.

1. Currently, the topIsLow information is not forwarded on the network. This is a bug, and it implies that it is not possible to have this information on the NWC. This information should be sent together with the ImageNetworkHeader which is written in a binary mode, using appendBlock/expectBlock, assuming that the length of the block is the size of the structure. I was originally going to propose to change the size of this structure, but this would break the protocol compatibility between YARP 3.4 and 3.5. Instead, I realized that there are a few fields that are "oversized" for what they transport, and we just need to add a boolean somewhere. For example the quantum field is a 32 bit integer (little endian), and we can resize it to a 16 bit integer, and add the topIsLow information in the 16 bit saved by this. This would ensure compatibility, but we just have to pay a little attention, because the 2 bytes below are zeros, and the default for topIsLow is true. The only problem will rise if an image is sent from YARP 3.5, and received from YARP 3.4. In this case, if any bit of the topIsLow field is set, the quantum will be wrong on the receiving side. I propose to fix this with a patch in the YARP 3.4 branch (and probably an extra release). In this way you won't have the issue if you are using the latest 3.4 release.

2. In the IFrameGrabberImage interface, I would like to change the getImageCrop to pass vertices by reference instead of by value. This shouldn't be a big issue for anyone, unless someone is implementing this method outside YARP:

     virtual bool getImageCrop(cropType_id_t cropType, 
-                              yarp::sig::VectorOf<std::pair<int, int>> vertices, 
+                              const yarp::sig::VectorOf<std::pair<int, int>>& vertices, 
                               ImageType& image);

3. In the IFrameGrabberImage interface, I would like to make the getImage and getImageCrop methods const. This is a bit more complicated, because it also implies that wherever this interface is used, it does not modify the internal state, and probably this is not the case in a few places.

4. The remote_grabber/grabberDual network protocol for the IFrameGrabberImage interface, currently implements only getImageCrop. The reply to this call returns [VOCAB_CROP] [VOCAB_IS] <width> <height> <raw image as pointer>. This information is not enough be sure to properly restore the image. In the NWC/NWS, I'm planning to change this to [VOCAB_CROP] [VOCAB_IS] <properly serialized image>. This will break the compatibility between NWC and grabberDual, and between remote_grabber and NWS, but only for this getImageCrop method, everything else would be fine.

What do you think of these changes? Do you think we should wait for one (or more) of these changes?

CC @lornat75 @randaz81 @traversaro @pattacini @vtikha @Nicogene @triccyx @ste93 @elandini84 @S-Dafarra @PeterBowman

[pattacini]

I'm pretty much fine with points 2, 3, and 4 as they seem to be minor. Point 1 appears to be somewhat more critical as it looks breaking from YARP-3.5 to any YARP-3.x with x<5 in case topIsLow is true. The break takes place without useful printouts on one of the sides, the sender or the receiver.

The condition YARP-3.5 -> YARP-3.x is not unusual as it happens when a user keeps his PC updated and connects it to the robot, which instead tends to receive updates less often.

No clue about the likelihood of topIsLow==true though. If it's uncommon, then we can get away with less.

cc @traversaro

[drdanz]

No clue about the likelihood of topIsLow==true though. If it's uncommon, then we can get away with less.

The topIsLow==true is actually the default case, therefore even though this is not exactly the best scenario, I was thinking to keep this field with an inverted value, i.e. 0 = true, 1 = false;

[drdanz]

I'm planning to add patch on the YARP 3.4 branch as well. This means that any previous release will work with YARP 3.5 in case of images where topIsLow==true, but to work with the other images you will have to update at least to the last patch version

[PeterBowman]

• Aren't 2 and 3 against SemVer, i.e. shouldn't those be postponed until YARP 4.x? I'm not opposing, though, just pointing this out in case it could be deemed relevant. See also Const correctness in YARP #1618.
• Regarding 2, if we are changing the signature, perhaps sig::utils::cropRect should be reviewed/harmonized as well. I made it accept unsigned int pairs of vertices (always positive pixel coordinates), while getImageCrop works with signed integers.
• Regarding 3, note there are also non-const getImage(Crop) methods in IFrameGrabberImageRaw.

PS (unrelated) I think http://www.yarp.it/ has not been updated in a while.

[drdanz]

• Aren't 2 and 3 against SemVer, i.e. shouldn't those be postponed until YARP 4.x?

Technically yes, but we've always had some tollerance over changes in yarp::dev, so I'm proposing to change it now, since we are doing this NWC/NWS implementation, that we will have to change later if we don't do this now.
We could also consider using a new interface, but I don't think it is worth...

• Regarding 2, if we are changing the signature, perhaps sig::utils::cropRect should be reviewed/harmonized as well. I made it accept unsigned int pairs of vertices (always positive pixel coordinates), while getImageCrop works with signed integers.

👍🏻

• Regarding 3, note there are also non-const getImage(Crop) methods in IFrameGrabberImageRaw.

You are right, sorry, I forgot to mention that the IFrameGrabberImage and IFrameGrabberImageRaw are going to be merged in a template, so both are influenced by these changes, see #2583