3 ➡️ 4

av/src/VideoEncoder_TEST.cc

@@ -32,10 +32,9 @@ class VideoEncoderTest : public common::testing::AutoLogFixture
 };
 
 /////////////////////////////////////////////////
-TEST_F(VideoEncoderTest, StartStop)
+TEST_F(VideoEncoderTest, StartStopDefault)
 {
   auto filePathMp4 = common::joinPaths(common::cwd(), "TMP_RECORDING.mp4");
-  auto filePathMpg = common::joinPaths(common::cwd(), "TMP_RECORDING.mpg");
 
   {
     VideoEncoder video;
@@ -44,33 +43,109 @@ TEST_F(VideoEncoderTest, StartStop)
     EXPECT_EQ(video.BitRate(), static_cast<unsigned int>(
           VIDEO_ENCODER_BITRATE_DEFAULT));
 
-    video.Start();
+    EXPECT_TRUE(video.Start());
     EXPECT_TRUE(video.IsEncoding());
     EXPECT_TRUE(common::exists(filePathMp4)) << filePathMp4;
     EXPECT_EQ(video.BitRate(), 920000u);
 
-    video.Stop();
+    EXPECT_TRUE(video.Stop());
     EXPECT_FALSE(video.IsEncoding());
-    EXPECT_FALSE(common::exists(filePathMpg)) << filePathMpg;
+  }
 
-    video.Start("mpg", "", 1024, 768);
+  // Check that temp files are removed when video goes out of scope
+  EXPECT_FALSE(common::exists(filePathMp4)) << filePathMp4;
+}
+
+/////////////////////////////////////////////////
+TEST_F(VideoEncoderTest, StartStopMpg)
+{
+  auto filePathMpg = common::joinPaths(common::cwd(), "TMP_RECORDING.mpg");
+
+  {
+    VideoEncoder video;
+    EXPECT_FALSE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), VIDEO_ENCODER_FORMAT_DEFAULT);
+    EXPECT_EQ(video.BitRate(), static_cast<unsigned int>(
+          VIDEO_ENCODER_BITRATE_DEFAULT));
+
+    EXPECT_TRUE(video.Start("mpg", "", 1024, 768));
     EXPECT_TRUE(video.IsEncoding());
     EXPECT_STREQ(video.Format().c_str(), "mpg");
     EXPECT_TRUE(common::exists(filePathMpg)) << filePathMpg;
+    EXPECT_TRUE(video.Stop());
+    EXPECT_FALSE(video.IsEncoding());
+  }
 
-    video.Start("mp4", "", 1024, 768);
-    EXPECT_TRUE(video.IsEncoding());
-    EXPECT_STREQ(video.Format().c_str(), "mpg");
+  EXPECT_FALSE(common::exists(filePathMpg)) << filePathMpg;
+}
+
+
+/////////////////////////////////////////////////
+TEST_F(VideoEncoderTest, StartStopMp4)
+{
+  auto filePathMp4 = common::joinPaths(common::cwd(), "TMP_RECORDING.mp4");
 
+  {
+    VideoEncoder video;
+    EXPECT_FALSE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), VIDEO_ENCODER_FORMAT_DEFAULT);
+    EXPECT_EQ(video.BitRate(), static_cast<unsigned int>(
+          VIDEO_ENCODER_BITRATE_DEFAULT));
+
+    EXPECT_TRUE(video.Start("mp4", "", 1024, 768));
+    EXPECT_TRUE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), "mp4");
+    EXPECT_TRUE(common::exists(filePathMp4)) << filePathMp4;
     video.Stop();
     EXPECT_FALSE(video.IsEncoding());
   }
 
-  // Check that temp files are removed when video goes out of scope
   EXPECT_FALSE(common::exists(filePathMp4)) << filePathMp4;
-  EXPECT_FALSE(common::exists(filePathMpg)) << filePathMpg;
 }
 
+/////////////////////////////////////////////////
+TEST_F(VideoEncoderTest, RepeatedStart)
+{
+  auto filePathMpg = common::joinPaths(common::cwd(), "TMP_RECORDING.mpg");
+  auto filePathMp4 = common::joinPaths(common::cwd(), "TMP_RECORDING.mp4");
+
+  {
+    VideoEncoder video;
+    EXPECT_FALSE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), VIDEO_ENCODER_FORMAT_DEFAULT);
+    EXPECT_EQ(video.BitRate(), static_cast<unsigned int>(
+          VIDEO_ENCODER_BITRATE_DEFAULT));
+
+    EXPECT_TRUE(video.Start("mp4", "", 1024, 768));
+    EXPECT_TRUE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), "mp4");
+    EXPECT_TRUE(common::exists(filePathMp4)) << filePathMp4;
+
+    // Calling start again should return false and not mutate any
+    // internal state of the VideoEncoder
+    EXPECT_FALSE(video.Start("mpg", "", 1024, 768));
+    EXPECT_TRUE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), "mp4");
+    EXPECT_TRUE(common::exists(filePathMp4)) << filePathMp4;
+    EXPECT_FALSE(common::exists(filePathMpg)) << filePathMpg;
+
+    EXPECT_TRUE(video.Stop());
+    EXPECT_FALSE(video.IsEncoding());
+
+    // Once the VideoEncoder has been stopped, a new run may start.
+    EXPECT_TRUE(video.Start("mpg", "", 1024, 768));
+    EXPECT_TRUE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), "mpg");
+    EXPECT_FALSE(common::exists(filePathMp4)) << filePathMp4;
+    EXPECT_TRUE(common::exists(filePathMpg)) << filePathMpg;
+  }
+
+  // All temporary files will be removed after exiting scope.
+  EXPECT_FALSE(common::exists(filePathMp4)) << filePathMp4;
+  EXPECT_FALSE(common::exists(filePathMpg)) << filePathMp4;
+}
+
+
 /////////////////////////////////////////////////
 TEST_F(VideoEncoderTest, Exists)
 {

events/include/ignition/common/Event.hh

@@ -156,8 +156,9 @@ namespace ignition
       private: class EventConnection
       {
         /// \brief Constructor
-        public: EventConnection(const bool _on, const std::function<T> &_cb)
-                : callback(_cb)
+        public: EventConnection(const bool _on, const std::function<T> &_cb,
+                        const ConnectionPtr &_publicConn)
+            : callback(_cb), publicConnection(_publicConn)
         {
           // Windows Visual Studio 2012 does not have atomic_bool constructor,
           // so we have to set "on" using operator=
@@ -169,6 +170,11 @@ namespace ignition
 
         /// \brief Callback function
         public: std::function<T> callback;
+
+        /// \brief A weak pointer to the Connection pointer returned by Connect.
+        /// This is used to clear the Connection's Event pointer during
+        /// destruction of an Event.
+        public: std::weak_ptr<Connection> publicConnection;
       };
 
       /// \def EvtConnectionMap
@@ -197,6 +203,16 @@ namespace ignition
     template<typename T, typename N>
     EventT<T, N>::~EventT()
     {
+      // Clear the Event pointer on all connections so that they are not
+      // accessed after this Event is destructed.
+      for (auto &conn : this->connections)
+      {
+        auto publicCon = conn.second->publicConnection.lock();
+        if (publicCon)
+        {
+          publicCon->event = nullptr;
+        }
+      }
       this->connections.clear();
     }
 
@@ -211,8 +227,10 @@ namespace ignition
         auto const &iter = this->connections.rbegin();
         index = iter->first + 1;
       }
-      this->connections[index].reset(new EventConnection(true, _subscriber));
-      return ConnectionPtr(new Connection(this, index));
+      auto connection = ConnectionPtr(new Connection(this, index));
+      this->connections[index].reset(
+          new EventConnection(true, _subscriber, connection));
+      return connection;
     }
 
     /// \brief Get the number of connections.
@@ -234,6 +252,14 @@ namespace ignition
       if (it != this->connections.end())
       {
         it->second->on = false;
+        // The destructor of std::function seems to crashes if the function it
+        // points to is in a shared library and has been unloaded by the time
+        // the destructor is invoked. It's not clear whether this is a bug in
+        // the implementation of std::function or not. To avoid the crash,
+        // we call the destructor here by setting `callback = nullptr` because
+        // it is likely that EventT::Disconnect is called before the shared
+        // library is unloaded via Connection::~Connection.
+        it->second->callback = nullptr;
         this->connectionsToRemove.push_back(it);
       }
     }

events/src/Event_TEST.cc

@@ -283,6 +283,21 @@ TEST_F(EventTest, typeid_test)
   EXPECT_NE(typeid(Event3).name(), typeid(Event2).name());
 }
 
+/////////////////////////////////////////////////
+TEST_F(EventTest, DestructionOrder)
+{
+  auto evt = std::make_unique<common::EventT<void()>>();
+  common::ConnectionPtr conn = evt->Connect(callback);
+  evt->Signal();
+  evt.reset();
+  // Sleep to avoid warning about deleting a connection right after creation.
+  IGN_SLEEP_MS(1);
+
+  // Check that this doesn't segfault.
+  conn.reset();
+  SUCCEED();
+}
+
 /////////////////////////////////////////////////
 int main(int argc, char **argv)
 {