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

Add play_next() API to the player class #762

Merged
merged 15 commits into from
May 13, 2021

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Apr 29, 2021

Added implementation for play_next() API

Part of the #696

Ready for review.

@MichaelOrlov
Copy link
Contributor Author

@emersonknapp FYI

@MichaelOrlov
Copy link
Contributor Author

MichaelOrlov commented Apr 29, 2021

@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
Copy link
Contributor Author

MichaelOrlov commented Apr 29, 2021

@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.

@MichaelOrlov MichaelOrlov force-pushed the michaelorlov/play_next branch 2 times, most recently from bd875c8 to c4019f9 Compare April 30, 2021 07:04
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>
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 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 MichaelOrlov marked this pull request as ready for review May 2, 2021 05:17
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner May 2, 2021 05:17
@MichaelOrlov MichaelOrlov requested review from mjeronimo, david-prody, emersonknapp and Karsten1987 and removed request for a team May 2, 2021 05:17
@MichaelOrlov
Copy link
Contributor Author

@emersonknapp @Karsten1987 Gentle ping for review.

@MichaelOrlov
Copy link
Contributor Author

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 Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

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.

…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
Copy link
Contributor Author

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

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

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

@MichaelOrlov
Copy link
Contributor Author

MichaelOrlov commented May 13, 2021

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 Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor Author

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

@MichaelOrlov MichaelOrlov merged commit c488315 into ros2:master May 13, 2021
@MichaelOrlov MichaelOrlov deleted the michaelorlov/play_next branch May 13, 2021 02:24
@Kaju-Bubanja
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

MichaelOrlov commented Jun 9, 2021

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

  1. Player::jump(ros_time_point). It will jump to arbitrary time point of recording without replaying messages.
  2. 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.

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.

3 participants