Remove KeyFrameRequestSender argument from RtpVideoStreamReceiver2.

Bug: webrtc:14249
Change-Id: Ia65c0681989725257595a2a8b4336c55967d4cec
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/267666
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Philip Eliasson <philipel@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37431}

video/rtp_video_stream_receiver2.cc

@@ -214,7 +214,6 @@ RtpVideoStreamReceiver2::RtpVideoStreamReceiver2(
     RtcpCnameCallback* rtcp_cname_callback,
     NackPeriodicProcessor* nack_periodic_processor,
     NackSender* nack_sender,
-    KeyFrameRequestSender* keyframe_request_sender,
     OnCompleteFrameCallback* complete_frame_callback,
     rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor,
     rtc::scoped_refptr<FrameTransformerInterface> frame_transformer,
@@ -243,7 +242,6 @@ RtpVideoStreamReceiver2::RtpVideoStreamReceiver2(
           config_.rtp.rtcp_xr.receiver_reference_time_report,
           config_.rtp.local_ssrc)),
       complete_frame_callback_(complete_frame_callback),
-      keyframe_request_sender_(keyframe_request_sender),
       keyframe_request_method_(config_.rtp.keyframe_method),
       // TODO(bugs.webrtc.org/10336): Let `rtcp_feedback_buffer_` communicate
       // directly with `rtp_rtcp_`.
@@ -684,9 +682,7 @@ void RtpVideoStreamReceiver2::RequestKeyFrame() {
   // TODO(bugs.webrtc.org/10336): Allow the sender to ignore key frame requests
   // issued by anything other than the LossNotificationController if it (the
   // sender) is relying on LNTF alone.
-  if (keyframe_request_sender_) {
+  if (keyframe_request_method_ == KeyFrameReqMethod::kPliRtcp) {
-    keyframe_request_sender_->RequestKeyFrame();
-  } else if (keyframe_request_method_ == KeyFrameReqMethod::kPliRtcp) {
     rtp_rtcp_->SendPictureLossIndication();
   } else if (keyframe_request_method_ == KeyFrameReqMethod::kFirRtcp) {
     rtp_rtcp_->SendFullIntraRequest();

video/rtp_video_stream_receiver2.h

@@ -93,7 +93,6 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender,
       NackSender* nack_sender,
       // The KeyFrameRequestSender is optional; if not provided, key frame
       // requests are sent via the internal RtpRtcp module.
-      KeyFrameRequestSender* keyframe_request_sender,
       OnCompleteFrameCallback* complete_frame_callback,
       rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor,
       rtc::scoped_refptr<FrameTransformerInterface> frame_transformer,
@@ -324,7 +323,6 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender,
   const std::unique_ptr<ModuleRtpRtcpImpl2> rtp_rtcp_;
 
   OnCompleteFrameCallback* complete_frame_callback_;
-  KeyFrameRequestSender* const keyframe_request_sender_;
   const KeyFrameReqMethod keyframe_request_method_;
 
   RtcpFeedbackBuffer rtcp_feedback_buffer_;

video/rtp_video_stream_receiver2_unittest.cc

@@ -37,12 +37,14 @@
 #include "test/gtest.h"
 #include "test/mock_frame_transformer.h"
 #include "test/mock_transport.h"
+#include "test/rtcp_packet_parser.h"
 #include "test/scoped_key_value_config.h"
 #include "test/time_controller/simulated_task_queue.h"
 #include "test/time_controller/simulated_time_controller.h"
 
 using ::testing::_;
 using ::testing::ElementsAre;
+using ::testing::Eq;
 using ::testing::Invoke;
 using ::testing::SizeIs;
 using ::testing::Values;
@@ -159,11 +161,13 @@ class RtpVideoStreamReceiver2Test : public ::testing::Test,
         TaskQueueBase::Current(), Clock::GetRealTimeClock(), &mock_transport_,
         nullptr, nullptr, &config_, rtp_receive_statistics_.get(), nullptr,
         nullptr, &nack_periodic_processor_, &mock_nack_sender_,
-        &mock_key_frame_request_sender_, &mock_on_complete_frame_callback_,
+        &mock_on_complete_frame_callback_, nullptr, nullptr, field_trials_);
-        nullptr, nullptr, field_trials_);
     rtp_video_stream_receiver_->AddReceiveCodec(kPayloadType,
                                                 kVideoCodecGeneric, {},
                                                 /*raw_payload=*/false);
+    ON_CALL(mock_transport_, SendRtcp)
+        .WillByDefault(
+            Invoke(&rtcp_packet_parser_, &test::RtcpPacketParser::Parse));
   }
 
   RTPVideoHeader GetDefaultH264VideoHeader() {
@@ -232,7 +236,7 @@ class RtpVideoStreamReceiver2Test : public ::testing::Test,
   VideoReceiveStreamInterface::Config config_;
   NackPeriodicProcessor nack_periodic_processor_;
   MockNackSender mock_nack_sender_;
-  MockKeyFrameRequestSender mock_key_frame_request_sender_;
+  test::RtcpPacketParser rtcp_packet_parser_;
   MockTransport mock_transport_;
   MockOnCompleteFrameCallback mock_on_complete_frame_callback_;
   std::unique_ptr<ReceiveStatistics> rtp_receive_statistics_;
@@ -710,9 +714,10 @@ TEST_F(RtpVideoStreamReceiver2Test, RequestKeyframeIfFirstFrameIsDelta) {
   rtp_packet.SetSequenceNumber(1);
   RTPVideoHeader video_header =
       GetGenericVideoHeader(VideoFrameType::kVideoFrameDelta);
-  EXPECT_CALL(mock_key_frame_request_sender_, RequestKeyFrame());
+
   rtp_video_stream_receiver_->OnReceivedPayloadData(data, rtp_packet,
                                                     video_header);
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(1));
 }
 
 TEST_F(RtpVideoStreamReceiver2Test, RequestKeyframeWhenPacketBufferGetsFull) {
@@ -734,9 +739,9 @@ TEST_F(RtpVideoStreamReceiver2Test, RequestKeyframeWhenPacketBufferGetsFull) {
     rtp_packet.SetSequenceNumber(rtp_packet.SequenceNumber() + 2);
   }
 
-  EXPECT_CALL(mock_key_frame_request_sender_, RequestKeyFrame());
   rtp_video_stream_receiver_->OnReceivedPayloadData(data, rtp_packet,
                                                     video_header);
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(1));
 }
 
 TEST_F(RtpVideoStreamReceiver2Test, SinkGetsRtpNotifications) {
@@ -1118,15 +1123,16 @@ TEST_F(RtpVideoStreamReceiver2DependencyDescriptorTest,
       stream_structure.templates[0];
   keyframe_descriptor_without_structure.frame_number = 0;
 
-  EXPECT_CALL(mock_key_frame_request_sender_, RequestKeyFrame).Times(2);
   InjectPacketWith(stream_structure, keyframe_descriptor_without_structure);
 
   // Not enough time since last keyframe request
   time_controller_.AdvanceTime(TimeDelta::Millis(500));
   InjectPacketWith(stream_structure, keyframe_descriptor_without_structure);
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(1));
 
   time_controller_.AdvanceTime(TimeDelta::Millis(501));
   InjectPacketWith(stream_structure, keyframe_descriptor_without_structure);
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(2));
 }
 
 TEST_F(RtpVideoStreamReceiver2Test, TransformFrame) {
@@ -1137,7 +1143,7 @@ TEST_F(RtpVideoStreamReceiver2Test, TransformFrame) {
   auto receiver = std::make_unique<RtpVideoStreamReceiver2>(
       TaskQueueBase::Current(), Clock::GetRealTimeClock(), &mock_transport_,
       nullptr, nullptr, &config_, rtp_receive_statistics_.get(), nullptr,
-      nullptr, &nack_periodic_processor_, &mock_nack_sender_, nullptr,
+      nullptr, &nack_periodic_processor_, &mock_nack_sender_,
       &mock_on_complete_frame_callback_, nullptr, mock_frame_transformer,
       field_trials_);
   receiver->AddReceiveCodec(kPayloadType, kVideoCodecGeneric, {},

video/video_receive_stream2.cc

@@ -236,7 +236,6 @@ VideoReceiveStream2::VideoReceiveStream2(
                                  &stats_proxy_,
                                  nack_periodic_processor,
                                  this,  // NackSender
-                                 nullptr,  // Use default KeyFrameRequestSender
                                  this,  // OnCompleteFrameCallback
                                  std::move(config_.frame_decryptor),
                                  std::move(config_.frame_transformer),

video/video_receive_stream2_unittest.cc

@@ -48,6 +48,7 @@
 #include "test/gmock.h"
 #include "test/gtest.h"
 #include "test/mock_transport.h"
+#include "test/rtcp_packet_parser.h"
 #include "test/run_loop.h"
 #include "test/time_controller/simulated_time_controller.h"
 #include "test/video_decoder_proxy_factory.h"
@@ -184,7 +185,7 @@ Timestamp ReceiveTimeForFrame(int id) {
 class VideoReceiveStream2Test : public ::testing::TestWithParam<bool> {
  public:
   auto DefaultDecodeAction() {
-    return testing::Invoke(&fake_decoder_, &test::FakeDecoder::Decode);
+    return Invoke(&fake_decoder_, &test::FakeDecoder::Decode);
   }
 
   bool UseMetronome() const { return GetParam(); }
@@ -207,22 +208,23 @@ class VideoReceiveStream2Test : public ::testing::TestWithParam<bool> {
 
     // By default, mock decoder factory is backed by VideoDecoderProxyFactory.
     ON_CALL(mock_h264_decoder_factory_, CreateVideoDecoder)
-        .WillByDefault(testing::Invoke(
+        .WillByDefault(
-            &h264_decoder_factory_,
+            Invoke(&h264_decoder_factory_,
                    &test::VideoDecoderProxyFactory::CreateVideoDecoder));
 
     // By default, mock decode will wrap the fake decoder.
     ON_CALL(mock_decoder_, Configure)
-        .WillByDefault(
+        .WillByDefault(Invoke(&fake_decoder_, &test::FakeDecoder::Configure));
-            testing::Invoke(&fake_decoder_, &test::FakeDecoder::Configure));
     ON_CALL(mock_decoder_, Decode).WillByDefault(DefaultDecodeAction());
     ON_CALL(mock_decoder_, RegisterDecodeCompleteCallback)
-        .WillByDefault(testing::Invoke(
+        .WillByDefault(
-            &fake_decoder_,
+            Invoke(&fake_decoder_,
                    &test::FakeDecoder::RegisterDecodeCompleteCallback));
     ON_CALL(mock_decoder_, Release)
+        .WillByDefault(Invoke(&fake_decoder_, &test::FakeDecoder::Release));
+    ON_CALL(mock_transport_, SendRtcp)
         .WillByDefault(
-            testing::Invoke(&fake_decoder_, &test::FakeDecoder::Release));
+            Invoke(&rtcp_packet_parser_, &test::RtcpPacketParser::Parse));
   }
   ~VideoReceiveStream2Test() override {
     if (video_receive_stream_) {
@@ -284,6 +286,7 @@ class VideoReceiveStream2Test : public ::testing::TestWithParam<bool> {
   FakeVideoRenderer fake_renderer_;
   cricket::FakeCall fake_call_;
   MockTransport mock_transport_;
+  test::RtcpPacketParser rtcp_packet_parser_;
   PacketRouter packet_router_;
   RtpStreamReceiverController rtp_stream_receiver_controller_;
   std::unique_ptr<webrtc::internal::VideoReceiveStream2> video_receive_stream_;
@@ -664,8 +667,6 @@ std::unique_ptr<test::FakeEncodedFrame> MakeFrame(VideoFrameType frame_type,
 TEST_P(VideoReceiveStream2Test, PassesFrameWhenEncodedFramesCallbackSet) {
   testing::MockFunction<void(const RecordableEncodedFrame&)> callback;
   video_receive_stream_->Start();
-  // Expect a keyframe request to be generated
-  EXPECT_CALL(mock_transport_, SendRtcp);
   EXPECT_CALL(callback, Call);
   video_receive_stream_->SetAndGetRecordingState(
       VideoReceiveStreamInterface::RecordingState(callback.AsStdFunction()),
@@ -673,6 +674,9 @@ TEST_P(VideoReceiveStream2Test, PassesFrameWhenEncodedFramesCallbackSet) {
   video_receive_stream_->OnCompleteFrame(
       MakeFrame(VideoFrameType::kVideoFrameKey, 0));
   EXPECT_TRUE(fake_renderer_.WaitForFrame(kDefaultTimeOut));
+
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(1));
+
   video_receive_stream_->Stop();
 }
 
@@ -680,7 +684,6 @@ TEST_P(VideoReceiveStream2Test, MovesEncodedFrameDispatchStateWhenReCreating) {
   testing::MockFunction<void(const RecordableEncodedFrame&)> callback;
   video_receive_stream_->Start();
   // Expect a key frame request over RTCP.
-  EXPECT_CALL(mock_transport_, SendRtcp).Times(1);
   video_receive_stream_->SetAndGetRecordingState(
       VideoReceiveStreamInterface::RecordingState(callback.AsStdFunction()),
       true);
@@ -689,6 +692,9 @@ TEST_P(VideoReceiveStream2Test, MovesEncodedFrameDispatchStateWhenReCreating) {
       video_receive_stream_->SetAndGetRecordingState(
           VideoReceiveStreamInterface::RecordingState(), false);
   RecreateReceiveStream(std::move(old_state));
+
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(1));
+
   video_receive_stream_->Stop();
 }
 
@@ -700,7 +706,6 @@ TEST_P(VideoReceiveStream2Test, RequestsKeyFramesUntilKeyFrameReceived) {
   RecreateReceiveStream();
   video_receive_stream_->Start();
 
-  EXPECT_CALL(mock_transport_, SendRtcp).Times(1).WillOnce(testing::Return(0));
   video_receive_stream_->GenerateKeyFrame();
   video_receive_stream_->OnCompleteFrame(
       MakeFrame(VideoFrameType::kVideoFrameDelta, 0));
@@ -713,9 +718,10 @@ TEST_P(VideoReceiveStream2Test, RequestsKeyFramesUntilKeyFrameReceived) {
   time_controller_.AdvanceTime(TimeDelta::Zero());
   testing::Mock::VerifyAndClearExpectations(&mock_transport_);
 
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(1));
+
   // T+keyframetimeout: still no key frame received, expect key frame request
   // sent again.
-  EXPECT_CALL(mock_transport_, SendRtcp).Times(1).WillOnce(testing::Return(0));
   time_controller_.AdvanceTime(tick);
   video_receive_stream_->OnCompleteFrame(
       MakeFrame(VideoFrameType::kVideoFrameDelta, 2));
@@ -723,9 +729,10 @@ TEST_P(VideoReceiveStream2Test, RequestsKeyFramesUntilKeyFrameReceived) {
   loop_.Flush();
   testing::Mock::VerifyAndClearExpectations(&mock_transport_);
 
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(2));
+
   // T+keyframetimeout: now send a key frame - we should not observe new key
   // frame requests after this.
-  EXPECT_CALL(mock_transport_, SendRtcp).Times(0);
   video_receive_stream_->OnCompleteFrame(
       MakeFrame(VideoFrameType::kVideoFrameKey, 3));
   EXPECT_THAT(fake_renderer_.WaitForFrame(kDefaultTimeOut), RenderedFrame());
@@ -734,6 +741,8 @@ TEST_P(VideoReceiveStream2Test, RequestsKeyFramesUntilKeyFrameReceived) {
       MakeFrame(VideoFrameType::kVideoFrameDelta, 4));
   EXPECT_THAT(fake_renderer_.WaitForFrame(kDefaultTimeOut), RenderedFrame());
   loop_.Flush();
+
+  EXPECT_THAT(rtcp_packet_parser_.pli()->num_packets(), Eq(2));
 }
 
 TEST_P(VideoReceiveStream2Test,
@@ -943,7 +952,7 @@ TEST_P(VideoReceiveStream2Test, FramesFastForwardOnSystemHalt) {
   InSequence seq;
   EXPECT_CALL(mock_decoder_,
               Decode(test::RtpTimestamp(kFirstRtpTimestamp), _, _))
-      .WillOnce(testing::DoAll(testing::Invoke([&] {
+      .WillOnce(testing::DoAll(Invoke([&] {
                                  // System halt will be simulated in the decode.
                                  time_controller_.AdvanceTime(k30FpsDelay * 2);
                                }),