Skip to content

Commit 8607af5

Browse files
ChuiVanfleetChui VanfleetMichaelOrlov
authored
Fix reindex duration bug when bag file durations overlap (#2036)
* Fix reindex duration bug when bag files overlap. Signed-off-by: Chui Vanfleet <[email protected]> * Set the duration instead of adding it. Signed-off-by: Chui Vanfleet <[email protected]> * Fixing some linting. Signed-off-by: Chui Vanfleet <[email protected]> * Cleanups in test_reindexer.cpp Signed-off-by: Michael Orlov <[email protected]> * Attempt to fix Windows build error Fix for error: "test_reindexer.cpp(50,1): error C2679: binary '=': no operator found which takes a right-hand operand of type 'std::chrono::system_clock::time_point' (or there is no acceptable conversion) Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Chui Vanfleet <[email protected]> Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Chui Vanfleet <[email protected]> Co-authored-by: Michael Orlov <[email protected]>
1 parent 9741bfc commit 8607af5

File tree

3 files changed

+142
-4
lines changed

3 files changed

+142
-4
lines changed

rosbag2_cpp/CMakeLists.txt

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,16 @@ if(BUILD_TESTING)
323323
${test_msgs_TARGETS}
324324
)
325325
endif()
326+
327+
ament_add_gmock(test_reindexer test/rosbag2_cpp/test_reindexer.cpp)
328+
if(TARGET test_reindexer)
329+
target_link_libraries(test_reindexer
330+
${PROJECT_NAME}
331+
rosbag2_storage::rosbag2_storage
332+
rosbag2_test_common::rosbag2_test_common
333+
)
334+
endif()
335+
326336
endif()
327337

328338
ament_package()

rosbag2_cpp/src/rosbag2_cpp/reindexer.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,8 @@ void Reindexer::aggregate_metadata(
164164
std::map<std::string, rosbag2_storage::TopicInformation> temp_topic_info;
165165
std::chrono::time_point<std::chrono::high_resolution_clock> latest_log_start =
166166
std::chrono::time_point<std::chrono::high_resolution_clock>::min();
167+
std::chrono::time_point<std::chrono::high_resolution_clock> ending_time =
168+
std::chrono::time_point<std::chrono::high_resolution_clock>::min();
167169

168170
// In order to most accurately reconstruct the metadata, we need to
169171
// visit each of the contained relative files files in the bag,
@@ -201,10 +203,8 @@ void Reindexer::aggregate_metadata(
201203
}
202204
}
203205

204-
if (temp_metadata.starting_time < metadata_.starting_time) {
205-
metadata_.starting_time = temp_metadata.starting_time;
206-
}
207-
metadata_.duration += temp_metadata.duration;
206+
metadata_.starting_time = std::min(temp_metadata.starting_time, metadata_.starting_time);
207+
ending_time = std::max(ending_time, temp_metadata.starting_time + temp_metadata.duration);
208208
ROSBAG2_CPP_LOG_DEBUG_STREAM("New duration: " + std::to_string(metadata_.duration.count()));
209209
metadata_.message_count += temp_metadata.message_count;
210210

@@ -234,6 +234,7 @@ void Reindexer::aggregate_metadata(
234234

235235
bag_reader->close();
236236
}
237+
metadata_.duration = ending_time - metadata_.starting_time;
237238

238239
// Convert the topic map into topic metadata
239240
for (auto & topic : temp_topic_info) {
Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
// Copyright 2019 Epiroc or its affiliates. All Rights Reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
16+
#include <gmock/gmock.h>
17+
18+
#include <filesystem>
19+
#include <memory>
20+
#include <string>
21+
#include <utility>
22+
#include <vector>
23+
#include <chrono>
24+
#include <fstream>
25+
26+
#include "rosbag2_cpp/reindexer.hpp"
27+
28+
#include "rosbag2_storage/bag_metadata.hpp"
29+
#include "rosbag2_storage/topic_metadata.hpp"
30+
#include "rosbag2_test_common/temporary_directory_fixture.hpp"
31+
32+
#include "mock_metadata_io.hpp"
33+
#include "mock_storage.hpp"
34+
#include "mock_storage_factory.hpp"
35+
36+
using namespace testing; // NOLINT
37+
namespace fs = std::filesystem;
38+
39+
class ReindexerTest : public rosbag2_test_common::TemporaryDirectoryFixture
40+
{
41+
public:
42+
ReindexerTest()
43+
{
44+
storage_ = std::make_shared<NiceMock<MockStorage>>();
45+
storage_uri_ = temporary_dir_path_;
46+
47+
// bag 1 and bag 2 are sequential
48+
bag_1_metadata_.bag_size = 10;
49+
bag_1_metadata_.duration = std::chrono::seconds{10};
50+
bag_1_metadata_.starting_time = std::chrono::high_resolution_clock::now();
51+
52+
bag_2_metadata_.bag_size = 15;
53+
bag_2_metadata_.duration = std::chrono::seconds{10};
54+
bag_2_metadata_.starting_time = bag_1_metadata_.starting_time + bag_1_metadata_.duration;
55+
56+
// bag 3 overlap with bag 1
57+
bag_3_metadata_.bag_size = 22;
58+
bag_3_metadata_.duration = std::chrono::seconds{15};
59+
bag_3_metadata_.starting_time = bag_1_metadata_.starting_time - std::chrono::seconds{1};
60+
// We mock storage and meatdata_io. However, the reindexer checking that the storage directory
61+
// is not empty, so we need to write some files to the storage directory.
62+
write_file(bag_1_filename);
63+
write_file(bag_2_filename);
64+
write_file(bag_3_filename);
65+
66+
auto topic_with_type = rosbag2_storage::TopicMetadata{
67+
0u, "topic", "test_msgs/BasicTypes", storage_serialization_format_, {}, ""};
68+
auto topics_and_types = std::vector<rosbag2_storage::TopicMetadata>{topic_with_type};
69+
bag_1_metadata_.topics_with_message_count.push_back({topic_with_type, 10});
70+
bag_2_metadata_.topics_with_message_count.push_back({topic_with_type, 10});
71+
bag_3_metadata_.topics_with_message_count.push_back({topic_with_type, 10});
72+
73+
auto storage_factory = std::make_unique<StrictMock<MockStorageFactory>>();
74+
auto metadata_io = std::make_unique<NiceMock<MockMetadataIo>>();
75+
EXPECT_CALL(*metadata_io, write_metadata)
76+
.WillRepeatedly([this](const std::string &, const rosbag2_storage::BagMetadata & metadata)
77+
{
78+
saved_metadata_.push_back(metadata);
79+
});
80+
EXPECT_CALL(*metadata_io, metadata_file_exists(_)).WillRepeatedly(Return(true));
81+
ON_CALL(*storage_, set_read_order).WillByDefault(Return(true));
82+
EXPECT_CALL(*storage_factory, open_read_only(_)).WillRepeatedly(Return(storage_));
83+
84+
EXPECT_CALL(*storage_, get_all_topics_and_types())
85+
.Times(AtMost(1)).WillRepeatedly(Return(topics_and_types));
86+
87+
EXPECT_CALL(*storage_, get_metadata()).Times(3)
88+
.WillOnce(Return(bag_1_metadata_))
89+
.WillOnce(Return(bag_2_metadata_))
90+
.WillOnce(Return(bag_3_metadata_));
91+
92+
reindexer_ = std::make_unique<rosbag2_cpp::Reindexer>(std::move(storage_factory),
93+
std::move(metadata_io));
94+
}
95+
96+
void write_file(const std::string & filename) const
97+
{
98+
std::ofstream outfile;
99+
outfile.open(fs::path{storage_uri_}.append(filename));
100+
outfile.close();
101+
}
102+
103+
~ReindexerTest() override = default;
104+
105+
std::shared_ptr<NiceMock<MockStorage>> storage_;
106+
std::string storage_serialization_format_ = "cdr";
107+
std::string storage_uri_;
108+
std::string bag_1_filename = "bag_1.mcap";
109+
std::string bag_2_filename = "bag_2.mcap";
110+
std::string bag_3_filename = "bag_3.mcap";
111+
rosbag2_storage::BagMetadata bag_1_metadata_;
112+
rosbag2_storage::BagMetadata bag_2_metadata_;
113+
rosbag2_storage::BagMetadata bag_3_metadata_;
114+
std::vector<rosbag2_storage::BagMetadata> saved_metadata_;
115+
std::unique_ptr<rosbag2_cpp::Reindexer> reindexer_;
116+
};
117+
118+
TEST_F(ReindexerTest, duration_and_start_time_correct)
119+
{
120+
rosbag2_storage::StorageOptions storage_options{storage_uri_, ""};
121+
reindexer_->reindex(storage_options);
122+
ASSERT_EQ(saved_metadata_.size(), 1u);
123+
EXPECT_EQ(saved_metadata_.back().starting_time.time_since_epoch().count(),
124+
bag_3_metadata_.starting_time.time_since_epoch().count());
125+
EXPECT_EQ(saved_metadata_.back().duration.count(),
126+
std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::seconds{21}).count());
127+
}

0 commit comments

Comments
 (0)