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

[jazzy] Add support for replaying multiple bags (backport #1848) #1873

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Dec 2, 2024

This PR adds support for replaying multiple bags with ros2 bag play and the underlying classes (rosbag2_transport::Player/rosbag2_py::Player).

To replay multiple bags, use the new -i,--input CLI option.

$ ros2 bag play -i bag1 -i bag2 -i bag3 [storage_id]

Like with the ros2 bag convert option, the -i,--input option takes an URI to the bag folder or dedicated bag file and an optional explicit storage ID. This option can be used multiple times to be able to specify multiple bags. We keep the bag_path positional argument for backward compatibility and simplicity of usage in cases where users need to play just one bag file. However, the standalone --storage CLI option has been deprecated. In the future, users will need to use -i bag [storage_id] if they need to specify the storage ID explicitly.

The rosbag2_transport::Player now uses a std::priority_queue, which required changing the logic and interaction between PlayerImpl::play_messages_from_queue() and PlayerImpl::play_next(). In short, message publication is now all handled by PlayerImpl::play_messages_from_queue(). PlayerImpl::play_next() simply tells it to play the next message and then waits for the result. While the queue is not lock-free anymore, this change does simplify the interaction between PlayerImpl::play_messages_from_queue() and PlayerImpl::play_next() a bit. Note: the priority queue depth is defined by the --read-ahead-queue-size CLI option, which is set to 1000 elements by default.

Messages are played in order, based on their recv_timestamp (recorder reception timestamp). However, with the priority queue, this could eventually be changed (e.g., through PlayOptions) to the send_timestamp (original publication timestamp).

Other notes for reviewers:

  1. Like ros2 bag convert, I added the -i, --input option that takes a bag URI and an optional explicit storage ID. This option can be used multiple times for multiple bags.
  2. I kept the bag_path positional argument (but made it optional) because it is quite nice to be able to just do ros2 bag play my-bag. However, I deprecated the storage ID option (--storage); users will have to use -i, --input to specify an explicit storage ID for an input bag.
  3. This PR doesn't introduce API/ABI breaking changes and can be backported to the jazzy branch. Some APIs needed to be added (mainly constructors) to support N input bags.

Signed-off-by: Christophe Bedard <[email protected]>
(cherry picked from commit 125db50)

# Conflicts:
#	shared_queues_vendor/CHANGELOG.rst
#	shared_queues_vendor/package.xml
@mergify mergify bot added the conflicts label Dec 2, 2024
@mergify mergify bot requested a review from a team as a code owner December 2, 2024 20:16
Copy link
Author

mergify bot commented Dec 2, 2024

Cherry-pick of 125db50 has failed:

On branch mergify/bp/jazzy/pr-1848
Your branch is up to date with 'origin/jazzy'.

You are currently cherry-picking commit 125db50b.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   .github/workflows/lint.yml
	modified:   .github/workflows/test.yml
	modified:   README.md
	modified:   ros2bag/ros2bag/api/__init__.py
	modified:   ros2bag/ros2bag/verb/convert.py
	modified:   ros2bag/ros2bag/verb/play.py
	modified:   ros2bag/test/test_api.py
	modified:   rosbag2/package.xml
	modified:   rosbag2_cpp/package.xml
	modified:   rosbag2_py/rosbag2_py/_transport.pyi
	modified:   rosbag2_py/src/rosbag2_py/_transport.cpp
	modified:   rosbag2_transport/CMakeLists.txt
	modified:   rosbag2_transport/include/rosbag2_transport/player.hpp
	modified:   rosbag2_transport/package.xml
	new file:   rosbag2_transport/src/rosbag2_transport/locked_priority_queue.hpp
	modified:   rosbag2_transport/src/rosbag2_transport/player.cpp
	modified:   rosbag2_transport/test/rosbag2_transport/mock_player.hpp
	modified:   rosbag2_transport/test/rosbag2_transport/test_composable_player.cpp
	modified:   rosbag2_transport/test/rosbag2_transport/test_play.cpp
	modified:   rosbag2_transport/test/rosbag2_transport/test_play_next.cpp
	modified:   rosbag2_transport/test/rosbag2_transport/test_play_seek.cpp
	deleted:    shared_queues_vendor/CMakeLists.txt

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by them: shared_queues_vendor/CHANGELOG.rst
	deleted by them: shared_queues_vendor/package.xml

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify bot requested review from gbiggs and emersonknapp and removed request for a team December 2, 2024 20:16
@MichaelOrlov MichaelOrlov changed the title Add support for replaying multiple bags (backport #1848) [jazzy] Add support for replaying multiple bags (backport #1848) Dec 2, 2024
@MichaelOrlov MichaelOrlov requested review from MichaelOrlov and removed request for gbiggs and emersonknapp December 2, 2024 20:17
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.

Notice: I reverted the deletion of the shared_queue_vendor package. Deleting a package in a stable release can break someone's dependencies.

@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@MichaelOrlov
Copy link
Contributor

Pulls: #1873
Gist: https://gist.githubusercontent.com/MichaelOrlov/1e9b14b709febab292277366e337eedf/raw/0d1f324a4130bac44cb26d0c8a18ecf31bc8bb83/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_transport rosbag2_py ros2bag rosbag2
TEST args: --packages-above rosbag2_cpp rosbag2_transport rosbag2_py ros2bag rosbag2
ROS Distro: jazzy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14895

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@MichaelOrlov MichaelOrlov merged commit 7470804 into jazzy Dec 3, 2024
11 of 12 checks passed
@MichaelOrlov MichaelOrlov deleted the mergify/bp/jazzy/pr-1848 branch December 3, 2024 04:42
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.

2 participants