Add support for replaying based on publication timestamp

[christophebedard]

• This is a follow-up to Add support for replaying multiple bags #1848.

This PR adds support for replaying messages chronologically based on their publication timestamp, i.e., rosbag2_storage::SerializedBagMessage::send_timestamp. To enable this, this PR adds a new enum option to rosbag2_transport::PlayOptions: MessageOrder message_order. The possible enum values are:

1. RECEIVED_TIMESTAMP: order chronologically by message reception timestamp, which is the default and the current behaviour
2. SENT_TIMESTAMP: order chronologically by message publication timestamp

This is exposed through ros2 bag play with a new --message-order {received,sent} option. The choices correspond to the MessageOrder values, with received (RECEIVED_TIMESTAMP) being the default.

This relies on the std::priority_queue added in #1848 to order messages. Since there is a chance that messages could still not be correctly ordered (mostly with SENT_TIMESTAMP), I mentioned it in the documentation for rosbag2_transport::PlayOptions::message_order documentation and the ros2 bag play --message-order option. I also mentioned increasing the read_ahead_queue_size value as a possible solution.

Note: This PR is not backportable since it has ABI incompatible changes, because we added a new field to the rosbag2_transport::PlayOptions structure and as a consequence changed the size of the PlayOptions structure.

[christophebedard]

I totally forgot to update the timestamp that the Player class uses based on the MessageOrder 🤦 I'll do that and add a test to cover it, because the current test was still passing.

[christophebedard]

I'll do that and add a test to cover it, because the current test was still passing.

Done.

[MichaelOrlov]

@christophebedard Thank you for your contribution.

• What is missing yet is the handling of the newly added play_options.message_order field in the get_play_options_from_node_params(..).
  Please add conversion from node parameters into the rosbag2_transport/src/rosbag2_transport/config_options_from_node_params.cpp
  also, in tests in the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_transport/test/rosbag2_transport/test_composable_player.cpp#L177-L181
  and in the resource file https://github.com/ros2/rosbag2/blob/rolling/rosbag2_transport/test/resources/player_node_params.yaml#L40-L43

• Also, I have a concern about naming MessageOrder::RECV_TIMESTAMP and wording recv. As usual, the naming is hard. However, let's try to keep it more readable and understandable, especially the user-facing API.
  I would like to ask you to rename all new declarations and usage of the RECV_TIMESTAMP, recv and corresponding CLI option to the more readable and understandable received and RECEIVED_TIMESTAMP.
  I know you were trying to be consistent and follow the naming of the underlying SerializedBagMessage::recv_timestamp. However, in my opinion, this is not the case when we need to follow consistency and sacrifice readability and understandability.
  Initially, we shortened received_timestamp to the recv_timestamp to be aligned with its other counterpart, send_timestamp, by the number of characters in names. And we did it for the following reasons:

1. It looks nice in the code when those two variables are aligned by the length, for instance in ceases:

   output_message->recv_timestamp = message->recv_timestamp;
   output_message->send_timestamp = message->send_timestamp;
   

   or

    const rcutils_time_point_value_t & recv_timestamp,
    const rcutils_time_point_value_t & send_timestamp);
   

2. The SerializedBagMessage::recv_timestamp is part of the rosbag2_storage namespace and sort of implementation details, and it is used a lot, mostly on underneath layers.

Although with the PlayOptions::message_order the situation is different.

1. We expose it for users as a structure for Player settings.
2. It is used mostly on upper layers rosbag2_transport and rosbag2_py
3. In the current implementation, there are no places where a nice alignment of the enum definition or CLI option would really matter or look nicer than without alignment.

[christophebedard]

Also, I have a concern about naming MessageOrder::RECV_TIMESTAMP and wording recv

Changed recv to received in 93ba439

I'm a bit annoyed that we're using "received" (past tense) and "send" (present tense), but that's how the timestamps are named.

[christophebedard]

What is missing yet is the handling of the newly added play_options.message_order field in the get_play_options_from_node_params(..).
Please add conversion from node parameters into the rosbag2_transport/src/rosbag2_transport/config_options_from_node_params.cpp
also, in tests in the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_transport/test/rosbag2_transport/test_composable_player.cpp#L177-L181
and in the resource file https://github.com/ros2/rosbag2/blob/rolling/rosbag2_transport/test/resources/player_node_params.yaml#L40-L43

Thanks, I missed that. I added that in c6f02bf

[MichaelOrlov]

@christophebedard As regards:

I'm a bit annoyed that we're using "received" (past tense) and "send" (present tense), but that's how the timestamps are named.

I agree. May I ask you to rename it to the past tense sent as well? (For a newly added code only in the scope of this PR)

[christophebedard]

I agree. May I ask you to rename it to the past tense sent as well? (For a newly added code only in the scope of this PR)

done in 57970c0

[MichaelOrlov]

• @christophebedard We may have a problem with seek see detailed explanation in the "Time normalize" bag written from API so that splits are in order #842. But it sounds like a followup to consider. Likely not in the scope of this PR.

[christophebedard]

I could mention this new change in that issue, since it affects it.

Also, that reminds me of the logic for starting_time_:

rosbag2/rosbag2_transport/src/rosbag2_transport/player.cpp

Lines 429 to 435 in c8feaea

	// Find earliest starting time
	const auto metadata = reader->get_metadata();
	const auto metadata_starting_time = std::chrono::duration_cast<std::chrono::nanoseconds>(
	metadata.starting_time.time_since_epoch()).count();
	if (metadata_starting_time < starting_time_) {
	starting_time_ = metadata_starting_time;
	}

. Is a bag's starting_time the recv_timestamp of the first message? Since we call reader->seek(starting_time_); and clock_->jump(starting_time_); before playing, but call clock_->jump(get_message_order_timestamp(message_ptr)); (which could use the send_timestamp), couldn't that lead to incorrect replaying?

[MichaelOrlov]

@christophebedard As regards:

Since we call reader->seek(starting_time_); and clock_->jump(starting_time_); before playing, but call clock_->jump(get_message_order_timestamp(message_ptr)); (which could use the send_timestamp), couldn't that lead to incorrect replaying?

No. The replaying order will be preserved.
We uses clock_->jump(get_message_order_timestamp(message_ptr)) only from play next where we ignore wait for message timestamp. In case if we will do resume after the play_next the next message timestamp will be in order since it is will be from the same priority queue and call for the clock_->sleep_utill(timestamp) will be correct.

The worst case scenario would be when we do reader->seek(starting_time_); and clock_->jump(starting_time_) at the very beginning before the first call for the clock_->sleep_until(get_message_order_timestamp(message_ptr)) and get_message_order_timestamp(message_ptr) will return timestamp in the past of the current clock time.
But nothing bad will happen we just will not sleep at all and publish the message right away, and this is expected behavior during the start of the payback.

[MichaelOrlov]

@christophebedard Thank you for the PR and for addressing my review comments.
Now it looks good to me.

[christophebedard]

Should I mention the new --message-order option in the README? https://github.com/ros2/rosbag2#replaying-data. Although I see that not all options are mentioned.

[MichaelOrlov]

Oh yes, I completely forgot about readme file. Yes please update it.
Updates for other options also will be appreciated.

[christophebedard]

See 71474ca. I only documented the main options.

[MichaelOrlov]

Pulls: #1876
Gist: https://gist.githubusercontent.com/MichaelOrlov/02d513e3cf1e39df4f7bfe98c42447ec/raw/efaa698a57b3fa3c0dedf99e81bbbade1a474082/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_transport
TEST args: --packages-above ros2bag rosbag2_py rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14918

• Linux
• Linux-aarch64
• Linux-rhel
• Windows