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

Use Reader's seek() method for seeking/jumping in Player #873

Merged
merged 2 commits into from
Sep 29, 2021

Conversation

lihui815
Copy link
Contributor

@lihui815 lihui815 commented Sep 23, 2021

Closes #824

The current Player->seek() was implemented before storage and by extension the reader exposed seek interface (#824). This PR updates the implementation to use the Reader's seek() method, which reduces the implementation complexity.

Other details:

  • Reader's seek() method does not require closing and reopening. Thus it's preferable to just keep the reader open from the constructor, and close it in the destructor. Other methods can just assume an open reader and set seek() as needed.
  • Removed the release_reader() method as the player is basically useless after releasing the reader anyway. This removes one more way the reader can get closed before play ends.
  • Player seek() method no longer special cases seeking to a timestamp after the end of the bag. Previously, if no messages are found after the seek time, the Player will reset to the original time stamp before the seek method was called. Now, seek will always be performed, and seeking to a later timestamp will either end bag play or loop according to player options. This makes seek behavior to be consistent with the behavior of playing until the end of the bag. There is also no reason to return true/false depending on the success of seek, as the seek will always be successful.

@lihui815 lihui815 requested a review from a team as a code owner September 23, 2021 19:51
@lihui815 lihui815 requested review from mjeronimo and removed request for a team September 23, 2021 19:51
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 implementation is so nice and clean - I love it!

I have one concern related to the edge-case behavior noted in the Player::seek docstring comment

If we seek to a time before the bag, e.g. bag_start minus 1 minute - this implementation sets the clock back there, which means that the full minute will have to elapse in real time before the beginning of the bag starts playing. I am thinking that we should "clamp" the clock->jump calls to the bag_start/end range. What do you think?

rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
@emersonknapp
Copy link
Collaborator

emersonknapp commented Sep 25, 2021

Sanity check build just to catch any weird compiler xplatform stuff early

Gist: https://gist.githubusercontent.com/emersonknapp/d58facdff381d74d9fc79f29e4165abc/raw/789a662a1682a36f3a38dcf78ef6dd7b34dc3604/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_transport rosbag2_tests rosbag2
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9083

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Sonia Jin <diegothemuich@gmail.com>
…s left) while in pause state

Signed-off-by: Sonia Jin <diegothemuich@gmail.com>
@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/d4c53f3799483d9438a478bdc793b532/raw/2d45f2d76b98e4754577b94965a7e9f31a09ad52/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_transport rosbag2_tests rosbag2
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9096

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@lihui815 lihui815 merged commit e6867e7 into ros2:master Sep 29, 2021
@lihui815 lihui815 deleted the sonia-seek branch September 29, 2021 20:38
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.

Seek/Jump API for Reader/Storage
2 participants