Buffer RTCP feedback messages in RtpVideoStreamReceiver

Currently, if LNTF and NACK messages are both created, they will
be sent out in separate RTCP messages. This is wasteful.
This CL is the first of in a series of CLs that will ensure that
these feedback messages can be buffered together, without introducing
more of a delay than the CPU time required to process both messages.

Bug: webrtc:10336
Change-Id: I950324112ee346695a12a17d025483ea5e99c732
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139112
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Elad Alon <eladalon@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28136}

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",
   ]

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<uint16_t>& 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<uint16_t>& 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<uint16_t>& sequence_numbers,
+                        bool buffering_allowed) {}
 
  protected:
   virtual ~NackSender() {}

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<uint16_t> 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

modules/video_coding/nack_module_unittest.cc

@@ -28,6 +28,11 @@ class TestNackModule : public ::testing::Test,
         keyframes_requested_(0) {}
 
   void SendNack(const std::vector<uint16_t>& sequence_numbers) override {
+    RTC_NOTREACHED();
+  }
+
+  void SendNack(const std::vector<uint16_t>& 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<uint16_t>& sequence_numbers) override {
+    RTC_NOTREACHED();
+  }
+
+  void SendNack(const std::vector<uint16_t>& sequence_numbers,
+                bool buffering_allowed) override {
     sent_nacks_.insert(sent_nacks_.end(), sequence_numbers.begin(),
                        sequence_numbers.end());
   }

video/rtp_video_stream_receiver.cc

@@ -75,6 +75,83 @@ std::unique_ptr<RtpRtcp> 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<uint16_t>& sequence_numbers) {
+  RTC_NOTREACHED();
+}
+
+void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendNack(
+    const std::vector<uint16_t>& 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<LossNotificationState>(
+      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<uint16_t> nack_sequence_numbers;
+  absl::optional<LossNotificationState> 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<LossNotificationController>(this, this);
+        absl::make_unique<LossNotificationController>(&rtcp_feedback_buffer_,
+                                                      &rtcp_feedback_buffer_);
   }
 
   if (config_.rtp.nack.rtp_history_ms != 0) {
-    nack_module_ = absl::make_unique<NackModule>(clock_, nack_sender, this);
+    nack_module_ = absl::make_unique<NackModule>(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,15 +516,19 @@ 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.
+      // |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) {
     reference_finder_->ManageFrame(std::move(frame));

video/rtp_video_stream_receiver.h

@@ -177,6 +177,68 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
   std::vector<webrtc::RtpSource> 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<uint16_t>& sequence_numbers) override;
+    void SendNack(const std::vector<uint16_t>& 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<uint16_t> 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<LossNotificationState> 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<RtpRtcp> 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<NackModule> nack_module_;
   std::unique_ptr<LossNotificationController> loss_notification_controller_;
+
   rtc::scoped_refptr<video_coding::PacketBuffer> packet_buffer_;
   std::unique_ptr<video_coding::RtpFrameReferenceFinder> reference_finder_;
   rtc::CriticalSection last_seq_num_cs_;

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<uint16_t>& sequence_numbers));
+  MOCK_METHOD2(SendNack,
+               void(const std::vector<uint16_t>& sequence_numbers,
+                    bool buffering_allowed));
 };
 
 class MockKeyFrameRequestSender : public KeyFrameRequestSender {

video/video_receive_stream.cc

@@ -514,6 +514,12 @@ void VideoReceiveStream::SetFrameDecryptor(
 
 void VideoReceiveStream::SendNack(
     const std::vector<uint16_t>& sequence_numbers) {
+  SendNack(sequence_numbers, true);
+}
+
+void VideoReceiveStream::SendNack(const std::vector<uint16_t>& sequence_numbers,
+                                  bool buffering_allowed) {
+  RTC_DCHECK(buffering_allowed);
   rtp_video_stream_receiver_.RequestPacketRetransmit(sequence_numbers);
 }
 

video/video_receive_stream.h

@@ -101,6 +101,10 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream,
 
   // Implements NackSender.
   void SendNack(const std::vector<uint16_t>& sequence_numbers) override;
+  // For this particular override of the interface,
+  // only (buffering_allowed == true) is acceptable.
+  void SendNack(const std::vector<uint16_t>& sequence_numbers,
+                bool buffering_allowed) override;
 
   // Implements video_coding::OnCompleteFrameCallback.
   void OnCompleteFrame(