Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Progress-bar for ros2 bag play #1836

Draft
wants to merge 5 commits into
base: rolling
Choose a base branch
from

Conversation

nicolaloi
Copy link
Contributor

@nicolaloi nicolaloi commented Oct 15, 2024

Closes #1762: Adds a progress bar for ros2 bag play, showing the bag time and duration, similar to what is seen in ROS1.

In ROS1, the progress bar was easy to handle because the rosbag play command didn't print additional output during playback. However, in ROS2, many logs can be printed during playback (e.g., keyboard control logs).

The progress bar is printed and continuously updated on the last line of the playback output. I have tested this initial implementation with keyboard controls (play, pause, play next, set rate), Ctrl+C, throwing an exception, and the flags --log-level debug, --delay X, and --start-offset X.

To try it, use the new --progress-bar flag when playing a bag.

@nicolaloi
Copy link
Contributor Author

nicolaloi commented Oct 15, 2024

video-example.mp4

(at 2.6-2.7 seconds of bag duration, the progress bar is slightly updating while in paused state because I was clicking the left arrow for the play next keyboard control)

Since ros2 bag play generates more logs than its ROS1 counterpart, the key aspect of this PR is to print a string shorter than any potential log message not coming from rosbag2_player. For example, with the current draft of the progress bar, this prints nicely:

[INFO] [1729028986.845909253] [rosbag2_player]: Pausing play.
 Bag Time 1718553782.525239  Duration 1.272325/6.974378 [P]

while this longer version of the progress bar doesn't work properly without extra line clearing (in this case, it's easy to clear the line since the logs come from rosbag2_player., but for "external" logs, this becomes more difficult):

[INFO] [1729028986.845909253] [rosbag2_player]: Pausing play.us:Running]
 Bag Time 1718553782.525239  Duration 1.272325/6.974378 [status:Paused]

@MichaelOrlov looking forward to your feedback, for now I have made a test MVP code to check the feasibility of the feature.

Signed-off-by: Nicola Loi <[email protected]>
@nicolaloi nicolaloi force-pushed the nicolaloi/bag-play-progress-bar branch from d503e5f to 2cb732d Compare October 15, 2024 22:24
@nicolaloi nicolaloi changed the title Start adding progress-bar for ros2 bag play [wip] Progress-bar for ros2 bag play Oct 16, 2024
@nicolaloi nicolaloi force-pushed the nicolaloi/bag-play-progress-bar branch from b815f9e to 4ba1a8c Compare October 20, 2024 22:56
@nicolaloi nicolaloi changed the title [wip] Progress-bar for ros2 bag play Progress-bar for ros2 bag play Oct 21, 2024
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolaloi Thank you for your contribution.
I have a few concerns about the design and implementation.
First, we need to limit the frequency with which we will print in std::cout. The best way to do this is to create a local variable, aka size_t progress_bar_update_freq_ (in Hz).
e.g. 1 Hz = once per/sec, 2 Hz = twice per second, 3 = three times per second etc..
Then, check the delay between the last call in the print_status(..). If the delay is more than the threshold, print; otherwise, skip.
We also need to move the check for the if (disable_progress_bar_) inside print_status(..) to make the logic cleaner and in one place.
Calling double current_time_secs = RCUTILS_NS_TO_S(static_cast<double>(clock_->now())); each time in the print_status(..) is very inefficient and expensive since we are locking mutex inside and transorming current time to the ROS time. We actually know the current ROS time at which we are playing. This is a timestamp from the message. I would suggest passing it to the print function as a parameter.
i.e., add tiemstamp parameter to the print_status(..) function and use message_ptr->recv_timestamp as a source of timestamp.

I found the implementation of the print_status(..) function a bit messy.
That is a quintessence of the modern C++17 with templates and old style plain C code.
Need to avoid using C code. Please use C++ alternatives. I am not sure if we really need to flush buffer each time. Flush is an expensive operation since this is a blocking system call. Need to avoid it if possible.

IMO templating print_status(..) function is overengineering and make code less readable and less understandable. The cleaner solution would be if using enum class values for different modes. In this case all extra wrappers like check_and_print_status() will not be needed. We will be able to call print_status(PrintStatus::Paused) directly from where we needed it.

rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
@nicolaloi
Copy link
Contributor Author

@MichaelOrlov thanks for the comments.

  • limit printing frequency, mutex and flushing: yes I already had a local change to allow the user to choose the update frequency from the CLI (default 10-20 Hz) to reduce the mutex locking and output stream flushing. But indeed using the timestamp of the last published message is better than calling clock_->now().
    -- The rationale of clock_->now() was to be able to have a consistent time flow if you want to print the progress bar at a fixed rate but messages are not close in time. For example, if the next message is 3 seconds in the future, for the next 3 seconds the progress bar time will appear frozen since it is printing the same timestamp. But yes in practice is a niche case.
    -- Regarding the flush, updating the progress bar without flushing will not update the printed time (or anyway it will get flushed automatically after some update calls, but there can be artifacts if other external ros logs are being printed). So instead of calling the progress bar without flushing, I would just reduce the calls to the progress bar printing with the already mentioned user-defined frequency update. I would say that 10-20 Hz can be a good default value? Or even lower to be conservative?
  • add timestamp parameter to the print_status: for this I would save it as a class attribute and use it within print_status instead of adding it as an input, since in many instances where print_status is being called there is no direct easy access to the last message.
  • print_status implementation: ok I will remove the template and use enums.

Should the progress bar be disabled by default, or enabled by default with a default low update frequency?

@nicolaloi nicolaloi force-pushed the nicolaloi/bag-play-progress-bar branch 2 times, most recently from 253256a to 44fa94a Compare October 28, 2024 23:35
nicolaloi and others added 2 commits October 28, 2024 23:38
Signed-off-by: Nicola Loi <[email protected]>
Signed-off-by: Nicola Loi <[email protected]>
@nicolaloi nicolaloi force-pushed the nicolaloi/bag-play-progress-bar branch from 44fa94a to 43fa0f8 Compare October 28, 2024 23:38
@nicolaloi
Copy link
Contributor Author

I have made changes as discussed.

Linked modifications:

I have modified the play_next function to accept a boolean input argument 'is_burst_mode' (default to false); so when play_next is called in burst mode, the print_status is not called at all, to avoid any delay (but now the progress bar will freeze during burst).

print_status will update the progress bar at the defined update frequency if the status is RUNNING; for the other states (or if the new boolean argument 'force_update' is true) it will update it disregarding the update frequency. This is to take care of the istances where a new log message from the rosbag player is being printed (like 'rate changed to 1.1', 'player paused', warnings..., anyway these are not high-frequency logging) to make the progress bar visible again. Otherwise, it will temporarily or permanently (permanently if the player gets paused) disappear since it is overwritten in the terminal by the previous log message.

@MichaelOrlov
Copy link
Contributor

@nicolaloi I have not had a chance yet to look deeply into the code after your latest changes. However, I have a few comments.

  1. I am ok with a flush; I anticipated that it is likely some constraints as you described.
  2. Making the progress bar enabled by default is a good idea. However, the frequency should be of integer type (size_t) in C++, and 10 Hz IMO is to fast. I would put 3-5 Hz at max by default.
  3. We don't need flag play_options.disable_progress_bar at all we can rely on the play_options.progress_bar_update rate. i.e. if it is 0 = disabled.

@nicolaloi
Copy link
Contributor Author

Ok, I'll set the default rate to 3 Hz.

For 2nd and 3rd points, there is no longer the --disable_progress_bar option, but a --progress_bar option that saves the frequency rate as a double, and if the value is ==0 the progress bar is disabled, and if <0 it is updated for every message. So the user has the extra option to enable it for each message if they need to, without having to set a large enough frequency rate. If it's not okay I'll change to size_t as you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add progress bar and timestamp when playing back a bag
2 participants