Wire up FlexfecSender in RTPSenderVideo.

webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 34 matching lines @@
 }  // namespace
 
 RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSender* rtp_sender)
     : rtp_sender_(rtp_sender),
       clock_(clock),
       video_type_(kRtpVideoGeneric),
       retransmission_settings_(kRetransmitBaseLayer),
       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),
       video_bitrate_(1000, RateStatistics::kBpsScale) {
   encoder_checker_.Detach();
 }
 
 RTPSenderVideo::~RTPSenderVideo() {}
 
 void RTPSenderVideo::SetVideoCodecType(RtpVideoCodecTypes video_type) {
@@ skipping 42 matching lines @@
   rtc::CritScope cs(&stats_crit_);
   video_bitrate_.Update(packet_size, clock_->TimeInMilliseconds());
   TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("webrtc_rtp"),
                        "Video::PacketNormal", "timestamp", rtp_timestamp,
                        "seqnum", seq_num);
 }
 
 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();
 
   std::unique_ptr<RtpPacketToSend> red_packet(
       new RtpPacketToSend(*media_packet));
   BuildRedPayload(*media_packet, red_packet.get());
 
   std::vector<std::unique_ptr<RedPacket>> fec_packets;
   StorageType fec_storage = kDontRetransmit;
   {
     // 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());
     }
     uint16_t num_fec_packets = ulpfec_generator_.NumAvailableFecPackets();

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

probably cleaner to hide this statement (and thus next block) under if
(ulpfec_enabled()) too.

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

Makes sense. Done.

     if (num_fec_packets > 0) {
       uint16_t first_fec_sequence_number =
           rtp_sender_->AllocateSequenceNumber(num_fec_packets);
       fec_packets = ulpfec_generator_.GetUlpfecPacketsAsRed(
           red_payload_type_, ulpfec_payload_type_, first_fec_sequence_number,
           media_packet->headers_size());
       RTC_DCHECK_EQ(num_fec_packets, fec_packets.size());
       if (retransmission_settings_ & kRetransmitFECPackets)
         fec_storage = kAllowRetransmission;
     }
@@ skipping 16 matching lines @@
     std::unique_ptr<RtpPacketToSend> rtp_packet(
         new RtpPacketToSend(*media_packet));
     RTC_CHECK(rtp_packet->Parse(fec_packet->data(), fec_packet->length()));
     rtp_packet->set_capture_time_ms(media_packet->capture_time_ms());
     uint16_t fec_sequence_number = rtp_packet->SequenceNumber();
     if (rtp_sender_->SendToNetwork(std::move(rtp_packet), fec_storage,
                                    RtpPacketSender::kLowPriority)) {
       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;
     }
   }
 }
 
+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),
   // a payload type of -1 means that the corresponding feature is
   // turned off.
   RTC_DCHECK_GE(red_payload_type, -1);
   RTC_DCHECK_LE(red_payload_type, 127);
   RTC_DCHECK_GE(ulpfec_payload_type, -1);
   RTC_DCHECK_LE(ulpfec_payload_type, 127);
 
   rtc::CritScope cs(&crit_);
   red_payload_type_ = red_payload_type;
   ulpfec_payload_type_ = ulpfec_payload_type;
 
   // Must not enable ULPFEC without RED.
   // TODO(brandtr): We currently support enabling RED without ULPFEC. Change
   // this when we have removed the RED/RTX send-side workaround, so that we
   // 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};
 }
 
 void RTPSenderVideo::GetUlpfecConfig(int* red_payload_type,
                                      int* ulpfec_payload_type) const {
   rtc::CritScope cs(&crit_);
   *red_payload_type = red_payload_type_;
   *ulpfec_payload_type = ulpfec_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();
+  if (ulpfec_enabled()) {
+    overhead += ulpfec_generator_.MaxPacketOverhead() +
+                (rtp_sender_->RtpHeaderLength() - kRtpHeaderSize);
+  }
   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,
                                FrameType frame_type,
                                int8_t payload_type,
                                uint32_t rtp_timestamp,
                                int64_t capture_time_ms,
                                const uint8_t* payload_data,
                                size_t payload_size,
                                const RTPFragmentationHeader* fragmentation,
@@ skipping 36 matching lines @@
 
   std::unique_ptr<RtpPacketizer> packetizer(RtpPacketizer::Create(
       video_type, max_data_payload_length,
       video_header ? &(video_header->codecHeader) : nullptr, frame_type));
 
   StorageType storage;
   bool red_enabled;
   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
   // output multiple partitions for VP8. Should remove below check after the
   // issue is fixed.
   const RTPFragmentationHeader* frag =
       (video_type == kRtpVideoVp8) ? NULL : fragmentation;
 
   packetizer->SetPayloadData(payload_data, payload_size, frag);
 
   bool first = true;
   bool last = false;
   while (!last) {
     std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(*rtp_header));
     uint8_t* payload = packet->AllocatePayload(max_data_payload_length);
     RTC_DCHECK(payload);
 
     size_t payload_bytes_in_packet = 0;
     if (!packetizer->NextPacket(payload, &payload_bytes_in_packet, &last))
       return false;
 
     packet->SetPayloadSize(payload_bytes_in_packet);
     packet->SetMarker(last);
     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);
     }
 
     if (first_frame) {
       if (first) {
         LOG(LS_INFO)
             << "Sent first RTP packet of the first video frame (pre-pacer)";
       }
       if (last) {
@@ skipping 23 matching lines @@
   rtc::CritScope cs(&crit_);
   return retransmission_settings_;
 }
 
 void RTPSenderVideo::SetSelectiveRetransmissions(uint8_t settings) {
   rtc::CritScope cs(&crit_);
   retransmission_settings_ = settings;
 }
 
 }  // namespace webrtc