Implemented the GetSources() in native code.

webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc
index 79e43ef073536d17e58d6f7b8200e0a7d79be743..e9d4a28ca72fe3a1a4bed6b6d3712ffa431a3bb0 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc
@@ -15,6 +15,9 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include <set>
+#include <vector>
+
 #include "webrtc/base/logging.h"
 #include "webrtc/common_types.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h"
@@ -23,6 +26,12 @@
 
 namespace webrtc {
 
+// Only return the sources in the last 10 seconds.
+static const int64_t kGetSourcesTimeoutMs = 10000;

[danilchap, 2017/04/05 16:24:21]

constants outside class doesn't have to be marked static.
+nit: put them right after "using RtpUtility::Payload"

[Zhi Huang, 2017/04/06 03:09:49]

Done.

[the sun, 2017/04/06 06:55:37]

They should still be made private to the translation unit from the linker's
point of view, otherwise crazy things can happen if a constant/function/class
with the same name is defined elsewhere. Anonymous namespaces are preferred for
this.

+
+// The maximum size of the sources lists.

[the sun, 2017/04/05 14:58:30]

nit: "The maximum combined size"...

[Zhi Huang, 2017/04/06 03:09:50]

Done.

+static const int kMaxSourceListsSize = 100;
+
 using RtpUtility::Payload;
 
 RtpReceiver* RtpReceiver::CreateVideoReceiver(
@@ -53,11 +62,10 @@ RtpReceiver* RtpReceiver::CreateAudioReceiver(
       RTPReceiverStrategy::CreateAudioStrategy(incoming_payload_callback));
 }
 
-RtpReceiverImpl::RtpReceiverImpl(
-    Clock* clock,
-    RtpFeedback* incoming_messages_callback,
-    RTPPayloadRegistry* rtp_payload_registry,
-    RTPReceiverStrategy* rtp_media_receiver)
+RtpReceiverImpl::RtpReceiverImpl(Clock* clock,
+                                 RtpFeedback* incoming_messages_callback,
+                                 RTPPayloadRegistry* rtp_payload_registry,
+                                 RTPReceiverStrategy* rtp_media_receiver)
     : clock_(clock),
       rtp_payload_registry_(rtp_payload_registry),
       rtp_media_receiver_(rtp_media_receiver),
@@ -160,6 +168,8 @@ bool RtpReceiverImpl::IncomingRtpPacket(
   webrtc_rtp_header.header = rtp_header;
   CheckCSRC(webrtc_rtp_header);
 
+  UpdateSources();
+
   size_t payload_data_length = payload_length - rtp_header.paddingLength;
 
   bool is_first_packet_in_frame = false;
@@ -203,6 +213,37 @@ TelephoneEventHandler* RtpReceiverImpl::GetTelephoneEventHandler() {
   return rtp_media_receiver_->GetTelephoneEventHandler();
 }
 
+const std::vector<RtpSource> RtpReceiverImpl::GetSources() {
+  int64_t now = clock_->TimeInMilliseconds();
+  std::vector<RtpSource> sources;
+
+  {
+    rtc::CritScope lock(&critical_section_rtp_receiver_);
+

[the sun, 2017/04/05 14:58:30]

Since we assume that the lists are sorted in the below loops, we should add a
DCHECK that they are, e.g.:

RTC_DCHECK(std::is_sorted(ssrc_sources_.begin(), ssrc_sources_.end(),
    [](const RtpSource& lhs, const RtpSource& rhs) { return lhs.timestamp() <
rhs.timestamp(); });
RTC_DCHECK(std::is_sorted(csrc_sources_.begin(), csrc_sources_.end(),
    [](const RtpSource& lhs, const RtpSource& rhs) { return lhs.timestamp() <
rhs.timestamp(); });

http://www.cplusplus.com/reference/algorithm/is_sorted/

[Zhi Huang, 2017/04/06 03:09:49]

Done.

+    std::set<uint32_t> selected_ssrcs;
+    for (auto rit = ssrc_sources_.rbegin(); rit != ssrc_sources_.rend();
+         ++rit) {
+      if (now - rit->timestamp() > kGetSourcesTimeoutMs) {

[the sun, 2017/04/05 14:58:30]

nit: () around the subtraction, here and below

[Zhi Huang, 2017/04/06 03:09:50]

Done.

+        break;
+      }
+      if (selected_ssrcs.find(rit->source_id()) == selected_ssrcs.end()) {

[danilchap, 2017/04/05 16:24:21]

to avoid double lookup you can use return value of the insert:
if (selected_ssrcs.insert(rit->source_id()).second) {
  sources.push_back(*rit);
}

[Zhi Huang, 2017/04/06 03:09:50]

Done.

+        selected_ssrcs.insert(rit->source_id());
+        sources.push_back(*rit);
+      }
+    }
+
+    for (auto rit = csrc_sources_.rbegin(); rit != csrc_sources_.rend();
+         ++rit) {
+      if (now - rit->timestamp() > kGetSourcesTimeoutMs) {
+        break;
+      }
+      sources.push_back(*rit);
+    }
+  }  // End critsect.
+
+  return sources;
+}
+
 bool RtpReceiverImpl::Timestamp(uint32_t* timestamp) const {
   rtc::CritScope lock(&critical_section_rtp_receiver_);
   if (!HaveReceivedFrame())
@@ -461,4 +502,56 @@ void RtpReceiverImpl::CheckCSRC(const WebRtcRTPHeader& rtp_header) {
   }
 }
 
+void RtpReceiverImpl::UpdateSources() {
+  rtc::CritScope lock(&critical_section_rtp_receiver_);
+  int64_t now = clock_->TimeInMilliseconds();
+
+  for (size_t i = 0; i < num_csrcs_; ++i) {
+    auto map_it = iterator_by_csrc_.find(current_remote_csrc_[i]);
+    if (map_it == iterator_by_csrc_.end()) {
+      // If it is a new CSRC, append a new object to the end of the list.
+      csrc_sources_.emplace_back(now, current_remote_csrc_[i],
+                                 RtpSourceType::RTP_CSRC_SOURCE);
+    } else {
+      // If it is an existing CSRC, move the object to the end of the list.
+      map_it->second->update_timestamp(now);
+      csrc_sources_.splice(csrc_sources_.end(), csrc_sources_, map_it->second);
+    }
+    // Update the unordered_map.
+    iterator_by_csrc_[current_remote_csrc_[i]] = std::prev(csrc_sources_.end());
+
+    // Remove the out of date objects if the lists are too large.

[hbos, 2017/04/05 11:15:41]

nit: I think this bit should be moved to the end of UpdateSources(), done once
and after |ssrc_sources_| is updated. Otherwise we could theoretically do it
|num_csrcs_| times, and UpdateSources() iterates all of |csrc_sources_|.

[the sun, 2017/04/05 14:58:29]

Good catch!

[Zhi Huang, 2017/04/06 03:09:49]

Done.

+    if ((csrc_sources_.size() + ssrc_sources_.size()) > kMaxSourceListsSize) {
+      UpdateSourceLists(now);
+    }
+  }
+
+  // If this is the first packet or the SSRC is changed, insert a new
+  // contributing source that uses the SSRC.
+  if (ssrc_sources_.size() == 0 ||
+      std::prev(ssrc_sources_.end())->source_id() != ssrc_) {
+    ssrc_sources_.emplace_back(now, ssrc_, RtpSourceType::RTP_SSRC_SOURCE);
+  } else {
+    std::prev(ssrc_sources_.end())->update_timestamp(now);
+  }
+}
+
+// Update the lists and remove the out of date objects.

[the sun, 2017/04/05 14:58:29]

With a different function name you wouldn't need the explaining comment, e.g.:
LimitSourceListSizes()

[Zhi Huang, 2017/04/06 03:09:50]

I'll remove the length limitation and just call this RemoveOutdatedSources().

[the sun, 2017/04/06 06:55:38]

Much better than my suggestion!

+void RtpReceiverImpl::UpdateSourceLists(int64_t now) {

[hbos, 2017/04/05 11:15:41]

We only throw away outdated objects. Max object count is never enforced. I think
this makes kMaxSourceListSize misleading. I think we should always update to
remove outdates objects. Because the lists are sorted, if the first element is
not outdated we can stop looking, so it is efficient not to have a
kMaxSourceListSize.

If we want to have a max source size for security reasons then we should really
enforce the list size, and throw away even up-to-date objects that exceed the
limit.

[the sun, 2017/04/05 14:58:29]

+1

[Zhi Huang, 2017/04/06 03:09:50]

I added this because I thought the idea is to make the per packet operation as
simple as possible. But I don't like kMaxSourceListSize either and I can't
justify why it is 100. I'll remove it.

+  for (auto it = csrc_sources_.begin(); it != csrc_sources_.end();) {
+    if (now - it->timestamp() <= kGetSourcesTimeoutMs) {
+      break;
+    }
+    iterator_by_csrc_.erase(it->source_id());
+    it = csrc_sources_.erase(it);

[Taylor Brandstetter, 2017/04/05 04:27:55]

extreme nit: I think this could just do "++it" here, and then outside the loop,
remove everything up to "it" with:

csrc_sources_.erase(csrc_soruces_.begin(), it);

Which could have a slight performance benefit?

[the sun, 2017/04/05 14:58:29]

Possibly, but likely not since it's a std::list. Would be consistent with the
below loop though so I think it would still make sense.

[Zhi Huang, 2017/04/06 03:09:50]

Done.

+  }
+
+  for (auto it = ssrc_sources_.begin(); it != ssrc_sources_.end(); ++it) {
+    if (now - it->timestamp() <= kGetSourcesTimeoutMs) {
+      ssrc_sources_.erase(ssrc_sources_.begin(), it);
+      break;
+    }

[hbos, 2017/04/05 11:15:41]

Because of the break this will at most remove one ssrc. If multiple packets
arrive the same time, ssrc_sources_ could contain outdated objects. It should
loop for as long as !empty && first element is outdated.

Since we're doing removal of the first element, how about using list instead of
vector?

[the sun, 2017/04/05 14:58:30]

The idea is to remove all old elements in one go. Missing test?

[Zhi Huang, 2017/04/06 03:09:49]

I don't think it will only remove one ssrc.

The iterator keeps moving until it finds the first one that is not out of date
(goes into the if condition), remove all the objects before it and then break.
There is at least one source that is not outdated, because this method is only
called when a new packet is received.

It seems that the difference is "erase then break" and "break then erase".

[Zhi Huang, 2017/04/06 03:09:50]

I've added a test for this. 
I agree with Taylor (his comments in the unit tests) that there is no clear way
to do black box test. So I exposed the crsc_sources_ and the ssrc_sources_ in
the RtpReceiverImpl and downcasted the RtpReceiver in the test. Is this
acceptable?

[the sun, 2017/04/06 06:55:37]

True, both works.

[the sun, 2017/04/06 06:55:38]

Yes, it's a good idea though to mark such functions in some way so their purpose
is clear to the casual reader.

+  }

[the sun, 2017/04/05 14:58:29]

The erase should go here, after searching for the first item to remain in the
list.

[Zhi Huang, 2017/04/06 03:09:50]

Alright. This looks more clear.

+}
+
 }  // namespace webrtc