Provide BagMetadata to the Player and deserialize QoS profiles for la…

[emersonknapp]

Part of #125

Reads and provides the BagMetadata to the Player, which stores it and deserializes the QoS profiles that it contains (if present). Does not change behavior, just makes the information available for use in the followup to this PR.

Signed-off-by: Emerson Knapp [email protected]

[piraka9011]

LGTM

[thomas-moulard]

Code looks OK. This requires some testing IMHO, or was it already covered somehow?

[thomas-moulard]

const auto& to avoid the copy

[thomas-moulard]

What about the unhappy path? I.e. YAML is not correct. Did we test this? Are we printing out a reasonable error message?

I would also suggest to update the documentation to explicitly state what type of exception may get thrown from the ctor. Seems like we're moving from sonething that never could fail, to something that can "easily" fail (given bad input). It's worth checking the implication on the codebase.

[thomas-moulard]

Is this really OK not to have metadata?

[piraka9011]

legacy rosbags

[zmichaels11]

ROS1 bags loaded with rosbag2_bag_v2 do not have metadata

[Karsten1987]

I somewhat have a problem with that change here as I think we duplicate work here. The reader instance does open the storage and receives the metadata itself. I don't see the point - and maybe that's being more clear with the succeeding PR - why we would open the metadata in the reader and in the player. Why not providing a method on the reader interface to get the metadata?

I am a bit worried that in the end we not only duplicate work, but also that if at some point we implement a different behavior in reading the metadata we have to make sure there's no inconsistency between rosbag2_transport and rosbag2_cpp. I am having for example the legacy rosbags in mind, but also any other storage plugin.

[emersonknapp]

@Karsten1987 the reader does provide get_all_topics_and_types which returns a std::vector<rosbag2_storage::TopicMetadata>, which theoretically contains the offered_qos_profiles. However, the sqlite storage plugin reads this metadata from the database, not the yaml file. Essentially right now we have all metadata duplicated between the YAML file and the database.

My understanding from a previous conversation is that the YAML file should be the only place that we continue updating, and that the sqlite database version of the metadata is there for legacy reasons only. If this is true, then we should probably remove the portion of the writer that writes metadata to the database. In the PR where I added the offered_qos_profiles field, I did update the sqlite storage plugin to write the new field into the database metadata, but I did this because I didn't realize the situation that we're in.

What should be the one true source of metadata? If it's the YAML file, then I can change the base reader to read it from the YAML file, instead of having the SQLite storage plugin override it to read from the database?

[Karsten1987]

However, the sqlite storage plugin reads this metadata from the database, not the yaml file. Essentially right now we have all metadata duplicated between the YAML file and the database.

Looking here I don't think this is true. The sqlite3 plugin (in fact every plugin) reads from the metadataio the content from the .yaml file if present. And I think that should be a only real source to do so. That's why I was proposing the getter function.

The reason why we still have the get_metadata() function on the storage interface is motivated by the fact that not every plugin has this metadata.yaml file, i.e. the ros1 bag files.
As discussed in #213 I was initially advocating to remove that function all together. But that would have implications to legacy bag files.
To that extend, I think we could though provide a standalone tool which generates the metadata.yaml for legacy bag files in order to finally get rid of it. But that's content for another discussion.

[emersonknapp]

Looking here I don't think this is true. The sqlite3 plugin (in fact every plugin) reads from the metadataio the content from the .yaml file if present.

You're right that it reads the YAML file into the private variable metadata_ there, but from there it's not used via existing interfaces.

The BaseReaderInterface provides get_all_topics_and_types, but this calls into the storage, which implements this as a database read

Should I add an interface get_metadata to the BaseReaderInterface to return its correctly-loaded metadata? And, change the SequentialReader's implementation of get_all_topics_and_types to also use this metadata instead of asking the storage? This seems like a sensible solution, since the initial metadata loading had the opportunity to ask the storage, if the yaml file was not found.

[Karsten1987]

Should I add an interface get_metadata to the BaseReaderInterface to return its correctly-loaded metadata? And, change the SequentialReader's implementation of get_all_topics_and_types to also use this metadata instead of asking the storage? This seems like a sensible solution, since the initial metadata loading had the opportunity to ask the storage, if the yaml file was not found.

Yes! To all :) I think this makes a lot of sense. But as with the metadata itself, I think we can't easily remove that function get_all_topics_and_types that easily from the storage interface, again because of the legacy ros1 bags.

[emersonknapp]

Agreed that we can't remove the storage interface right now, but we can skip using it here. I'll go ahead and update the SequentialReader interfaces.

[emersonknapp]

Closing in favor of exposing this data through the Reader - starting here #372

[Karsten1987]

@emersonknapp do you want to keep this branch alive or can it be deleted?

[emersonknapp]

Deleted