Wire up FlexfecSender in RTPSenderVideo.

webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
index 916561ea876bbc40af46433dca401e7a5519f6b7..dea269b43589f33140bf02445df98c32e13a64d6 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -52,6 +52,7 @@ RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSender* rtp_sender)
       last_rotation_(kVideoRotation_0),
       red_payload_type_(-1),
       ulpfec_payload_type_(-1),
+      flexfec_sender_(nullptr),  // TODO(brandtr): Wire up in future CL.
       delta_fec_params_{0, 1, kFecMaskRandom},
       key_fec_params_{0, 1, kFecMaskRandom},
       fec_bitrate_(1000, RateStatistics::kBpsScale),
@@ -114,7 +115,7 @@ void RTPSenderVideo::SendVideoPacket(std::unique_ptr<RtpPacketToSend> packet,
 void RTPSenderVideo::SendVideoPacketAsRed(
     std::unique_ptr<RtpPacketToSend> media_packet,
     StorageType media_packet_storage,
-    bool protect) {
+    bool protect_packet) {
   uint32_t rtp_timestamp = media_packet->Timestamp();
   uint16_t media_seq_num = media_packet->SequenceNumber();
 
@@ -128,7 +129,7 @@ void RTPSenderVideo::SendVideoPacketAsRed(
     // Only protect while creating RED and FEC packets, not when sending.
     rtc::CritScope cs(&crit_);
     red_packet->SetPayloadType(red_payload_type_);
-    if (protect) {
+    if (ulpfec_enabled() && protect_packet) {
       ulpfec_generator_.AddRtpPacketAndGenerateFec(
           media_packet->data(), media_packet->payload_size(),
           media_packet->headers_size());
@@ -170,7 +171,7 @@ void RTPSenderVideo::SendVideoPacketAsRed(
       rtc::CritScope cs(&stats_crit_);
       fec_bitrate_.Update(fec_packet->length(), clock_->TimeInMilliseconds());
       TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
-                           "Video::PacketFec", "timestamp", rtp_timestamp,
+                           "Video::PacketUlpfec", "timestamp", rtp_timestamp,
                            "seqnum", fec_sequence_number);
     } else {
       LOG(LS_WARNING) << "Failed to send FEC packet " << fec_sequence_number;
@@ -178,6 +179,43 @@ void RTPSenderVideo::SendVideoPacketAsRed(
   }
 }
 
+std::vector<std::unique_ptr<RtpPacketToSend>> RTPSenderVideo::GetFlexfecPackets(

[danilchap, 2016/11/08 11:45:08]

try to order functions same as in header file, i.e. move these two functions to
the bottom of the .cc

[brandtr, 2016/11/09 10:40:00]

I changed the .h file instead :)

Now at least SendVideoPacket, SendVideoPacketAsRedMaybeWithUlpfec, and
SendVideoPacketWithFlexfec are locally ordered. The member functions in this .cc
file are not globally ordered though :/

+    const RtpPacketToSend& media_packet) {
+  RTC_DCHECK(flexfec_sender_);
+
+  std::vector<std::unique_ptr<RtpPacketToSend>> fec_packets;
+  if (!flexfec_sender_->AddRtpPacketAndGenerateFec(media_packet)) {
+    return fec_packets;
+  }
+  if (flexfec_sender_->FecAvailable()) {
+    fec_packets = flexfec_sender_->GetFecPackets();
+  }
+
+  return fec_packets;
+}
+
+void RTPSenderVideo::SendFlexfecPackets(
+    std::vector<std::unique_ptr<RtpPacketToSend>>* fec_packets) {
+  for (auto& fec_packet : *fec_packets) {
+    uint32_t timestamp = fec_packet->Timestamp();
+    uint16_t seq_num = fec_packet->SequenceNumber();
+    // We do not support retransmitting lost FlexFEC packets.
+    constexpr StorageType kFecStorage = kDontRetransmit;

[danilchap, 2016/11/08 11:45:08]

the enums values probably self-explained already, extra const (specially
constexpr) feels redundant.

[brandtr, 2016/11/09 10:40:00]

Removed.

+    constexpr RtpPacketSender::Priority kFecPriority =
+        RtpPacketSender::kLowPriority;
+    if (rtp_sender_->SendToNetwork(std::move(fec_packet), kFecStorage,
+                                   kFecPriority)) {
+      // TODO(brandtr): Wire up stats here.

[danilchap, 2016/11/08 11:45:08]

may be resolve this TODO right away? or is it more complicated than copying two
lines from similar codepath for ulpfec

[brandtr, 2016/11/09 10:40:00]

Yes, it's more complicated, since I want to do a concerted effort to design the
stats for FlexFEC. At the same time, I will probably rename the old stats. I
haven't gotten that far in the design yet, so therefore I leave this TODO here.

+      TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
+                           "Video::PacketFlexfec", "timestamp", timestamp,
+                           "seqnum", seq_num);
+    } else {
+      LOG(LS_WARNING) << "Failed to send FlexFEC packet " << seq_num;
+    }
+  }
+  fec_packets->clear();
+}
+
 void RTPSenderVideo::SetUlpfecConfig(int red_payload_type,
                                      int ulpfec_payload_type) {
   // Sanity check. Per the definition of UlpfecConfig (see config.h),
@@ -198,7 +236,7 @@ void RTPSenderVideo::SetUlpfecConfig(int red_payload_type,
   // ensure that RED and ULPFEC are only enabled together.
   RTC_DCHECK(red_enabled() || !ulpfec_enabled());
 
-  // Reset FEC rates.
+  // Reset FEC parameters.
   delta_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom};
   key_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom};
 }
@@ -211,29 +249,34 @@ void RTPSenderVideo::GetUlpfecConfig(int* red_payload_type,
 }
 
 size_t RTPSenderVideo::FecPacketOverhead() const {
+  // Overhead is FEC headers plus RED for FEC header plus anything in RTP
+  // header beyond the 12 bytes base header (CSRC list, extensions...)
+  // This reason for the header extensions to be included here is that
+  // from an FEC viewpoint, they are part of the payload to be protected.
+  // (The base RTP header is already protected by the FEC header.)
+
+  if (flexfec_enabled()) {
+    return flexfec_sender_->MaxPacketOverhead() +
+           (rtp_sender_->RtpHeaderLength() - kRtpHeaderSize);
+  }
+
   rtc::CritScope cs(&crit_);
   size_t overhead = 0;
   if (red_enabled()) {
-    // Overhead is FEC headers plus RED for FEC header plus anything in RTP
-    // header beyond the 12 bytes base header (CSRC list, extensions...)
-    // This reason for the header extensions to be included here is that
-    // from an FEC viewpoint, they are part of the payload to be protected.
-    // (The base RTP header is already protected by the FEC header.)
-    return ulpfec_generator_.MaxPacketOverhead() + kRedForFecHeaderLength +
-           (rtp_sender_->RtpHeaderLength() - kRtpHeaderSize);
+    overhead += kRedForFecHeaderLength;
+  }
+  if (ulpfec_enabled()) {
+    overhead += ulpfec_generator_.MaxPacketOverhead() +
+                (rtp_sender_->RtpHeaderLength() - kRtpHeaderSize);
   }
-  if (ulpfec_enabled())
-    overhead += ulpfec_generator_.MaxPacketOverhead();
   return overhead;
 }
 
 void RTPSenderVideo::SetFecParameters(const FecProtectionParams& delta_params,
                                       const FecProtectionParams& key_params) {
   rtc::CritScope cs(&crit_);
-  if (ulpfec_enabled()) {
-    delta_fec_params_ = delta_params;
-    key_fec_params_ = key_params;
-  }
+  delta_fec_params_ = delta_params;
+  key_fec_params_ = key_params;
 }
 
 bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
@@ -290,11 +333,20 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
   bool first_frame = first_frame_sent_();
   {
     rtc::CritScope cs(&crit_);
+
+    // Media packet storage.
+    storage = packetizer->GetStorageType(retransmission_settings_);
+
+    // FEC settings.
     const FecProtectionParams& fec_params =
         frame_type == kVideoFrameKey ? key_fec_params_ : delta_fec_params_;
-    ulpfec_generator_.SetFecParameters(fec_params);
-    storage = packetizer->GetStorageType(retransmission_settings_);
     red_enabled = this->red_enabled();
+    if (flexfec_enabled()) {

[danilchap, 2016/11/08 11:45:08]

in this file it is more common to omit {} for one-line if block.

[brandtr, 2016/11/09 10:40:00]

Yep, removed.

+      flexfec_sender_->SetFecParameters(fec_params);
+    }
+    if (ulpfec_enabled()) {
+      ulpfec_generator_.SetFecParameters(fec_params);
+    }
   }
 
   // TODO(changbin): we currently don't support to configure the codec to
@@ -321,9 +373,25 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type,
     if (!rtp_sender_->AssignSequenceNumber(packet.get()))
       return false;
 
-    if (red_enabled) {
-      SendVideoPacketAsRed(std::move(packet), storage,
-                           packetizer->GetProtectionType() == kProtectedPacket);
+    const bool protect_packet =
+        (packetizer->GetProtectionType() == kProtectedPacket);
+    // Note that we either send non-encapsulated media packets with FlexFEC

[danilchap, 2016/11/08 11:45:08]

this comments feels like repeating (not explaining) code just below, i.e.
redundant. May be remove it or reduce it to part that is not in code below:
mentioning that ulpfec hides inside ...AsRed function.
(Or rename or split SendVideoPacketAsRed to SendVideoPacketWithUlpfec)

[brandtr, 2016/11/09 10:40:00]

Agree that the comment is a bit superfluous. I have renamed SendVideoPacketAsRed
to better reflect what it is doing. Note that this convoluted name is sort of
necessary, since we may use RED without ULPFEC currently. (We will remove this
possibility in the future, but that will take at least ~3 Chrome versions to
do.)

+    // packets, or RED-encapsulated media packets with our without ULPFEC,
+    // packets, or non-encapsulated media packets without any FEC packets.
+    // If both FlexFEC and RED+ULPFEC are enabled, FlexFEC takes priority.
+    // TODO(brandtr): Remove this FlexFEC code path, when FlexfecSender
+    // is wired up to PacedSender instead.
+    if (flexfec_enabled()) {
+      std::vector<std::unique_ptr<RtpPacketToSend>> fec_packets;
+      if (protect_packet)
+        fec_packets = GetFlexfecPackets(*packet);
+      // Send media packet as usual.
+      SendVideoPacket(std::move(packet), storage);
+      // Send available FlexFEC packets after main media packet is sent.
+      if (!fec_packets.empty())
+        SendFlexfecPackets(&fec_packets);

[danilchap, 2016/11/08 11:45:08]

may be cleaner to change SendFlexfecPackets to take fec_packets by value.

[brandtr, 2016/11/09 10:40:00]

Yes, that would have been nicer. Now that function doesn't exist any more
though.

+    } else if (red_enabled) {
+      SendVideoPacketAsRed(std::move(packet), storage, protect_packet);
     } else {
       SendVideoPacket(std::move(packet), storage);
     }