Add play_next() API to the player class #762

MichaelOrlov · 2021-04-29T01:04:57Z

Added implementation for play_next() API

Part of the #696

Ready for review.

MichaelOrlov · 2021-04-29T01:05:35Z

MichaelOrlov · 2021-04-29T20:48:35Z

@emersonknapp It turns out that it's not so easy to implement this feature. Simple clock->jump() doesn't guarantee that next message will be played.
In the current implementation it's only guarantee that message will be processed from queue but it could be skipped from publishing due to the filtration applied to the topics.

I am also worry about undefined behavior when more than one message recorded with the same timestamp.
Currently this situation unlikely to happen since we are using single threaded executor for processing subscriptions during recording but sill possible because we are using system_clock for assigning timestamp for received message. In theory system_clock could be adjusted back in time and next message could have the same timestamp as the previous one.
The probability to get messages with the same timestamp will increase significantly if we will start using some multi-threaded executor or similar add-hog approach for processing messages from subscriptions.

Just want to let you know that I am still WIP on the core implementation for this feature.
I am trying to solve at least problem with guarantee to really play next message rather than just process it from queue.

MichaelOrlov · 2021-04-29T22:08:46Z

@emersonknapp It seems I've fixed problem with skipped message by topic filtration in play_next().
Also partially solved problem with undefined behavior when more than one message recorded with the same timestamp.
At least re-qualified it after fixes to the disclaimer: "There are no guarantee that only one message will be played if bag will have multiple messages with the same timestamp going in the sequence."

Could you please briefly take a look on the implementation? If you don't have major concerns - I will focus on unit tests.

Update: I removed constraints for messages recorded with the same timestamp and even added special unit test for this case.

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

…e published. Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

…iltration. Also partially solved issue with undefined behaviour when more than one message recorded with the same timestamp. Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

…ying queue do reinterpret_cast to internal buffer when peeking element. Signed-off-by: Michael Orlov <morlovmr@gmail.com>

…eue_ flag for unit tests Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

…fter_play_next test Signed-off-by: Michael Orlov <morlovmr@gmail.com>

MichaelOrlov · 2021-05-05T06:59:28Z

@emersonknapp @Karsten1987 Gentle ping for review.

MichaelOrlov · 2021-05-11T20:16:06Z

Gist: https://gist.githubusercontent.com/MichaelOrlov/c3b0bf9b1ae3b8d91512033016be367c/raw/cca877b3fa1f8044e93ce81b2b6aa630a5144767/ros2.repos

BUILD args: --packages-up-to rosbag2_cpp rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_cpp rosbag2_transport rosbag2_tests rosbag2
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8432/

Linux
Linux-aarch64
macOS
Windows

emersonknapp

Sorry for the slow feedback - let's try and iterate on this quickly this week.

It looks good, but I had some comments I'd like to address before we merge.

rosbag2_transport/include/rosbag2_transport/player.hpp

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_transport/include/rosbag2_transport/player.hpp

rosbag2_test_common/include/rosbag2_test_common/subscription_manager.hpp

rosbag2_transport/test/rosbag2_transport/test_play_next.cpp

rosbag2_test_common/include/rosbag2_test_common/subscription_manager.hpp

rosbag2_transport/include/rosbag2_transport/player.hpp

…ayback mode. i.e. Player::play() hasn't been started Signed-off-by: Michael Orlov <morlovmr@gmail.com>

…he private publish_message(message) method Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

…r_matched Signed-off-by: Michael Orlov <morlovmr@gmail.com>

MichaelOrlov · 2021-05-12T00:50:31Z

@emersonknapp I've addressed your comments and suggestions for changes. Please verify or do a second round of review.

emersonknapp

This looks great - thanks for the revision! Feel free to merge after verifying green ci_launcher

MichaelOrlov · 2021-05-13T01:05:55Z

Rerunning CI after making changes per review.
Gist: https://gist.githubusercontent.com/MichaelOrlov/c3b0bf9b1ae3b8d91512033016be367c/raw/cca877b3fa1f8044e93ce81b2b6aa630a5144767/ros2.repos

BUILD args: --packages-up-to rosbag2_cpp rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_cpp rosbag2_transport rosbag2_tests rosbag2
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8445/

Linux
Linux-aarch64
macOS
Windows

MichaelOrlov · 2021-05-13T01:07:37Z

@emersonknapp Thank you for the review and very useful suggestions.

Kaju-Bubanja · 2021-06-09T15:37:39Z

Awesome feature, thanks a bunch, I'm actively using it. Do you plan on doing the reverse too? Like play_previous()? Would it be difficult to implement? Say in general but also just for some buffer like the last 10 messages? Because in my use case and I assume the most common one, one has a tricky situation and wants to look at the data in detail while playback is paused. Then I step forward and encounter something special and then I would like to go back just 1 frame without having to replay the whole bag.

emersonknapp · 2021-06-09T18:56:45Z

Do you plan on doing the reverse too? Like play_previous()?

We would love to support this feature in rosbag2 - but there isn't active development on it at the moment.

Would it be difficult to implement?

It may require a little design work to figure out how to make this work - we'll also need the same capability for arbitrary jump-in-time, or if we wanted to play bags backward. Currently the rosbag2_storage API only reads linearly in one direction, so the player is structured that way as well, in terms of how its reading/cacheing is set up. There is no technical limitation on the SQLite3 backend that enforces this, though, and I imagine many other possible backends would be fine as well.

MichaelOrlov · 2021-06-09T20:58:25Z

Hi @Kaju-Bubanja. I think your case with playing previous message will be covered with sequence of API calls:

Player::jump(ros_time_point). It will jump to arbitrary time point of recording without replaying messages.
Player::play_next()

The idea is that you will be able to advance internal player clock in some time point in the past and replay messages one by one.

However Player::jump(ros_time_point) feature is in WIP and not available yet.

MichaelOrlov force-pushed the michaelorlov/play_next branch 2 times, most recently from bd875c8 to c4019f9 Compare April 30, 2021 07:04

MichaelOrlov added 4 commits April 30, 2021 12:32

Add play_next() API to the player class

7d6f97a

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Make play_next() as blocking call. Add wait until next message will b…

43f2034

…e published. Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Simplified logic in play_next().

0f19e9a

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Fix for issue with unplayed message in play_next() due to the topic f…

b6853c6

…iltration. Also partially solved issue with undefined behaviour when more than one message recorded with the same timestamp. Signed-off-by: Michael Orlov <morlovmr@gmail.com>

MichaelOrlov force-pushed the michaelorlov/play_next branch from c4019f9 to b6853c6 Compare April 30, 2021 19:32

MichaelOrlov added 7 commits May 1, 2021 13:01

Put skip_message_in_main_play_loop_ under the TSA guard and some renames

4132d6f

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Scoped shared_ptr for message to avoid memory corruption since underl…

ee356cd

…ying queue do reinterpret_cast to internal buffer when peeking element. Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Move publishers to protected section and add playing_messages_from_qu…

3e715d6

…eue_ flag for unit tests Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Add spin_and_wait_for_matched in SubscriptionManager

40e9764

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Add unit tests for play_next()

1529502

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Add lower bound check for expected playback time after play_next() call

6656c1d

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Add one more play_next() iteration for play_respect_messages_timing_a…

8cf1d4e

…fter_play_next test Signed-off-by: Michael Orlov <morlovmr@gmail.com>

MichaelOrlov changed the title ~~(WIP) Add play_next() API to the player class~~ Add play_next() API to the player class May 2, 2021

MichaelOrlov marked this pull request as ready for review May 2, 2021 05:17

MichaelOrlov requested a review from a team as a code owner May 2, 2021 05:17

MichaelOrlov requested review from mjeronimo, david-prody, emersonknapp and Karsten1987 and removed request for a team May 2, 2021 05:17

MichaelOrlov self-assigned this May 5, 2021

This was referenced May 5, 2021

[Epic] Rosbag2 playback time control and simulation time #696

Closed

Expose play_next() API via service #767

Merged

emersonknapp requested changes May 11, 2021

View reviewed changes

MichaelOrlov added 4 commits May 11, 2021 15:52

Add constraint that play_next() will return false if player not in pl…

dc78e93

…ayback mode. i.e. Player::play() hasn't been started Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Move part of the code responsible for the publishing one message to t…

6bd90a1

…he private publish_message(message) method Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Fix for undefined behaviour when resume() call happen during play_next()

adec837

Signed-off-by: Michael Orlov <morlovmr@gmail.com>

Replace std::list with std::vector for publishers in spin_and_wait_fo…

1b8fdcf

…r_matched Signed-off-by: Michael Orlov <morlovmr@gmail.com>

emersonknapp approved these changes May 13, 2021

View reviewed changes

MichaelOrlov merged commit c488315 into ros2:master May 13, 2021

MichaelOrlov deleted the michaelorlov/play_next branch May 13, 2021 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add play_next() API to the player class #762

Add play_next() API to the player class #762

MichaelOrlov commented Apr 29, 2021 •

edited

Loading

MichaelOrlov commented Apr 29, 2021

MichaelOrlov commented Apr 29, 2021 •

edited

Loading

MichaelOrlov commented Apr 29, 2021 •

edited

Loading

MichaelOrlov commented May 5, 2021

MichaelOrlov commented May 11, 2021

emersonknapp left a comment

MichaelOrlov commented May 12, 2021

emersonknapp left a comment •

edited

Loading

MichaelOrlov commented May 13, 2021 •

edited

Loading

MichaelOrlov commented May 13, 2021

Kaju-Bubanja commented Jun 9, 2021

emersonknapp commented Jun 9, 2021

MichaelOrlov commented Jun 9, 2021 •

edited

Loading

Add play_next() API to the player class #762

Add play_next() API to the player class #762

Conversation

MichaelOrlov commented Apr 29, 2021 • edited Loading

MichaelOrlov commented Apr 29, 2021

MichaelOrlov commented Apr 29, 2021 • edited Loading

MichaelOrlov commented Apr 29, 2021 • edited Loading

MichaelOrlov commented May 5, 2021

MichaelOrlov commented May 11, 2021

emersonknapp left a comment

Choose a reason for hiding this comment

MichaelOrlov commented May 12, 2021

emersonknapp left a comment • edited Loading

Choose a reason for hiding this comment

MichaelOrlov commented May 13, 2021 • edited Loading

MichaelOrlov commented May 13, 2021

Kaju-Bubanja commented Jun 9, 2021

emersonknapp commented Jun 9, 2021

MichaelOrlov commented Jun 9, 2021 • edited Loading

MichaelOrlov commented Apr 29, 2021 •

edited

Loading

MichaelOrlov commented Apr 29, 2021 •

edited

Loading

MichaelOrlov commented Apr 29, 2021 •

edited

Loading

emersonknapp left a comment •

edited

Loading

MichaelOrlov commented May 13, 2021 •

edited

Loading

MichaelOrlov commented Jun 9, 2021 •

edited

Loading