Attach TransportFeedbackPacketLossTracker to ANA (PLR only)

webrtc/voice_engine/channel.cc

Index: webrtc/voice_engine/channel.cc
diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc
index 2791f7f8b4ed555ed9da3e8d3f45ab6b9fa18171..8e5a4ca30ae12971fc2a981bd59c4150f8ef0885 100644
--- a/webrtc/voice_engine/channel.cc
+++ b/webrtc/voice_engine/channel.cc
@@ -32,6 +32,7 @@
 #include "webrtc/modules/rtp_rtcp/include/receive_statistics.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_receiver.h"
+#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_receiver_strategy.h"
 #include "webrtc/modules/utility/include/process_thread.h"
 #include "webrtc/system_wrappers/include/trace.h"
@@ -50,6 +51,11 @@ namespace {
 constexpr int64_t kMaxRetransmissionWindowMs = 1000;
 constexpr int64_t kMinRetransmissionWindowMs = 30;
 
+// TODO(elad.alon): Subsequent CL will make these values experiment-dependent.
+constexpr size_t kPacketLossTrackerMinWindowSize = 100;
+constexpr size_t kPacketLossTrackerMaxWindowSize = 200;
+constexpr size_t kPacketLossTrackerMinPairsNumForRplr = 50;
+
 }  // namespace
 
 const int kTelephoneEventAttenuationdB = 10;
@@ -202,13 +208,15 @@ class TransportFeedbackProxy : public TransportFeedbackObserver {
   }
 
   // Implements TransportFeedbackObserver.
-  void AddPacket(uint16_t sequence_number,
+  void AddPacket(uint32_t ssrc,
+                 uint16_t sequence_number,
                  size_t length,
                  int probe_cluster_id) override {
     RTC_DCHECK(pacer_thread_.CalledOnValidThread());
     rtc::CritScope lock(&crit_);
     if (feedback_observer_)
-      feedback_observer_->AddPacket(sequence_number, length, probe_cluster_id);
+      feedback_observer_->AddPacket(ssrc, sequence_number, length,
+                                    probe_cluster_id);
   }
   void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override {
     RTC_DCHECK(network_thread_.CalledOnValidThread());
@@ -934,7 +942,12 @@ Channel::Channel(int32_t channelId,
       rtp_packet_sender_proxy_(new RtpPacketSenderProxy()),
       retransmission_rate_limiter_(new RateLimiter(Clock::GetRealTimeClock(),
                                                    kMaxRetransmissionWindowMs)),
-      decoder_factory_(config.acm_config.decoder_factory) {
+      decoder_factory_(config.acm_config.decoder_factory),
+      // TODO(elad.alon): Subsequent CL experiments with PLR source.
+      use_twcc_plr_for_ana_(false),

[sprang_webrtc, 2017/02/03 11:57:04]

This is always false?

[elad.alon_webrtc.org, 2017/02/03 15:54:02]

For now, yes. This is one CL in a series; one of the next ones (probably the
last one) will make this configuration experiment-driven. (Please note the TODO
above this line.)

+      packet_loss_tracker_(kPacketLossTrackerMaxWindowSize,
+                           kPacketLossTrackerMinWindowSize,
+                           kPacketLossTrackerMinPairsNumForRplr) {
   WEBRTC_TRACE(kTraceMemory, kTraceVoice, VoEId(_instanceId, _channelId),
                "Channel::Channel() - ctor");
   AudioCodingModule::Config acm_config(config.acm_config);
@@ -1347,6 +1360,8 @@ void Channel::SetBitRate(int bitrate_bps, int64_t probing_interval_ms) {
 }
 
 void Channel::OnIncomingFractionLoss(int fraction_lost) {
+  if (use_twcc_plr_for_ana_)
+    return;
   audio_coding_->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) {
     if (*encoder)
       (*encoder)->OnReceivedUplinkPacketLossFraction(fraction_lost / 255.0f);
@@ -2905,6 +2920,32 @@ void Channel::OnOverheadChanged(size_t overhead_bytes_per_packet) {
   UpdateOverheadForEncoder();
 }
 
+void Channel::HandleTransportFeedback(
+    const std::vector<uint16_t>& packets_sent_since_last_feedback,
+    const rtcp::TransportFeedback& feedback) {
+  for (uint16_t sent_sequence_number : packets_sent_since_last_feedback) {
+    packet_loss_tracker_.OnPacketAdded(sent_sequence_number);

[sprang_webrtc, 2017/02/03 11:57:04]

Will this really work?
Maybe I'm missing something, but it looks to me like you are filtering out the
transport wide sequence numbers/packed id for the packets belonging to the ssrc
associated with this channel. But they will then contain "gaps" for packets sent
on other ssrcs. Won't they then be interpreted as packet losses by the tracker?

[elad.alon_webrtc.org, 2017/02/03 15:54:02]

You've reviewed 2629883003, but skipped 2632203002 - that's what you're missing.
:-)
(Would it be helpful to give the stacked CLs sequence numbers, perhaps?)

+  }
+
+  // TODO(elad.alon): Needed even when !use_twcc_plr_for_ana_, because
+  // subsequent CLs will introduce pushing RPLR down to ANA, too, and that
+  // would happen unconditionally.
+  // (Rationale for unconditionality: it's cheap enough, and once
+  // experimentation is over, we either always do it or never do it, in which
+  // case the code involved will just be deleted.)
+  packet_loss_tracker_.OnReceivedTransportFeedback(feedback);
+
+  if (use_twcc_plr_for_ana_) {
+    rtc::Optional<float> plr = packet_loss_tracker_.GetPacketLossRate();
+    if (plr) {
+      audio_coding_->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) {
+        if (*encoder)
+          (*encoder)->OnReceivedUplinkPacketLossFraction(*plr);
+      });
+    }
+  }
+}
+
 int Channel::RegisterExternalMediaProcessing(ProcessingTypes type,
                                              VoEMediaProcess& processObject) {
   WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId),