diff --git a/modules/BUILD.gn b/modules/BUILD.gn index b4c88b706f..197b543f27 100644 --- a/modules/BUILD.gn +++ b/modules/BUILD.gn @@ -54,6 +54,7 @@ rtc_source_set("module_api") { "../api:rtp_headers", "../api/video:video_frame_type", "../modules/rtp_rtcp:rtp_video_header", + "../rtc_base:deprecation", # TODO(bugs.webrtc.org/10336): Remove. "../rtc_base:safe_conversions", "../rtc_base/system:rtc_export", ] diff --git a/modules/include/module_common_types.h b/modules/include/module_common_types.h index 5226509d63..9295ce5db8 100644 --- a/modules/include/module_common_types.h +++ b/modules/include/module_common_types.h @@ -20,6 +20,7 @@ #include "modules/include/module_common_types_public.h" #include "modules/include/module_fec_types.h" #include "modules/rtp_rtcp/source/rtp_video_header.h" +#include "rtc_base/deprecation.h" // TODO(bugs.webrtc.org/10336): Remove. #include "rtc_base/system/rtc_export.h" namespace webrtc { @@ -66,7 +67,17 @@ class CallStatsObserver { // Interface used by NackModule and JitterBuffer. class NackSender { public: - virtual void SendNack(const std::vector& sequence_numbers) = 0; + // // TODO(bugs.webrtc.org/10336): Update downstream and remove this method. + // Make the one remaining version of SendNack() pure virtual again. + RTC_DEPRECATED virtual void SendNack( + const std::vector& sequence_numbers) = 0; + + // If |buffering_allowed|, other feedback messages (e.g. key frame requests) + // may be added to the same outgoing feedback message. In that case, it's up + // to the user of the interface to ensure that when all buffer-able messages + // have been added, the feedback message is triggered. + virtual void SendNack(const std::vector& sequence_numbers, + bool buffering_allowed) {} protected: virtual ~NackSender() {} diff --git a/modules/video_coding/nack_module.cc b/modules/video_coding/nack_module.cc index 0c69b57749..9ae74c7915 100644 --- a/modules/video_coding/nack_module.cc +++ b/modules/video_coding/nack_module.cc @@ -138,8 +138,11 @@ int NackModule::OnReceivedPacket(uint16_t seq_num, // Are there any nacks that are waiting for this seq_num. std::vector nack_batch = GetNackBatch(kSeqNumOnly); - if (!nack_batch.empty()) - nack_sender_->SendNack(nack_batch); + if (!nack_batch.empty()) { + // This batch of NACKs is triggered externally; the initiator can + // batch them with other feedback messages. + nack_sender_->SendNack(nack_batch, /*buffering_allowed=*/true); + } return 0; } @@ -178,8 +181,11 @@ void NackModule::Process() { nack_batch = GetNackBatch(kTimeOnly); } - if (!nack_batch.empty()) - nack_sender_->SendNack(nack_batch); + if (!nack_batch.empty()) { + // This batch of NACKs is triggered externally; there is no external + // initiator who can batch them with other feedback messages. + nack_sender_->SendNack(nack_batch, /*buffering_allowed=*/false); + } } // Update the next_process_time_ms_ in intervals to achieve diff --git a/modules/video_coding/nack_module_unittest.cc b/modules/video_coding/nack_module_unittest.cc index 0fd3d3252b..5d88702f53 100644 --- a/modules/video_coding/nack_module_unittest.cc +++ b/modules/video_coding/nack_module_unittest.cc @@ -28,6 +28,11 @@ class TestNackModule : public ::testing::Test, keyframes_requested_(0) {} void SendNack(const std::vector& sequence_numbers) override { + RTC_NOTREACHED(); + } + + void SendNack(const std::vector& sequence_numbers, + bool buffering_allowed) override { sent_nacks_.insert(sent_nacks_.end(), sequence_numbers.begin(), sequence_numbers.end()); } @@ -303,6 +308,11 @@ class TestNackModuleWithFieldTrial : public ::testing::Test, keyframes_requested_(0) {} void SendNack(const std::vector& sequence_numbers) override { + RTC_NOTREACHED(); + } + + void SendNack(const std::vector& sequence_numbers, + bool buffering_allowed) override { sent_nacks_.insert(sent_nacks_.end(), sequence_numbers.begin(), sequence_numbers.end()); } diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 4e51385139..3a741e0eeb 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -75,6 +75,83 @@ std::unique_ptr CreateRtpRtcpModule( static const int kPacketLogIntervalMs = 10000; +RtpVideoStreamReceiver::RtcpFeedbackBuffer::RtcpFeedbackBuffer( + KeyFrameRequestSender* key_frame_request_sender, + NackSender* nack_sender, + LossNotificationSender* loss_notification_sender) + : key_frame_request_sender_(key_frame_request_sender), + nack_sender_(nack_sender), + loss_notification_sender_(loss_notification_sender), + request_key_frame_(false) { + RTC_DCHECK(key_frame_request_sender_); + RTC_DCHECK(nack_sender_); + RTC_DCHECK(loss_notification_sender_); +} + +void RtpVideoStreamReceiver::RtcpFeedbackBuffer::RequestKeyFrame() { + rtc::CritScope lock(&cs_); + request_key_frame_ = true; +} + +void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendNack( + const std::vector& sequence_numbers) { + RTC_NOTREACHED(); +} + +void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendNack( + const std::vector& sequence_numbers, + bool buffering_allowed) { + RTC_DCHECK(!sequence_numbers.empty()); + rtc::CritScope lock(&cs_); + nack_sequence_numbers_.insert(nack_sequence_numbers_.end(), + sequence_numbers.cbegin(), + sequence_numbers.cend()); + if (!buffering_allowed) { + // Note that while *buffering* is not allowed, *batching* is, meaning that + // previously buffered messages may be sent along with the current message. + SendBufferedRtcpFeedback(); + } +} + +void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendLossNotification( + uint16_t last_decoded_seq_num, + uint16_t last_received_seq_num, + bool decodability_flag) { + rtc::CritScope lock(&cs_); + RTC_DCHECK(lntf_state_) + << "SendLossNotification() called twice in a row with no call to " + "SendBufferedRtcpFeedback() in between."; + lntf_state_ = absl::make_optional( + last_decoded_seq_num, last_received_seq_num, decodability_flag); +} + +// TODO(bugs.webrtc.org/10336): Make SendBufferedRtcpFeedback() actually +// set everything, then send it all together. +void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendBufferedRtcpFeedback() { + bool request_key_frame = false; + std::vector nack_sequence_numbers; + absl::optional lntf_state; + + { + rtc::CritScope lock(&cs_); + std::swap(request_key_frame, request_key_frame_); + std::swap(nack_sequence_numbers, nack_sequence_numbers_); + std::swap(lntf_state, lntf_state_); + } + + if (request_key_frame) { + key_frame_request_sender_->RequestKeyFrame(); + } else if (!nack_sequence_numbers.empty()) { + nack_sender_->SendNack(nack_sequence_numbers, true); + } + + if (lntf_state) { + loss_notification_sender_->SendLossNotification( + lntf_state->last_decoded_seq_num, lntf_state->last_received_seq_num, + lntf_state->decodability_flag); + } +} + RtpVideoStreamReceiver::RtpVideoStreamReceiver( Clock* clock, Transport* transport, @@ -105,6 +182,9 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( receive_stats_proxy)), complete_frame_callback_(complete_frame_callback), keyframe_request_sender_(keyframe_request_sender), + // TODO(bugs.webrtc.org/10336): Let |rtcp_feedback_buffer_| communicate + // directly with |rtp_rtcp_|. + rtcp_feedback_buffer_(this, nack_sender, this), has_received_frame_(false), frames_decryptable_(false) { constexpr bool remb_candidate = true; @@ -148,11 +228,13 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( if (config_.rtp.lntf.enabled) { loss_notification_controller_ = - absl::make_unique(this, this); + absl::make_unique(&rtcp_feedback_buffer_, + &rtcp_feedback_buffer_); } if (config_.rtp.nack.rtp_history_ms != 0) { - nack_module_ = absl::make_unique(clock_, nack_sender, this); + nack_module_ = absl::make_unique(clock_, &rtcp_feedback_buffer_, + &rtcp_feedback_buffer_); process_thread_->RegisterModule(nack_module_.get(), RTC_FROM_HERE); } @@ -269,6 +351,7 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( if (packet.sizeBytes == 0) { NotifyReceiverOfEmptyPacket(packet.seqNum); + rtcp_feedback_buffer_.SendBufferedRtcpFeedback(); return 0; } @@ -283,7 +366,8 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( switch (tracker_.CopyAndFixBitstream(&packet)) { case video_coding::H264SpsPpsTracker::kRequestKeyframe: - RequestKeyFrame(); + rtcp_feedback_buffer_.RequestKeyFrame(); + rtcp_feedback_buffer_.SendBufferedRtcpFeedback(); RTC_FALLTHROUGH(); case video_coding::H264SpsPpsTracker::kDrop: return 0; @@ -297,6 +381,7 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( packet.dataPtr = data; } + rtcp_feedback_buffer_.SendBufferedRtcpFeedback(); packet_buffer_->InsertPacket(&packet); return 0; } @@ -380,6 +465,9 @@ void RtpVideoStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) { } void RtpVideoStreamReceiver::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 { @@ -428,14 +516,18 @@ void RtpVideoStreamReceiver::OnAssembledFrame( descriptor->FrameDependenciesDiffs()); } - // Request a key frame as soon as possible. + // If frames arrive before a key frame, they would not be decodable. + // In that case, request a key frame ASAP. if (!has_received_frame_) { - has_received_frame_ = true; if (frame->FrameType() != VideoFrameType::kVideoFrameKey) { - // TODO(bugs.webrtc.org/10336): Allow the sender to ignore these messages - // if it is relying on LNTF alone. - RequestKeyFrame(); + // |loss_notification_controller_|, if present, would have already + // requested a key frame when the first packet for the non-key frame + // had arrived, so no need to replicate the request. + if (!loss_notification_controller_) { + RequestKeyFrame(); + } } + has_received_frame_ = true; } if (buffered_frame_decryptor_ == nullptr) { diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index 6a63ad50fc..db73bab034 100644 --- a/video/rtp_video_stream_receiver.h +++ b/video/rtp_video_stream_receiver.h @@ -177,6 +177,68 @@ class RtpVideoStreamReceiver : public LossNotificationSender, std::vector GetSources() const; private: + // Used for buffering RTCP feedback messages and sending them all together. + // Note: + // 1. Key frame requests and NACKs are mutually exclusive, with the + // former taking precedence over the latter. + // 2. Loss notifications are orthogonal to either. (That is, may be sent + // alongside either.) + class RtcpFeedbackBuffer : public KeyFrameRequestSender, + public NackSender, + public LossNotificationSender { + public: + RtcpFeedbackBuffer(KeyFrameRequestSender* key_frame_request_sender, + NackSender* nack_sender, + LossNotificationSender* loss_notification_sender); + + ~RtcpFeedbackBuffer() override = default; + + // KeyFrameRequestSender implementation. + void RequestKeyFrame() override; + + // NackSender implementation. + void SendNack(const std::vector& sequence_numbers) override; + void SendNack(const std::vector& sequence_numbers, + bool buffering_allowed) override; + + // LossNotificationSender implementation. + void SendLossNotification(uint16_t last_decoded_seq_num, + uint16_t last_received_seq_num, + bool decodability_flag) override; + + // Send all RTCP feedback messages buffered thus far. + void SendBufferedRtcpFeedback(); + + private: + KeyFrameRequestSender* const key_frame_request_sender_; + NackSender* const nack_sender_; + LossNotificationSender* const loss_notification_sender_; + + // NACKs are accessible from two threads due to nack_module_ being a module. + rtc::CriticalSection cs_; + + // Key-frame-request-related state. + bool request_key_frame_ RTC_GUARDED_BY(cs_); + + // NACK-related state. + std::vector nack_sequence_numbers_ RTC_GUARDED_BY(cs_); + + // LNTF-related state. + struct LossNotificationState { + LossNotificationState(uint16_t last_decoded_seq_num, + uint16_t last_received_seq_num, + bool decodability_flag) + : last_decoded_seq_num(last_decoded_seq_num), + last_received_seq_num(last_received_seq_num), + decodability_flag(decodability_flag) {} + + uint16_t last_decoded_seq_num; + uint16_t last_received_seq_num; + bool decodability_flag; + }; + absl::optional lntf_state_ RTC_GUARDED_BY(cs_); + }; + // Entry point doing non-stats work for a received packet. Called // for the same packet both before and after RED decapsulation. void ReceivePacket(const RtpPacketReceived& packet); @@ -206,11 +268,13 @@ class RtpVideoStreamReceiver : public LossNotificationSender, const std::unique_ptr rtp_rtcp_; - // Members for the new jitter buffer experiment. video_coding::OnCompleteFrameCallback* complete_frame_callback_; KeyFrameRequestSender* const keyframe_request_sender_; + + RtcpFeedbackBuffer rtcp_feedback_buffer_; std::unique_ptr nack_module_; std::unique_ptr loss_notification_controller_; + rtc::scoped_refptr packet_buffer_; std::unique_ptr reference_finder_; rtc::CriticalSection last_seq_num_cs_; diff --git a/video/rtp_video_stream_receiver_unittest.cc b/video/rtp_video_stream_receiver_unittest.cc index d9c10713b9..24a49f00de 100644 --- a/video/rtp_video_stream_receiver_unittest.cc +++ b/video/rtp_video_stream_receiver_unittest.cc @@ -56,6 +56,9 @@ class MockTransport : public Transport { class MockNackSender : public NackSender { public: MOCK_METHOD1(SendNack, void(const std::vector& sequence_numbers)); + MOCK_METHOD2(SendNack, + void(const std::vector& sequence_numbers, + bool buffering_allowed)); }; class MockKeyFrameRequestSender : public KeyFrameRequestSender { diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index 99b83c9855..c8f14d84a4 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -514,6 +514,12 @@ void VideoReceiveStream::SetFrameDecryptor( void VideoReceiveStream::SendNack( const std::vector& sequence_numbers) { + SendNack(sequence_numbers, true); +} + +void VideoReceiveStream::SendNack(const std::vector& sequence_numbers, + bool buffering_allowed) { + RTC_DCHECK(buffering_allowed); rtp_video_stream_receiver_.RequestPacketRetransmit(sequence_numbers); } diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h index 89dc36f4b9..9d182187e3 100644 --- a/video/video_receive_stream.h +++ b/video/video_receive_stream.h @@ -101,6 +101,10 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, // Implements NackSender. void SendNack(const std::vector& sequence_numbers) override; + // For this particular override of the interface, + // only (buffering_allowed == true) is acceptable. + void SendNack(const std::vector& sequence_numbers, + bool buffering_allowed) override; // Implements video_coding::OnCompleteFrameCallback. void OnCompleteFrame(