Fix dropped frames not counted issue

There's been reports of dropped frames that are not counted and
correctly reported by getStats().

If a HW decoder is used and the system is provoked by stressing
the system, I've been able to reproduce this problem. It turns out
that we've missed frames that are dropped because there is no
callback to the Decoded() function.

This CL restructures the code so that dropped frames are counted
even in cases where there's no corresponding callback for some frames.

Bug: webrtc:11229
Change-Id: I0216edba3733399c188649908d459ee86a9093d0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214783
Commit-Queue: Johannes Kron <kron@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33671}

modules/video_coding/BUILD.gn

@@ -970,6 +970,7 @@ if (rtc_include_tests) {
       "session_info_unittest.cc",
       "test/stream_generator.cc",
       "test/stream_generator.h",
+      "timestamp_map_unittest.cc",
       "timing_unittest.cc",
       "unique_timestamp_counter_unittest.cc",
       "utility/decoded_frames_history_unittest.cc",

modules/video_coding/generic_decoder.cc

@@ -93,16 +93,27 @@ void VCMDecodedFrameCallback::Decoded(VideoFrame& decodedImage,
   // callbacks from one call to Decode().
   absl::optional<VCMFrameInformation> frameInfo;
   int timestamp_map_size = 0;
+  int dropped_frames = 0;
   {
     MutexLock lock(&lock_);
+    int initial_timestamp_map_size = _timestampMap.Size();
     frameInfo = _timestampMap.Pop(decodedImage.timestamp());
     timestamp_map_size = _timestampMap.Size();
+    // _timestampMap.Pop() erases all frame upto the specified timestamp and
+    // return the frame info for this timestamp if it exists. Thus, the
+    // difference in the _timestampMap size before and after Pop() will show
+    // internally dropped frames.
+    dropped_frames =
+        initial_timestamp_map_size - timestamp_map_size - (frameInfo ? 1 : 0);
+  }
+
+  if (dropped_frames > 0) {
+    _receiveCallback->OnDroppedFrames(dropped_frames);
   }
 
   if (!frameInfo) {
     RTC_LOG(LS_WARNING) << "Too many frames backed up in the decoder, dropping "
                            "this one.";
-    _receiveCallback->OnDroppedFrames(1);
     return;
   }
 
@@ -197,17 +208,29 @@ void VCMDecodedFrameCallback::OnDecoderImplementationName(
 
 void VCMDecodedFrameCallback::Map(uint32_t timestamp,
                                   const VCMFrameInformation& frameInfo) {
+  int dropped_frames = 0;
+  {
     MutexLock lock(&lock_);
+    int initial_size = _timestampMap.Size();
     _timestampMap.Add(timestamp, frameInfo);
+    // If no frame is dropped, the new size should be |initial_size| + 1
+    dropped_frames = (initial_size + 1) - _timestampMap.Size();
+  }
+  if (dropped_frames > 0) {
+    _receiveCallback->OnDroppedFrames(dropped_frames);
+  }
 }
 
-int32_t VCMDecodedFrameCallback::Pop(uint32_t timestamp) {
+void VCMDecodedFrameCallback::ClearTimestampMap() {
+  int dropped_frames = 0;
+  {
     MutexLock lock(&lock_);
-  if (_timestampMap.Pop(timestamp) == absl::nullopt) {
-    return VCM_GENERAL_ERROR;
+    dropped_frames = _timestampMap.Size();
+    _timestampMap.Clear();
+  }
+  if (dropped_frames > 0) {
+    _receiveCallback->OnDroppedFrames(dropped_frames);
   }
-  _receiveCallback->OnDroppedFrames(1);
-  return VCM_OK;
 }
 
 VCMGenericDecoder::VCMGenericDecoder(std::unique_ptr<VideoDecoder> decoder)
@@ -281,11 +304,10 @@ int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, Timestamp now) {
   if (ret < WEBRTC_VIDEO_CODEC_OK) {
     RTC_LOG(LS_WARNING) << "Failed to decode frame with timestamp "
                         << frame.Timestamp() << ", error code: " << ret;
-    _callback->Pop(frame.Timestamp());
-    return ret;
+    _callback->ClearTimestampMap();
   } else if (ret == WEBRTC_VIDEO_CODEC_NO_OUTPUT) {
-    // No output
-    _callback->Pop(frame.Timestamp());
+    // No output.
+    _callback->ClearTimestampMap();
   }
   return ret;
 }

modules/video_coding/generic_decoder.h

@@ -45,7 +45,7 @@ class VCMDecodedFrameCallback : public DecodedImageCallback {
   void OnDecoderImplementationName(const char* implementation_name);
 
   void Map(uint32_t timestamp, const VCMFrameInformation& frameInfo);
-  int32_t Pop(uint32_t timestamp);
+  void ClearTimestampMap();
 
  private:
   SequenceChecker construction_thread_;
@@ -104,7 +104,6 @@ class VCMGenericDecoder {
 
  private:
   VCMDecodedFrameCallback* _callback;
-  VCMFrameInformation _frameInfos[kDecoderFrameMemoryLength];
   std::unique_ptr<VideoDecoder> decoder_;
   VideoCodecType _codecType;
   const bool _isExternal;

modules/video_coding/timestamp_map.cc

@@ -69,4 +69,11 @@ size_t VCMTimestampMap::Size() const {
              : next_add_idx_ + capacity_ - next_pop_idx_;
 }
 
+void VCMTimestampMap::Clear() {
+  while (!IsEmpty()) {
+    ring_buffer_[next_pop_idx_].timestamp = 0;
+    next_pop_idx_ = (next_pop_idx_ + 1) % capacity_;
+  }
+}
+
 }  // namespace webrtc

modules/video_coding/timestamp_map.h

@@ -43,6 +43,7 @@ class VCMTimestampMap {
   void Add(uint32_t timestamp, const VCMFrameInformation& data);
   absl::optional<VCMFrameInformation> Pop(uint32_t timestamp);
   size_t Size() const;
+  void Clear();
 
  private:
   struct TimestampDataTuple {

modules/video_coding/timestamp_map_unittest.cc

@@ -0,0 +1,121 @@
+/*
+ *  Copyright (c) 2021 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "modules/video_coding/timestamp_map.h"
+
+#include "test/gmock.h"
+#include "test/gtest.h"
+
+namespace webrtc {
+namespace video_coding {
+namespace {
+constexpr int kTimestampMapSize = 6;
+constexpr int kTimestamp1 = 1;
+constexpr int kTimestamp2 = 2;
+constexpr int kNoExistingTimestamp3 = 3;
+constexpr int kTimestamp4 = 4;
+constexpr int kTimestamp5 = 5;
+constexpr int kTimestamp6 = 6;
+constexpr int kTimestamp7 = 7;
+constexpr int64_t kRenderTime1 = 1000;
+constexpr int64_t kRenderTime2 = 2000;
+constexpr int64_t kRenderTime4 = 4000;
+constexpr int64_t kRenderTime5 = 5000;
+constexpr int64_t kRenderTime6 = 6000;
+constexpr int64_t kRenderTime7 = 7000;
+}  // namespace
+
+class VcmTimestampMapTest : public ::testing::Test {
+ protected:
+  VcmTimestampMapTest() : _timestampMap(kTimestampMapSize) {}
+
+  void SetUp() override {
+    _timestampMap.Add(kTimestamp1, VCMFrameInformation({kRenderTime1}));
+    _timestampMap.Add(kTimestamp2, VCMFrameInformation({kRenderTime2}));
+    _timestampMap.Add(kTimestamp4, VCMFrameInformation({kRenderTime4}));
+  }
+
+  VCMTimestampMap _timestampMap;
+};
+
+TEST_F(VcmTimestampMapTest, PopExistingFrameInfo) {
+  EXPECT_EQ(_timestampMap.Size(), 3u);
+  auto frameInfo = _timestampMap.Pop(kTimestamp1);
+  ASSERT_TRUE(frameInfo);
+  EXPECT_EQ(frameInfo->renderTimeMs, kRenderTime1);
+  frameInfo = _timestampMap.Pop(kTimestamp2);
+  ASSERT_TRUE(frameInfo);
+  EXPECT_EQ(frameInfo->renderTimeMs, kRenderTime2);
+  frameInfo = _timestampMap.Pop(kTimestamp4);
+  ASSERT_TRUE(frameInfo);
+  EXPECT_EQ(frameInfo->renderTimeMs, kRenderTime4);
+}
+
+TEST_F(VcmTimestampMapTest, PopNonexistingClearsOlderFrameInfos) {
+  auto frameInfo = _timestampMap.Pop(kNoExistingTimestamp3);
+  EXPECT_FALSE(frameInfo);
+  EXPECT_EQ(_timestampMap.Size(), 1u);
+}
+
+TEST_F(VcmTimestampMapTest, SizeIsIncrementedWhenAddingNewFrameInfo) {
+  EXPECT_EQ(_timestampMap.Size(), 3u);
+  _timestampMap.Add(kTimestamp5, VCMFrameInformation({kRenderTime5}));
+  EXPECT_EQ(_timestampMap.Size(), 4u);
+  _timestampMap.Add(kTimestamp6, VCMFrameInformation({kRenderTime6}));
+  EXPECT_EQ(_timestampMap.Size(), 5u);
+}
+
+TEST_F(VcmTimestampMapTest, SizeIsDecreasedWhenPoppingFrameInfo) {
+  EXPECT_EQ(_timestampMap.Size(), 3u);
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp1));
+  EXPECT_EQ(_timestampMap.Size(), 2u);
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp2));
+  EXPECT_EQ(_timestampMap.Size(), 1u);
+  EXPECT_FALSE(_timestampMap.Pop(kNoExistingTimestamp3));
+  EXPECT_EQ(_timestampMap.Size(), 1u);
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp4));
+  EXPECT_EQ(_timestampMap.Size(), 0u);
+}
+
+TEST_F(VcmTimestampMapTest, ClearEmptiesMap) {
+  EXPECT_EQ(_timestampMap.Size(), 3u);
+  _timestampMap.Clear();
+  EXPECT_EQ(_timestampMap.Size(), 0u);
+  // Clear empty map does nothing.
+  _timestampMap.Clear();
+  EXPECT_EQ(_timestampMap.Size(), 0u);
+}
+
+TEST_F(VcmTimestampMapTest, PopLastAddedClearsMap) {
+  EXPECT_EQ(_timestampMap.Size(), 3u);
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp4));
+  EXPECT_EQ(_timestampMap.Size(), 0u);
+}
+
+TEST_F(VcmTimestampMapTest, LastAddedIsDiscardedIfMapGetsFull) {
+  EXPECT_EQ(_timestampMap.Size(), 3u);
+  _timestampMap.Add(kTimestamp5, VCMFrameInformation({kRenderTime5}));
+  EXPECT_EQ(_timestampMap.Size(), 4u);
+  _timestampMap.Add(kTimestamp6, VCMFrameInformation({kRenderTime6}));
+  EXPECT_EQ(_timestampMap.Size(), 5u);
+  _timestampMap.Add(kTimestamp7, VCMFrameInformation({kRenderTime7}));
+  // Size is not incremented since the oldest element is discarded.
+  EXPECT_EQ(_timestampMap.Size(), 5u);
+  EXPECT_FALSE(_timestampMap.Pop(kTimestamp1));
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp2));
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp4));
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp5));
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp6));
+  EXPECT_TRUE(_timestampMap.Pop(kTimestamp7));
+  EXPECT_EQ(_timestampMap.Size(), 0u);
+}
+
+}  // namespace video_coding
+}  // namespace webrtc