From e2d83d6560272ee68cf99c4fd4f78a437adeb98c Mon Sep 17 00:00:00 2001 From: sprang Date: Fri, 19 Feb 2016 09:03:26 -0800 Subject: [PATCH] Use CallStats for RTT in Call, rather than VideoSendStream::GetRtt() Also move some stats reporting from vie_channel to send stats proxy BUG= Review URL: https://codereview.webrtc.org/1669623004 Cr-Commit-Position: refs/heads/master@{#11688} --- webrtc/call/call.cc | 10 +-- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 8 ++- webrtc/video/call_stats.cc | 58 ++++++++++----- webrtc/video/call_stats.h | 5 ++ webrtc/video/call_stats_unittest.cc | 21 +++++- webrtc/video/end_to_end_tests.cc | 10 +-- webrtc/video/send_statistics_proxy.cc | 16 ++++- webrtc/video/send_statistics_proxy.h | 3 + webrtc/video/video_send_stream.cc | 15 ---- webrtc/video/video_send_stream.h | 1 - webrtc/video/vie_channel.cc | 72 ------------------- webrtc/video/vie_channel.h | 14 ---- 12 files changed, 92 insertions(+), 141 deletions(-) diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index f7cdd827a2..eba0c96c31 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -511,15 +511,7 @@ Call::Stats Call::GetStats() const { stats.send_bandwidth_bps = send_bandwidth; stats.recv_bandwidth_bps = recv_bandwidth; stats.pacer_delay_ms = congestion_controller_->GetPacerQueuingDelayMs(); - { - ReadLockScoped read_lock(*send_crit_); - // TODO(solenberg): Add audio send streams. - for (const auto& kv : video_send_ssrcs_) { - int rtt_ms = kv.second->GetRtt(); - if (rtt_ms > 0) - stats.rtt_ms = rtt_ms; - } - } + stats.rtt_ms = call_stats_->rtcp_rtt_stats()->LastProcessedRtt(); return stats; } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index e6e2187de7..2c8786b906 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -182,8 +182,12 @@ int32_t ModuleRtpRtcpImpl::Process() { // Get processed rtt. if (process_rtt) { last_rtt_process_time_ = now; - if (rtt_stats_) - set_rtt_ms(rtt_stats_->LastProcessedRtt()); + if (rtt_stats_) { + // Make sure we have a valid RTT before setting. + int64_t last_rtt = rtt_stats_->LastProcessedRtt(); + if (last_rtt >= 0) + set_rtt_ms(last_rtt); + } } // For sending streams, make sure to not send a SR before media has been sent. diff --git a/webrtc/video/call_stats.cc b/webrtc/video/call_stats.cc index ef5f0b211d..1ee4625552 100644 --- a/webrtc/video/call_stats.cc +++ b/webrtc/video/call_stats.cc @@ -10,11 +10,11 @@ #include "webrtc/video/call_stats.h" -#include - #include +#include "webrtc/base/checks.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "webrtc/system_wrappers/include/metrics.h" #include "webrtc/system_wrappers/include/tick_util.h" namespace webrtc { @@ -34,17 +34,17 @@ void RemoveOldReports(int64_t now, std::list* reports) { } int64_t GetMaxRttMs(std::list* reports) { + if (reports->empty()) + return -1; int64_t max_rtt_ms = 0; - for (std::list::const_iterator it = reports->begin(); - it != reports->end(); ++it) { - max_rtt_ms = std::max(it->rtt, max_rtt_ms); - } + for (const CallStats::RttTime& rtt_time : *reports) + max_rtt_ms = std::max(rtt_time.rtt, max_rtt_ms); return max_rtt_ms; } int64_t GetAvgRttMs(std::list* reports) { if (reports->empty()) { - return 0; + return -1; } int64_t sum = 0; for (std::list::const_iterator it = reports->begin(); @@ -55,13 +55,13 @@ int64_t GetAvgRttMs(std::list* reports) { } void UpdateAvgRttMs(std::list* reports, int64_t* avg_rtt) { - uint32_t cur_rtt_ms = GetAvgRttMs(reports); - if (cur_rtt_ms == 0) { + int64_t cur_rtt_ms = GetAvgRttMs(reports); + if (cur_rtt_ms == -1) { // Reset. - *avg_rtt = 0; + *avg_rtt = -1; return; } - if (*avg_rtt == 0) { + if (*avg_rtt == -1) { // Initialize. *avg_rtt = cur_rtt_ms; return; @@ -94,11 +94,15 @@ CallStats::CallStats(Clock* clock) : clock_(clock), rtcp_rtt_stats_(new RtcpObserver(this)), last_process_time_(clock_->TimeInMilliseconds()), - max_rtt_ms_(0), - avg_rtt_ms_(0) {} + max_rtt_ms_(-1), + avg_rtt_ms_(-1), + sum_avg_rtt_ms_(0), + num_avg_rtt_(0), + time_of_first_rtt_ms_(-1) {} CallStats::~CallStats() { - assert(observers_.empty()); + RTC_DCHECK(observers_.empty()); + UpdateHistograms(); } int64_t CallStats::TimeUntilNextProcess() { @@ -118,12 +122,15 @@ int32_t CallStats::Process() { UpdateAvgRttMs(&reports_, &avg_rtt_ms_); // If there is a valid rtt, update all observers with the max rtt. - // TODO(asapersson): Consider changing this to report the average rtt. - if (max_rtt_ms_ > 0) { + if (max_rtt_ms_ >= 0) { + RTC_DCHECK_GE(avg_rtt_ms_, 0); for (std::list::iterator it = observers_.begin(); it != observers_.end(); ++it) { (*it)->OnRttUpdate(avg_rtt_ms_, max_rtt_ms_); } + // Sum for Histogram of average RTT reported over the entire call. + sum_avg_rtt_ms_ += avg_rtt_ms_; + ++num_avg_rtt_; } return 0; } @@ -160,7 +167,24 @@ void CallStats::DeregisterStatsObserver(CallStatsObserver* observer) { void CallStats::OnRttUpdate(int64_t rtt) { rtc::CritScope cs(&crit_); - reports_.push_back(RttTime(rtt, clock_->TimeInMilliseconds())); + int64_t now_ms = clock_->TimeInMilliseconds(); + reports_.push_back(RttTime(rtt, now_ms)); + if (time_of_first_rtt_ms_ == -1) + time_of_first_rtt_ms_ = now_ms; +} + +void CallStats::UpdateHistograms() { + rtc::CritScope cs(&crit_); + if (time_of_first_rtt_ms_ == -1 || num_avg_rtt_ < 1) + return; + + int64_t elapsed_sec = + (clock_->TimeInMilliseconds() - time_of_first_rtt_ms_) / 1000; + if (elapsed_sec >= metrics::kMinRunTimeInSeconds) { + int64_t avg_rtt_ms = (sum_avg_rtt_ms_ + num_avg_rtt_ / 2) / num_avg_rtt_; + RTC_HISTOGRAM_COUNTS_10000( + "WebRTC.Video.AverageRoundTripTimeInMilliseconds", avg_rtt_ms); + } } } // namespace webrtc diff --git a/webrtc/video/call_stats.h b/webrtc/video/call_stats.h index 8a09b7eed7..385f6ffbe0 100644 --- a/webrtc/video/call_stats.h +++ b/webrtc/video/call_stats.h @@ -58,6 +58,8 @@ class CallStats : public Module { int64_t avg_rtt_ms() const; private: + void UpdateHistograms(); + Clock* const clock_; // Protecting all members. rtc::CriticalSection crit_; @@ -68,6 +70,9 @@ class CallStats : public Module { // The last RTT in the statistics update (zero if there is no valid estimate). int64_t max_rtt_ms_; int64_t avg_rtt_ms_; + int64_t sum_avg_rtt_ms_ GUARDED_BY(crit_); + int64_t num_avg_rtt_ GUARDED_BY(crit_); + int64_t time_of_first_rtt_ms_ GUARDED_BY(crit_); // All Rtt reports within valid time interval, oldest first. std::list reports_; diff --git a/webrtc/video/call_stats_unittest.cc b/webrtc/video/call_stats_unittest.cc index 6226a5bf6e..2421cc7148 100644 --- a/webrtc/video/call_stats_unittest.cc +++ b/webrtc/video/call_stats_unittest.cc @@ -13,7 +13,9 @@ #include "webrtc/base/scoped_ptr.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "webrtc/system_wrappers/include/metrics.h" #include "webrtc/system_wrappers/include/tick_util.h" +#include "webrtc/test/histogram.h" #include "webrtc/video/call_stats.h" using ::testing::_; @@ -45,7 +47,7 @@ TEST_F(CallStatsTest, AddAndTriggerCallback) { RtcpRttStats* rtcp_rtt_stats = call_stats_->rtcp_rtt_stats(); call_stats_->RegisterStatsObserver(&stats_observer); fake_clock_.AdvanceTimeMilliseconds(1000); - EXPECT_EQ(0, rtcp_rtt_stats->LastProcessedRtt()); + EXPECT_EQ(-1, rtcp_rtt_stats->LastProcessedRtt()); const int64_t kRtt = 25; rtcp_rtt_stats->OnRttUpdate(kRtt); @@ -57,7 +59,7 @@ TEST_F(CallStatsTest, AddAndTriggerCallback) { fake_clock_.AdvanceTimeMilliseconds(kRttTimeOutMs); EXPECT_CALL(stats_observer, OnRttUpdate(_, _)).Times(0); call_stats_->Process(); - EXPECT_EQ(0, rtcp_rtt_stats->LastProcessedRtt()); + EXPECT_EQ(-1, rtcp_rtt_stats->LastProcessedRtt()); call_stats_->DeregisterStatsObserver(&stats_observer); } @@ -201,4 +203,19 @@ TEST_F(CallStatsTest, LastProcessedRtt) { call_stats_->DeregisterStatsObserver(&stats_observer); } +TEST_F(CallStatsTest, ProducesHistogramMetrics) { + const int64_t kRtt = 123; + RtcpRttStats* rtcp_rtt_stats = call_stats_->rtcp_rtt_stats(); + rtcp_rtt_stats->OnRttUpdate(kRtt); + fake_clock_.AdvanceTimeMilliseconds(metrics::kMinRunTimeInSeconds * 1000); + rtcp_rtt_stats->OnRttUpdate(kRtt); + call_stats_->Process(); + call_stats_.reset(); + + EXPECT_EQ(1, test::NumHistogramSamples( + "WebRTC.Video.AverageRoundTripTimeInMilliseconds")); + EXPECT_EQ(kRtt, test::LastHistogramSample( + "WebRTC.Video.AverageRoundTripTimeInMilliseconds")); +} + } // namespace webrtc diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 1c804098e8..3f22b5229a 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -2106,12 +2106,6 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx, if (MinMetricRunTimePassed()) observation_complete_.Set(); - // GetStats calls GetSendChannelRtcpStatistics - // (via VideoSendStream::GetRtt) which updates ReportBlockStats used by - // WebRTC.Video.SentPacketsLostInPercent. - // TODO(asapersson): Remove dependency on calling GetStats. - sender_call_->GetStats(); - return SEND_PACKET; } @@ -2212,8 +2206,8 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx, EXPECT_EQ(1, test::NumHistogramSamples( "WebRTC.Video.KeyFramesReceivedInPermille")); - EXPECT_EQ(1, test::NumHistogramSamples( - "WebRTC.Video.SentPacketsLostInPercent")); + EXPECT_EQ( + 1, test::NumHistogramSamples(video_prefix + "SentPacketsLostInPercent")); EXPECT_EQ(1, test::NumHistogramSamples( "WebRTC.Video.ReceivedPacketsLostInPercent")); diff --git a/webrtc/video/send_statistics_proxy.cc b/webrtc/video/send_statistics_proxy.cc index bd0d5186e1..11dd241afa 100644 --- a/webrtc/video/send_statistics_proxy.cc +++ b/webrtc/video/send_statistics_proxy.cc @@ -89,7 +89,8 @@ SendStatisticsProxy::UmaSamplesContainer::UmaSamplesContainer( max_sent_width_per_timestamp_(0), max_sent_height_per_timestamp_(0), input_frame_rate_tracker_(100u, 10u), - sent_frame_rate_tracker_(100u, 10u) {} + sent_frame_rate_tracker_(100u, 10u), + first_rtcp_stats_time_ms_(-1) {} SendStatisticsProxy::UmaSamplesContainer::~UmaSamplesContainer() { UpdateHistograms(); @@ -166,6 +167,16 @@ void SendStatisticsProxy::UmaSamplesContainer::UpdateHistograms() { RTC_HISTOGRAMS_COUNTS_100000(kIndex, uma_prefix_ + "SendSideDelayMaxInMs", max_delay_ms); } + int fraction_lost = report_block_stats_.FractionLostInPercent(); + if (first_rtcp_stats_time_ms_ != -1) { + int64_t elapsed_time_ms = Clock::GetRealTimeClock()->TimeInMilliseconds() - + first_rtcp_stats_time_ms_; + if (elapsed_time_ms / 1000 >= metrics::kMinRunTimeInSeconds && + fraction_lost != -1) { + RTC_HISTOGRAMS_PERCENTAGE( + kIndex, uma_prefix_ + "SentPacketsLostInPercent", fraction_lost); + } + } } void SendStatisticsProxy::SetContentType( @@ -354,6 +365,9 @@ void SendStatisticsProxy::StatisticsUpdated(const RtcpStatistics& statistics, return; stats->rtcp_stats = statistics; + uma_container_->report_block_stats_.Store(statistics, 0, ssrc); + if (uma_container_->first_rtcp_stats_time_ms_ == -1) + uma_container_->first_rtcp_stats_time_ms_ = clock_->TimeInMilliseconds(); } void SendStatisticsProxy::CNameChanged(const char* cname, uint32_t ssrc) {} diff --git a/webrtc/video/send_statistics_proxy.h b/webrtc/video/send_statistics_proxy.h index d2b9b565b3..b978fab10b 100644 --- a/webrtc/video/send_statistics_proxy.h +++ b/webrtc/video/send_statistics_proxy.h @@ -24,6 +24,7 @@ #include "webrtc/modules/video_coding/include/video_coding_defines.h" #include "webrtc/system_wrappers/include/clock.h" #include "webrtc/video/overuse_frame_detector.h" +#include "webrtc/video/report_block_stats.h" #include "webrtc/video/vie_encoder.h" #include "webrtc/video_send_stream.h" @@ -163,6 +164,8 @@ class SendStatisticsProxy : public CpuOveruseMetricsObserver, SampleCounter max_delay_counter_; rtc::RateTracker input_frame_rate_tracker_; rtc::RateTracker sent_frame_rate_tracker_; + int64_t first_rtcp_stats_time_ms_; + ReportBlockStats report_block_stats_; }; rtc::scoped_ptr uma_container_ GUARDED_BY(crit_); diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 1be17632ed..e6674c8531 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -571,21 +571,6 @@ void VideoSendStream::SignalNetworkState(NetworkState state) { vie_channel_.SetRTCPMode(RtcpMode::kOff); } -int64_t VideoSendStream::GetRtt() const { - webrtc::RtcpStatistics rtcp_stats; - uint16_t frac_lost; - uint32_t cumulative_lost; - uint32_t extended_max_sequence_number; - uint32_t jitter; - int64_t rtt_ms; - if (vie_channel_.GetSendRtcpStatistics(&frac_lost, &cumulative_lost, - &extended_max_sequence_number, - &jitter, &rtt_ms) == 0) { - return rtt_ms; - } - return -1; -} - int VideoSendStream::GetPaddingNeededBps() const { return vie_encoder_.GetPaddingNeededBps(); } diff --git a/webrtc/video/video_send_stream.h b/webrtc/video/video_send_stream.h index 96d060eb30..2a5df87cc4 100644 --- a/webrtc/video/video_send_stream.h +++ b/webrtc/video/video_send_stream.h @@ -72,7 +72,6 @@ class VideoSendStream : public webrtc::VideoSendStream, typedef std::map RtpStateMap; RtpStateMap GetRtpStates() const; - int64_t GetRtt() const; int GetPaddingNeededBps() const; private: diff --git a/webrtc/video/vie_channel.cc b/webrtc/video/vie_channel.cc index b9e8357585..6d2c441f99 100644 --- a/webrtc/video/vie_channel.cc +++ b/webrtc/video/vie_channel.cc @@ -32,7 +32,6 @@ #include "webrtc/video/call_stats.h" #include "webrtc/video/payload_router.h" #include "webrtc/video/receive_statistics_proxy.h" -#include "webrtc/video/report_block_stats.h" namespace webrtc { @@ -107,11 +106,7 @@ ViEChannel::ViEChannel(Transport* transport, nack_history_size_sender_(kMinSendSidePacketHistorySize), max_nack_reordering_threshold_(kMaxPacketAgeToNack), pre_render_callback_(NULL), - report_block_stats_sender_(new ReportBlockStats()), - time_of_first_rtt_ms_(-1), - rtt_sum_ms_(0), last_rtt_ms_(0), - num_rtts_(0), rtp_rtcp_modules_( CreateRtpRtcpModules(!sender, vie_receiver_.GetReceiveStatistics(), @@ -188,17 +183,6 @@ ViEChannel::~ViEChannel() { void ViEChannel::UpdateHistograms() { int64_t now = Clock::GetRealTimeClock()->TimeInMilliseconds(); - { - rtc::CritScope lock(&crit_); - int64_t elapsed_sec = (now - time_of_first_rtt_ms_) / 1000; - if (time_of_first_rtt_ms_ != -1 && num_rtts_ > 0 && - elapsed_sec > metrics::kMinRunTimeInSeconds) { - int64_t avg_rtt_ms = (rtt_sum_ms_ + num_rtts_ / 2) / num_rtts_; - RTC_HISTOGRAM_COUNTS_10000( - "WebRTC.Video.AverageRoundTripTimeInMilliseconds", avg_rtt_ms); - } - } - if (sender_) { RtcpPacketTypeCounter rtcp_counter; GetSendRtcpPacketTypeCounter(&rtcp_counter); @@ -215,11 +199,6 @@ void ViEChannel::UpdateHistograms() { "WebRTC.Video.UniqueNackRequestsReceivedInPercent", rtcp_counter.UniqueNackRequestsInPercent()); } - int fraction_lost = report_block_stats_sender_->FractionLostInPercent(); - if (fraction_lost != -1) { - RTC_HISTOGRAM_PERCENTAGE("WebRTC.Video.SentPacketsLostInPercent", - fraction_lost); - } } StreamDataCounters rtp; @@ -645,53 +624,6 @@ int32_t ViEChannel::GetRemoteRTCPCName(char rtcp_cname[]) { return rtp_rtcp_modules_[0]->RemoteCNAME(remoteSSRC, rtcp_cname); } -int32_t ViEChannel::GetSendRtcpStatistics(uint16_t* fraction_lost, - uint32_t* cumulative_lost, - uint32_t* extended_max, - uint32_t* jitter_samples, - int64_t* rtt_ms) const { - // Aggregate the report blocks associated with streams sent on this channel. - std::vector report_blocks; - for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) - rtp_rtcp->RemoteRTCPStat(&report_blocks); - - if (report_blocks.empty()) - return -1; - - uint32_t remote_ssrc = vie_receiver_.GetRemoteSsrc(); - std::vector::const_iterator it = report_blocks.begin(); - for (; it != report_blocks.end(); ++it) { - if (it->remoteSSRC == remote_ssrc) - break; - } - if (it == report_blocks.end()) { - // We have not received packets with an SSRC matching the report blocks. To - // have a chance of calculating an RTT we will try with the SSRC of the - // first report block received. - // This is very important for send-only channels where we don't know the - // SSRC of the other end. - remote_ssrc = report_blocks[0].remoteSSRC; - } - - // TODO(asapersson): Change report_block_stats to not rely on - // GetSendRtcpStatistics to be called. - RTCPReportBlock report = - report_block_stats_sender_->AggregateAndStore(report_blocks); - *fraction_lost = report.fractionLost; - *cumulative_lost = report.cumulativeLost; - *extended_max = report.extendedHighSeqNum; - *jitter_samples = report.jitter; - - int64_t dummy; - int64_t rtt = 0; - if (rtp_rtcp_modules_[0]->RTT(remote_ssrc, &rtt, &dummy, &dummy, &dummy) != - 0) { - return -1; - } - *rtt_ms = rtt; - return 0; -} - void ViEChannel::RegisterSendChannelRtcpStatisticsCallback( RtcpStatisticsCallback* callback) { for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) @@ -915,11 +847,7 @@ void ViEChannel::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { vcm_->SetReceiveChannelParameters(max_rtt_ms); rtc::CritScope lock(&crit_); - if (time_of_first_rtt_ms_ == -1) - time_of_first_rtt_ms_ = Clock::GetRealTimeClock()->TimeInMilliseconds(); - rtt_sum_ms_ += avg_rtt_ms; last_rtt_ms_ = avg_rtt_ms; - ++num_rtts_; } int ViEChannel::ProtectionRequest(const FecProtectionParams* delta_fec_params, diff --git a/webrtc/video/vie_channel.h b/webrtc/video/vie_channel.h index 8e9cc1b08d..ba4cc1fd5b 100644 --- a/webrtc/video/vie_channel.h +++ b/webrtc/video/vie_channel.h @@ -41,7 +41,6 @@ class PacketRouter; class PayloadRouter; class ProcessThread; class ReceiveStatisticsProxy; -class ReportBlockStats; class RtcpRttStats; class ViEChannelProtectionCallback; class ViERTPObserver; @@ -116,14 +115,6 @@ class ViEChannel : public VCMFrameTypeCallback, // Gets the CName of the incoming stream. int32_t GetRemoteRTCPCName(char rtcp_cname[]); - // Returns statistics reported by the remote client in an RTCP packet. - // TODO(pbos): Remove this along with VideoSendStream::GetRtt(). - int32_t GetSendRtcpStatistics(uint16_t* fraction_lost, - uint32_t* cumulative_lost, - uint32_t* extended_max, - uint32_t* jitter_samples, - int64_t* rtt_ms) const; - // Called on receipt of RTCP report block from remote side. void RegisterSendChannelRtcpStatisticsCallback( RtcpStatisticsCallback* callback); @@ -375,12 +366,7 @@ class ViEChannel : public VCMFrameTypeCallback, int max_nack_reordering_threshold_; I420FrameCallback* pre_render_callback_ GUARDED_BY(crit_); - const rtc::scoped_ptr report_block_stats_sender_; - - int64_t time_of_first_rtt_ms_ GUARDED_BY(crit_); - int64_t rtt_sum_ms_ GUARDED_BY(crit_); int64_t last_rtt_ms_ GUARDED_BY(crit_); - size_t num_rtts_ GUARDED_BY(crit_); // RtpRtcp modules, declared last as they use other members on construction. const std::vector rtp_rtcp_modules_;