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 6364202aa65ccc573acbb76e29034e813d5e32a3..a146ceb8e977bc0d8f302dd665d2e46dff669f43 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;

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

In this context, the abbreviations PLR and RPLR will be difficult to understand.
Maybe it is better to type the words out?

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

Done.

+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()),

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

Remove.

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

Done.

+      worker_queue_(worker_queue),
       config_(config),
       audio_state_(audio_state),
       bitrate_allocator_(bitrate_allocator),
-      send_side_cc_(send_side_cc) {
+      send_side_cc_(send_side_cc),
+      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);
+  send_side_cc_->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();
+  send_side_cc_->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) {

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

Add thread checkers to document on which thread(s) this may be called.

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

Done.

+  // 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

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

Which following issues?

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

The one immediately after the hyphen. Minyue has asked me to use the imperative
mood when formulating TODOs. (The "this" refers to the function call right above
the TODO. Moved it to before and reworded a bit now.)

[minyue-webrtc, 2017/03/22 16:27:40]

The work "following" makes people think an issue outside the TODO. So

Take care of the issue that, after OnPacketAdded() xxx can become unknown, but
currently such information is not passed down.

[elad.alon_webrtc.org, 2017/03/22 16:34:19]

I think the wording of the latest patchset (PS24) is clear enough. I'll keep
that, unless someone has a strong objection.

[the sun, 2017/03/23 11:11:14]

The wording has not changed in the sense that the use of hyphen is still
confusing. In fact, you could just remove the "Take care of the following known
issue" part - TODO() already means that. Same goes below ("Resolve the following
known issue").

[elad.alon_webrtc.org, 2017/03/23 13:37:30]

Minyue, I'm going to take up this suggestion, as it fits my personal preference.
I don't think TODOs have to be formulated in the imperative. The presence of a
TODO, followed by a description of a problem, implicitly indicates the problem
is supposed to be taken care of at a later time.

[minyue-webrtc, 2017/03/23 13:57:16]

You are to make it
// TODO(elad.alon): This could potentially reset the window, setting both PLR
and RPLR to unknown.
?
This is a fact, I don't know what the action to it will be. You may need to add
"Consider signal such an event to lower layer."

[elad.alon_webrtc.org, 2017/03/23 18:04:23]

OK

+    // potentially reset the window, setting both PLR and RPLR to unknown.
+  }
+}
+
+void AudioSendStream::OnNewTransportFeedbackVector(
+    const std::vector<PacketFeedback>& packet_feedback_vector) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  rtc::CritScope lock(&packet_loss_tracker_cs_);

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

packet_loss_tracker_ and channel_proxy_ are both set up in ctor and never
changed during the lifetime of the stream. So, what are you protecting with this
lock? Internals in the PLT (potential race between
OnNewTransportFeedbackVector() and GetPacketLossRate(), or some subtle
dependency between the PLT and the voe::Channel?

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

PLT internals. OnPacketAdded() and OnNewTransportFeedbackVector() are called on
separate threads. I'll move the channel_proxy_ out of the critical section,
though.

[the sun, 2017/03/23 11:11:14]

This one is interesting and tricky.

On one hand I'd argue that since the shared resource lives in the PLT, that's
where the lock should be. We cannot know at this level what the PLT functions
may access. Another argument for keeping the lock in the PLT is to grab it for
the shortest time possible, and only in the PLT do we know exactly what resource
is accessed from multiple threads. We'd then also need to add thread checkers in
the PLT (which may be a good idea anyway, given the ambiguous threading
situation).

On the other hand, I could also argue the same way it appears that you have: let
the PLT make no promises about thread safety and handle that in the client. This
is appealing because why should we impose a threading model on a class
unnecessarily? But, IIUC OnNewTransportFeedbackVector() and OnPacketAdded() are
only called from SendSideCongestionController, so by the same reasoning, why
should SSCC assume that the TransportFeedbackAdapterObserver is anything but
single threaded? So then the lock should be in SSCC.

In other words, it seems AudioSendStream is not the right place for this lock.
Either you should add it in the caller, SSCC, or you should add a lock to PLT
(which is the approach most WebRTC code takes).

[elad.alon_webrtc.org, 2017/03/23 13:37:30]

One additional thing to consider is that the CL immediately on top of this one
introduces a read of the RPLR. The two metrics are read using separate
functions, which I think is more appropriate than reading both metrics at the
same time. 

By placing the lock here, we reap two rewards:
1. Lesser reward - only need to acquire the lock a single time.
2. Greater reward - the state is guaranteed to not change between reading the
PLR and RPLR, so someone who looks at their relation could not get a distorted
picture.

You might argue that this is solvable using an API that reads both at the same
time, but something about that feels a bit off to me. Wdyt, though?

[minyue-webrtc, 2017/03/23 13:57:16]

The "greater reward" isn't big. It is not a big deal if they are queried at
slightly different time. (if they are to be compared in a stricter sense, it
might be an issue, but not the case now).
I also like letting PLT to handle data races.

+  packet_loss_tracker_.OnNewTransportFeedbackVector(packet_feedback_vector);
+  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 that the previously sent value is no
+  // longer relevant. 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_;