commit ca2c43042b730f4543b0a811f50990da57956577
author Elad Alon <eladalon@webrtc.org> Mon May 27 20:43:10 2019
committer Commit Bot <commit-bot@chromium.org> Tue May 28 08:23:08 2019
tree 16042c7179b051331a2ba1d4d9f797034d3541fb
parent 3a072de0d28f5a1531a4f1292c326961014486d9

Allow both LNTF to coexist with NACKs and key frame requests

LNTF (loss notifications) are no longer mutually exclusive with
NACK and key frames; a receiver may send both, and the sender would
be allowed to choose which to regard.

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

video/rtp_video_stream_receiver.cc

diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc
index 641af8a..adca218 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -166,11 +166,12 @@
 
   process_thread_->RegisterModule(rtp_rtcp_.get(), RTC_FROM_HERE);
 
-  // TODO(bugs.webrtc.org/10662): NACK and LNTF shouldn't be mutually exclusive.
   if (config_.rtp.lntf.enabled) {
     loss_notification_controller_ =
         absl::make_unique<LossNotificationController>(this, this);
-  } else if (config_.rtp.nack.rtp_history_ms != 0) {
+  }
+
+  if (config_.rtp.nack.rtp_history_ms != 0) {
     nack_module_ = absl::make_unique<NackModule>(clock_, nack_sender, this);
     process_thread_->RegisterModule(nack_module_.get(), RTC_FROM_HERE);
   }
@@ -261,6 +262,19 @@
                    ntp_estimator_.Estimate(rtp_header.timestamp));
   packet.generic_descriptor = generic_descriptor;
 
+  if (loss_notification_controller_) {
+    if (is_recovered) {
+      // TODO(bugs.webrtc.org/10336): Implement support for reordering.
+      RTC_LOG(LS_WARNING)
+          << "LossNotificationController does not support reordering.";
+    } else {
+      // TODO(bugs.webrtc.org/10336): Coordinate with |nack_module_| to make
+      // sure that only a single RTCP packet is produced if both a LNTF as
+      // well as a NACK (or key frame request) should be issued.
+      loss_notification_controller_->OnReceivedPacket(packet);
+    }
+  }
+
   if (nack_module_) {
     const bool is_keyframe =
         video_header.is_first_packet_in_frame &&
@@ -268,22 +282,11 @@
 
     packet.timesNacked = nack_module_->OnReceivedPacket(
         rtp_header.sequenceNumber, is_keyframe, is_recovered);
-
   } else {
     packet.timesNacked = -1;
   }
   packet.receive_time_ms = clock_->TimeInMilliseconds();
 
-  if (loss_notification_controller_) {
-    if (is_recovered) {
-      // TODO(bugs.webrtc.org/10336): Implement support for reordering.
-      RTC_LOG(LS_WARNING)
-          << "LossNotificationController does not support reordering.";
-    } else {
-      loss_notification_controller_->OnReceivedPacket(packet);
-    }
-  }
-
   if (packet.sizeBytes == 0) {
     NotifyReceiverOfEmptyPacket(packet.seqNum);
     return 0;
@@ -439,15 +442,18 @@
         frame->first_seq_num(), descriptor->FrameId(),
         descriptor->Discardable().value_or(false),
         descriptor->FrameDependenciesDiffs());
-  } else if (!has_received_frame_) {
-    // Request a key frame as soon as possible.
+  }
+
+  // Request a key frame as soon as possible.
+  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();
     }
   }
 
-  has_received_frame_ = true;
-
   if (buffered_frame_decryptor_ == nullptr) {
     reference_finder_->ManageFrame(std::move(frame));
   } else {
@@ -670,6 +676,7 @@
                                    /* is _recovered = */ false);
   }
   if (loss_notification_controller_) {
+    // TODO(bugs.webrtc.org/10336): Handle empty packets.
     RTC_LOG(LS_WARNING)
         << "LossNotificationController does not expect empty packets.";
   }