Increment frame count once for duplicate frame with FrameBuffer3

R=philipel@webrtc.org

Change-Id: I9a961417f86fd1f968a42730249d479e3ebd4784
Bug: webrtc:13343, webrtc:13755
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/252583
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Evan Shrubsole <eshr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36100}

video/BUILD.gn

@@ -309,6 +309,7 @@ rtc_library("frame_buffer_proxy") {
     "../api/metronome",
     "../api/task_queue",
     "../api/video:encoded_frame",
+    "../api/video:video_rtp_headers",
     "../modules/video_coding",
     "../modules/video_coding:frame_buffer",
     "../modules/video_coding:frame_helpers",

video/frame_buffer_proxy.cc

@@ -17,6 +17,8 @@
 #include "absl/base/attributes.h"
 #include "absl/functional/bind_front.h"
 #include "api/sequence_checker.h"
+#include "api/video/encoded_frame.h"
+#include "api/video/video_content_type.h"
 #include "modules/video_coding/frame_buffer2.h"
 #include "modules/video_coding/frame_buffer3.h"
 #include "modules/video_coding/frame_helpers.h"
@@ -138,6 +140,25 @@ static constexpr int kMaxFramesHistory = 1 << 13;
 // low-latency renderer is used.
 static constexpr size_t kZeroPlayoutDelayDefaultMaxDecodeQueueSize = 8;
 
+struct FrameMetadata {
+  explicit FrameMetadata(const EncodedFrame& frame)
+      : is_last_spatial_layer(frame.is_last_spatial_layer),
+        is_keyframe(frame.is_keyframe()),
+        size(frame.size()),
+        contentType(frame.contentType()),
+        delayed_by_retransmission(frame.delayed_by_retransmission()),
+        rtp_timestamp(frame.Timestamp()),
+        receive_time(frame.ReceivedTime()) {}
+
+  const bool is_last_spatial_layer;
+  const bool is_keyframe;
+  const size_t size;
+  const VideoContentType contentType;
+  const bool delayed_by_retransmission;
+  const uint32_t rtp_timestamp;
+  const int64_t receive_time;
+};
+
 // Encapsulates use of the new frame buffer for use in VideoReceiveStream. This
 // behaves the same as the FrameBuffer2Proxy but uses frame_buffer3 instead.
 // Responsibilities from frame_buffer2, like stats, jitter and frame timing
@@ -218,14 +239,19 @@ class FrameBuffer3Proxy : public FrameBufferProxy {
   absl::optional<int64_t> InsertFrame(
       std::unique_ptr<EncodedFrame> frame) override {
     RTC_DCHECK_RUN_ON(&worker_sequence_checker_);
-    if (frame->is_last_spatial_layer)
-      stats_proxy_->OnCompleteFrame(frame->is_keyframe(), frame->size(),
-                                    frame->contentType());
-    if (!frame->delayed_by_retransmission())
-      timing_->IncomingTimestamp(frame->Timestamp(), frame->ReceivedTime());
-
+    FrameMetadata metadata(*frame);
+    int complete_units = buffer_->GetTotalNumberOfContinuousTemporalUnits();
     buffer_->InsertFrame(std::move(frame));
-    MaybeScheduleFrameForRelease();
+    // Don't update stats or frame timing if the inserted frame did not complete
+    // a new temporal layer.
+    if (complete_units < buffer_->GetTotalNumberOfContinuousTemporalUnits()) {
+      stats_proxy_->OnCompleteFrame(metadata.is_keyframe, metadata.size,
+                                    metadata.contentType);
+      if (!metadata.delayed_by_retransmission)
+        timing_->IncomingTimestamp(metadata.rtp_timestamp,
+                                   metadata.receive_time);
+      MaybeScheduleFrameForRelease();
+    }
 
     return buffer_->LastContinuousFrameId();
   }

video/frame_buffer_proxy_unittest.cc

@@ -198,6 +198,12 @@ class VCMReceiveStatisticsCallbackMock : public VCMReceiveStatisticsCallback {
               (const TimingFrameInfo& info),
               (override));
 };
+
+bool IsFrameBuffer2Enabled() {
+  return field_trial::FindFullName("WebRTC-FrameBuffer3")
+             .find("arm:FrameBuffer2") != std::string::npos;
+}
+
 }  // namespace
 
 constexpr auto kMaxWaitForKeyframe = TimeDelta::Millis(500);
@@ -635,6 +641,64 @@ TEST_P(FrameBufferProxyTest, TestStatsCallback) {
   time_controller_.AdvanceTime(TimeDelta::Zero());
 }
 
+TEST_P(FrameBufferProxyTest, FrameCompleteCalledOnceForDuplicateFrame) {
+  EXPECT_CALL(stats_callback_,
+              OnCompleteFrame(true, kFrameSize, VideoContentType::UNSPECIFIED))
+      .Times(1);
+
+  StartNextDecodeForceKeyframe();
+  proxy_->InsertFrame(Builder().Id(0).Time(0).AsLast().Build());
+  proxy_->InsertFrame(Builder().Id(0).Time(0).AsLast().Build());
+  // Flush stats posted on the decode queue.
+  time_controller_.AdvanceTime(TimeDelta::Zero());
+}
+
+TEST_P(FrameBufferProxyTest, FrameCompleteCalledOnceForSingleTemporalUnit) {
+  StartNextDecodeForceKeyframe();
+
+  // `OnCompleteFrame` should not be called for the first two frames since they
+  // do not complete the temporal layer.
+  EXPECT_CALL(stats_callback_, OnCompleteFrame(_, _, _)).Times(0);
+  proxy_->InsertFrame(Builder().Id(0).Time(0).Build());
+  proxy_->InsertFrame(Builder().Id(1).Time(0).Refs({0}).Build());
+  time_controller_.AdvanceTime(TimeDelta::Zero());
+  // Flush stats posted on the decode queue.
+  ::testing::Mock::VerifyAndClearExpectations(&stats_callback_);
+
+  // Note that this frame is not marked as a keyframe since the last spatial
+  // layer has dependencies.
+  EXPECT_CALL(stats_callback_,
+              OnCompleteFrame(false, kFrameSize, VideoContentType::UNSPECIFIED))
+      .Times(1);
+  proxy_->InsertFrame(Builder().Id(2).Time(0).Refs({0, 1}).AsLast().Build());
+  // Flush stats posted on the decode queue.
+  time_controller_.AdvanceTime(TimeDelta::Zero());
+}
+
+TEST_P(FrameBufferProxyTest, FrameCompleteCalledOnceForCompleteTemporalUnit) {
+  // FrameBuffer2 logs the complete frame on the arrival of the last layer.
+  if (IsFrameBuffer2Enabled())
+    return;
+  StartNextDecodeForceKeyframe();
+
+  // `OnCompleteFrame` should not be called for the first two frames since they
+  // do not complete the temporal layer. Frame 1 arrives later, at which time
+  // this frame can finally be considered complete.
+  EXPECT_CALL(stats_callback_, OnCompleteFrame(_, _, _)).Times(0);
+  proxy_->InsertFrame(Builder().Id(0).Time(0).Build());
+  proxy_->InsertFrame(Builder().Id(2).Time(0).Refs({0, 1}).AsLast().Build());
+  time_controller_.AdvanceTime(TimeDelta::Zero());
+  // Flush stats posted on the decode queue.
+  ::testing::Mock::VerifyAndClearExpectations(&stats_callback_);
+
+  EXPECT_CALL(stats_callback_,
+              OnCompleteFrame(false, kFrameSize, VideoContentType::UNSPECIFIED))
+      .Times(1);
+  proxy_->InsertFrame(Builder().Id(1).Time(0).Refs({0}).Build());
+  // Flush stats posted on the decode queue.
+  time_controller_.AdvanceTime(TimeDelta::Zero());
+}
+
 // Note: This test takes a long time to run if the fake metronome is active.
 // Since the test needs to wait for the timestamp to rollover, it has a fake
 // delay of around 6.5 hours. Even though time is simulated, this will be
@@ -690,8 +754,7 @@ TEST_P(FrameBufferProxyTest, NextFrameWithOldTimestamp) {
                           .AsLast()
                           .Build());
   // FrameBuffer2 drops the frame, while FrameBuffer3 will continue the stream.
-  if (field_trial::FindFullName("WebRTC-FrameBuffer3")
-          .find("arm:FrameBuffer2") == std::string::npos) {
+  if (!IsFrameBuffer2Enabled()) {
     EXPECT_THAT(WaitForFrameOrTimeout(kFps30Delay), Frame(WithId(2)));
   } else {
     EXPECT_THAT(WaitForFrameOrTimeout(kMaxWaitForFrame), TimedOut());