diff --git a/video/rtp_video_stream_receiver2.cc b/video/rtp_video_stream_receiver2.cc index 207da6296e..1ed460764d 100644 --- a/video/rtp_video_stream_receiver2.cc +++ b/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 frame_decryptor, rtc::scoped_refptr 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_) { - keyframe_request_sender_->RequestKeyFrame(); - } else if (keyframe_request_method_ == KeyFrameReqMethod::kPliRtcp) { + if (keyframe_request_method_ == KeyFrameReqMethod::kPliRtcp) { rtp_rtcp_->SendPictureLossIndication(); } else if (keyframe_request_method_ == KeyFrameReqMethod::kFirRtcp) { rtp_rtcp_->SendFullIntraRequest(); diff --git a/video/rtp_video_stream_receiver2.h b/video/rtp_video_stream_receiver2.h index 8ed6ea0100..d0db53c635 100644 --- a/video/rtp_video_stream_receiver2.h +++ b/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 frame_decryptor, rtc::scoped_refptr frame_transformer, @@ -324,7 +323,6 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender, const std::unique_ptr rtp_rtcp_; OnCompleteFrameCallback* complete_frame_callback_; - KeyFrameRequestSender* const keyframe_request_sender_; const KeyFrameReqMethod keyframe_request_method_; RtcpFeedbackBuffer rtcp_feedback_buffer_; diff --git a/video/rtp_video_stream_receiver2_unittest.cc b/video/rtp_video_stream_receiver2_unittest.cc index 0507da9627..21803baaf9 100644 --- a/video/rtp_video_stream_receiver2_unittest.cc +++ b/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_, - nullptr, nullptr, field_trials_); + &mock_on_complete_frame_callback_, 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 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( 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, {}, diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index f333c446a7..bee153484b 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -235,9 +235,8 @@ VideoReceiveStream2::VideoReceiveStream2( &stats_proxy_, &stats_proxy_, nack_periodic_processor, - this, // NackSender - nullptr, // Use default KeyFrameRequestSender - this, // OnCompleteFrameCallback + this, // NackSender + this, // OnCompleteFrameCallback std::move(config_.frame_decryptor), std::move(config_.frame_transformer), call->trials()), diff --git a/video/video_receive_stream2_unittest.cc b/video/video_receive_stream2_unittest.cc index 5b2543092f..d8dcb18b02 100644 --- a/video/video_receive_stream2_unittest.cc +++ b/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 { 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 { // By default, mock decoder factory is backed by VideoDecoderProxyFactory. ON_CALL(mock_h264_decoder_factory_, CreateVideoDecoder) - .WillByDefault(testing::Invoke( - &h264_decoder_factory_, - &test::VideoDecoderProxyFactory::CreateVideoDecoder)); + .WillByDefault( + Invoke(&h264_decoder_factory_, + &test::VideoDecoderProxyFactory::CreateVideoDecoder)); // By default, mock decode will wrap the fake decoder. ON_CALL(mock_decoder_, Configure) - .WillByDefault( - testing::Invoke(&fake_decoder_, &test::FakeDecoder::Configure)); + .WillByDefault(Invoke(&fake_decoder_, &test::FakeDecoder::Configure)); ON_CALL(mock_decoder_, Decode).WillByDefault(DefaultDecodeAction()); ON_CALL(mock_decoder_, RegisterDecodeCompleteCallback) - .WillByDefault(testing::Invoke( - &fake_decoder_, - &test::FakeDecoder::RegisterDecodeCompleteCallback)); - ON_CALL(mock_decoder_, Release) .WillByDefault( - testing::Invoke(&fake_decoder_, &test::FakeDecoder::Release)); + Invoke(&fake_decoder_, + &test::FakeDecoder::RegisterDecodeCompleteCallback)); + ON_CALL(mock_decoder_, Release) + .WillByDefault(Invoke(&fake_decoder_, &test::FakeDecoder::Release)); + ON_CALL(mock_transport_, SendRtcp) + .WillByDefault( + Invoke(&rtcp_packet_parser_, &test::RtcpPacketParser::Parse)); } ~VideoReceiveStream2Test() override { if (video_receive_stream_) { @@ -284,6 +286,7 @@ class VideoReceiveStream2Test : public ::testing::TestWithParam { 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 video_receive_stream_; @@ -664,8 +667,6 @@ std::unique_ptr MakeFrame(VideoFrameType frame_type, TEST_P(VideoReceiveStream2Test, PassesFrameWhenEncodedFramesCallbackSet) { testing::MockFunction 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 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); }),