ros-perception / point_cloud_transport

Point Cloud Compression for ROS
BSD 3-Clause "New" or "Revised" License
78 stars 7 forks source link

API suggestions #46

Open devrite opened 10 months ago

devrite commented 10 months ago

Hi everyone,

thanks for the work and also avoiding some pitfalls off the image transport API. Making it easier for language bindings or offline usage.

some comments. Let me know what you think and whether something of the mentioned is wrong and which ones you think are worthy of adapting.

You already provide the possibility to convert to generic messages. It may be possible to remove the sub/adv from the plugins altogether or even implement a single message type for all compressions:

Could be handled by the compressed message already.

I still see that a fixed node type is being used in the API instead of interfaces, requiring a per node type API.

https://github.com/ros-perception/point_cloud_transport/blob/dd55394db3f69725ed9b661814a9abbcfc6b73eb/point_cloud_transport/include/point_cloud_transport/point_cloud_transport.hpp#L105-L110

What is nice is that you can do generic publishing already, still lacking in image_transport as far as I can see.

To summarize any transport/compression package in ROS should get as many of these:

Depending on the points you want to fulfill I would consider them early on as some will probably require for incompatible APIs and break stuff if you implement it later on.

Personally I would at least make it node agnostic and try to implement it is generic pub/sub or single message type.

Best regards!

john-maidbot commented 9 months ago

Hello, sorry for the delayed reply. And thank you for your thorough suggestions!

Make the codec the actual plugin to be of use offline scenarios or language bindings

Make it possible to configure (param) a preferred plugin to choose the best/fastest implementation to be used

devrite commented 9 months ago

Make the codec the actual plugin to be of use offline scenarios or language bindings.

Yours is already there. I just referenced it for completion. They only thing is that it could be separated into multiple plugins (one with publisher/sub if needed and the other just being the codec). In this case it would need to support a single message or accept serialized (dec) messages and return serialied messages (enc) in the API for codec agnostic interfaces.

Which make it easier to use in python or write language bindings in general and support an offline use case in a straight forward way, but it is at least possible todo that now (as you are not requried to create the sub/pub) if I am not mistaken. For reference in image_transport this is not the case.

w.r.t. having a single message to separate pub/sub from compression

Yes, for implemntations it means they would have to encode their own fields in either a separate field fields/codec header or just store it in the data blob as part of their serialization/encoding/decoding. It makes it less direct or viewable of course and may introduce a small overhead. But if I think of most video codecs that is what they do when they break it down into (network) messages. There is a container format (avi,mkv, ...) or a transport format (RTP etc.). If I think of image transport/CompressedImage it already has a codec string that allows to integrate png/jpeg already and theoretically could integrate anything. You loose the ability to directly inspect special fields though if they are not encoded into such a readable string and are than encode into the data blob.

But you can still do your own messages. In this cases you would only work with serialized messages at the interfaces. In this case one would also not need any pub/sub implementation in the plugins but it would limit the plugins to one message channel. Of course one could also make it a vector of messages with different topics/message type ids (return upon init or so).

If you do both, then essentially one cad decide to use your message or define it's own as it is both generic on top level.

If there is a decision to do at least one of those I don't have an opinion on if we should do both. Either way once we have generic messages at the interface we would not be able to enforce a single message for all anyway..

atm users can already choose a preferred transport by subscribing to it, right? Maybe I am misunderstanding what you are suggesting though 🤔 can you provide an example please?

Yep, based on remappings basically. If you have only one message type then you would either still have multiple topic names or implement to just load the configured plugin and possible topic name if you want to load multiple. Same on sub side.

In image_transport it looks currently like this:

In order to connect and the correct plugins you have to remap on sub side even though they are the same type and set the param to the plugin to use. The advantage of this approach is that you can still connect to both, in case the performance degrades with multiple subs to the same accelerated implementation.

Which is fine but then the topic name carries the plugin implementation instead of just the information about the message type. In this case for recordings it is a little less-obvious if you do not remap and the one using your bag does not have that plugin.

For an offline use case it would be nice to have an API that selects based on the message type and returns just some plugin able to decode that type and a possibility to the select one of those per an argument.

sorry for my ignorance, what do you mean by "streaming codecs"? Do you mean compression that makes use of message sequences rather than individual messages?

Yep. I don't see much use case here until there is a plugin that can do it for sequences instead of "static" PCL maps. I know that there are libs/servers which sequentially stream in PCL data from low/qual to high, but they do not really work on sequences but rather static huge maps. Which would usually use and equivalent storage and renderer on client side instead of a huge PCL message. So my knowledge on PCL sequence compression is rather limited. In this case it might be better to just use image compression on range/intensity images?


Besides I do not know if there are any accelerated implementations for PCL compression. So I see it at lower priority as it can be done via remapping if needed.

It is only that I see coding from input/output side (e.g., these "messages" have the following providers/users/impls and will be supplied at that interface (topic)) rather than that topic uses that coding and that impl. If just see the topic as the interface then it could be nicer to just make the information of the interface visible the associated message/content not the impl. But it is a fair compromise to leave it as is.

But it would affect the API or usage via params. So, if one or the other is preferred than you would make the decision early.