Playing of bag which is recorded with split by duration/size is not working properly .

[Narendarselva]

Description

Playing of bags recorded with split by duration/size is not playing properly.
It plays only the last recorded .db3.

Expected Behavior

It should play all the .db3 files which are recorded instead of last recorded .db3

Actual Behavior

Let say we recorder a bag with duration split of 10 sec for 50 secs.
Then we will get a total of 5 .db3 files recorded at regular 10 secs interval.
When we try to replay the bag only the last recorded db3 alone is playing.

To Reproduce

Record a bag with duration split:
-- ros2 bag record -a -d 10
Play the same recorded bag:
-- ros2 bag play

System (please complete the following information)

• OS: Ubuntu 20.04
• ROS 2 Distro: galactic
• Version: master

Additional context

[Narendarselva]

Issue is due to updating overall starting_time & duration of metadata with the last recorded db3 files start_time & duration.

index b53a554..f352739 100644
--- a/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp
+++ b/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp
@@ -296,18 +296,14 @@ void SequentialWriter::write(std::shared_ptr<rosbag2_storage::SerializedBagMessa
split_bagfile();

 // Update bagfile starting time

• metadata_.starting_time = std::chrono::high_resolution_clock::now();
  metadata_.files.back().starting_time = std::chrono::high_resolution_clock::now();
  }

const auto message_timestamp = std::chrono::time_pointstd::chrono::high_resolution_clock(
std::chrono::nanoseconds(message->time_stamp));

• metadata_.starting_time = std::min(metadata_.starting_time, message_timestamp);

  metadata_.files.back().starting_time =
  std::min(metadata_.files.back().starting_time, message_timestamp);

• const auto duration = message_timestamp - metadata_.starting_time;

• metadata_.duration = std::max(metadata_.duration, duration);

  const auto file_duration = message_timestamp - metadata_.files.back().starting_time;
  metadata_.files.back().duration =
  @@ -362,7 +358,7 @@ bool SequentialWriter::should_split_bagfile() const
  auto max_duration_ns = std::chrono::duration_caststd::chrono::nanoseconds(
  std::chrono::seconds(storage_options_.max_bagfile_duration));
  should_split = should_split ||

•  ((std::chrono::high_resolution_clock::now() - metadata_.starting_time) > max_duration_ns);
  

•  ((std::chrono::high_resolution_clock::now() - metadata_.files.back().starting_time) > max_duration_ns);
  

  }

  return should_split;
  @@ -371,6 +367,10 @@ bool SequentialWriter::should_split_bagfile() const
  void SequentialWriter::finalize_metadata()
  {
  metadata_.bag_size = 0;

• metadata_.starting_time = metadata_.files.front().starting_time;

• for (const auto & file : metadata_.files) {

• metadata_.duration += file.duration;
  

• }

Added the fix for the same.

[Kaju-Bubanja]

This is still an issue in master, could you make a PR or reformat your fix so that it is clearer what needs to be changed where, so that I could make a PR out of it. You can paste code using triple backticks. Also do I understand this right, that your fix is only for the writer and already recorded bags with the split option can't be played back from the first bag?

[Kaju-Bubanja]

Ok, there is a simple fix for already recorded bags. Just replace the starting_time in the metadata.yaml with the starting time of the first bag, instead of the last bag. After that everything plays as it should.

[fujitatomoya]

@Kaju-Bubanja @MichaelOrlov can we backport this fix to humble? i understand #1022 breaks the ABI compatibility, so asking for help if we can go another path to keep it. how about deciding if that is the 1st message using with metadata_.starting_time? i think that is doable?

[Kaju-Bubanja]

We are using humble, so I would be interested in a backport. But I am full with work in the foreseeable future and won't have capacity to backport it. Only later in the year could I put aside some time, which is probably too late for it to be useful for humble

[fujitatomoya]

actually humble branch has is_first_message_ flag already (came in with #1037), so we will not break ABI compatibility at all. i can come up with the quick fix.