From 2f5554dae54d25918c2092f266eb8edf70801037 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Wed, 29 May 2019 13:35:14 +0200 Subject: [PATCH] Make KeyFrameRequestSender injectable in RtpVideoStreamReceiver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a partial revert of https://webrtc-review.googlesource.com/c/src/+/130101. The KeyFrameRequestSender argument is added back to the constructor of RtpVideoStreamReceiver. It is optional; if a null pointer is passed, key frame requests are sent via the internal RtpRtcp module, and this is how the class is used by VideoReceiveStream. An injectable KeyFrameRequestSender is useful for tests, for downstream applications that want to route key frame requests elsewhere, and should also aid later migration to RtcpTransciever. Bug: None Change-Id: Idf9baeed21570625ad74e9afbe38f7ea5bf79feb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139107 Commit-Queue: Niels Moller Reviewed-by: Åsa Persson Cr-Commit-Position: refs/heads/master@{#28102} --- video/rtp_video_stream_receiver.cc | 40 +++++++-------------- video/rtp_video_stream_receiver.h | 21 +++-------- video/rtp_video_stream_receiver_unittest.cc | 16 +++------ video/video_receive_stream.cc | 5 +-- 4 files changed, 23 insertions(+), 59 deletions(-) diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index adca218f38..4e51385139 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -85,32 +85,7 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( ReceiveStatisticsProxy* receive_stats_proxy, ProcessThread* process_thread, NackSender* nack_sender, - video_coding::OnCompleteFrameCallback* complete_frame_callback, - rtc::scoped_refptr 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 rtp_rtcp, - PacketRouter* packet_router, - const VideoReceiveStream::Config* config, - ReceiveStatistics* rtp_receive_statistics, - ReceiveStatisticsProxy* receive_stats_proxy, - ProcessThread* process_thread, - NackSender* nack_sender, + KeyFrameRequestSender* keyframe_request_sender, video_coding::OnCompleteFrameCallback* complete_frame_callback, rtc::scoped_refptr frame_decryptor) : clock_(clock), @@ -123,8 +98,13 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, this)), receiving_(false), last_packet_log_ms_(-1), - rtp_rtcp_(std::move(rtp_rtcp)), + rtp_rtcp_(CreateRtpRtcpModule(clock, + 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; @@ -400,7 +380,11 @@ void RtpVideoStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) { } void RtpVideoStreamReceiver::RequestKeyFrame() { - rtp_rtcp_->RequestKeyFrame(); + if (keyframe_request_sender_) { + keyframe_request_sender_->RequestKeyFrame(); + } else { + rtp_rtcp_->RequestKeyFrame(); + } } void RtpVideoStreamReceiver::SendLossNotification( diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index e57a32d01d..6a63ad50fc 100644 --- a/video/rtp_video_stream_receiver.h +++ b/video/rtp_video_stream_receiver.h @@ -77,25 +77,11 @@ class RtpVideoStreamReceiver : public LossNotificationSender, ReceiveStatisticsProxy* receive_stats_proxy, ProcessThread* process_thread, NackSender* nack_sender, + // The KeyFrameRequestSender is optional; if not provided, key frame + // requests are sent via the internal RtpRtcp module. + KeyFrameRequestSender* keyframe_request_sender, video_coding::OnCompleteFrameCallback* complete_frame_callback, rtc::scoped_refptr frame_decryptor); - - // Constructor with injected RtpRtcp. Intended for tests only! - RtpVideoStreamReceiver( - Clock* clock, - std::unique_ptr 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 frame_decryptor); - ~RtpVideoStreamReceiver() override; void AddReceiveCodec(const VideoCodec& video_codec, @@ -222,6 +208,7 @@ 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 nack_module_; std::unique_ptr loss_notification_controller_; rtc::scoped_refptr packet_buffer_; diff --git a/video/rtp_video_stream_receiver_unittest.cc b/video/rtp_video_stream_receiver_unittest.cc index 981421c1eb..d9c10713b9 100644 --- a/video/rtp_video_stream_receiver_unittest.cc +++ b/video/rtp_video_stream_receiver_unittest.cc @@ -16,7 +16,6 @@ #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" @@ -140,7 +139,8 @@ class RtpVideoStreamReceiverTest : public ::testing::Test { rtp_video_stream_receiver_ = absl::make_unique( Clock::GetRealTimeClock(), &mock_transport_, nullptr, nullptr, &config_, rtp_receive_statistics_.get(), nullptr, process_thread_.get(), - &mock_nack_sender_, &mock_on_complete_frame_callback_, nullptr); + &mock_nack_sender_, &mock_key_frame_request_sender_, + &mock_on_complete_frame_callback_, nullptr); } RTPVideoHeader GetDefaultH264VideoHeader() { @@ -199,6 +199,7 @@ 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 process_thread_; @@ -542,15 +543,6 @@ 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; - - rtp_video_stream_receiver_ = absl::make_unique( - 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 data({1, 2, 3, 4}); @@ -559,7 +551,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_rtp_rtcp, RequestKeyFrame()); + EXPECT_CALL(mock_key_frame_request_sender_, RequestKeyFrame()); rtp_video_stream_receiver_->OnReceivedPayloadData( data.data(), data.size(), rtp_header, video_header, absl::nullopt, false); } diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index c495f4e5b9..99b83c9855 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -204,8 +204,9 @@ VideoReceiveStream::VideoReceiveStream( rtp_receive_statistics_.get(), &stats_proxy_, process_thread_, - this, // NackSender - this, // OnCompleteFrameCallback + this, // NackSender + nullptr, // Use default KeyFrameRequestSender + this, // OnCompleteFrameCallback config_.frame_decryptor), rtp_stream_sync_(this), max_wait_for_keyframe_ms_(KeyframeIntervalSettings::ParseFromFieldTrials()