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.

Sign up for GitHub

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

Jump to bottom

Add play-next-n functionality #963

Closed
wants to merge 2 commits into from
Closed

Add play-next-n functionality #963

wants to merge 2 commits into from

Conversation

gbiggs
Copy link
Member

@gbiggs gbiggs commented Feb 17, 2022

This PR extends the play-next-message service/function with the ability to play the next N messages instead of just one message.

This work was co-authored by @agalbachicar.

@gbiggs
Add play-next-n functionality
c0d6e3e 
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs gbiggs added the enhancement New feature or request label Feb 17, 2022
@gbiggs gbiggs marked this pull request as ready for review February 17, 2022 07:15
@gbiggs gbiggs requested a review from a team as a code owner February 17, 2022 07:15
@gbiggs gbiggs requested review from emersonknapp and jhdcs and removed request for a team February 17, 2022 07:15
@jhdcs
Copy link
Contributor

jhdcs commented Feb 17, 2022

I'm not seeing any obvious issues, apart from the CI failing. Once that's sorted out I'll give it another once-over just to triple-check.

MichaelOrlov
MichaelOrlov requested changes Feb 19, 2022
View reviewed changes
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.

@gbiggs Thank you for your contribution.

I have a few requests for changes and some concerns/questions overall for the design of this feature.
I am curios about in what cases this feature aka playing multiple messages in "burst" mode without respecting their timings will be useful?
I would (and I think many others) expect that if play_next(N) would exist it should respect timings between following messages.
Also it looks like if we will add such feature it will be more appropriate to change API for play next N operation to return number of played messages instead of justboolean value.
In current implementation for instance if someone requested to play 5 messages and for some circumstance play_next(N) will really play 3 of them play_next(N) will still return true and it will not be possible to recognize whether those messages was lost on the transport layer or they are was just filtered out or missing inside bag.

rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_play_next.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_play_next.cpp Show resolved Hide resolved
@agalbachicar agalbachicar mentioned this pull request Mar 1, 2022
Merged
@agalbachicar @gbiggs
Fixes comments of play_next N messages. (#12)
ddc768f 
* Reverts the implementation of play_next to not use optional.

* Respects message timing when calling into play_next().

* Removes play_next_no_message_must_succeed test.

* Removes player->wait_for_playback_to_start().

* Update rosbag2_interfaces/srv/PlayNext.srv

Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>

* Update rosbag2_transport/include/rosbag2_transport/player.hpp

Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>

Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs
Copy link
Member Author

gbiggs commented Mar 15, 2022

@MichaelOrlov: @agalbachicar has addressed your comments. Can you please re-review?

@MichaelOrlov
Copy link
Contributor

@gbiggs Following questions are still unclear for me.

I am curios about in what cases this feature aka playing multiple messages in "burst" mode without respecting their timings will be useful?
I would (and I think many others) expect that if play_next(N) would exist it should respect timings between following messages.

Could you please clarify?

MichaelOrlov
MichaelOrlov requested changes Mar 19, 2022
View reviewed changes
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.

@gbiggs @agalbachicar
I have a significant concerns about this feature and it's implementation. Therefore I can't approve this PR.

  1. You didn't provide any explanations what would be a real world scenario for usage of this new feature and API changes. i.e. why following case doesn't work for you?
  for (size_t i = 0; i < num_msgs_in_bag_; i++) {
    player->play_next();
  }

We use to creating new issue for feature development and discussion or at least need to provide justification and detailed explanations for proposed API changes and reasoning for them.
2. Current implementation broke generic behavior for play_next() which was before your changes. e.g play_next() shall play 1 message without delays.
3. Tests start failing and you just quietly adjust timings between messages to let tests pass with new broken functionality. This is unfair and unacceptable!!!
In particular in play_next_playing_all_messages_without_delays test.

Taking in to the account all above.
Please:

  • Provide justification and explanation what would be a reasoning for these changes and usage of this new API
  • Revert timings and irrelevant changes in tests. I've made suggestions in line.
  • Fix logic in core player.cpp to preserve existing functionality and to be able to pass existing tests. We need to keep backward compatibility.

rosbag2_transport/include/rosbag2_transport/player.hpp
ROSBAG2_TRANSPORT_PUBLIC
virtual bool play_next(const std::optional<uint64_t> num_messages = std::nullopt);
virtual uint64_t play_next(const uint64_t num_messages);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
virtual uint64_t play_next(const uint64_t num_messages);
virtual uint64_t play_next(const uint64_t num_messages = 1u);

rosbag2_transport/src/rosbag2_transport/player.cpp
{
if (!clock_->is_paused()) {
RCLCPP_WARN_STREAM(get_logger(), "Called play next, but not in paused state.");
return false;
return 0u;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (num_messages == 0) {
RCLCPP_WARN_STREAM(get_logger(), "Called play next for zero number of messages");
return 0u;
}

rosbag2_transport/src/rosbag2_transport/player.cpp
Comment on lines -330 to -335
// Wait for player to be ready for playback messages from queue i.e. wait for Player:play() to
// be called if not yet and queue to be filled with messages.
{
std::unique_lock<std::mutex> lk(ready_to_play_from_queue_mutex_);
ready_to_play_from_queue_cv_.wait(lk, [this] {return is_ready_to_play_from_queue_;});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO. It doesn't make sense moving wait for playback start inside loop.

rosbag2_transport/test/rosbag2_transport/test_play_next.cpp
@@ -46,7 +46,7 @@ TEST_F(RosBag2PlayTestFixture, play_next_with_false_preconditions) {
auto player = std::make_shared<MockPlayer>(std::move(reader), storage_options_, play_options_);

ASSERT_FALSE(player->is_paused());
ASSERT_FALSE(player->play_next());
ASSERT_EQ(0u, player->play_next(1u));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ASSERT_EQ(0u, player->play_next(1u));
ASSERT_EQ(player->play_next(), 0u);

rosbag2_transport/test/rosbag2_transport/test_play_next.cpp
Comment on lines +63 to +66
serialize_test_message("topic1", 210, primitive_message),
serialize_test_message("topic1", 330, primitive_message),
serialize_test_message("topic1", 460, primitive_message),
serialize_test_message("topic1", 590, primitive_message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Following timings was made thousands on purpose. It doesn't have impact on the test execution time.
Please revert changes. This is affect test logic.

Suggested change
serialize_test_message("topic1", 210, primitive_message),
serialize_test_message("topic1", 330, primitive_message),
serialize_test_message("topic1", 460, primitive_message),
serialize_test_message("topic1", 590, primitive_message)
serialize_test_message("topic1", 2100, primitive_message),
serialize_test_message("topic1", 3300, primitive_message),
serialize_test_message("topic1", 4600, primitive_message),
serialize_test_message("topic1", 5900, primitive_message)

rosbag2_transport/test/rosbag2_transport/test_play_seek.cpp

// Jump on third message (1200 ms)
player->seek((start_time_ms_ + message_spacing_ms_ * 2) * 1000000);
EXPECT_TRUE(player->play_next());
EXPECT_EQ(1u, player->play_next(1u));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EXPECT_EQ(1u, player->play_next(1u));
ASSERT_EQ(player->play_next(), 1u);

rosbag2_transport/test/rosbag2_transport/test_play_seek.cpp
Comment on lines +235 to +236
EXPECT_EQ(num_msgs_in_bag_, player->play_next(num_msgs_in_bag_));
EXPECT_EQ(0u, player->play_next(1u)); // Make sure there are no messages to play
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert changes. It does affect test execution time.

Suggested change
EXPECT_EQ(num_msgs_in_bag_, player->play_next(num_msgs_in_bag_));
EXPECT_EQ(0u, player->play_next(1u)); // Make sure there are no messages to play
for (size_t i = 0; i < num_msgs_in_bag_; i++) {
ASSERT_EQ(player->play_next(), 1u);
}
ASSERT_EQ(player->play_next(), 0u); // Make sure there are no messages to play

rosbag2_transport/test/rosbag2_transport/test_play_seek.cpp

// Jump on third message (1200 ms)
player->seek((start_time_ms_ + message_spacing_ms_ * 2) * 1000000);
EXPECT_TRUE(player->play_next());
EXPECT_EQ(1u, player->play_next(1u));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EXPECT_EQ(1u, player->play_next(1u));
ASSERT_EQ(player->play_next(), 1u);

rosbag2_transport/test/rosbag2_transport/test_play_seek.cpp
Comment on lines +279 to +280
EXPECT_EQ(num_msgs_in_bag_, player->play_next(num_msgs_in_bag_));
EXPECT_EQ(0u, player->play_next(1u)); // Make sure there are no messages to play
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert changes. It does affect test execution time.

Suggested change
EXPECT_EQ(num_msgs_in_bag_, player->play_next(num_msgs_in_bag_));
EXPECT_EQ(0u, player->play_next(1u)); // Make sure there are no messages to play
for (size_t i = 0; i < num_msgs_in_bag_; i++) {
ASSERT_EQ(player->play_next(), 1u);
}
ASSERT_EQ(player->play_next(), 0u); // Make sure there are no messages to play

rosbag2_transport/test/rosbag2_transport/test_play_seek.cpp

// Jump in timestamp equal to the timestamp in last message + 1 nanosecond
player->seek((start_time_ms_ + message_spacing_ms_ * (num_msgs_in_bag_ - 1)) * 1000000 + 1);
EXPECT_FALSE(player->play_next());
EXPECT_EQ(0u, player->play_next(1u));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EXPECT_EQ(0u, player->play_next(1u));
ASSERT_EQ(player->play_next(), 0u);

@gbiggs
Copy link
Member Author

gbiggs commented Mar 23, 2022

@MichaelOrlov Our use case is playing a subset of messages from a bag, but not in real-time, and doing it from the command line. A for-loop over play_next would work programmatically, but we want to be able to do it from the command line. This is useful for rapidly processing the data in a bag.

I guess what we are looking for is a "burst mode" playback. Would this PR be acceptable if we changed it to provide a burst function in the API rather than modifying the play_next function?

@agalbachicar
Copy link
Contributor

@MichaelOrlov , on top of @gbiggs comment , I just would like to add where all this started. Please take a look at the meeting notes of the working group on 2021/05/21. The scope of our use case extended from "play N seconds and pause" (see #960) to that and "play N messages".

Anyway, @gbiggs proposal stands. The burst verb might be more appropriate. We are eager to hear your thoughts.

@MichaelOrlov
Copy link
Contributor

@gbiggs @agalbachicar Will play_for N seconds API along side with another parameter rate 10x will work for you as burst mode?

@gbiggs
Copy link
Member Author

gbiggs commented Mar 24, 2022

We would prefer to burst the messages as fast as possible and in controllable batch sizes.

@MichaelOrlov
Copy link
Contributor

@gbiggs You can set up even more aggressive rate 100x for example.
This is looks more logically correct and useful, since it's difficult to predict how many messages need to play for debug purpose.
It's more natural to operate with amount of time for replay rather than number of messages.
Thoughts?
IMO I would refrain from creating separate play_in_burst(N) API or changing existing play_next().

@gbiggs
Copy link
Member Author

gbiggs commented Mar 24, 2022

Our use case is not for debugging, it's for processing of data from a bag. For example, we may want to filter the topics in a bag down to a single image topic, then pull out 10 messages at a time from that topic and process them through a machine learning algorithm. This is not time-based, it is number-of-messages-based, and if we are doing massively parallel computation (as an example) then we want to get all the computation pipelines full as fast as possible.

@MichaelOrlov
Copy link
Contributor

@gbiggs Ok. Thanks for the explanation.
In this case let's go with separate API aka play_in_burst(num_messages)

@gbiggs
Copy link
Member Author

gbiggs commented Mar 24, 2022

@MichaelOrlov I have created a new PR that implements burst mode. I will close this PR as unmergeable.

@gbiggs gbiggs closed this Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelOrlov MichaelOrlov MichaelOrlov requested changes

@emersonknapp emersonknapp Awaiting requested review from emersonknapp emersonknapp is a code owner automatically assigned from ros2/tooling-reviewers

@jhdcs jhdcs Awaiting requested review from jhdcs jhdcs is a code owner automatically assigned from ros2/tooling-reviewers

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gbiggs @jhdcs @MichaelOrlov @agalbachicar