Add FlexfecSender.

webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc

Index: webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc
diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc b/webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e5598c184e19f7408099422b2aa42663b4fd268e
--- /dev/null
+++ b/webrtc/modules/rtp_rtcp/source/flexfec_sender_impl.cc
@@ -0,0 +1,167 @@
+/*
+ *  Copyright (c) 2016 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/modules/rtp_rtcp/source/flexfec_sender_impl.h"
+
+#include <utility>
+
+#include "webrtc/base/logging.h"
+#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
+#include "webrtc/modules/rtp_rtcp/source/forward_error_correction.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h"
+
+namespace webrtc {
+
+namespace {
+
+constexpr uint16_t kMaxInitRtpSeqNumber = 32767;  // 2^15 -1.
+
+// Conversion between NTP millisecond time and RTP timestamp.
+constexpr int kMsToRtpTimestamp = 90;

[danilchap, 2016/10/20 15:12:57]

does the frequency has to be same as for video stream?

[brandtr, 2016/10/24 12:52:08]

It doesn't have to be, but it's "RECOMMENDED" that the FlexFEC timestamp rate
matches that of the protected media stream, which currently only is video. (When
we support protecting audio, we might do that in combination with protecting
video, so then the RTP timestamp rate should probably still be 90 kHz.)

I have updated the comment and replaced the "90" literal with a constant from
rtp_rtcp_defines.h.

[danilchap, 2016/10/24 13:40:31]

Sorry used too strong wording, I meant 'is there a reason' instead of 'has to'.
There is!

+
+// How often to log the generated FEC packets to the text log.
+constexpr int kPacketLogIntervalMs = 10000;
+
+}  // namespace
+
+std::unique_ptr<FlexfecSender> FlexfecSender::Create(
+    int flexfec_payload_type,
+    uint32_t flexfec_ssrc,
+    uint32_t protected_media_ssrc,
+    std::vector<RtpExtension> rtp_header_extensions,
+    Clock* clock) {
+  // Don't instantiate this object if FlexFEC is disabled.
+  RTC_DCHECK_GE(flexfec_payload_type, 0);
+  RTC_DCHECK_LE(flexfec_payload_type, 127);
+  return std::unique_ptr<FlexfecSender>(new FlexfecSenderImpl(
+      flexfec_payload_type, flexfec_ssrc, protected_media_ssrc,
+      rtp_header_extensions, clock));

[danilchap, 2016/10/20 15:12:57]

std::move(rtp_header_extensions)
(otherwise vector will be copied)
or pass that vector by const& (since you do not store it)

[brandtr, 2016/10/24 12:52:07]

Yep, this should be const&. Thanks!

+}
+
+FlexfecSenderImpl::FlexfecSenderImpl(
+    int flexfec_payload_type,
+    uint32_t flexfec_ssrc,
+    uint32_t protected_media_ssrc,
+    std::vector<RtpExtension> rtp_header_extensions,
+    Clock* clock)
+    : clock_(clock),
+      random_(clock_->TimeInMicroseconds()),
+      last_generated_packet_ms_(-1),
+      flexfec_payload_type_(flexfec_payload_type),
+      // Initialize the RTP sequence numbers and timestamp offset randomly.
+      // (This is not intended to be cryptographically strong.)
+      seq_num_(random_.Rand(1, kMaxInitRtpSeqNumber)),
+      timestamp_offset_(random_.Rand<uint32_t>()),
+      flexfec_ssrc_(flexfec_ssrc),
+      protected_media_ssrc_(protected_media_ssrc),
+      ulpfec_sender_(ForwardErrorCorrection::CreateFlexfec()),
+      rtp_header_extension_map_() {
+  // It's OK to create this object on a different thread/task queue than
+  // the one used during main operation.
+  sequence_checker_.Detach();
+
+  // Register RTP header extensions.

[danilchap, 2016/10/20 15:12:57]

Does FlexfecSender has to know full list of bwe extensions?
RTPSender need to know that list since it is RTPSender that set them.
Not sure if it is currently avoidable (without creating dedicated RTPSender just
for flexfec and keeping that logic there)
look like TODO for me to provide better interface for supported extensions
lists.

[brandtr, 2016/10/24 12:52:07]

Yes, it needs to know about all BWE extensions, since we are not reusing the RTP
header extension map from the RTPSender here, but we do call ReserveExtension on
the newly create RtpPackets. I agree that it's not super nice to list a bunch of
header extensions here however, but that seemed like a simple solution for me.

With the current design (FlexfecSender connected to RTPSender), we _could_ have
a method "RegisterExtension" in the FlexfecSender interface, which would be
called from RTPSender when that registers an extension. I'm not sure its worth
it though, because with the new design (FlexfecSender connected to PacedSender),
the FlexfecSender object would probably be created by Call, where the RTP header
extensions are available as a std::vector<RtpExtension> in the VideoSendStream
config. What do you think?

Creating a dedicated RTPSender for FlexFEC wouldn't work, since the
FlexfecSender would then not have access to the RTP media packets created by the
"media RTPSender".

+  for (const auto& extension : rtp_header_extensions) {
+    if (extension.uri == RtpExtension::kTransportSequenceNumberUri) {
+      rtp_header_extension_map_.Register(kRtpExtensionTransportSequenceNumber,
+                                         extension.id);
+    } else if (extension.uri == RtpExtension::kAbsSendTimeUri) {
+      rtp_header_extension_map_.Register(kRtpExtensionAbsoluteSendTime,
+                                         extension.id);
+    } else if (extension.uri == RtpExtension::kTimestampOffsetUri) {
+      rtp_header_extension_map_.Register(kRtpExtensionTransmissionTimeOffset,
+                                         extension.id);
+    } else {
+      LOG(LS_WARNING) << "RTP header extension with id: " << extension.id
+                      << ", uri: " << extension.uri
+                      << ", is unsupported by FlexfecSender.";
+    }
+  }
+}
+
+FlexfecSenderImpl::~FlexfecSenderImpl() = default;
+
+// We are reusing the implementation from ProducerFec for SetFecParameters,
+// AddRtpPacketAndGenerateFec, and FecAvailable.
+void FlexfecSenderImpl::SetFecParameters(const FecProtectionParams* params) {
+  RTC_DCHECK(sequence_checker_.CalledSequentially());
+  ulpfec_sender_.SetFecParameters(params);
+}
+
+int FlexfecSenderImpl::AddRtpPacketAndGenerateFec(RtpPacketToSend* packet) {

[danilchap, 2016/10/20 15:12:57]

does parameter need to be non-const? May be it can it be const&

[brandtr, 2016/10/24 12:52:07]

Good idea, done!

+  RTC_DCHECK(sequence_checker_.CalledSequentially());
+  if (packet->Ssrc() != protected_media_ssrc_) {
+    return 0;
+  }
+  return ulpfec_sender_.AddRtpPacketAndGenerateFec(
+      packet->data(), packet->payload_size(), packet->headers_size());
+}
+
+bool FlexfecSenderImpl::FecAvailable() const {
+  RTC_DCHECK(sequence_checker_.CalledSequentially());
+  return ulpfec_sender_.FecAvailable();
+}
+
+std::vector<std::unique_ptr<RtpPacketToSend>>
+FlexfecSenderImpl::GetFecPackets() {
+  RTC_DCHECK(sequence_checker_.CalledSequentially());
+
+  std::vector<std::unique_ptr<RtpPacketToSend>> fec_packets_to_send;
+  fec_packets_to_send.reserve(ulpfec_sender_.generated_fec_packets_.size());
+  for (const auto& fec_packet : ulpfec_sender_.generated_fec_packets_) {
+    std::unique_ptr<RtpPacketToSend> fec_packet_to_send(
+        new RtpPacketToSend(&rtp_header_extension_map_));
+
+    // RTP header.
+    fec_packet_to_send->SetMarker(false);
+    fec_packet_to_send->SetPayloadType(flexfec_payload_type_);
+    fec_packet_to_send->SetSequenceNumber(seq_num_++);
+    fec_packet_to_send->SetTimestamp(
+        timestamp_offset_ +
+        kMsToRtpTimestamp *
+            static_cast<uint32_t>(clock_->CurrentNtpInMilliseconds()));

[danilchap, 2016/10/20 15:12:57]

Does it have to be based on ntp? may be base it on TimeInMilliseconds (or
microseconds) (advantage: that one is monotonic, ntp is not)
either way do cast after multiplication for proper wrapping.
Or (in case you want to keep it ntp based) use function from
rtp_rtcp/source/time_util.h:
NtpToRtp(NtpTime(*clock_), kSToRtpTimestamp)
or
CurrentRtp(*clock_, kSToRtpTimestamp)
(frequency should be per second then, i.e. 90000)

[brandtr, 2016/10/24 12:52:08]

Cool, didn't think about the monotonicity. It can be based on
TimeInMilliseconds, so changing to that. Also moving the multiplication inside
the cast. Note that the multiplication is outside the cast in the place where I
got inspired:
https://cs.chromium.org/chromium/src/third_party/webrtc/video/vie_encoder.cc?...

+    fec_packet_to_send->SetSsrc(flexfec_ssrc_);
+    // Reserve extensions, if registered. These will be set by the RtpSender.

[danilchap, 2016/10/20 15:12:57]

fec_packet_to_send->set_capture_time_ms(...)
otherwise TransmissionOffset will not be set by RTPSender

[brandtr, 2016/10/24 12:52:07]

Done.

+    fec_packet_to_send->ReserveExtension<AbsoluteSendTime>();
+    fec_packet_to_send->ReserveExtension<TransmissionOffset>();
+    fec_packet_to_send->ReserveExtension<TransportSequenceNumber>();
+
+    // RTP payload.
+    uint8_t* payload = fec_packet_to_send->AllocatePayload(fec_packet->length);
+    memcpy(payload, fec_packet->data, fec_packet->length);
+
+    fec_packets_to_send.push_back(std::move(fec_packet_to_send));
+  }
+  ulpfec_sender_.ResetState();
+
+  // Periodically log generated FEC packets.
+  int64_t now_ms = clock_->TimeInMilliseconds();
+  if (!fec_packets_to_send.empty() &&
+      now_ms - last_generated_packet_ms_ > kPacketLogIntervalMs) {
+    std::stringstream ss;

[danilchap, 2016/10/20 15:12:57]

probably better to avoid stringstream:
Currently string will be always generated, even if later discarded.
if logged directly to LOG(LS_INFO) and log disabled, all operator<< will be
mostly noop.

[brandtr, 2016/10/24 12:52:08]

Done.

+    ss << "Generated " << fec_packets_to_send.size() << " FlexFEC packets with "
+       << "payload type: " << flexfec_payload_type_
+       << " and SSRC: " << flexfec_ssrc_ << ".";
+    LOG(LS_INFO) << ss.str();
+    last_generated_packet_ms_ = now_ms;
+  }
+
+  return fec_packets_to_send;
+}
+
+// This member function is called from the pacer thread sometimes. No need
+// for synchronization however, since it always returns the same constant.
+// TODO(brandtr): Remove the underlying call and make the packet overhead
+// explicit here.
+size_t FlexfecSenderImpl::MaxPacketOverhead() const {
+  return ulpfec_sender_.MaxPacketOverhead();
+}
+
+}  // namespace webrtc