From 2328a94ec7ee545a26e8220c5ae157e1b3b5144f Mon Sep 17 00:00:00 2001 From: stefan Date: Fri, 7 Aug 2015 04:27:51 -0700 Subject: [PATCH] Add average rtt to CallStatsObserver and an average rtt histogram. TBR=mflodman@webrtc.org BUG=webrtc:4711,webrtc:4548 Review URL: https://codereview.webrtc.org/1279543005 Cr-Commit-Position: refs/heads/master@{#9687} --- .../modules/interface/module_common_types.h | 2 +- .../remote_bitrate_estimator_abs_send_time.cc | 5 +- .../remote_bitrate_estimator_abs_send_time.h | 2 +- .../remote_bitrate_estimator_single_stream.cc | 5 +- .../remote_bitrate_estimator_single_stream.h | 2 +- .../test/estimators/remb.cc | 2 +- .../test/estimators/send_side.cc | 2 +- webrtc/video_engine/call_stats.cc | 2 +- webrtc/video_engine/call_stats_unittest.cc | 59 +++++++------------ webrtc/video_engine/vie_channel.cc | 28 +++++++-- webrtc/video_engine/vie_channel.h | 6 +- webrtc/video_engine/vie_channel_group.cc | 4 +- 12 files changed, 65 insertions(+), 54 deletions(-) diff --git a/webrtc/modules/interface/module_common_types.h b/webrtc/modules/interface/module_common_types.h index 232e695e76..02ce03f153 100644 --- a/webrtc/modules/interface/module_common_types.h +++ b/webrtc/modules/interface/module_common_types.h @@ -440,7 +440,7 @@ struct FecProtectionParams { // CallStats object using RegisterStatsObserver. class CallStatsObserver { public: - virtual void OnRttUpdate(int64_t rtt_ms) = 0; + virtual void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) = 0; virtual ~CallStatsObserver() {} }; diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index c5faf1617f..2050ea1410 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -372,9 +372,10 @@ void RemoteBitrateEstimatorAbsSendTime::UpdateEstimate(int64_t now_ms) { } } -void RemoteBitrateEstimatorAbsSendTime::OnRttUpdate(int64_t rtt) { +void RemoteBitrateEstimatorAbsSendTime::OnRttUpdate(int64_t avg_rtt_ms, + int64_t max_rtt_ms) { CriticalSectionScoped cs(crit_sect_.get()); - remote_rate_.SetRtt(rtt); + remote_rate_.SetRtt(avg_rtt_ms); } void RemoteBitrateEstimatorAbsSendTime::RemoveStream(unsigned int ssrc) { diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h index 0d3802a89e..1bee6bd1f0 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h @@ -83,7 +83,7 @@ class RemoteBitrateEstimatorAbsSendTime : public RemoteBitrateEstimator { // deleted. int32_t Process() override; int64_t TimeUntilNextProcess() override; - void OnRttUpdate(int64_t rtt) override; + void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; void RemoveStream(unsigned int ssrc) override; bool LatestEstimate(std::vector* ssrcs, unsigned int* bitrate_bps) const override; diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc index 762a3170ae..6fd54e97f9 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc @@ -182,9 +182,10 @@ void RemoteBitrateEstimatorSingleStream::UpdateEstimate(int64_t now_ms) { } } -void RemoteBitrateEstimatorSingleStream::OnRttUpdate(int64_t rtt) { +void RemoteBitrateEstimatorSingleStream::OnRttUpdate(int64_t avg_rtt_ms, + int64_t max_rtt_ms) { CriticalSectionScoped cs(crit_sect_.get()); - remote_rate_->SetRtt(rtt); + remote_rate_->SetRtt(avg_rtt_ms); } void RemoteBitrateEstimatorSingleStream::RemoveStream(unsigned int ssrc) { diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h index 0432355b20..5ca2100b19 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h @@ -34,7 +34,7 @@ class RemoteBitrateEstimatorSingleStream : public RemoteBitrateEstimator { bool was_paced) override; int32_t Process() override; int64_t TimeUntilNextProcess() override; - void OnRttUpdate(int64_t rtt) override; + void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; void RemoveStream(unsigned int ssrc) override; bool LatestEstimate(std::vector* ssrcs, unsigned int* bitrate_bps) const override; diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc b/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc index f0d57ad9bd..8ec2b728db 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc @@ -77,7 +77,7 @@ RembReceiver::RembReceiver(int flow_id, bool plot) ss << "Estimate_" << flow_id_ << "#1"; estimate_log_prefix_ = ss.str(); // Default RTT in RemoteRateControl is 200 ms ; 50 ms is more realistic. - estimator_->OnRttUpdate(50); + estimator_->OnRttUpdate(50, 50); } RembReceiver::~RembReceiver() { diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc index b003d4538c..1f95f654eb 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc @@ -61,7 +61,7 @@ void FullBweSender::GiveFeedback(const FeedbackPacket& feedback) { int64_t rtt_ms = clock_->TimeInMilliseconds() - feedback.latest_send_time_ms(); - rbe_->OnRttUpdate(rtt_ms); + rbe_->OnRttUpdate(rtt_ms, rtt_ms); BWE_TEST_LOGGING_PLOT(1, "RTT", clock_->TimeInMilliseconds(), rtt_ms); rbe_->IncomingPacketFeedbackVector(packet_feedback_vector); diff --git a/webrtc/video_engine/call_stats.cc b/webrtc/video_engine/call_stats.cc index 6e51eebe5a..92855e379f 100644 --- a/webrtc/video_engine/call_stats.cc +++ b/webrtc/video_engine/call_stats.cc @@ -123,7 +123,7 @@ int32_t CallStats::Process() { if (max_rtt_ms_ > 0) { for (std::list::iterator it = observers_.begin(); it != observers_.end(); ++it) { - (*it)->OnRttUpdate(max_rtt_ms_); + (*it)->OnRttUpdate(avg_rtt_ms_, max_rtt_ms_); } } return 0; diff --git a/webrtc/video_engine/call_stats_unittest.cc b/webrtc/video_engine/call_stats_unittest.cc index 0febbd0198..bfe52da5e7 100644 --- a/webrtc/video_engine/call_stats_unittest.cc +++ b/webrtc/video_engine/call_stats_unittest.cc @@ -27,7 +27,7 @@ class MockStatsObserver : public CallStatsObserver { MockStatsObserver() {} virtual ~MockStatsObserver() {} - MOCK_METHOD1(OnRttUpdate, void(int64_t)); + MOCK_METHOD2(OnRttUpdate, void(int64_t, int64_t)); }; class CallStatsTest : public ::testing::Test { @@ -48,15 +48,13 @@ TEST_F(CallStatsTest, AddAndTriggerCallback) { const int64_t kRtt = 25; rtcp_rtt_stats->OnRttUpdate(kRtt); - EXPECT_CALL(stats_observer, OnRttUpdate(kRtt)) - .Times(1); + EXPECT_CALL(stats_observer, OnRttUpdate(kRtt, kRtt)).Times(1); call_stats_->Process(); EXPECT_EQ(kRtt, rtcp_rtt_stats->LastProcessedRtt()); const int64_t kRttTimeOutMs = 1500 + 10; TickTime::AdvanceFakeClock(kRttTimeOutMs); - EXPECT_CALL(stats_observer, OnRttUpdate(_)) - .Times(0); + EXPECT_CALL(stats_observer, OnRttUpdate(_, _)).Times(0); call_stats_->Process(); EXPECT_EQ(0, rtcp_rtt_stats->LastProcessedRtt()); @@ -70,27 +68,23 @@ TEST_F(CallStatsTest, ProcessTime) { rtcp_rtt_stats->OnRttUpdate(100); // Time isn't updated yet. - EXPECT_CALL(stats_observer, OnRttUpdate(_)) - .Times(0); + EXPECT_CALL(stats_observer, OnRttUpdate(_, _)).Times(0); call_stats_->Process(); // Advance clock and verify we get an update. TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(stats_observer, OnRttUpdate(_)) - .Times(1); + EXPECT_CALL(stats_observer, OnRttUpdate(_, _)).Times(1); call_stats_->Process(); // Advance clock just too little to get an update. TickTime::AdvanceFakeClock(999); rtcp_rtt_stats->OnRttUpdate(100); - EXPECT_CALL(stats_observer, OnRttUpdate(_)) - .Times(0); + EXPECT_CALL(stats_observer, OnRttUpdate(_, _)).Times(0); call_stats_->Process(); // Advance enough to trigger a new update. TickTime::AdvanceFakeClock(1); - EXPECT_CALL(stats_observer, OnRttUpdate(_)) - .Times(1); + EXPECT_CALL(stats_observer, OnRttUpdate(_, _)).Times(1); call_stats_->Process(); call_stats_->DeregisterStatsObserver(&stats_observer); @@ -113,10 +107,8 @@ TEST_F(CallStatsTest, MultipleObservers) { // Verify both observers are updated. TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(stats_observer_1, OnRttUpdate(kRtt)) - .Times(1); - EXPECT_CALL(stats_observer_2, OnRttUpdate(kRtt)) - .Times(1); + EXPECT_CALL(stats_observer_1, OnRttUpdate(kRtt, kRtt)).Times(1); + EXPECT_CALL(stats_observer_2, OnRttUpdate(kRtt, kRtt)).Times(1); call_stats_->Process(); // Deregister the second observer and verify update is only sent to the first @@ -124,20 +116,16 @@ TEST_F(CallStatsTest, MultipleObservers) { call_stats_->DeregisterStatsObserver(&stats_observer_2); rtcp_rtt_stats->OnRttUpdate(kRtt); TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(stats_observer_1, OnRttUpdate(kRtt)) - .Times(1); - EXPECT_CALL(stats_observer_2, OnRttUpdate(kRtt)) - .Times(0); + EXPECT_CALL(stats_observer_1, OnRttUpdate(kRtt, kRtt)).Times(1); + EXPECT_CALL(stats_observer_2, OnRttUpdate(kRtt, kRtt)).Times(0); call_stats_->Process(); // Deregister the first observer. call_stats_->DeregisterStatsObserver(&stats_observer_1); rtcp_rtt_stats->OnRttUpdate(kRtt); TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(stats_observer_1, OnRttUpdate(kRtt)) - .Times(0); - EXPECT_CALL(stats_observer_2, OnRttUpdate(kRtt)) - .Times(0); + EXPECT_CALL(stats_observer_1, OnRttUpdate(kRtt, kRtt)).Times(0); + EXPECT_CALL(stats_observer_2, OnRttUpdate(kRtt, kRtt)).Times(0); call_stats_->Process(); } @@ -153,16 +141,15 @@ TEST_F(CallStatsTest, ChangeRtt) { // Set a first value and verify the callback is triggered. const int64_t kFirstRtt = 100; rtcp_rtt_stats->OnRttUpdate(kFirstRtt); - EXPECT_CALL(stats_observer, OnRttUpdate(kFirstRtt)) - .Times(1); + EXPECT_CALL(stats_observer, OnRttUpdate(kFirstRtt, kFirstRtt)).Times(1); call_stats_->Process(); // Increase rtt and verify the new value is reported. TickTime::AdvanceFakeClock(1000); const int64_t kHighRtt = kFirstRtt + 20; + const int64_t kAvgRtt1 = 103; rtcp_rtt_stats->OnRttUpdate(kHighRtt); - EXPECT_CALL(stats_observer, OnRttUpdate(kHighRtt)) - .Times(1); + EXPECT_CALL(stats_observer, OnRttUpdate(kAvgRtt1, kHighRtt)).Times(1); call_stats_->Process(); // Increase time enough for a new update, but not too much to make the @@ -170,16 +157,16 @@ TEST_F(CallStatsTest, ChangeRtt) { // in the callback. TickTime::AdvanceFakeClock(1000); const int64_t kLowRtt = kFirstRtt - 20; + const int64_t kAvgRtt2 = 102; rtcp_rtt_stats->OnRttUpdate(kLowRtt); - EXPECT_CALL(stats_observer, OnRttUpdate(kHighRtt)) - .Times(1); + EXPECT_CALL(stats_observer, OnRttUpdate(kAvgRtt2, kHighRtt)).Times(1); call_stats_->Process(); // Advance time to make the high report invalid, the lower rtt should now be // in the callback. TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(stats_observer, OnRttUpdate(kLowRtt)) - .Times(1); + const int64_t kAvgRtt3 = 95; + EXPECT_CALL(stats_observer, OnRttUpdate(kAvgRtt3, kLowRtt)).Times(1); call_stats_->Process(); call_stats_->DeregisterStatsObserver(&stats_observer); @@ -198,8 +185,7 @@ TEST_F(CallStatsTest, LastProcessedRtt) { const int64_t kAvgRtt = 20; rtcp_rtt_stats->OnRttUpdate(kRttLow); rtcp_rtt_stats->OnRttUpdate(kRttHigh); - EXPECT_CALL(stats_observer, OnRttUpdate(kRttHigh)) - .Times(1); + EXPECT_CALL(stats_observer, OnRttUpdate(kAvgRtt, kRttHigh)).Times(1); call_stats_->Process(); EXPECT_EQ(kAvgRtt, rtcp_rtt_stats->LastProcessedRtt()); @@ -207,8 +193,7 @@ TEST_F(CallStatsTest, LastProcessedRtt) { TickTime::AdvanceFakeClock(1000); rtcp_rtt_stats->OnRttUpdate(kRttLow); rtcp_rtt_stats->OnRttUpdate(kRttHigh); - EXPECT_CALL(stats_observer, OnRttUpdate(kRttHigh)) - .Times(1); + EXPECT_CALL(stats_observer, OnRttUpdate(kAvgRtt, kRttHigh)).Times(1); call_stats_->Process(); EXPECT_EQ(kAvgRtt, rtcp_rtt_stats->LastProcessedRtt()); diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index df371ed43c..b8664cecc8 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/webrtc/video_engine/vie_channel.cc @@ -49,8 +49,8 @@ class ChannelStatsObserver : public CallStatsObserver { virtual ~ChannelStatsObserver() {} // Implements StatsObserver. - virtual void OnRttUpdate(int64_t rtt) { - owner_->OnRttUpdate(rtt); + virtual void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { + owner_->OnRttUpdate(avg_rtt_ms, max_rtt_ms); } private: @@ -119,6 +119,9 @@ ViEChannel::ViEChannel(int32_t channel_id, 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), + num_rtts_(0), rtp_rtcp_modules_( CreateRtpRtcpModules(ViEModuleId(engine_id_, channel_id_), !sender, @@ -198,6 +201,17 @@ ViEChannel::~ViEChannel() { void ViEChannel::UpdateHistograms() { int64_t now = Clock::GetRealTimeClock()->TimeInMilliseconds(); + { + CriticalSectionScoped cs(crit_.get()); + 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); @@ -1119,8 +1133,14 @@ bool ViEChannel::ChannelDecodeProcess() { return true; } -void ViEChannel::OnRttUpdate(int64_t rtt) { - vcm_->SetReceiveChannelParameters(rtt); +void ViEChannel::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { + vcm_->SetReceiveChannelParameters(max_rtt_ms); + + CriticalSectionScoped cs(crit_.get()); + if (time_of_first_rtt_ms_ == -1) + time_of_first_rtt_ms_ = Clock::GetRealTimeClock()->TimeInMilliseconds(); + rtt_sum_ms_ += avg_rtt_ms; + ++num_rtts_; } int ViEChannel::ProtectionRequest(const FecProtectionParams* delta_fec_params, diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h index 6c5782e494..85b18cf645 100644 --- a/webrtc/video_engine/vie_channel.h +++ b/webrtc/video_engine/vie_channel.h @@ -318,7 +318,7 @@ class ViEChannel : public VCMFrameTypeCallback, static bool ChannelDecodeThreadFunction(void* obj); bool ChannelDecodeProcess(); - void OnRttUpdate(int64_t rtt); + void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms); int ProtectionRequest(const FecProtectionParams* delta_fec_params, const FecProtectionParams* key_fec_params, @@ -488,6 +488,10 @@ class ViEChannel : public VCMFrameTypeCallback, 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_); + size_t num_rtts_ GUARDED_BY(crit_); + // RtpRtcp modules, declared last as they use other members on construction. const std::vector rtp_rtcp_modules_; size_t num_active_rtp_rtcp_modules_ GUARDED_BY(crit_); diff --git a/webrtc/video_engine/vie_channel_group.cc b/webrtc/video_engine/vie_channel_group.cc index cf6b19ce51..b1a21f09d5 100644 --- a/webrtc/video_engine/vie_channel_group.cc +++ b/webrtc/video_engine/vie_channel_group.cc @@ -70,9 +70,9 @@ class WrappingBitrateEstimator : public RemoteBitrateEstimator { return rbe_->TimeUntilNextProcess(); } - void OnRttUpdate(int64_t rtt) override { + void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override { CriticalSectionScoped cs(crit_sect_.get()); - rbe_->OnRttUpdate(rtt); + rbe_->OnRttUpdate(avg_rtt_ms, max_rtt_ms); } void RemoveStream(unsigned int ssrc) override {