diff --git a/av/src/VideoEncoder_TEST.cc b/av/src/VideoEncoder_TEST.cc index bc9ef3b83..495b11a7d 100644 --- a/av/src/VideoEncoder_TEST.cc +++ b/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( 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( + 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( + 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( + 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) { diff --git a/bitbucket-pipelines.yml b/bitbucket-pipelines.yml deleted file mode 100644 index 7359e19bd..000000000 --- a/bitbucket-pipelines.yml +++ /dev/null @@ -1,70 +0,0 @@ -image: ubuntu:bionic - -options: - max-time: 20 - -pipelines: - default: - - step: - script: - - apt-get update - - apt-get -y install - cmake pkg-config python ruby-ronn uuid-dev - libavcodec-dev libavdevice-dev libavformat-dev libavutil-dev - libfreeimage-dev libgts-dev libswscale-dev libtinyxml2-dev - curl git cppcheck g++-8 - - update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-8 800 --slave /usr/bin/g++ g++ /usr/bin/g++-8 --slave /usr/bin/gcov gcov /usr/bin/gcov-8 - - gcc -v - - g++ -v - - gcov -v - # lcov - - git clone https://github.com/linux-test-project/lcov.git - - cd lcov - # lcov was broken briefly (see https://github.com/linux-test-project/lcov/issues/55 ) - # checkout an explicit commit to avoid future regressions - - git checkout 04335632c371b5066e722298c9f8c6f11b210201 - - make install - - cd .. - - # Ignition dependencies - - echo "deb http://packages.osrfoundation.org/gazebo/ubuntu-stable bionic main" > /etc/apt/sources.list.d/gazebo-stable.list - - apt-key adv --keyserver keyserver.ubuntu.com --recv-keys D2486D2DD83DB69272AFE98867170598AF249743 - - apt-get update - - apt-get -y install - libignition-cmake2-dev - libignition-math6-dev - # Ignition cmake (uncomment to build from a specific branch) - # - git clone http://github.com/ignitionrobotics/ign-cmake -b gz11 - # - cd ign-cmake - # - mkdir build - # - cd build - # - cmake .. -DBUILD_TESTING=OFF - # - make -j4 install - # - cd ../.. - # Ignition math (uncomment to build from a specific branch) - # - git clone http://github.com/ignitionrobotics/ign-math -b gz11 - # - cd ign-math - # - mkdir build - # - cd build - # - cmake .. -DBUILD_TESTING=OFF - # - make -j4 install - # - cd ../.. - # Build - - mkdir build - - cd build - - cmake .. -DCMAKE_BUILD_TYPE=coverage - - make - # Tests - - make test - - make install - # Coverage - - make coverage - # Use a special version of codecov for handling gcc8 output. - - bash <(curl -s https://raw.githubusercontent.com/codecov/codecov-bash/4678d212cce2078bbaaf5027af0c0dafaad6a095/codecov) -X gcovout -X gcov - - make codecheck - # Examples - - cd ../examples - - mkdir build - - cd build - - cmake .. - - make diff --git a/events/include/ignition/common/Event.hh b/events/include/ignition/common/Event.hh index ca1dfbe2a..d1097613b 100644 --- a/events/include/ignition/common/Event.hh +++ b/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 &_cb) - : callback(_cb) + public: EventConnection(const bool _on, const std::function &_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 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 publicConnection; }; /// \def EvtConnectionMap @@ -197,6 +203,16 @@ namespace ignition template EventT::~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); } } diff --git a/events/src/Event_TEST.cc b/events/src/Event_TEST.cc index 9a64f9203..388bd6b0d 100644 --- a/events/src/Event_TEST.cc +++ b/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::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) {