Make FlexFEC packets paceable through RTPSender.

webrtc/modules/rtp_rtcp/source/rtp_sender.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_sender.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
index 36915c53afeb7fdf9c3a18bf7de53ce4d7f67f84..4dfd407fd50bf233dbab10831080c374fe8779dc 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
@@ -41,6 +41,8 @@ constexpr uint16_t kMaxInitRtpSeqNumber = 32767;  // 2^15 -1.
 constexpr uint32_t kTimestampTicksPerMs = 90;
 constexpr int kBitrateStatisticsWindowMs = 1000;
 
+constexpr size_t kFlexfecPacketsToStoreBeforePacing = 50;
+
 const char* FrameTypeToString(FrameType frame_type) {
   switch (frame_type) {
     case kEmptyFrame:
@@ -94,6 +96,7 @@ RTPSender::RTPSender(
       payload_type_map_(),
       rtp_header_extension_map_(),
       packet_history_(clock),
+      flexfec_packet_history_(clock),
       // Statistics
       rtp_stats_callback_(nullptr),
       total_bitrate_sent_(kBitrateStatisticsWindowMs,
@@ -127,6 +130,13 @@ RTPSender::RTPSender(
   // Random start, 16 bits. Can't be 0.
   sequence_number_rtx_ = random_.Rand(1, kMaxInitRtpSeqNumber);
   sequence_number_ = random_.Rand(1, kMaxInitRtpSeqNumber);
+
+  // Store FlexFEC packets in the packet history data structure, so they can
+  // be found when paced.
+  if (flexfec_sender) {
+    flexfec_packet_history_.SetStorePacketsStatus(
+        true, kFlexfecPacketsToStoreBeforePacing);
+  }
 }
 
 RTPSender::~RTPSender() {
@@ -685,15 +695,25 @@ void RTPSender::OnReceivedRtcpReportBlocks(
 }
 
 // Called from pacer when we can send the packet.
-bool RTPSender::TimeToSendPacket(uint16_t sequence_number,
+bool RTPSender::TimeToSendPacket(uint32_t ssrc,
+                                 uint16_t sequence_number,
                                  int64_t capture_time_ms,
                                  bool retransmission,
                                  int probe_cluster_id) {
-  std::unique_ptr<RtpPacketToSend> packet =
-      packet_history_.GetPacketAndSetSendTime(sequence_number, 0,
-                                              retransmission);
+  std::unique_ptr<RtpPacketToSend> packet;
+  if (ssrc == SSRC()) {
+    packet = packet_history_.GetPacketAndSetSendTime(sequence_number, 0,
+                                                     retransmission);
+  } else if (ssrc == FlexfecSsrc()) {
+    packet = flexfec_packet_history_.GetPacketAndSetSendTime(sequence_number, 0,
+                                                             retransmission);
+  } else {
+    // Incorrect SSRC. Allow sending to continue.

[danilchap, 2016/11/11 15:26:06]

because of if (!packet) return true; this block can be omitted,
but if you want to keep it, better RTC_NOTREACHABLE since checks this is right
module should be done outside.

[brandtr, 2016/11/14 09:48:09]

Right. Removing block.

+    return true;
+  }
+
   if (!packet) {
-    // Packet cannot be found. Allow sending to continue.
+    // Packet cannot be found.

[stefan-webrtc, 2016/11/13 13:38:18]

Should we log a warning?

[brandtr, 2016/11/14 09:48:09]

That's already done within the packet history itself:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...

     return true;
   }
 
@@ -836,14 +856,21 @@ bool RTPSender::SendToNetwork(std::unique_ptr<RtpPacketToSend> packet,
                                     NackOverheadRate() / 1000, packet->Ssrc());
   }
 
+  uint32_t ssrc = packet->Ssrc();
+  rtc::Optional<uint32_t> flexfec_ssrc = FlexfecSsrc();
   if (paced_sender_) {
     uint16_t seq_no = packet->SequenceNumber();
-    uint32_t ssrc = packet->Ssrc();
     // Correct offset between implementations of millisecond time stamps in
     // TickTime and Clock.
     int64_t corrected_time_ms = packet->capture_time_ms() + clock_delta_ms_;
     size_t payload_length = packet->payload_size();
-    packet_history_.PutRtpPacket(std::move(packet), storage, false);
+    if (ssrc == flexfec_ssrc) {
+      // Store FlexFEC packets in the history here, so they can be found
+      // when the pacer calls TimeToSendPacket.
+      flexfec_packet_history_.PutRtpPacket(std::move(packet), storage, false);
+    } else {
+      packet_history_.PutRtpPacket(std::move(packet), storage, false);
+    }
 
     paced_sender_->InsertPacket(priority, ssrc, seq_no, corrected_time_ms,
                                 payload_length, false);
@@ -879,8 +906,13 @@ bool RTPSender::SendToNetwork(std::unique_ptr<RtpPacketToSend> packet,
     UpdateRtpStats(*packet, false, false);
   }
 
-  // Mark the packet as sent in the history even if send failed. Dropping a
-  // packet here should be treated as any other packet drop so we should be
+  // There is no need to store FlexFEC packets in the history here, since we
+  // will never retransmit those.
+  if (ssrc == flexfec_ssrc)

[danilchap, 2016/11/11 15:26:06]

comment hint that instead you should check:
if (storage != kDontRetransmit) {
  RTC_DCHECK_EQ(ssrc, Ssrc());
  packet_histry_.Put();
}

return sent;

[brandtr, 2016/11/14 09:48:09]

This would change how we store the packets in the packet history though. Right
now, we insert the packets even if storage == kDontRetransmit. But then we never
return them from the packet history...
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...
I'm not familiar enough with that data structure to know if this is reasonable
or not.

Just wanted to confirm that this is an intentional idea in your suggestion?

[danilchap, 2016/11/14 11:20:33]

I think it is reasonable (no point to store something you would never retrieve),
but on second though - doesn't belong to this CL, so just DCHECK(storage ==
kNoRetransmit) to be safer.

On third though - it is not even needed, if sender is configured in no pacer, no
nack mode, then history should be disabled altogether.

(Current behavior is not nice left over from my refactoring - before we stored
the packet before checking if pacer is enabled or not. I've split these two
cases, but didn't notice that in second case no need to store the packet all the
time).

+    return sent;
+
+  // Mark the media packet as sent in the history even if send failed. Dropping
+  // a packet here should be treated as any other packet drop so we should be
   // ready for a retransmission.
   packet_history_.PutRtpPacket(std::move(packet), storage, true);
 
@@ -1089,6 +1121,13 @@ uint32_t RTPSender::SSRC() const {
   return ssrc_;
 }
 
+rtc::Optional<uint32_t> RTPSender::FlexfecSsrc() const {
+  if (video_) {
+    return video_->FlexfecSsrc();
+  }
+  return rtc::Optional<uint32_t>();
+}
+
 void RTPSender::SetCsrcs(const std::vector<uint32_t>& csrcs) {
   assert(csrcs.size() <= kRtpCsrcSize);
   rtc::CritScope lock(&send_critsect_);