Add topic_id returned by storage to the TopicMetadata

[MichaelOrlov]

• Relates Use topic_id instead of topic_name in inner rosbag2 buffers and data structures #1553
• Rationale. To be able to distinguish topics by unique topic ID rather than by topic name in the future.
  Motivation is the same as in Use topic_id instead of topic_name in inner rosbag2 buffers and data structures #1553

1. Users complain multiple times for instance here Use middleware send and receive timestamps from message_info during recording #1531 (comment) that rosbag2 stores messages in internal buffers in a very inefficient way and that memory footprint for the same data payload could be reduced down to the factor of the 2 or 3 by using topic_id represented as integer type instead of full topic_name represented as a string for each message.
   This is very meaningful when using the snapshot feature and recording on the host HW with limited resources.
2. Another motivating factor for using topic_id instead of the topic_name in internal data structures and buffers is the ability to differentiate different publishers with the same topic name. It might be a situation for instance when multiple publishers exist on the same topic but have different QoS settings. Another use case could be when multiple publishers exist on the same topic but with different versions of the type definitions.
   Currently, we don't support these use cases in the rosbag2. However, if we will differentiate topics by topic id in inner rosbag2 representation on all layers we would be able to add support for the aforementioned use cases in the future.

About the size of the type for the newly added topic_id field.
In MCAP specification we reserved 16 bit for the topic_id. This is the ChannelId, the MCAP inner type that corresponds to the uint16_t. When we were creating the MCAP format we decided that the ability to differentiate a maximum 65535 topics would be enough in the vast majority of cases. However, in the SQLite3 storage plugin, we are using topic_id as an index to map records from the messages DB table to the corresponding topics table. According to the SQLite3 specification, the index is the int64_t data type and can't be changed.
Therefore the type of the newly added topic_id filed shall be no less than int64_t and shall be with the sign.
Update:
Decided to keep uint16_t for topic_id in the TopicMetada and have a hash map to int64_t indexes inside the sql storage plugin. The performance will not be affected during recording because currently we already have an internal hash map lookup to find match for topic_name to the topic_id index. It will be similar hash map lookup, however we will be using external topic_id instead of topic_name.

[MichaelOrlov]

Moving it to the draft again.
Going to implement proposal form the #1553 (comment)
Will keep uint16_t for topic_id in the TopicMetada and have a hash map to int64_t indexes inside the sql storage plugin.

Motivation:
Well, on one hand, it will require keeping a hash map with two types of IDs in the SQLite3 storage plugin side and having a hash lookup operation for each message read/write. The hash map is not very memory efficient but still could take less memory footprint rather than keeping an extra 6 bytes per message. Especially when we are having a lot of high-frequency messages like IMU. Another drawback of this is a little bit of performance hit for lookup for inner ID in the hash map for each message write.
However, on the other hand, we probably can sacrifice a bit of performance penalty on the SQLite3 plugin side for the sake of a better memory footprint.
Please note that MCAP storage plugin will not be affected and users will have benefits using rosbag2 with it.

[MichaelOrlov]

Ready for review

[fujitatomoya]

@MichaelOrlov thanks for the PR. this one looks big change, i will try review in this week. you can go ahead to assign this to me.

[MichaelOrlov]

@fujitatomoya Thanks for helping with review.
While it looks like a big PR with a lot of changes although, it is because it affects a lot of tests where we need to add 0 or 1 for initializing the newly added field to the TopicMetadata. The most of the changes where need to look at and review are in the sqlite3_storage package (there a few files)

[fujitatomoya]

overall lgtm, a few comments.

[MichaelOrlov]

@emersonknapp @clalancette Could some of you please give formal approval for this PR to be able to merge it?
@fujitatomoya already reviewed it.

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/9d2827dcef7e219879574e49386f1596/raw/16f993bb8bcfd59af7ac24dbeaa4bec51ead6950/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_examples rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_examples rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13237

• Linux
• Linux-aarch64
• Windows

[emersonknapp]

lgtm

[MichaelOrlov]

Re-run CI with the correctly listed rosbag2_examples_cpp and, rosbag2_examples_py instead rosbag2_examples package.
Gist: https://gist.githubusercontent.com/MichaelOrlov/7b5b5d317f83b855c612ecd3a833a50e/raw/16f993bb8bcfd59af7ac24dbeaa4bec51ead6950/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_cpp rosbag2_examples_cpp rosbag2_examples_py rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_compression rosbag2_cpp rosbag2_examples_cpp rosbag2_examples_py rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13240

• Linux
• Linux-aarch64
• Windows

[DLu]

There's not much information in the migration notes, and all the examples seem to just use the value 0. So if I'm writing a bag file from scratch (using Python say), should each TopicMetaData i construct use id=0?

MetroRobots/classic_bags#16

[MichaelOrlov]

The topic_id shall be unique for each topic. The numbering is implementation details for storage plugins.
In the following example

        topic_info = rosbag2_py._storage.TopicMetadata(
            id=0,
            name='synthetic',
            type='example_interfaces/msg/Int32',
            serialization_format='cdr')
        self.writer.create_topic(topic_info)

it is assigned to 0 because it will be overwritten by the storage plugin after a call to the writer.create_topic(topic_info)
You can get assigned value by checking topic_info.id after writer.create_topic(topic_info).

Therefore - no special steps for migration are required.

[DLu]

I guess what I'm trying to figure out is why zero is not the default value, if it is just going to be overwritten. Then the TopicMetadata constructor could have stayed the same.

[MichaelOrlov]

Well, it is zero-initialized by default. The TopicMetadata is a struct and doesn't have any explicit constructors.
However, the constructor could be added.
As always, PRs with improvements are welcome!

[DLu]

In the Python bindings introduced by @r7vme in #1569, there is an explicit constructor, and id is first argument. However, I will admit I'm not fully familiar with "stubs" or the ... notation.