Attach TransportFeedbackPacketLossTracker to ANA (PLR only)

webrtc/audio/audio_send_stream.cc

Index: webrtc/audio/audio_send_stream.cc
diff --git a/webrtc/audio/audio_send_stream.cc b/webrtc/audio/audio_send_stream.cc
index 438d1cc78a5aca5d7657b6368bfbac03fa5aed8e..20132bbfdfe6b47fdc4af11f35f625f3529d1ad2 100644
--- a/webrtc/audio/audio_send_stream.cc
+++ b/webrtc/audio/audio_send_stream.cc
@@ -40,6 +40,11 @@ bool IsCodec(const webrtc::CodecInst& codec, const char* ref_name) {
 }  // namespace
 
 namespace internal {
+// TODO(elad.alon): Subsequent CL will make these values experiment-dependent.
+constexpr size_t kPacketLossTrackerMaxWindowSizeMs = 15000;
+constexpr size_t kPlrMinNumAckedPackets = 50;
+constexpr size_t kRplrMinNumAckedPairs = 40;
+
 AudioSendStream::AudioSendStream(
     const webrtc::AudioSendStream::Config& config,
     const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
@@ -49,11 +54,15 @@ AudioSendStream::AudioSendStream(
     BitrateAllocator* bitrate_allocator,
     RtcEventLog* event_log,
     RtcpRttStats* rtcp_rtt_stats)
-    : worker_queue_(worker_queue),
+    : clock_(Clock::GetRealTimeClock()),

[michaelt, 2017/03/21 10:53:26]

I think the code would be better testable when we inject the clock rather than
construct it here.

[elad.alon_webrtc.org, 2017/03/21 17:23:14]

It's equally testable both ways; please see how the UT is providing the
timestamps. Otherwise, I'd have had to use a SimulatedClock that I'd have to
manually increment in the UT, achieving the same effect. I have no significant
preference, but I lean towards keep it as is, thereby minimizing changes.
Minyue?

[minyue-webrtc, 2017/03/22 07:51:39]

I think it is fine to keep it as it is, since we cannot test how clock interacts
with packet_loss_tracker_ here (otherwise, packet_loss_tracker_ needs to be
injected too), and the logic between clock and packet_loss_tracker_ is simple.

But Elad, what UT do you talk about in your comment. I see no new 
audio_send_stream_unittest, and think you meant packet_loss_tracker_unittests. 

[elad.alon_webrtc.org, 2017/03/22 09:36:30]

Yes, that's what I meant. I thought Michael was talking about
packet_loss_tracker_unittests. If he's talking about audio_send_stream_unittest,
then I agree with him in principle, but since the clock_ is not really used for
anything except for the tracker, and the tracker would soon move to the encoder,
I think it's simpler to simply construct (as you've said).

[michaelt, 2017/03/23 09:57:45]

I was talking about audio_send_stream_unittest :).
I'm ok with not testing the functionality in audio_send_stream if we move the
tracker later.

But as "the sun" said we should remove "clock_" anyway an replace it by the new
time interface "webrtc/base/timeutils.h"

[elad.alon_webrtc.org, 2017/03/23 10:00:15]

Done in latest patchset.

+      worker_queue_(worker_queue),
       config_(config),
       audio_state_(audio_state),
       bitrate_allocator_(bitrate_allocator),
-      congestion_controller_(congestion_controller) {
+      congestion_controller_(congestion_controller),
+      packet_loss_tracker_(kPacketLossTrackerMaxWindowSizeMs,
+                           kPlrMinNumAckedPackets,
+                           kRplrMinNumAckedPairs) {
   LOG(LS_INFO) << "AudioSendStream: " << config_.ToString();
   RTC_DCHECK_NE(config_.voe_channel_id, -1);
   RTC_DCHECK(audio_state_.get());
@@ -72,6 +81,7 @@ AudioSendStream::AudioSendStream(
                                 config_.rtp.nack.rtp_history_ms / 20);
 
   channel_proxy_->RegisterExternalTransport(config.send_transport);
+  congestion_controller_->RegisterTransportFeedbackAdapterObserver(this);
 
   for (const auto& extension : config.rtp.extensions) {
     if (extension.uri == RtpExtension::kAudioLevelUri) {
@@ -96,6 +106,7 @@ AudioSendStream::AudioSendStream(
 AudioSendStream::~AudioSendStream() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "~AudioSendStream: " << config_.ToString();
+  congestion_controller_->DeRegisterTransportFeedbackAdapterObserver(this);
   channel_proxy_->DeRegisterExternalTransport();
   channel_proxy_->ResetCongestionControlObjects();
   channel_proxy_->SetRtcEventLog(nullptr);
@@ -247,6 +258,31 @@ uint32_t AudioSendStream::OnBitrateUpdated(uint32_t bitrate_bps,
   return 0;
 }
 
+void AudioSendStream::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) {

[michaelt, 2017/03/21 10:53:26]

Can you add some unit test for these functions ?

[elad.alon_webrtc.org, 2017/03/21 17:23:14]

This is just piping. Also, keep in mind that we plan to move it out of here
soon, so I think creating a new UT for this piping, then removing it, is
unnecessary.

[minyue-webrtc, 2017/03/22 07:51:39]

I agree to omit a test in AudioSendStream.

+  // Only packets that belong to this stream are of interest.
+  if (ssrc == config_.rtp.ssrc) {
+    rtc::CritScope lock(&packet_loss_tracker_cs_);
+    packet_loss_tracker_.OnPacketAdded(seq_num, clock_->TimeInMilliseconds());
+    // TODO(elad.alon): Take care of the following known issue - this could
+    // potentially reset the window, sending both PLR and RPLR to UNKNOWN.
+  }
+}
+
+void AudioSendStream::OnNewTransportFeedbacks(
+    const std::vector<PacketFeedback>& packet_feedbacks) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  rtc::CritScope lock(&packet_loss_tracker_cs_);
+  packet_loss_tracker_.OnPacketFeedbacks(packet_feedbacks);
+  const auto plr = packet_loss_tracker_.GetPacketLossRate();
+  // TODO(elad.alon): Resolve the following known issue - if PLR goes back
+  // to unknown, no indication is given, which leads the lower layers to think
+  // that the old value is still correct. This will be taken care of with some
+  // refactoring which is now being done.
+  if (plr) {
+    channel_proxy_->OnTwccBasedUplinkPacketLossRate(*plr);
+  }
+}
+
 const webrtc::AudioSendStream::Config& AudioSendStream::config() const {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   return config_;