Attach TransportFeedbackPacketLossTracker to ANA (PLR only)

webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 293 matching lines @@
   int64_t send_time_ms;
   // Packet identifier, incremented with 1 for every packet generated by the
   // sender.
   uint16_t sequence_number;
   // Size of the packet excluding RTP headers.
   size_t payload_size;
   // Pacing information about this packet.
   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;
 
   virtual void OnTransportFeedback(const rtcp::TransportFeedback& feedback) = 0;
 
   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;
 
   virtual int64_t LastProcessedRtt() const = 0;
 
   virtual ~RtcpRttStats() {}
 };
 
 // Null object version of RtpFeedback.
@@ skipping 68 matching lines @@
 class TransportSequenceNumberAllocator {
  public:
   TransportSequenceNumberAllocator() {}
   virtual ~TransportSequenceNumberAllocator() {}
 
   virtual uint16_t AllocateSequenceNumber() = 0;
 };
 
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_RTP_RTCP_INCLUDE_RTP_RTCP_DEFINES_H_