Fix video encoder timing

av/include/ignition/common/Video.hh

@@ -58,6 +58,15 @@ namespace ignition
       /// \return the height
       public: int Height() const;
 
+      /// \brief Convenience type alias for duration
+      /// where 1000000 is the same as AV_TIME_BASE fractional seconds
+      public:
+        using Length = std::chrono::duration<int64_t, std::ratio<1, 1000000>>;
+
+      /// \brief Get the duration of the video
+      /// \return the duration
+      public: Length Duration() const;
+
       /// \brief Get the next frame of the video.
       /// \param[out] _img Image in which the frame is stored
       /// \return  false on error

av/src/Video.cc

@@ -292,3 +292,9 @@ int Video::Height() const
 {
   return this->dataPtr->codecCtx->height;
 }
+
+/////////////////////////////////////////////////
+Video::Length Video::Duration() const
+{
+  return Video::Length(this->dataPtr->formatCtx->duration);
+}

av/src/VideoEncoder.cc

@@ -76,6 +76,9 @@ class ignition::common::VideoEncoderPrivate
   /// \brief Previous time when the frame is added.
   public: std::chrono::steady_clock::time_point timePrev;
 
+  /// \brief Time when the first frame is added.
+  public: std::chrono::steady_clock::time_point timeStart;
+
   /// \brief Number of frames in the video
   public: uint64_t frameCount = 0;
 
@@ -125,7 +128,8 @@ bool VideoEncoder::Start(const std::string &_format,
   // This will be true if Stop has been called, but not reset. We will reset
   // automatically to prevent any errors.
   if (this->dataPtr->formatCtx || this->dataPtr->avInFrame ||
-      this->dataPtr->avOutFrame || this->dataPtr->swsCtx)
+      this->dataPtr->avOutFrame || this->dataPtr->swsCtx ||
+      this->dataPtr->frameCount > 0u)
   {
     this->Reset();
   }
@@ -505,9 +509,14 @@ bool VideoEncoder::AddFrame(const unsigned char *_frame,
   auto dt = _timestamp - this->dataPtr->timePrev;
 
   // Skip frames that arrive faster than the video's fps
-  if (dt < std::chrono::duration<double>(1.0/this->dataPtr->fps))
+  double period = 1.0/this->dataPtr->fps;
+  if (this->dataPtr->frameCount > 0u &&
+      dt < std::chrono::duration<double>(period))
     return false;
 
+  if (this->dataPtr->frameCount == 0u)
+    this->dataPtr->timeStart = _timestamp;
+
   this->dataPtr->timePrev = _timestamp;
 
   // Cause the sws to be recreated on image resize
@@ -575,97 +584,111 @@ bool VideoEncoder::AddFrame(const unsigned char *_frame,
       this->dataPtr->avOutFrame->data,
       this->dataPtr->avOutFrame->linesize);
 
-  this->dataPtr->avOutFrame->pts = this->dataPtr->frameCount++;
-
-#if LIBAVCODEC_VERSION_INT < AV_VERSION_INT(57, 40, 101)
-  int gotOutput = 0;
-  AVPacket avPacket;
-  av_init_packet(&avPacket);
-  avPacket.data = nullptr;
-  avPacket.size = 0;
+  // compute frame number based on timestamp of current image
+  auto timeSinceStart = std::chrono::duration_cast<std::chrono::milliseconds>(
+      _timestamp - this->dataPtr->timeStart);
+  double durationSec = timeSinceStart.count() / 1000.0;
+  uint64_t frameNumber = durationSec / period;
 
-  int ret = avcodec_encode_video2(this->dataPtr->codecCtx, &avPacket,
-      this->dataPtr->avOutFrame, &gotOutput);
+  uint64_t frameDiff = frameNumber + 1 - this->dataPtr->frameCount;
 
-  if (ret >= 0 && gotOutput == 1)
+  // make sure we have continuous pts (frame number) otherwise some decoders
+  // may not be happy. So encode more (duplicate) frames until the current frame
+  // number
+  for (uint64_t i = 0u; i < frameDiff; ++i)
   {
-    avPacket.stream_index = this->dataPtr->videoStream->index;
-
-    // Scale timestamp appropriately.
-    if (avPacket.pts != static_cast<int64_t>(AV_NOPTS_VALUE))
-    {
-      avPacket.pts = av_rescale_q(avPacket.pts,
-          this->dataPtr->codecCtx->time_base,
-          this->dataPtr->videoStream->time_base);
-    }
-
-    if (avPacket.dts != static_cast<int64_t>(AV_NOPTS_VALUE))
-    {
-      avPacket.dts = av_rescale_q(
-          avPacket.dts,
-          this->dataPtr->codecCtx->time_base,
-          this->dataPtr->videoStream->time_base);
-    }
-
-    // Write frame to disk
-    ret = av_interleaved_write_frame(this->dataPtr->formatCtx, &avPacket);
-
-    if (ret < 0)
-    {
-      ignerr << "Error writing frame" << std::endl;
-      return false;
-    }
-  }
-
-  av_free_packet(&avPacket);
+    this->dataPtr->avOutFrame->pts = this->dataPtr->frameCount++;
 
-// #else for libavcodec version check
-#else
-
-  AVPacket *avPacket = av_packet_alloc();
-  av_init_packet(avPacket);
-
-  avPacket->data = nullptr;
-  avPacket->size = 0;
-
-  int ret = avcodec_send_frame(this->dataPtr->codecCtx,
-                               this->dataPtr->avOutFrame);
+#if LIBAVCODEC_VERSION_INT < AV_VERSION_INT(57, 40, 101)
+    int gotOutput = 0;
+    AVPacket avPacket;
+    av_init_packet(&avPacket);
+    avPacket.data = nullptr;
+    avPacket.size = 0;
 
-  // This loop will retrieve and write available packets
-  while (ret >= 0)
-  {
-    ret = avcodec_receive_packet(this->dataPtr->codecCtx, avPacket);
+    int ret = avcodec_encode_video2(this->dataPtr->codecCtx, &avPacket,
+        this->dataPtr->avOutFrame, &gotOutput);
 
-    // Potential performance improvement: Queue the packets and write in
-    // a separate thread.
-    if (ret >= 0)
+    if (ret >= 0 && gotOutput == 1)
     {
-      avPacket->stream_index = this->dataPtr->videoStream->index;
+      avPacket.stream_index = this->dataPtr->videoStream->index;
 
       // Scale timestamp appropriately.
-      if (avPacket->pts != static_cast<int64_t>(AV_NOPTS_VALUE))
+      if (avPacket.pts != static_cast<int64_t>(AV_NOPTS_VALUE))
       {
-        avPacket->pts = av_rescale_q(avPacket->pts,
+        avPacket.pts = av_rescale_q(avPacket.pts,
             this->dataPtr->codecCtx->time_base,
             this->dataPtr->videoStream->time_base);
       }
 
-      if (avPacket->dts != static_cast<int64_t>(AV_NOPTS_VALUE))
+      if (avPacket.dts != static_cast<int64_t>(AV_NOPTS_VALUE))
       {
-        avPacket->dts = av_rescale_q(
-            avPacket->dts,
+        avPacket.dts = av_rescale_q(
+            avPacket.dts,
             this->dataPtr->codecCtx->time_base,
             this->dataPtr->videoStream->time_base);
       }
 
       // Write frame to disk
-      if (av_interleaved_write_frame(this->dataPtr->formatCtx, avPacket) < 0)
+      ret = av_interleaved_write_frame(this->dataPtr->formatCtx, &avPacket);
+
+      if (ret < 0)
+      {
         ignerr << "Error writing frame" << std::endl;
+        return false;
+      }
     }
-  }
 
-  av_packet_unref(avPacket);
+    av_free_packet(&avPacket);
+
+    // #else for libavcodec version check
+#else
+
+    AVPacket *avPacket = av_packet_alloc();
+    av_init_packet(avPacket);
+
+    avPacket->data = nullptr;
+    avPacket->size = 0;
+
+    int ret = avcodec_send_frame(this->dataPtr->codecCtx,
+                                 this->dataPtr->avOutFrame);
+
+    // This loop will retrieve and write available packets
+    while (ret >= 0)
+    {
+      ret = avcodec_receive_packet(this->dataPtr->codecCtx, avPacket);
+
+      // Potential performance improvement: Queue the packets and write in
+      // a separate thread.
+      if (ret >= 0)
+      {
+        avPacket->stream_index = this->dataPtr->videoStream->index;
+
+        // Scale timestamp appropriately.
+        if (avPacket->pts != static_cast<int64_t>(AV_NOPTS_VALUE))
+        {
+          avPacket->pts = av_rescale_q(avPacket->pts,
+              this->dataPtr->codecCtx->time_base,
+              this->dataPtr->videoStream->time_base);
+        }
+
+        if (avPacket->dts != static_cast<int64_t>(AV_NOPTS_VALUE))
+        {
+          avPacket->dts = av_rescale_q(
+              avPacket->dts,
+              this->dataPtr->codecCtx->time_base,
+              this->dataPtr->videoStream->time_base);
+        }
+
+        // Write frame to disk
+        if (av_interleaved_write_frame(this->dataPtr->formatCtx, avPacket) < 0)
+          ignerr << "Error writing frame" << std::endl;
+      }
+    }
+
+    av_packet_unref(avPacket);
 #endif
+  }
   return true;
 }
 
@@ -755,4 +778,6 @@ void VideoEncoder::Reset()
   this->dataPtr->bitRate = VIDEO_ENCODER_BITRATE_DEFAULT;
   this->dataPtr->fps = VIDEO_ENCODER_FPS_DEFAULT;
   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();
 }

test/integration/CMakeLists.txt

@@ -6,13 +6,22 @@ ign_get_sources(tests)
 # FIXME the mesh test does not work
 list(REMOVE_ITEM tests mesh.cc)
 
+if (${SKIP_av})
+  list(REMOVE_ITEM tests encoder_timing.cc)
+endif()
+
 ign_build_tests(
   TYPE INTEGRATION
-  SOURCES ${tests})
+  SOURCES ${tests}
+)
 
 if(TARGET INTEGRATION_plugin)
   # We add this dependency to make sure that DummyPlugins gets generated
   # before INTEGRATION_plugin so that its auto-generated header is available.
   # We do not want to link INTEGRATION_plugin to the DummyPlugins library.
   add_dependencies(INTEGRATION_plugin IGNDummyPlugins)
 endif()
+
+if(TARGET INTEGRATION_encoder_timing)
+  target_link_libraries(INTEGRATION_encoder_timing ${PROJECT_LIBRARY_TARGET_NAME}-av)
+endif()

test/integration/encoder_timing.cc

@@ -0,0 +1,62 @@
+/*
+ * Copyright (C) 2020 Open Source Robotics Foundation
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+*/
+#include <gtest/gtest.h>
+#include <array>
+#include "ignition/common/VideoEncoder.hh"
+#include "ignition/common/Video.hh"
+#include "test_config.h"
+#include "test/util.hh"
+
+using namespace ignition;
+using namespace common;
+
+const unsigned int kSize = 10;
+const std::array<unsigned char, kSize*kSize> kFrame = {};
+
+// set to 720ms because video duration missing additional 18 frames
+//    which may be due to how video encoding works
+const std::chrono::milliseconds kTol(720);
+
+void durationTest(VideoEncoder &_vidEncoder, Video &_video,
+                  const int &_fps, const int &_seconds)
+{
+  _vidEncoder.Start("mp4", "", kSize, kSize, _fps, 0);
+
+  int frameCount = 0;
+  while (frameCount != _fps*_seconds)
+  {
+    if (_vidEncoder.AddFrame(kFrame.data(), kSize, kSize))
+      ++frameCount;
+  }
+
+  _vidEncoder.Stop();
+  _video.Load(common::joinPaths(common::cwd(), "/TMP_RECORDING.mp4"));
+
+  EXPECT_NEAR(std::chrono::duration_cast<std::chrono::milliseconds>(
+              _video.Duration()).count(),
+              _seconds*1000,
+              kTol.count());
+}
+
+TEST(EncoderTimingTest, Duration)
+{
+  VideoEncoder vidEncoder;
+  Video video;
+
+  durationTest(vidEncoder, video, 50, 1);
+  durationTest(vidEncoder, video, 30, 2);
+  durationTest(vidEncoder, video, 25, 5);
+}