[kilted] Fix for multibag replay stagnation (backport #2158) #2169
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add "high_freq_topics_does_not_starve_in_multibag_playback" test Signed-off-by: Michael Orlov <morlovmr@gmail.com> * Quick fix for slow playback when one reader has lower freq topic - Add cache for the last element from each reader and taking messages from cache in a chronological order. - Also use "rcpputils::unique_lock" consistently for the reader_mutex_ Signed-off-by: Michael Orlov <morlovmr@gmail.com> * Add ReadersWrapper class that encapsulates readers and messages cache - The ReadersWrapper class manages multiple rosbag2_cpp::Reader instances, allowing for chronological reading of messages across all readers. It maintains a cache of the next message from each reader and provides methods to retrieve the next message in chronological order, seek to a specific timestamp across all readers, and apply filters to all readers. It also provides access to the earliest and latest timestamps across all readers. Signed-off-by: Michael Orlov <morlovmr@gmail.com> * Add unit tests for ReadersWrapper class Signed-off-by: Michael Orlov <morlovmr@gmail.com> * Delete default ctor and copy/move operators for ReadersWrapper class Signed-off-by: Michael Orlov <morlovmr@gmail.com> * Rename ReadersWrapper class to the ReadersManager Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Rename get_next_chronological_message_from_cache - Reasoning: Not mentioning cache in the name because this is some implementation details and could be changed in the future. However, the API needs to be stable and clear. - Also added a comment about reasoning for usage pre-cache inside implementation file. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Rename "no_messages_in_cache" to the "has_next()" Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Renames for mutex and condition_variable - Rename "skip_message_in_main_play_loop_mutex_" to the "main_play_loop_mutex_" and "playback_finished_cv_" into the "is_in_playback_cv_". Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Fix for messages_queue access race in seek(time) - Stop storage loading thread before doing seek to avoid race condition - Return from seek at the beginning if player is not in playback mode. Reasoning: We will seek to the beginning of the bag when will start play() anyway. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Speedup "load_storage_content()" interruption - Check "load_storage_content_" inside "enqueue_up_to_boundary()" Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Fix for play_fails_gracefully_if_needed_coverter_plugin_does_not_exist Add missing converter options when open readers Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Michael Orlov <michael.orlov@apex.ai> (cherry picked from commit bfce7ac)
MichaelOrlov
changed the title
MichaelOrlov
approved these changes
Sep 13, 2025
Contributor
|
Pulls: #2169
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a problem for slow playback when one reader has a lower frequency topic than others
Added cache for the last element from each reader and taking messages from cache in a chronological order instead of the round-robin order.
To mitigate "spaggetish" code after the fix, this PR also introduces the
ReadersWrapperclass, which will encapsulate readers with their options and message cache.The
ReadersWrapperclass manages multiplerosbag2_cpp::Readerinstances, allowing for chronological reading of messages across all readers. It maintains a cache of the next message from each reader and provides methods to retrieve the next message in chronological order, seek to a specific timestamp across all readers, and apply filters to all readers. It also provides access to the earliest and latest timestamps across all readers.Fixes When using multibag replay, the frequency of the messages on topics is reduced and inconsistent over the time #2156
Depends on the Bugfix for incorrect MCAPStorage::seek(timestamp) when timestamp is current #2157
Is this user-facing behavior change?
Fixed a bug where playback of the higher frequency topics can slow down when present slow frequency topics in another playing bag.
Did you use Generative AI?
Yes. I am using GitHub Copilot, GPT-4.1, in my workflow. in particular to help with unit tests and documentation.
Additional Information
This PR can be backported to the other distros.
This is an automatic backport of pull request #2158 done by [Mergify](https://mergify.com).