cleanup & better test cases

cleanup

remoe &

typo

update comment

Author: deanlee

selfdrive/ui/replay/replay.cc

@@ -2,6 +2,7 @@
 
 #include <QApplication>
 #include <QDebug>
+
 #include "cereal/services.h"
 #include "selfdrive/camerad/cameras/camera_common.h"
 #include "selfdrive/common/timing.h"
@@ -25,7 +26,7 @@ inline void precise_nano_sleep(long sleep_ns) {
 
 Replay::Replay(QString route, QStringList allow, QStringList block, SubMaster *sm_, bool dcam, bool ecam, QObject *parent)
     : sm(sm_), load_dcam(dcam), load_ecam(ecam), QObject(parent) {
-  std::vector<const char*> s;
+  std::vector<const char *> s;
   for (const auto &it : services) {
     if ((allow.size() == 0 || allow.contains(it.name)) &&
         !block.contains(it.name)) {
@@ -62,14 +63,13 @@ bool Replay::load() {
 
 void Replay::start(int seconds) {
   seekTo(seconds);
-  qDebug() << "start from" << seconds;
   // start stream thread
   thread = new QThread;
   QObject::connect(thread, &QThread::started, [=]() { stream(); });
   thread->start();
 }
 
-void Replay::updateEvents(const std::function<bool()>& lambda) {
+void Replay::updateEvents(const std::function<bool()> &lambda) {
   // set updating_events to true to force stream thread relase the lock and wait for evnets_udpated.
   updating_events_ = true;
   {
@@ -83,24 +83,24 @@ void Replay::updateEvents(const std::function<bool()>& lambda) {
 void Replay::seekTo(int seconds, bool relative) {
   if (segments_.empty()) return;
 
-  // cannot do seek while queueSegment is running, the queueSegment will wake up the stream thread, 
-  // and change the current_segment_ to a value different from seek in the event loop.
-  std::unique_lock lk(queue_lock_);
-
+  bool segment_loaded = false;
   updateEvents([&]() {
     if (relative) {
       seconds += ((cur_mono_time_ - route_start_ts_) * 1e-9);
     }
+    qInfo() << "seeking to" << seconds;
+
     cur_mono_time_ = route_start_ts_ + std::clamp(seconds, 0, (int)segments_.size() * 60) * 1e9;
     current_segment_ = std::min(seconds / 60, (int)segments_.size() - 1);
-    // always emit segmentChanged. the current_segment_ may not correct when seeking cross boundary or invalid segments. 
-    // e.g. seekTo 60s(segment 1), but segment 0 is end at 60.021, or segment 1 is invalid.
-    emit segmentChanged();
-
-    qInfo() << "seeking to " << seconds;
-    // return true if segment is already loaded
-    return segments_[current_segment_] && segments_[current_segment_]->isLoaded();
+    segment_loaded = std::find(segments_merged_.begin(), segments_merged_.end(), current_segment_) != segments_merged_.end();
+    return segment_loaded;
   });
+
+  if (!segment_loaded) {
+    // always emit segmentChanged if segment is not loaded.
+    // the current_segment_ may not valid when seeking cross boundary or seeking to an invalid segment.
+    emit segmentChanged();
+  }
 }
 
 void Replay::pause(bool pause) {
@@ -119,7 +119,6 @@ void Replay::setCurrentSegment(int n) {
 
 // maintain the segment window
 void Replay::queueSegment() {
-  std::unique_lock lk(queue_lock_);
   // fetch segments forward
   int cur_seg = current_segment_.load();
   int end_idx = cur_seg;
@@ -173,20 +172,18 @@ void Replay::mergeSegments(int cur_seg, int end_idx) {
         auto it = std::find_if(new_events->begin(), new_events->end(), [=](auto e) { return e->which == cereal::Event::Which::INIT_DATA; });
         if (it != new_events->end()) {
           route_start_ts_ = (*it)->mono_time;
-          // cur_mono_time_ may be set by seek when route_start_ts_ = 0
+          // cur_mono_time_ is set by seekTo int start() before get route_start_ts_
           cur_mono_time_ += route_start_ts_;
         }
       }
 
       events_ = new_events;
       segments_merged_ = segments_need_merge;
-      return segments_[cur_seg] && segments_[cur_seg]->isLoaded();
+      return true;
     });
     delete prev_events;
   } else {
-    updateEvents([&]() {
-      return segments_[cur_seg] && segments_[cur_seg]->isLoaded();
-    });
+    updateEvents([]() { return true; });
   }
 
   // free segments out of current semgnt window.

selfdrive/ui/replay/replay.h

@@ -38,7 +38,6 @@ protected slots:
   QThread *thread;
 
   // logs
-  std::mutex queue_lock_;
   std::mutex stream_lock_;
   std::condition_variable stream_cv_;
   std::atomic<bool> updating_events_ = false;

selfdrive/ui/replay/route.h

@@ -30,10 +30,12 @@ class Route {
   inline int size() const { return segments_.size(); }
   inline SegmentFile &at(int n) { return segments_[n]; }
 
+  // public for unit tests
+  std::vector<SegmentFile> segments_;
+
 protected:
   bool loadFromJson(const QString &json);
   QString route_;
-  std::vector<SegmentFile> segments_;
 };
 
 class Segment : public QObject {
@@ -48,9 +50,6 @@ class Segment : public QObject {
   std::unique_ptr<LogReader> log;
   std::unique_ptr<FrameReader> frames[MAX_CAMERAS] = {};
 
-  // public for unit tests
-  bool loaded_ = false, valid_ = false;
-
 signals:
   void loadFinished();
 

selfdrive/ui/replay/tests/test_replay.cc

@@ -99,70 +99,80 @@ TEST_CASE("Segment") {
 
 // helper class for unit tests
 class TestReplay : public Replay {
- public:
+public:
   TestReplay(const QString &route) : Replay(route, {}, {}) {}
   void test_seek();
 
- protected:
-  void testSeekTo(int seek_to, int invalid_segment = -1);
+protected:
+  void testSeekTo(int seek_to, const std::set<int> &invalid_segments = {});
 };
 
-void TestReplay::testSeekTo(int seek_to, int invalid_segment) {
+void TestReplay::testSeekTo(int seek_to, const std::set<int> &invalid_segments) {
   seekTo(seek_to);
 
   // wait for seek finish
-  std::unique_lock lk(stream_lock_);
-  stream_cv_.wait(lk, [=]() { return events_updated_ == true; });
-  events_updated_ = false;
-
-  // verify result
-  INFO("seek to [" << seek_to << "s segment " << seek_to / 60 << "]");
-  REQUIRE(!events_->empty());
-  REQUIRE(is_events_ordered(*events_));
-  REQUIRE(uint64_t(route_start_ts_ + seek_to * 1e9) == cur_mono_time_);
-
-  Event cur_event(cereal::Event::Which::INIT_DATA, cur_mono_time_);
-  auto eit = std::upper_bound(events_->begin(), events_->end(), &cur_event, Event::lessThan());
-
-  REQUIRE(eit != events_->end());
-  const int seek_to_segment = seek_to / 60;
-  const int event_seconds = ((*eit)->mono_time - route_start_ts_) / 1e9;
-  current_segment_ = event_seconds / 60;
-  INFO("event [" << event_seconds << "s segment " << current_segment_ << "]");
-  REQUIRE(event_seconds >= seek_to);
-  if (seek_to_segment != invalid_segment) {
-    REQUIRE(current_segment_ == seek_to_segment);  // in the same segment
-    REQUIRE(event_seconds == seek_to); // at the same time
-  } else {
-    // has invalid_segment
-    if (current_segment_ == seek_to_segment) {
-      // seek cross-boundary. e.g. seek_to 60s(segment 1), but segment 0 end at 60.021.
-      REQUIRE(event_seconds == seek_to);
+  while (true) {
+    std::unique_lock lk(stream_lock_);
+    stream_cv_.wait(lk, [=]() { return events_updated_ == true; });
+    events_updated_ = false;
+
+    // verify result
+    REQUIRE(uint64_t(route_start_ts_ + seek_to * 1e9) == cur_mono_time_);
+
+    Event cur_event(cereal::Event::Which::INIT_DATA, cur_mono_time_);
+    auto eit = std::upper_bound(events_->begin(), events_->end(), &cur_event, Event::lessThan());
+    if (eit == events_->end()) {
+      qDebug() << "waiting for events...";
+      continue;
+    }
+
+    INFO("seek to [" << seek_to << "s segment " << seek_to / 60 << "]");
+    REQUIRE(!events_->empty());
+    REQUIRE(is_events_ordered(*events_));
+
+    REQUIRE(eit != events_->end());
+    const int seek_to_segment = seek_to / 60;
+    const int event_seconds = ((*eit)->mono_time - route_start_ts_) / 1e9;
+    current_segment_ = event_seconds / 60;
+    INFO("event [" << event_seconds << "s segment " << current_segment_ << "]");
+    REQUIRE(event_seconds >= seek_to);
+    if (invalid_segments.find(seek_to_segment) == invalid_segments.end()) {
+      REQUIRE(current_segment_ == seek_to_segment);  // in the same segment
+      REQUIRE(event_seconds == seek_to); // at the same time
     } else {
-      REQUIRE(current_segment_ == seek_to_segment + 1);
+      if (current_segment_ == seek_to_segment) {
+        // seek cross-boundary. e.g. seek_to 60s(segment 1), but segment 0 end at 60.021 and segemnt 1 is invalid.
+        REQUIRE(event_seconds == seek_to);
+      } else {
+        REQUIRE(current_segment_ > seek_to_segment);
+        REQUIRE(invalid_segments.find(current_segment_) == invalid_segments.end());
+      }
     }
+    break;
   }
 }
 
 void TestReplay::test_seek() {
   QEventLoop loop;
 
   std::thread thread = std::thread([&]() {
-    // random seek 500 times in one segment
-    for (int i = 0; i < 500; ++i) {
+    const int loop_count = 100;
+    // random seek in one segment
+    for (int i = 0; i < loop_count; ++i) {
       testSeekTo(random_int(0, 60));
     }
-    // random seek 500 times in 3 segments (this test if fast)
-    for (int i = 0; i < 500; ++i) {
-      testSeekTo(random_int(0, 60 * 3 - 1));
+    // random seek in 3 segments
+    for (int i = 0; i < loop_count; ++i) {
+      testSeekTo(random_int(0, 60 * 3));
     }
-
-    // random seek 500 times in 3 segments.segment 1 is invalid.
-    // invalid segmen 1
-    segments_[1]->valid_ = segments_[1]->loaded_ = false;
-    queueSegment();
-    for (int i = 0; i < 500; ++i) {
-      testSeekTo(random_int(0, 60 * 3 - 1), 1);
+    // random seek in invalid segments
+    std::set<int> invalid_segments{5, 6, 7, 9};
+    for (int i : invalid_segments) {
+      route_->segments_[i].rlog = route_->segments_[i].qlog = "";
+      route_->segments_[i].road_cam = route_->segments_[i].qcamera = "";
+    }
+    for (int i = 0; i < loop_count; ++i) {
+      testSeekTo(random_int(4 * 60, 60 * 10), invalid_segments);
     }
     loop.quit();
   });