From c5267d251a99a047a32dcc8d00c8bc2e39cfc735 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 18 Sep 2017 13:57:19 +0200 Subject: [PATCH] Simplify ReceiveStatistics: merge GetActiveStatisticians into RtcpReportBlocks BUG=webrtc:8016 Change-Id: Ie38a86b730298039915baaac12b7fd97a5440345 Reviewed-on: https://webrtc-review.googlesource.com/1842 Commit-Queue: Danil Chapovalov Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#19891} --- modules/rtp_rtcp/include/receive_statistics.h | 6 -- .../source/receive_statistics_impl.cc | 64 ++++++++++--------- .../rtp_rtcp/source/receive_statistics_impl.h | 8 +-- .../source/receive_statistics_unittest.cc | 22 ++----- 4 files changed, 41 insertions(+), 59 deletions(-) diff --git a/modules/rtp_rtcp/include/receive_statistics.h b/modules/rtp_rtcp/include/receive_statistics.h index f707e56d29..ee7dbbbd8d 100644 --- a/modules/rtp_rtcp/include/receive_statistics.h +++ b/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 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; diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index f1db54719b..7f3d5bb1eb 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/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(); - it != statisticians_.end(); ++it) { - it->second->SetMaxReorderingThreshold(max_reordering_threshold); + for (auto& statistician : statisticians_) { + statistician.second->SetMaxReorderingThreshold(max_reordering_threshold); } } @@ -482,7 +480,11 @@ void ReceiveStatisticsImpl::DataCountersUpdated(const StreamDataCounters& stats, std::vector ReceiveStatisticsImpl::RtcpReportBlocks( size_t max_blocks) { - StatisticianMap statisticians = GetActiveStatisticians(); + std::map statisticians; + { + rtc::CritScope cs(&receive_statistics_lock_); + statisticians = statisticians_; + } std::vector result; result.reserve(std::min(max_blocks, statisticians.size())); for (auto& statistician : statisticians) { @@ -494,7 +496,7 @@ std::vector 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(); diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index 55110eac62..dd17d77178 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/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 StatisticianImplMap; - Clock* const clock_; rtc::CriticalSection receive_statistics_lock_; - StatisticianImplMap statisticians_; + std::map statisticians_; RtcpStatisticsCallback* rtcp_stats_callback_; StreamDataCountersCallback* rtp_stats_callback_; diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc index 283f51b1ec..a832383310 100644 --- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/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, statisticians.size()); + EXPECT_EQ(2u, receive_statistics_->RtcpReportBlocks(3).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, statisticians.size()); + EXPECT_EQ(1u, receive_statistics_->RtcpReportBlocks(3).size()); clock_.AdvanceTimeMilliseconds(1000); // kSsrc2 should have timed out. - statisticians = receive_statistics_->GetActiveStatisticians(); - EXPECT_EQ(0u, statisticians.size()); + EXPECT_EQ(0u, receive_statistics_->RtcpReportBlocks(3).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, statisticians.size()); + EXPECT_EQ(1u, receive_statistics_->RtcpReportBlocks(3).size()); StreamStatistician* statistician = receive_statistics_->GetStatistician(kSsrc1); ASSERT_TRUE(statistician != NULL);