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}
diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn
index a3187c2..96b48b6 100644
--- a/modules/video_coding/BUILD.gn
+++ b/modules/video_coding/BUILD.gn
@@ -970,6 +970,7 @@
"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",
diff --git a/modules/video_coding/generic_decoder.cc b/modules/video_coding/generic_decoder.cc
index 85c68da..621fd73 100644
--- a/modules/video_coding/generic_decoder.cc
+++ b/modules/video_coding/generic_decoder.cc
@@ -93,16 +93,27 @@
// 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::Map(uint32_t timestamp,
const VCMFrameInformation& frameInfo) {
- MutexLock lock(&lock_);
- _timestampMap.Add(timestamp, 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) {
- MutexLock lock(&lock_);
- if (_timestampMap.Pop(timestamp) == absl::nullopt) {
- return VCM_GENERAL_ERROR;
+void VCMDecodedFrameCallback::ClearTimestampMap() {
+ int dropped_frames = 0;
+ {
+ MutexLock lock(&lock_);
+ dropped_frames = _timestampMap.Size();
+ _timestampMap.Clear();
}
- _receiveCallback->OnDroppedFrames(1);
- return VCM_OK;
+ if (dropped_frames > 0) {
+ _receiveCallback->OnDroppedFrames(dropped_frames);
+ }
}
VCMGenericDecoder::VCMGenericDecoder(std::unique_ptr<VideoDecoder> decoder)
@@ -281,11 +304,10 @@
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;
}
diff --git a/modules/video_coding/generic_decoder.h b/modules/video_coding/generic_decoder.h
index 2ff6b20..8e79cb4 100644
--- a/modules/video_coding/generic_decoder.h
+++ b/modules/video_coding/generic_decoder.h
@@ -45,7 +45,7 @@
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 @@
private:
VCMDecodedFrameCallback* _callback;
- VCMFrameInformation _frameInfos[kDecoderFrameMemoryLength];
std::unique_ptr<VideoDecoder> decoder_;
VideoCodecType _codecType;
const bool _isExternal;
diff --git a/modules/video_coding/timestamp_map.cc b/modules/video_coding/timestamp_map.cc
index 7762e36..f6fb818 100644
--- a/modules/video_coding/timestamp_map.cc
+++ b/modules/video_coding/timestamp_map.cc
@@ -69,4 +69,11 @@
: 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
diff --git a/modules/video_coding/timestamp_map.h b/modules/video_coding/timestamp_map.h
index 86ce55b..dc20a05 100644
--- a/modules/video_coding/timestamp_map.h
+++ b/modules/video_coding/timestamp_map.h
@@ -43,6 +43,7 @@
void Add(uint32_t timestamp, const VCMFrameInformation& data);
absl::optional<VCMFrameInformation> Pop(uint32_t timestamp);
size_t Size() const;
+ void Clear();
private:
struct TimestampDataTuple {
diff --git a/modules/video_coding/timestamp_map_unittest.cc b/modules/video_coding/timestamp_map_unittest.cc
new file mode 100644
index 0000000..5e90786
--- /dev/null
+++ b/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
