Attach TransportFeedbackPacketLossTracker to ANA (PLR only)

webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h

Index: webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h
diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h
index 49bdf55479e13904c718110dae490572b2dbbe42..67338d0936b739a8bb4c39c8c74eb0eea50ad6b2 100644
--- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h
+++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h
@@ -311,13 +311,32 @@ struct PacketFeedback {
   PacedPacketInfo pacing_info;
 };
 
+// Records when a packet was sent, according to its transport sequence number.
+struct SentTransportPacketRecord {

[minyue-webrtc, 2017/03/15 10:54:13]

I don't see the need of this struct, because first of all, the buffering of
SentTransportPacketRecord isn't necessary

[elad.alon_webrtc.org, 2017/03/16 18:37:35]

This was necessary when the design was to keep the tracker ownership in Channel.
Now that we've redesigned, it's no longer necessary.

+  uint16_t sequence_number;
+  int64_t sent_time_ms;
+};
+
+// TODO(elad.alon): The only implementers of this interface are

[minyue-webrtc, 2017/03/15 10:54:13]

I think this comment should be placed above TransportFeedbackAdapterObserver,
and reword it a bit, starting with

"TODO(elad.alon): Merge TransportFeedbackAdapterObserver into
TransportFeedbackObserver when xxx." add additional info after this summary
sentence.

[elad.alon_webrtc.org, 2017/03/16 18:37:35]

1. I've reworded to begin with the action, then provide the background to it.
2. I feel it's weird that a comment that talks a lot about
TransportFeedbackObserver is placed above TransportFeedbackAdapterObserver
instead. Also, that a TODO whose resolution would be triggered by the deletion
of TransportFeedbackObserver is placed elsewhere, where a person removing
TransportFeedbackObserver would not see it. Please reconsider.

[minyue-webrtc, 2017/03/17 09:20:31]

Not a big deal. And it will be more weird to talk about
TransportFeedbackAdapterObserver before it is defined.

Also, that a TODO whose resolution would be triggered by the deletion
Is TransportFeedbackAdapterObserver or TransportFeedbackObserver to be removed?
I am confused. 

You wrote "Then, we'll remove TransportFeedbackAdapterObserver". So the action
is "to remove TransportFeedbackAdapterObserver", isn't it? And therefore, I
suggested putting the comment next to TransportFeedbackAdapterObserver.
Otherwise, the comment can be forgotten when you remove
TransportFeedbackAdapterObserver.

[elad.alon_webrtc.org, 2017/03/17 10:10:32]

I think I see the source of the misunderstanding. There are two observers - the
one *currently* called "TransportFeedbackObserver" (let's call it O1) and the
one *currently* called "TransportFeedbackAdapterObserver" (let's call it O2). As
per Stefan, plans are in motion that will remove the necessity for O1. At that
point, O1 will be removed, and O2 can assume its name -
TransportFeedbackObserver. Therefore, I've placed the TODO next to O1, so that
whoever removes O1 either resolves the TODO or alerts me about it. Given that,
wdyt?

+// CongestionController and TransportFeedbackProxy.
+// CongestionController uses it to expose only a small subset of its
+// capabilities.
+// TransportFeedbackProxy is a temporary solution which exists to allow passing
+// the CongestionController down at a later stage.
+// Ideally, we'd get rid of this altogether, exposing CC. This will be done
+// after the order-of-creation issues have been resolved, and
+// TransportFeedbackProxy therefore removed. Then, we'll remove
+// TransportFeedbackAdapterObserver, reuse the name
+// TransportFeedbackObserver for TransportFeedbackAdapterObserver, and only
+// have one observer.
 class TransportFeedbackObserver {
  public:
   TransportFeedbackObserver() {}
   virtual ~TransportFeedbackObserver() {}
 
   // Note: Transport-wide sequence number as sequence number.
-  virtual void AddPacket(uint16_t sequence_number,
+  virtual void AddPacket(uint32_t ssrc,
+                         uint16_t sequence_number,
                          size_t length,
                          const PacedPacketInfo& pacing_info) = 0;
 
@@ -326,6 +345,16 @@ class TransportFeedbackObserver {
   virtual std::vector<PacketFeedback> GetTransportFeedbackVector() const = 0;
 };
 
+// See TODO above TransportFeedbackObserver for explanation of the difference
+// between these two.
+class TransportFeedbackAdapterObserver {
+ public:
+  virtual ~TransportFeedbackAdapterObserver() = default;
+
+  virtual void OnPacketAdded(uint32_t ssrc, uint16_t seq_num) = 0;
+  virtual void OnTransportFeedback(const rtcp::TransportFeedback& feedback) = 0;
+};
+
 class RtcpRttStats {
  public:
   virtual void OnRttUpdate(int64_t rtt) = 0;