Simplify ReceiveStatistics: merge GetActiveStatisticians into RtcpReportBlocks

BUG=webrtc:8016

Change-Id: Ie38a86b730298039915baaac12b7fd97a5440345
Reviewed-on: https://webrtc-review.googlesource.com/1842
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#19891}

modules/rtp_rtcp/include/receive_statistics.h

@@ -55,8 +55,6 @@ class StreamStatistician {
   virtual bool IsPacketInOrder(uint16_t sequence_number) const = 0;
 };
 
-typedef std::map<uint32_t, StreamStatistician*> StatisticianMap;
-
 class ReceiveStatistics : public ReceiveStatisticsProvider {
  public:
   ~ReceiveStatistics() override = default;
@@ -72,10 +70,6 @@ class ReceiveStatistics : public ReceiveStatisticsProvider {
   virtual void FecPacketReceived(const RTPHeader& header,
                                  size_t packet_length) = 0;
 
-  // Returns a map of all statisticians which have seen an incoming packet
-  // during the last two seconds.
-  virtual StatisticianMap GetActiveStatisticians() const = 0;
-
   // Returns a pointer to the statistician of an ssrc.
   virtual StreamStatistician* GetStatistician(uint32_t ssrc) const = 0;
 

modules/rtp_rtcp/source/receive_statistics_impl.cc

@@ -197,6 +197,28 @@ bool StreamStatisticianImpl::GetStatistics(RtcpStatistics* statistics,
   return true;
 }
 
+bool StreamStatisticianImpl::GetActiveStatisticsAndReset(
+    RtcpStatistics* statistics) {
+  {
+    rtc::CritScope cs(&stream_lock_);
+    if (clock_->CurrentNtpInMilliseconds() - last_receive_time_ntp_.ToMs() >=
+        kStatisticsTimeoutMs) {
+      // Not active.
+      return false;
+    }
+    if (received_seq_first_ == 0 &&
+        receive_counters_.transmitted.payload_bytes == 0) {
+      // We have not received anything.
+      return false;
+    }
+
+    *statistics = CalculateRtcpStatistics();
+  }
+
+  rtcp_callback_->StatisticsUpdated(*statistics, ssrc_);
+  return true;
+}
+
 RtcpStatistics StreamStatisticianImpl::CalculateRtcpStatistics() {
   RtcpStatistics stats;
 
@@ -296,13 +318,6 @@ uint32_t StreamStatisticianImpl::BitrateReceived() const {
   return incoming_bitrate_.Rate(clock_->TimeInMilliseconds()).value_or(0);
 }
 
-void StreamStatisticianImpl::LastReceiveTimeNtp(uint32_t* secs,
-                                                uint32_t* frac) const {
-  rtc::CritScope cs(&stream_lock_);
-  *secs = last_receive_time_ntp_.seconds();
-  *frac = last_receive_time_ntp_.fractions();
-}
-
 bool StreamStatisticianImpl::IsRetransmitOfOldPacket(
     const RTPHeader& header, int64_t min_rtt) const {
   rtc::CritScope cs(&stream_lock_);
@@ -380,7 +395,7 @@ void ReceiveStatisticsImpl::IncomingPacket(const RTPHeader& header,
   StreamStatisticianImpl* impl;
   {
     rtc::CritScope cs(&receive_statistics_lock_);
-    StatisticianImplMap::iterator it = statisticians_.find(header.ssrc);
+    auto it = statisticians_.find(header.ssrc);
     if (it != statisticians_.end()) {
       impl = it->second;
     } else {
@@ -400,7 +415,7 @@ void ReceiveStatisticsImpl::FecPacketReceived(const RTPHeader& header,
   StreamStatisticianImpl* impl;
   {
     rtc::CritScope cs(&receive_statistics_lock_);
-    StatisticianImplMap::iterator it = statisticians_.find(header.ssrc);
+    auto it = statisticians_.find(header.ssrc);
     // Ignore FEC if it is the first packet.
     if (it == statisticians_.end())
       return;
@@ -409,26 +424,10 @@ void ReceiveStatisticsImpl::FecPacketReceived(const RTPHeader& header,
   impl->FecPacketReceived(header, packet_length);
 }
 
-StatisticianMap ReceiveStatisticsImpl::GetActiveStatisticians() const {
-  StatisticianMap active_statisticians;
-  rtc::CritScope cs(&receive_statistics_lock_);
-  for (StatisticianImplMap::const_iterator it = statisticians_.begin();
-       it != statisticians_.end(); ++it) {
-    uint32_t secs;
-    uint32_t frac;
-    it->second->LastReceiveTimeNtp(&secs, &frac);
-    if (clock_->CurrentNtpInMilliseconds() -
-        Clock::NtpToMs(secs, frac) < kStatisticsTimeoutMs) {
-      active_statisticians[it->first] = it->second;
-    }
-  }
-  return active_statisticians;
-}
-
 StreamStatistician* ReceiveStatisticsImpl::GetStatistician(
     uint32_t ssrc) const {
   rtc::CritScope cs(&receive_statistics_lock_);
-  StatisticianImplMap::const_iterator it = statisticians_.find(ssrc);
+  auto it = statisticians_.find(ssrc);
   if (it == statisticians_.end())
     return NULL;
   return it->second;
@@ -437,9 +436,8 @@ StreamStatistician* ReceiveStatisticsImpl::GetStatistician(
 void ReceiveStatisticsImpl::SetMaxReorderingThreshold(
     int max_reordering_threshold) {
   rtc::CritScope cs(&receive_statistics_lock_);
-  for (StatisticianImplMap::iterator it = statisticians_.begin();
+  for (auto& statistician : statisticians_) {
-       it != statisticians_.end(); ++it) {
+    statistician.second->SetMaxReorderingThreshold(max_reordering_threshold);
-    it->second->SetMaxReorderingThreshold(max_reordering_threshold);
   }
 }
 
@@ -482,7 +480,11 @@ void ReceiveStatisticsImpl::DataCountersUpdated(const StreamDataCounters& stats,
 
 std::vector<rtcp::ReportBlock> ReceiveStatisticsImpl::RtcpReportBlocks(
     size_t max_blocks) {
-  StatisticianMap statisticians = GetActiveStatisticians();
+  std::map<uint32_t, StreamStatisticianImpl*> statisticians;
+  {
+    rtc::CritScope cs(&receive_statistics_lock_);
+    statisticians = statisticians_;
+  }
   std::vector<rtcp::ReportBlock> result;
   result.reserve(std::min(max_blocks, statisticians.size()));
   for (auto& statistician : statisticians) {
@@ -494,7 +496,7 @@ std::vector<rtcp::ReportBlock> ReceiveStatisticsImpl::RtcpReportBlocks(
 
     // Do we have receive statistics to send?
     RtcpStatistics stats;
-    if (!statistician.second->GetStatistics(&stats, true))
+    if (!statistician.second->GetActiveStatisticsAndReset(&stats))
       continue;
     result.emplace_back();
     rtcp::ReportBlock& block = result.back();

modules/rtp_rtcp/source/receive_statistics_impl.h

@@ -31,7 +31,9 @@ class StreamStatisticianImpl : public StreamStatistician {
                          StreamDataCountersCallback* rtp_callback);
   virtual ~StreamStatisticianImpl() {}
 
+  // |reset| here and in next method restarts calculation of fraction_lost stat.
   bool GetStatistics(RtcpStatistics* statistics, bool reset) override;
+  bool GetActiveStatisticsAndReset(RtcpStatistics* statistics);
   void GetDataCounters(size_t* bytes_received,
                        uint32_t* packets_received) const override;
   void GetReceiveStreamDataCounters(
@@ -46,7 +48,6 @@ class StreamStatisticianImpl : public StreamStatistician {
                       bool retransmitted);
   void FecPacketReceived(const RTPHeader& header, size_t packet_length);
   void SetMaxReorderingThreshold(int max_reordering_threshold);
-  virtual void LastReceiveTimeNtp(uint32_t* secs, uint32_t* frac) const;
 
  private:
   bool InOrderPacketInternal(uint16_t sequence_number) const;
@@ -108,7 +109,6 @@ class ReceiveStatisticsImpl : public ReceiveStatistics,
                       bool retransmitted) override;
   void FecPacketReceived(const RTPHeader& header,
                          size_t packet_length) override;
-  StatisticianMap GetActiveStatisticians() const override;
   StreamStatistician* GetStatistician(uint32_t ssrc) const override;
   void SetMaxReorderingThreshold(int max_reordering_threshold) override;
 
@@ -125,11 +125,9 @@ class ReceiveStatisticsImpl : public ReceiveStatistics,
   void DataCountersUpdated(const StreamDataCounters& counters,
                            uint32_t ssrc) override;
 
-  typedef std::map<uint32_t, StreamStatisticianImpl*> StatisticianImplMap;
-
   Clock* const clock_;
   rtc::CriticalSection receive_statistics_lock_;
-  StatisticianImplMap statisticians_;
+  std::map<uint32_t, StreamStatisticianImpl*> statisticians_;
 
   RtcpStatisticsCallback* rtcp_stats_callback_;
   StreamDataCountersCallback* rtp_stats_callback_;

modules/rtp_rtcp/source/receive_statistics_unittest.cc

@@ -71,8 +71,7 @@ TEST_F(ReceiveStatisticsTest, TwoIncomingSsrcs) {
   EXPECT_EQ(600u, bytes_received);
   EXPECT_EQ(2u, packets_received);
 
-  StatisticianMap statisticians = receive_statistics_->GetActiveStatisticians();
+  EXPECT_EQ(2u, receive_statistics_->RtcpReportBlocks(3).size());
-  EXPECT_EQ(2u, statisticians.size());
   // Add more incoming packets and verify that they are registered in both
   // access methods.
   receive_statistics_->IncomingPacket(header1_, kPacketSize1, false);
@@ -80,13 +79,6 @@ TEST_F(ReceiveStatisticsTest, TwoIncomingSsrcs) {
   receive_statistics_->IncomingPacket(header2_, kPacketSize2, false);
   ++header2_.sequenceNumber;
 
-  statisticians[kSsrc1]->GetDataCounters(&bytes_received, &packets_received);
-  EXPECT_EQ(300u, bytes_received);
-  EXPECT_EQ(3u, packets_received);
-  statisticians[kSsrc2]->GetDataCounters(&bytes_received, &packets_received);
-  EXPECT_EQ(900u, bytes_received);
-  EXPECT_EQ(3u, packets_received);
-
   receive_statistics_->GetStatistician(kSsrc1)->GetDataCounters(
       &bytes_received, &packets_received);
   EXPECT_EQ(300u, bytes_received);
@@ -103,26 +95,22 @@ TEST_F(ReceiveStatisticsTest, ActiveStatisticians) {
   clock_.AdvanceTimeMilliseconds(1000);
   receive_statistics_->IncomingPacket(header2_, kPacketSize2, false);
   ++header2_.sequenceNumber;
-  StatisticianMap statisticians = receive_statistics_->GetActiveStatisticians();
   // Nothing should time out since only 1000 ms has passed since the first
   // packet came in.
-  EXPECT_EQ(2u, statisticians.size());
+  EXPECT_EQ(2u, receive_statistics_->RtcpReportBlocks(3).size());
 
   clock_.AdvanceTimeMilliseconds(7000);
   // kSsrc1 should have timed out.
-  statisticians = receive_statistics_->GetActiveStatisticians();
+  EXPECT_EQ(1u, receive_statistics_->RtcpReportBlocks(3).size());
-  EXPECT_EQ(1u, statisticians.size());
 
   clock_.AdvanceTimeMilliseconds(1000);
   // kSsrc2 should have timed out.
-  statisticians = receive_statistics_->GetActiveStatisticians();
+  EXPECT_EQ(0u, receive_statistics_->RtcpReportBlocks(3).size());
-  EXPECT_EQ(0u, statisticians.size());
 
   receive_statistics_->IncomingPacket(header1_, kPacketSize1, false);
   ++header1_.sequenceNumber;
   // kSsrc1 should be active again and the data counters should have survived.
-  statisticians = receive_statistics_->GetActiveStatisticians();
+  EXPECT_EQ(1u, receive_statistics_->RtcpReportBlocks(3).size());
-  EXPECT_EQ(1u, statisticians.size());
   StreamStatistician* statistician =
       receive_statistics_->GetStatistician(kSsrc1);
   ASSERT_TRUE(statistician != NULL);