Let RtpVideoStreamReceiver implement KeyFrameRequestSender

Bug: None
Change-Id: I02c89aa169b63ddb6e9ec289c783f3e85d07841e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130101
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28053}

video/rtp_video_stream_receiver.cc

@@ -85,7 +85,32 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
     ReceiveStatisticsProxy* receive_stats_proxy,
     ProcessThread* process_thread,
     NackSender* nack_sender,
-    KeyFrameRequestSender* keyframe_request_sender,
+    video_coding::OnCompleteFrameCallback* complete_frame_callback,
+    rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor)
+    : RtpVideoStreamReceiver(clock,
+                             CreateRtpRtcpModule(clock,
+                                                 rtp_receive_statistics,
+                                                 transport,
+                                                 rtt_stats,
+                                                 receive_stats_proxy),
+                             packet_router,
+                             config,
+                             rtp_receive_statistics,
+                             receive_stats_proxy,
+                             process_thread,
+                             nack_sender,
+                             complete_frame_callback,
+                             frame_decryptor) {}
+
+RtpVideoStreamReceiver::RtpVideoStreamReceiver(
+    Clock* clock,
+    std::unique_ptr<RtpRtcp> rtp_rtcp,
+    PacketRouter* packet_router,
+    const VideoReceiveStream::Config* config,
+    ReceiveStatistics* rtp_receive_statistics,
+    ReceiveStatisticsProxy* receive_stats_proxy,
+    ProcessThread* process_thread,
+    NackSender* nack_sender,
     video_coding::OnCompleteFrameCallback* complete_frame_callback,
     rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor)
     : clock_(clock),
@@ -98,13 +123,8 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
       ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, this)),
       receiving_(false),
       last_packet_log_ms_(-1),
-      rtp_rtcp_(CreateRtpRtcpModule(clock,
+      rtp_rtcp_(std::move(rtp_rtcp)),
-                                    rtp_receive_statistics_,
-                                    transport,
-                                    rtt_stats,
-                                    receive_stats_proxy)),
       complete_frame_callback_(complete_frame_callback),
-      keyframe_request_sender_(keyframe_request_sender),
       has_received_frame_(false),
       frames_decryptable_(false) {
   constexpr bool remb_candidate = true;
@@ -140,11 +160,9 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
 
   if (webrtc::field_trial::IsEnabled("WebRTC-RtcpLossNotification")) {
     loss_notification_controller_ =
-        absl::make_unique<LossNotificationController>(keyframe_request_sender_,
+        absl::make_unique<LossNotificationController>(this, this);
-                                                      this);
   } else if (config_.rtp.nack.rtp_history_ms != 0) {
-    nack_module_ = absl::make_unique<NackModule>(clock_, nack_sender,
+    nack_module_ = absl::make_unique<NackModule>(clock_, nack_sender, this);
-                                                 keyframe_request_sender);
     process_thread_->RegisterModule(nack_module_.get(), RTC_FROM_HERE);
   }
 
@@ -273,7 +291,7 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData(
 
     switch (tracker_.CopyAndFixBitstream(&packet)) {
       case video_coding::H264SpsPpsTracker::kRequestKeyframe:
-        keyframe_request_sender_->RequestKeyFrame();
+        RequestKeyFrame();
         RTC_FALLTHROUGH();
       case video_coding::H264SpsPpsTracker::kDrop:
         return 0;
@@ -414,7 +432,7 @@ void RtpVideoStreamReceiver::OnAssembledFrame(
   } else if (!has_received_frame_) {
     // Request a key frame as soon as possible.
     if (frame->FrameType() != VideoFrameType::kVideoFrameKey) {
-      keyframe_request_sender_->RequestKeyFrame();
+      RequestKeyFrame();
     }
   }
 

video/rtp_video_stream_receiver.h

@@ -58,6 +58,7 @@ class UlpfecReceiver;
 class RtpVideoStreamReceiver : public LossNotificationSender,
                                public RecoveredPacketReceiver,
                                public RtpPacketSinkInterface,
+                               public KeyFrameRequestSender,
                                public video_coding::OnAssembledFrameCallback,
                                public video_coding::OnCompleteFrameCallback,
                                public OnDecryptedFrameCallback,
@@ -76,9 +77,25 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
       ReceiveStatisticsProxy* receive_stats_proxy,
       ProcessThread* process_thread,
       NackSender* nack_sender,
-      KeyFrameRequestSender* keyframe_request_sender,
       video_coding::OnCompleteFrameCallback* complete_frame_callback,
       rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor);
+
+  // Constructor with injected RtpRtcp. Intended for tests only!
+  RtpVideoStreamReceiver(
+      Clock* clock,
+      std::unique_ptr<RtpRtcp> rtp_rtcp,
+      // The packet router is optional; if provided, the RtpRtcp module for this
+      // stream is registered as a candidate for sending REMB and transport
+      // feedback.
+      PacketRouter* packet_router,
+      const VideoReceiveStream::Config* config,
+      ReceiveStatistics* rtp_receive_statistics,
+      ReceiveStatisticsProxy* receive_stats_proxy,
+      ProcessThread* process_thread,
+      NackSender* nack_sender,
+      video_coding::OnCompleteFrameCallback* complete_frame_callback,
+      rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor);
+
   ~RtpVideoStreamReceiver() override;
 
   void AddReceiveCodec(const VideoCodec& video_codec,
@@ -119,7 +136,7 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
   void OnRecoveredPacket(const uint8_t* packet, size_t packet_length) override;
 
   // Send an RTCP keyframe request.
-  void RequestKeyFrame();
+  void RequestKeyFrame() override;
 
   // Implements LossNotificationSender.
   void SendLossNotification(uint16_t last_decoded_seq_num,
@@ -205,7 +222,6 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
 
   // Members for the new jitter buffer experiment.
   video_coding::OnCompleteFrameCallback* complete_frame_callback_;
-  KeyFrameRequestSender* const keyframe_request_sender_;
   std::unique_ptr<NackModule> nack_module_;
   std::unique_ptr<LossNotificationController> loss_notification_controller_;
   rtc::scoped_refptr<video_coding::PacketBuffer> packet_buffer_;

video/rtp_video_stream_receiver_unittest.cc

@@ -16,6 +16,7 @@
 #include "api/video/video_frame_type.h"
 #include "common_video/h264/h264_common.h"
 #include "media/base/media_constants.h"
+#include "modules/rtp_rtcp/mocks/mock_rtp_rtcp.h"
 #include "modules/rtp_rtcp/source/rtp_format.h"
 #include "modules/rtp_rtcp/source/rtp_generic_frame_descriptor.h"
 #include "modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension.h"
@@ -139,8 +140,7 @@ class RtpVideoStreamReceiverTest : public ::testing::Test {
     rtp_video_stream_receiver_ = absl::make_unique<RtpVideoStreamReceiver>(
         Clock::GetRealTimeClock(), &mock_transport_, nullptr, nullptr, &config_,
         rtp_receive_statistics_.get(), nullptr, process_thread_.get(),
-        &mock_nack_sender_, &mock_key_frame_request_sender_,
+        &mock_nack_sender_, &mock_on_complete_frame_callback_, nullptr);
-        &mock_on_complete_frame_callback_, nullptr);
   }
 
   RTPVideoHeader GetDefaultH264VideoHeader() {
@@ -199,7 +199,6 @@ class RtpVideoStreamReceiverTest : public ::testing::Test {
   const webrtc::test::ScopedFieldTrials override_field_trials_;
   VideoReceiveStream::Config config_;
   MockNackSender mock_nack_sender_;
-  MockKeyFrameRequestSender mock_key_frame_request_sender_;
   MockTransport mock_transport_;
   MockOnCompleteFrameCallback mock_on_complete_frame_callback_;
   std::unique_ptr<ProcessThread> process_thread_;
@@ -543,6 +542,15 @@ TEST_F(RtpVideoStreamReceiverTest, PaddingInMediaStream) {
 }
 
 TEST_F(RtpVideoStreamReceiverTest, RequestKeyframeIfFirstFrameIsDelta) {
+  // Keep raw pointer, but pass ownership to RtpVideoStreamReceiver. Object
+  // stays alive for the duration of this test.
+  auto* mock_rtp_rtcp = new ::testing::NiceMock<MockRtpRtcp>;
+
+  rtp_video_stream_receiver_ = absl::make_unique<RtpVideoStreamReceiver>(
+      Clock::GetRealTimeClock(), absl::WrapUnique(mock_rtp_rtcp), nullptr,
+      &config_, rtp_receive_statistics_.get(), nullptr, process_thread_.get(),
+      &mock_nack_sender_, &mock_on_complete_frame_callback_, nullptr);
+
   RTPHeader rtp_header;
   RTPVideoHeader video_header;
   const std::vector<uint8_t> data({1, 2, 3, 4});
@@ -551,7 +559,7 @@ TEST_F(RtpVideoStreamReceiverTest, RequestKeyframeIfFirstFrameIsDelta) {
   video_header.is_last_packet_in_frame = true;
   video_header.codec = kVideoCodecGeneric;
   video_header.frame_type = VideoFrameType::kVideoFrameDelta;
-  EXPECT_CALL(mock_key_frame_request_sender_, RequestKeyFrame());
+  EXPECT_CALL(*mock_rtp_rtcp, RequestKeyFrame());
   rtp_video_stream_receiver_->OnReceivedPayloadData(
       data.data(), data.size(), rtp_header, video_header, absl::nullopt, false);
 }

video/video_receive_stream.cc

@@ -205,7 +205,6 @@ VideoReceiveStream::VideoReceiveStream(
                                  &stats_proxy_,
                                  process_thread_,
                                  this,  // NackSender
-                                 this,  // KeyFrameRequestSender
                                  this,  // OnCompleteFrameCallback
                                  config_.frame_decryptor),
       rtp_stream_sync_(this),

video/video_receive_stream.h

@@ -46,7 +46,6 @@ namespace internal {
 class VideoReceiveStream : public webrtc::VideoReceiveStream,
                            public rtc::VideoSinkInterface<VideoFrame>,
                            public NackSender,
-                           public KeyFrameRequestSender,
                            public video_coding::OnCompleteFrameCallback,
                            public Syncable,
                            public CallStatsObserver,
@@ -103,9 +102,6 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream,
   // Implements NackSender.
   void SendNack(const std::vector<uint16_t>& sequence_numbers) override;
 
-  // Implements KeyFrameRequestSender.
-  void RequestKeyFrame() override;
-
   // Implements video_coding::OnCompleteFrameCallback.
   void OnCompleteFrame(
       std::unique_ptr<video_coding::EncodedFrame> frame) override;
@@ -141,6 +137,7 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream,
 
   void UpdatePlayoutDelays() const
       RTC_EXCLUSIVE_LOCKS_REQUIRED(playout_delay_lock_);
+  void RequestKeyFrame();
 
   SequenceChecker worker_sequence_checker_;
   SequenceChecker module_process_sequence_checker_;