Allowing buffering a LNTF (loss notification) feedback message in RTCPSender

Loss notifications may either be sent immediately, or wait until another
RTCP feedback message is sent.

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

DEPS

@@ -1633,6 +1633,7 @@ include_rules = [
   "+absl/algorithm/container.h",
   "+absl/base/attributes.h",
   "+absl/base/config.h",
+  "+absl/base/macros.h",
   "+absl/container/inlined_vector.h",
   "+absl/memory/memory.h",
   "+absl/meta/type_traits.h",

abseil-in-webrtc.md

@@ -22,8 +22,9 @@ adds the first use.
   `absl::is_trivially_destructible` from `absl/meta/type_traits.h`.
 * `absl::variant` and related stuff from `absl/types/variant.h`.
 * The functions in `absl/algorithm/algorithm.h` and
-  `absl/algorithm/container.h`
+  `absl/algorithm/container.h`.
-* The macros in `absl/base/attributes.h` and `absl/base/config.h`
+* The macros in `absl/base/attributes.h`, `absl/base/config.h` and
+  `absl/base/macros.h`.
 
 ## **Disallowed**
 

modules/include/module_common_types.h

@@ -100,7 +100,8 @@ class LossNotificationSender {
 
   virtual void SendLossNotification(uint16_t last_decoded_seq_num,
                                     uint16_t last_received_seq_num,
-                                    bool decodability_flag) = 0;
+                                    bool decodability_flag,
+                                    bool buffering_allowed) = 0;
 };
 
 }  // namespace webrtc

modules/rtp_rtcp/BUILD.gn

@@ -472,6 +472,7 @@ if (rtc_include_tests) {
       "../../test:test_support",
       "../video_coding:codec_globals_headers",
       "//third_party/abseil-cpp/absl/algorithm:container",
+      "//third_party/abseil-cpp/absl/base:core_headers",
       "//third_party/abseil-cpp/absl/memory",
       "//third_party/abseil-cpp/absl/types:optional",
     ]

modules/rtp_rtcp/include/rtp_rtcp.h

@@ -435,7 +435,8 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface {
   // Returns -1 on failure else 0.
   virtual int32_t SendLossNotification(uint16_t last_decoded_seq_num,
                                        uint16_t last_received_seq_num,
-                                       bool decodability_flag) = 0;
+                                       bool decodability_flag,
+                                       bool buffering_allowed) = 0;
 };
 
 }  // namespace webrtc

modules/rtp_rtcp/mocks/mock_rtp_rtcp.h

@@ -151,10 +151,11 @@ class MockRtpRtcp : public RtpRtcp {
   MOCK_METHOD1(SetTargetSendBitrate, void(uint32_t bitrate_bps));
   MOCK_METHOD1(SetKeyFrameRequestMethod, int32_t(KeyFrameRequestMethod method));
   MOCK_METHOD0(RequestKeyFrame, int32_t());
-  MOCK_METHOD3(SendLossNotification,
+  MOCK_METHOD4(SendLossNotification,
                int32_t(uint16_t last_decoded_seq_num,
                        uint16_t last_received_seq_num,
-                       bool decodability_flag));
+                       bool decodability_flag,
+                       bool buffering_allowed));
   MOCK_METHOD0(Process, void());
   MOCK_METHOD1(RegisterSendChannelRtpStatisticsCallback,
                void(StreamDataCountersCallback*));

modules/rtp_rtcp/source/rtcp_sender.cc

@@ -215,7 +215,8 @@ int32_t RTCPSender::SetSendingStatus(const FeedbackState& feedback_state,
 int32_t RTCPSender::SendLossNotification(const FeedbackState& feedback_state,
                                          uint16_t last_decoded_seq_num,
                                          uint16_t last_received_seq_num,
-                                         bool decodability_flag) {
+                                         bool decodability_flag,
+                                         bool buffering_allowed) {
   rtc::CritScope lock(&critical_section_rtcp_sender_);
 
   loss_notification_state_.last_decoded_seq_num = last_decoded_seq_num;
@@ -224,7 +225,11 @@ int32_t RTCPSender::SendLossNotification(const FeedbackState& feedback_state,
 
   SetFlag(kRtcpLossNotification, /*is_volatile=*/true);
 
-  // Send immediately.
+  if (buffering_allowed) {
+    // The loss notification will be batched with additional feedback messages.
+    return 0;
+  }
+
   return SendCompoundRTCP(feedback_state,
                           {RTCPPacketType::kRtcpLossNotification});
 }

modules/rtp_rtcp/source/rtcp_sender.h

@@ -117,7 +117,8 @@ class RTCPSender {
   int32_t SendLossNotification(const FeedbackState& feedback_state,
                                uint16_t last_decoded_seq_num,
                                uint16_t last_received_seq_num,
-                               bool decodability_flag);
+                               bool decodability_flag,
+                               bool buffering_allowed);
 
   void SetRemb(int64_t bitrate_bps, std::vector<uint32_t> ssrcs);
 

modules/rtp_rtcp/source/rtcp_sender_unittest.cc

@@ -10,6 +10,7 @@
 
 #include <memory>
 
+#include "absl/base/macros.h"
 #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/bye.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/common_header.h"
@@ -366,28 +367,61 @@ TEST_F(RtcpSenderTest, SendPli) {
 TEST_F(RtcpSenderTest, SendNack) {
   rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize);
   const uint16_t kList[] = {0, 1, 16};
-  const int32_t kListLength = sizeof(kList) / sizeof(kList[0]);
+  EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpNack,
-  EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpNack, kListLength,
+                                      ABSL_ARRAYSIZE(kList), kList));
-                                      kList));
   EXPECT_EQ(1, parser()->nack()->num_packets());
   EXPECT_EQ(kSenderSsrc, parser()->nack()->sender_ssrc());
   EXPECT_EQ(kRemoteSsrc, parser()->nack()->media_ssrc());
   EXPECT_THAT(parser()->nack()->packet_ids(), ElementsAre(0, 1, 16));
 }
 
-TEST_F(RtcpSenderTest, SendLossNotification) {
+TEST_F(RtcpSenderTest, SendLossNotificationBufferingNotAllowed) {
   rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize);
   constexpr uint16_t kLastDecoded = 0x1234;
   constexpr uint16_t kLastReceived = 0x4321;
   constexpr bool kDecodabilityFlag = true;
-  const int32_t result = rtcp_sender_->SendLossNotification(
+  constexpr bool kBufferingAllowed = false;
-      feedback_state(), kLastDecoded, kLastReceived, kDecodabilityFlag);
+  EXPECT_EQ(rtcp_sender_->SendLossNotification(feedback_state(), kLastDecoded,
-  EXPECT_EQ(result, 0);
+                                               kLastReceived, kDecodabilityFlag,
-  EXPECT_EQ(1, parser()->loss_notification()->num_packets());
+                                               kBufferingAllowed),
+            0);
+  EXPECT_EQ(parser()->processed_rtcp_packets(), 1u);
+  EXPECT_EQ(parser()->loss_notification()->num_packets(), 1);
   EXPECT_EQ(kSenderSsrc, parser()->loss_notification()->sender_ssrc());
   EXPECT_EQ(kRemoteSsrc, parser()->loss_notification()->media_ssrc());
 }
 
+TEST_F(RtcpSenderTest, SendLossNotificationBufferingAllowed) {
+  rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize);
+  constexpr uint16_t kLastDecoded = 0x1234;
+  constexpr uint16_t kLastReceived = 0x4321;
+  constexpr bool kDecodabilityFlag = true;
+  constexpr bool kBufferingAllowed = true;
+  EXPECT_EQ(rtcp_sender_->SendLossNotification(feedback_state(), kLastDecoded,
+                                               kLastReceived, kDecodabilityFlag,
+                                               kBufferingAllowed),
+            0);
+
+  // No RTCP messages sent yet.
+  ASSERT_EQ(parser()->processed_rtcp_packets(), 0u);
+
+  // Sending another messages triggers sending the LNTF messages as well.
+  const uint16_t kList[] = {0, 1, 16};
+  EXPECT_EQ(rtcp_sender_->SendRTCP(feedback_state(), kRtcpNack,
+                                   ABSL_ARRAYSIZE(kList), kList),
+            0);
+
+  // Exactly one packet was produced, and it contained both the buffered LNTF
+  // as well as the message that had triggered the packet.
+  EXPECT_EQ(parser()->processed_rtcp_packets(), 1u);
+  EXPECT_EQ(parser()->loss_notification()->num_packets(), 1);
+  EXPECT_EQ(parser()->loss_notification()->sender_ssrc(), kSenderSsrc);
+  EXPECT_EQ(parser()->loss_notification()->media_ssrc(), kRemoteSsrc);
+  EXPECT_EQ(parser()->nack()->num_packets(), 1);
+  EXPECT_EQ(parser()->nack()->sender_ssrc(), kSenderSsrc);
+  EXPECT_EQ(parser()->nack()->media_ssrc(), kRemoteSsrc);
+}
+
 TEST_F(RtcpSenderTest, RembNotIncludedBeforeSet) {
   rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize);
 

modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -754,10 +754,11 @@ int32_t ModuleRtpRtcpImpl::RequestKeyFrame() {
 
 int32_t ModuleRtpRtcpImpl::SendLossNotification(uint16_t last_decoded_seq_num,
                                                 uint16_t last_received_seq_num,
-                                                bool decodability_flag) {
+                                                bool decodability_flag,
+                                                bool buffering_allowed) {
   return rtcp_sender_.SendLossNotification(
       GetFeedbackState(), last_decoded_seq_num, last_received_seq_num,
-      decodability_flag);
+      decodability_flag, buffering_allowed);
 }
 
 void ModuleRtpRtcpImpl::SetRemoteSSRC(const uint32_t ssrc) {

modules/rtp_rtcp/source/rtp_rtcp_impl.h

@@ -263,7 +263,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp {
 
   int32_t SendLossNotification(uint16_t last_decoded_seq_num,
                                uint16_t last_received_seq_num,
-                               bool decodability_flag) override;
+                               bool decodability_flag,
+                               bool buffering_allowed) override;
 
   bool LastReceivedNTP(uint32_t* NTPsecs,
                        uint32_t* NTPfrac,

modules/video_coding/loss_notification_controller.cc

@@ -181,7 +181,7 @@ void LossNotificationController::HandleLoss(uint16_t last_received_seq_num,
                        last_decodable_non_discardable_->first_seq_num));
     loss_notification_sender_->SendLossNotification(
         last_decodable_non_discardable_->first_seq_num, last_received_seq_num,
-        decodability_flag);
+        decodability_flag, /*buffering_allowed=*/true);
   } else {
     key_frame_request_sender_->RequestKeyFrame();
   }

modules/video_coding/loss_notification_controller_unittest.cc

@@ -95,7 +95,9 @@ class LossNotificationControllerBaseTest : public ::testing::Test,
   // LossNotificationSender implementation.
   void SendLossNotification(uint16_t last_decoded_seq_num,
                             uint16_t last_received_seq_num,
-                            bool decodability_flag) override {
+                            bool decodability_flag,
+                            bool buffering_allowed) override {
+    EXPECT_TRUE(buffering_allowed);  // (Flag useful elsewhere.)
     EXPECT_FALSE(LastKeyFrameRequest());
     EXPECT_FALSE(LastLossNotification());
     last_loss_notification_.emplace(last_decoded_seq_num, last_received_seq_num,

test/rtcp_packet_parser.cc

@@ -22,8 +22,11 @@ RtcpPacketParser::RtcpPacketParser() = default;
 RtcpPacketParser::~RtcpPacketParser() = default;
 
 bool RtcpPacketParser::Parse(const void* data, size_t length) {
+  ++processed_rtcp_packets_;
+
   const uint8_t* const buffer = static_cast<const uint8_t*>(data);
   const uint8_t* const buffer_end = buffer + length;
+
   rtcp::CommonHeader header;
   for (const uint8_t* next_packet = buffer; next_packet != buffer_end;
        next_packet = header.NextPacket()) {

test/rtcp_packet_parser.h

@@ -105,6 +105,7 @@ class RtcpPacketParser {
     return &transport_feedback_;
   }
   uint32_t sender_ssrc() const { return sender_ssrc_; }
+  size_t processed_rtcp_packets() const { return processed_rtcp_packets_; }
 
  private:
   PacketCounter<rtcp::App> app_;
@@ -124,6 +125,7 @@ class RtcpPacketParser {
   PacketCounter<rtcp::Tmmbr> tmmbr_;
   PacketCounter<rtcp::TransportFeedback> transport_feedback_;
   uint32_t sender_ssrc_ = 0;
+  size_t processed_rtcp_packets_ = 0;
 };
 
 }  // namespace test

video/rtp_video_stream_receiver.cc

@@ -116,7 +116,9 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendNack(
 void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendLossNotification(
     uint16_t last_decoded_seq_num,
     uint16_t last_received_seq_num,
-    bool decodability_flag) {
+    bool decodability_flag,
+    bool buffering_allowed) {
+  RTC_DCHECK(buffering_allowed);
   rtc::CritScope lock(&cs_);
   RTC_DCHECK(lntf_state_)
       << "SendLossNotification() called twice in a row with no call to "
@@ -125,8 +127,6 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendLossNotification(
       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;
@@ -139,17 +139,24 @@ void RtpVideoStreamReceiver::RtcpFeedbackBuffer::SendBufferedRtcpFeedback() {
     std::swap(lntf_state, lntf_state_);
   }
 
+  if (lntf_state) {
+    // If either a NACK or a key frame request is sent, we should buffer
+    // the LNTF and wait for them (NACK or key frame request) to trigger
+    // the compound feedback message.
+    // Otherwise, the LNTF should be sent out immediately.
+    const bool buffering_allowed =
+        request_key_frame || !nack_sequence_numbers.empty();
+
+    loss_notification_sender_->SendLossNotification(
+        lntf_state->last_decoded_seq_num, lntf_state->last_received_seq_num,
+        lntf_state->decodability_flag, buffering_allowed);
+  }
+
   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(
@@ -478,10 +485,11 @@ void RtpVideoStreamReceiver::RequestKeyFrame() {
 void RtpVideoStreamReceiver::SendLossNotification(
     uint16_t last_decoded_seq_num,
     uint16_t last_received_seq_num,
-    bool decodability_flag) {
+    bool decodability_flag,
+    bool buffering_allowed) {
   RTC_DCHECK(config_.rtp.lntf.enabled);
   rtp_rtcp_->SendLossNotification(last_decoded_seq_num, last_received_seq_num,
-                                  decodability_flag);
+                                  decodability_flag, buffering_allowed);
 }
 
 bool RtpVideoStreamReceiver::IsUlpfecEnabled() const {

video/rtp_video_stream_receiver.h

@@ -127,7 +127,8 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
   // Implements LossNotificationSender.
   void SendLossNotification(uint16_t last_decoded_seq_num,
                             uint16_t last_received_seq_num,
-                            bool decodability_flag) override;
+                            bool decodability_flag,
+                            bool buffering_allowed) override;
 
   bool IsUlpfecEnabled() const;
   bool IsRetransmissionsEnabled() const;
@@ -204,7 +205,8 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
     // LossNotificationSender implementation.
     void SendLossNotification(uint16_t last_decoded_seq_num,
                               uint16_t last_received_seq_num,
-                              bool decodability_flag) override;
+                              bool decodability_flag,
+                              bool buffering_allowed) override;
 
     // Send all RTCP feedback messages buffered thus far.
     void SendBufferedRtcpFeedback();