Wire up FlexfecSender in RTP module and VideoSendStream.

webrtc/video/video_send_stream.cc

 /*
  *  Copyright (c) 2013 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.
  */
 #include "webrtc/video/video_send_stream.h"
@@ skipping 26 matching lines @@
 namespace {
 
 std::vector<RtpRtcp*> CreateRtpRtcpModules(
     Transport* outgoing_transport,
     RtcpIntraFrameObserver* intra_frame_callback,
     RtcpBandwidthObserver* bandwidth_callback,
     TransportFeedbackObserver* transport_feedback_callback,
     RtcpRttStats* rtt_stats,
     RtpPacketSender* paced_sender,
     TransportSequenceNumberAllocator* transport_sequence_number_allocator,
+    FlexfecSender* flexfec_sender,
     SendStatisticsProxy* stats_proxy,
     SendDelayStats* send_delay_stats,
     RtcEventLog* event_log,
     RateLimiter* retransmission_rate_limiter,
     size_t num_modules) {
   RTC_DCHECK_GT(num_modules, 0u);
   RtpRtcp::Configuration configuration;
   ReceiveStatistics* null_receive_statistics = configuration.receive_statistics;
   configuration.audio = false;
   configuration.receiver_only = false;
+  configuration.flexfec_sender = flexfec_sender;
   configuration.receive_statistics = null_receive_statistics;
   configuration.outgoing_transport = outgoing_transport;
   configuration.intra_frame_callback = intra_frame_callback;
   configuration.bandwidth_callback = bandwidth_callback;
   configuration.transport_feedback_callback = transport_feedback_callback;
   configuration.rtt_stats = rtt_stats;
   configuration.rtcp_packet_type_counter_observer = stats_proxy;
   configuration.paced_sender = paced_sender;
   configuration.transport_sequence_number_allocator =
       transport_sequence_number_allocator;
   configuration.send_bitrate_observer = stats_proxy;
   configuration.send_frame_count_observer = stats_proxy;
   configuration.send_side_delay_observer = stats_proxy;
   configuration.send_packet_observer = send_delay_stats;
   configuration.event_log = event_log;
   configuration.retransmission_rate_limiter = retransmission_rate_limiter;
 
   std::vector<RtpRtcp*> modules;
   for (size_t i = 0; i < num_modules; ++i) {
     RtpRtcp* rtp_rtcp = RtpRtcp::CreateRtpRtcp(configuration);
     rtp_rtcp->SetSendingStatus(false);
     rtp_rtcp->SetSendingMediaStatus(false);
     rtp_rtcp->SetRTCPStatus(RtcpMode::kCompound);
     modules.push_back(rtp_rtcp);
   }
   return modules;
 }
 
+// TODO(brandtr): Update this function when we support multistream protection.
+std::unique_ptr<FlexfecSender> MaybeCreateFlexfecSender(
+    const VideoSendStream::Config& config) {
+  if (config.rtp.flexfec.flexfec_payload_type == -1) {

[stefan-webrtc, 2016/11/14 15:28:39]

Depending on answer to the other CL, this might be < 0

[brandtr, 2016/11/15 09:19:08]

Changed.

+    return nullptr;
+  }
+  RTC_DCHECK_GE(config.rtp.flexfec.flexfec_payload_type, 0);
+  RTC_DCHECK_LE(config.rtp.flexfec.flexfec_payload_type, 127);
+  if (config.rtp.flexfec.flexfec_ssrc == 0) {
+    LOG(LS_ERROR) << "FlexFEC is enabled, but no FlexFEC SSRC given. "
+                     "Therefore disabling FlexFEC.";
+    return nullptr;
+  }
+  if (config.rtp.flexfec.protected_media_ssrcs.empty()) {
+    LOG(LS_ERROR) << "FlexFEC is enabled, but no protected media SSRC given. "
+                     "Therefore disabling FlexFEC.";
+    return nullptr;
+  }
+
+  if (config.rtp.ssrcs.size() > 1) {
+    LOG(LS_WARNING) << "Both FlexFEC and simulcast are enabled. This "

[stefan-webrtc, 2016/11/14 15:28:39]

What would it have meant if it was supported? How is it different to the if
statement below covering multiple media streams?

[brandtr, 2016/11/15 09:19:08]

This is an unfortunate side-effect of the way that we have wired up the
FlexfecSender with RTPSenderVideo. There is only a single FlexfecSender, owned
by VideoSendStream, whose address is propagated down to the RTPSenderVideo. If
there are multiple RTPSenderVideo's, e.g. in the simulcast scenario, this leads
to a problem, since we can only protect a single media stream but the single
FlexfecSender may be fed RTP packets from multiple RTPSenderVideos.

My idea was to ensure that we don't enable FlexFEC for simulcast in the first
place, and then DCHECK in the FlexfecSender that we only receive media packets
from one media ssrc. Another approach would to allow for simulcast here, and
change the DCHECK to a runtime check.

The if statement below checks that the FlexfecSender is not configured to
protect multiple media streams, which is different from the problem described
above that has to do with the wire up.

+                       "combination is however not supported by our current "
+                       "FlexFEC implementation. Therefore disabling FlexFEC.";
+    return nullptr;
+  }
+
+  if (config.rtp.flexfec.protected_media_ssrcs.size() > 1) {
+    LOG(LS_WARNING)
+        << "The supplied FlexfecConfig contained multiple protected "
+           "media streams, but our implementation currently only "
+           "supports protecting a single media stream. "
+           "To avoid confusion, disabling FlexFEC completely.";
+    return nullptr;
+  }
+
+  RTC_DCHECK_EQ(1U, config.rtp.flexfec.protected_media_ssrcs.size());
+  return std::unique_ptr<FlexfecSender>(new FlexfecSender(
+      config.rtp.flexfec.flexfec_payload_type, config.rtp.flexfec.flexfec_ssrc,
+      config.rtp.flexfec.protected_media_ssrcs[0], config.rtp.extensions,
+      Clock::GetRealTimeClock()));
+}
+
 }  // namespace
 
 std::string
 VideoSendStream::Config::EncoderSettings::ToString() const {
   std::stringstream ss;
   ss << "{payload_name: " << payload_name;
   ss << ", payload_type: " << payload_type;
   ss << ", encoder: " << (encoder ? "(VideoEncoder)" : "nullptr");
   ss << '}';
   return ss.str();
@@ skipping 31 matching lines @@
   ss << ", extensions: [";
   for (size_t i = 0; i < extensions.size(); ++i) {
     ss << extensions[i].ToString();
     if (i != extensions.size() - 1)
       ss << ", ";
   }
   ss << ']';
 
   ss << ", nack: {rtp_history_ms: " << nack.rtp_history_ms << '}';
   ss << ", ulpfec: " << ulpfec.ToString();
+  ss << ", flexfec: " << flexfec.ToString();
   ss << ", rtx: " << rtx.ToString();
   ss << ", c_name: " << c_name;
   ss << '}';
   return ss.str();
 }
 
 std::string VideoSendStream::Config::ToString() const {
   std::stringstream ss;
   ss << "{encoder_settings: " << encoder_settings.ToString();
   ss << ", rtp: " << rtp.ToString();
@@ skipping 172 matching lines @@
 
   rtc::CriticalSection encoder_activity_crit_sect_;
   CheckEncoderActivityTask* check_encoder_activity_task_
       GUARDED_BY(encoder_activity_crit_sect_);
 
   CallStats* const call_stats_;
   CongestionController* const congestion_controller_;
   BitrateAllocator* const bitrate_allocator_;
   VieRemb* const remb_;
 
+  // TODO(brandtr): Consider moving this to a new FlexfecSendStream class.
+  std::unique_ptr<FlexfecSender> flexfec_sender_;
+
   rtc::CriticalSection ivf_writers_crit_;
   std::unique_ptr<IvfFileWriter> file_writers_[kMaxSimulcastStreams] GUARDED_BY(
       ivf_writers_crit_);
 
   int max_padding_bitrate_;
   int encoder_min_bitrate_bps_;
   uint32_t encoder_max_bitrate_bps_;
   uint32_t encoder_target_rate_bps_;
 
   ViEEncoder* const vie_encoder_;
@@ skipping 316 matching lines @@
     : stats_proxy_(stats_proxy),
       config_(config),
       suspended_ssrcs_(std::move(suspended_ssrcs)),
       module_process_thread_(nullptr),
       worker_queue_(worker_queue),
       check_encoder_activity_task_(nullptr),
       call_stats_(call_stats),
       congestion_controller_(congestion_controller),
       bitrate_allocator_(bitrate_allocator),
       remb_(remb),
+      flexfec_sender_(MaybeCreateFlexfecSender(*config_)),
       max_padding_bitrate_(0),
       encoder_min_bitrate_bps_(0),
       encoder_max_bitrate_bps_(initial_encoder_max_bitrate),
       encoder_target_rate_bps_(0),
       vie_encoder_(vie_encoder),
       encoder_feedback_(Clock::GetRealTimeClock(),
                         config_->rtp.ssrcs,
                         vie_encoder),
       protection_bitrate_calculator_(Clock::GetRealTimeClock(), this),
       bandwidth_observer_(congestion_controller_->GetBitrateController()
                               ->CreateRtcpBandwidthObserver()),
       rtp_rtcp_modules_(CreateRtpRtcpModules(
           config_->send_transport,
           &encoder_feedback_,
           bandwidth_observer_.get(),
           congestion_controller_->GetTransportFeedbackObserver(),
           call_stats_->rtcp_rtt_stats(),
           congestion_controller_->pacer(),
           congestion_controller_->packet_router(),
+          flexfec_sender_.get(),
           stats_proxy_,
           send_delay_stats,
           event_log,
           congestion_controller_->GetRetransmissionRateLimiter(),
           config_->rtp.ssrcs.size())),
       payload_router_(rtp_rtcp_modules_,
                       config_->encoder_settings.payload_type),
       weak_ptr_factory_(this) {
   RTC_DCHECK_RUN_ON(worker_queue_);
   LOG(LS_INFO) << "VideoSendStreamInternal: " << config_->ToString();
@@ skipping 243 matching lines @@
       RTC_DCHECK(ok);
     }
   }
 
   return result;
 }
 
 void VideoSendStreamImpl::ConfigureProtection() {
   RTC_DCHECK_RUN_ON(worker_queue_);
 
+  // Consistency of FlexFEC parameters is checked in MaybeCreateFlexfecSender.
+  const bool flexfec_enabled = (flexfec_sender_ != nullptr);
+
+  // Consistency of NACK and RED+ULPFEC parameters is checked in this function.
   const bool nack_enabled = config_->rtp.nack.rtp_history_ms > 0;
   int red_payload_type = config_->rtp.ulpfec.red_payload_type;
   int ulpfec_payload_type = config_->rtp.ulpfec.ulpfec_payload_type;
 
   // Shorthands.
   auto IsRedEnabled = [&]() { return red_payload_type >= 0; };
+  auto DisableRed = [&]() { red_payload_type = -1; };
   auto IsUlpfecEnabled = [&]() { return ulpfec_payload_type >= 0; };
   auto DisableUlpfec = [&]() { ulpfec_payload_type = -1; };
 
+  // If enabled, FlexFEC takes priority over RED+ULPFEC.
+  if (flexfec_enabled) {
+    // We can safely disable RED here, because if the remote supports FlexFEC,
+    // we know that it has a receiver without the RED/RTX workaround.
+    // See http://crbug.com/webrtc/6650 for more information.

[brandtr, 2016/11/14 15:01:57]

holmer, can you verify that this thinking is correct?

[stefan-webrtc, 2016/11/14 15:28:40]

I believe it is. :)

[brandtr, 2016/11/15 09:19:08]

Acknowledged.

+    if (IsRedEnabled()) {
+      LOG(LS_WARNING) << "Both FlexFEC and RED are configured. Disabling RED.";

[stefan-webrtc, 2016/11/14 15:28:40]

Should we warn, or should we simply have an info log? I can imagine that this
may be common if we don't filter it out in, e.g., webrtcvideoengine2.cc.

[brandtr, 2016/11/15 09:19:08]

Yes, going with LS_INFO is more coherent with how the Video{Send,Receive}Stream
configs are logged. Changing to that.

+      DisableRed();
+    }
+    if (IsUlpfecEnabled()) {
+      LOG(LS_WARNING)
+          << "Both FlexFEC and ULPFEC are configured. Disabling ULPFEC.";

[stefan-webrtc, 2016/11/14 15:28:40]

Same here.

[brandtr, 2016/11/15 09:19:08]

Done.

+      DisableUlpfec();
+    }
+  }
+
   // Payload types without picture ID cannot determine that a stream is complete
   // without retransmitting FEC, so using ULPFEC + NACK for H.264 (for instance)
   // is a waste of bandwidth since FEC packets still have to be transmitted.
   // Note that this is not the case with FlexFEC.
   if (nack_enabled && IsUlpfecEnabled() &&
       !PayloadTypeSupportsSkippingFecPackets(
           config_->encoder_settings.payload_name)) {
     LOG(LS_WARNING)
         << "Transmitting payload type without picture ID using "
            "NACK+ULPFEC is a waste of bandwidth since ULPFEC packets "
@@ skipping 27 matching lines @@
     // Set NACK.
     rtp_rtcp->SetStorePacketsStatus(
         nack_enabled || congestion_controller_->pacer(),
         kMinSendSidePacketHistorySize);
     // Set RED/ULPFEC information.
     for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
       rtp_rtcp->SetUlpfecConfig(red_payload_type, ulpfec_payload_type);
     }
   }
 
-  protection_bitrate_calculator_.SetProtectionMethod(IsUlpfecEnabled(),
-                                                     nack_enabled);
+  // Currently, both ULPFEC and FlexFEC use the same FEC rate calculation logic,
+  // so enable that logic if either of those FEC schemes are enabled.
+  protection_bitrate_calculator_.SetProtectionMethod(
+      flexfec_enabled || IsUlpfecEnabled(), nack_enabled);
 }
 
 void VideoSendStreamImpl::ConfigureSsrcs() {
   RTC_DCHECK_RUN_ON(worker_queue_);
   // Configure regular SSRCs.
   for (size_t i = 0; i < config_->rtp.ssrcs.size(); ++i) {
     uint32_t ssrc = config_->rtp.ssrcs[i];
     RtpRtcp* const rtp_rtcp = rtp_rtcp_modules_[i];
     rtp_rtcp->SetSSRC(ssrc);
 
@@ skipping 125 matching lines @@
 }
 
 void VideoSendStreamImpl::SetTransportOverhead(
     int transport_overhead_per_packet) {
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_)
     rtp_rtcp->SetTransportOverhead(transport_overhead_per_packet);
 }
 
 }  // namespace internal
 }  // namespace webrtc