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 0f0831c8b1f8cbd737a7b919e4a277b04cb87ff1..091ff6c0f3c691cb587daf616a8379fefda55164 100644
--- a/webrtc/voice_engine/channel.cc
+++ b/webrtc/voice_engine/channel.cc
@@ -246,13 +246,14 @@ class TransportFeedbackProxy : public TransportFeedbackObserver {
   }
 
   // Implements TransportFeedbackObserver.
-  void AddPacket(uint16_t sequence_number,
+  void AddPacket(uint32_t ssrc,
+                 uint16_t sequence_number,
                  size_t length,
                  const PacedPacketInfo& pacing_info) override {
     RTC_DCHECK(pacer_thread_.CalledOnValidThread());
     rtc::CritScope lock(&crit_);
     if (feedback_observer_)
-      feedback_observer_->AddPacket(sequence_number, length, pacing_info);
+      feedback_observer_->AddPacket(ssrc, sequence_number, length, pacing_info);
   }
 
   void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override {
@@ -395,7 +396,7 @@ class VoERtcpObserver : public RtcpBandwidthObserver {
           (fraction_lost_aggregate + total_number_of_packets / 2) /
           total_number_of_packets;
     }
-    owner_->OnIncomingFractionLoss(weighted_fraction_lost);
+    owner_->OnUplinkPacketLossRate(weighted_fraction_lost / 255.0f);

[stefan-webrtc, 2017/03/22 12:03:01]

Is this a bug fix? Should be mentioned somewhere in that case.

[elad.alon_webrtc.org, 2017/03/22 14:34:47]

It's not. I'm just moving the place where it divides by 255.0f, to make things
clearer. Otherwise, OnTwccBasedUplinkPacketLossRate() and
OnUplinkPacketLossRate() get different inputs, which is confusing.

   }
 
  private:
@@ -902,7 +903,9 @@ 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) {

[the sun, 2017/03/22 12:06:42]

Ok, but does it really need to be a class member? Will the experiment affect
separate channel instances differently?

[elad.alon_webrtc.org, 2017/03/22 14:34:47]

If it ends up populated from a FieldTrial - please see
https://codereview.webrtc.org/2684773002. However, one does not want to call
webrtc::field_trial::FindFullName() multiple times, as it involves
string-comparison, and is thus wasteful. The alternative of a global is, IMHO,
inferior to a member variable. It's true that multiple instances would mean
memory gets allocated for this multiple times, but if we go down to that
resolution, I'd argue that cache-locality would be better with a member than
with a global.

   WEBRTC_TRACE(kTraceMemory, kTraceVoice, VoEId(_instanceId, _channelId),
                "Channel::Channel() - ctor");
   AudioCodingModule::Config acm_config(config.acm_config);
@@ -1301,10 +1304,22 @@ void Channel::SetBitRate(int bitrate_bps, int64_t probing_interval_ms) {
   retransmission_rate_limiter_->SetMaxRate(bitrate_bps);
 }
 
-void Channel::OnIncomingFractionLoss(int fraction_lost) {
+void Channel::OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) {

[the sun, 2017/03/22 12:06:42]

ossu@ - should this be done differently, considering your upcoming changes?
Would that affect the relationships in this CL in any way?

[ossu, 2017/03/22 14:05:16]

Thanks for the heads-up!

We're still handing the encoder over to the ACM, so this should continue to
work. Eventually, we'll want to move stuff from Channel and possibly have the
encoder listen directly to this kind of feedback, rather than having to plumb it
manually. That's outside the scope of this and of my current CLs, though.

+  if (!use_twcc_plr_for_ana_)
+    return;
   audio_coding_->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) {
     if (*encoder)
-      (*encoder)->OnReceivedUplinkPacketLossFraction(fraction_lost / 255.0f);
+      (*encoder)->OnReceivedUplinkPacketLossFraction(packet_loss_rate);
+  });
+}
+
+void Channel::OnUplinkPacketLossRate(float packet_loss_rate) {
+  if (use_twcc_plr_for_ana_)
+    return;
+  audio_coding_->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) {
+    if (*encoder) {
+      (*encoder)->OnReceivedUplinkPacketLossFraction(packet_loss_rate);
+    }
   });
 }