From 8b7bc5d7010c84ac57459518fe18309ef5fee1dd Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Tue, 25 Sep 2018 12:05:27 +0000 Subject: [PATCH] Revert "Second reland of "Optimize execution time of RTPSender::UpdateDelayStatistics"" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 9def3b45ef06de9e068e8f4d1644e9d508baa913. Reason for revert: webrtc_perf_tests fails on Mac-10.12. Original change's description: > Second reland of "Optimize execution time of RTPSender::UpdateDelayStatistics" > > The reland has a lot of additional DCHECKS for easier debugging, > so in debug builds it will actually be a ~2x slowdown compared to the old code. > The excessive DCHECKS should be removed in a followup CL. > > Bug: webrtc:9439 > Change-Id: I8389cd84f1ca12c29cc6993f0d2cf7e6d7dd8379 > Reviewed-on: https://webrtc-review.googlesource.com/101761 > Reviewed-by: Åsa Persson > Reviewed-by: Björn Terelius > Commit-Queue: Johannes Kron > Cr-Commit-Position: refs/heads/master@{#24821} TBR=terelius@webrtc.org,asapersson@webrtc.org,kron@webrtc.org Change-Id: I98c4c96d552858d0299d49993e9b9be6a6204dfe No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9439 Reviewed-on: https://webrtc-review.googlesource.com/101860 Reviewed-by: Johannes Kron Commit-Queue: Johannes Kron Cr-Commit-Position: refs/heads/master@{#24825} --- modules/rtp_rtcp/source/rtp_sender.cc | 96 ++++--------------- modules/rtp_rtcp/source/rtp_sender.h | 4 - .../rtp_rtcp/source/rtp_sender_unittest.cc | 70 -------------- 3 files changed, 17 insertions(+), 153 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index f9f7b6f618..d69a6f07b6 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -137,9 +137,6 @@ RTPSender::RTPSender( packet_history_(clock), flexfec_packet_history_(clock), // Statistics - send_delays_(), - max_delay_it_(send_delays_.end()), - sum_delays_ms_(0), rtp_stats_callback_(nullptr), total_bitrate_sent_(kBitrateStatisticsWindowMs, RateStatistics::kBpsScale), @@ -999,32 +996,12 @@ bool RTPSender::SendToNetwork(std::unique_ptr packet, return sent; } -void RTPSender::RecomputeMaxSendDelay() { - max_delay_it_ = send_delays_.begin(); - for (auto it = send_delays_.begin(); it != send_delays_.end(); ++it) { - if (it->second >= max_delay_it_->second) { - max_delay_it_ = it; - } - } -} - -bool RTPSender::ValidIterator() { - if (max_delay_it_ == send_delays_.end()) - return send_delays_.size() == 0; - for (auto it = send_delays_.begin(); it != send_delays_.end(); ++it) { - if (max_delay_it_ == it) { - return true; - } - } - return false; -} - void RTPSender::UpdateDelayStatistics(int64_t capture_time_ms, int64_t now_ms) { if (!send_side_delay_observer_ || capture_time_ms <= 0) return; uint32_t ssrc; - int avg_delay_ms = 0; + int64_t avg_delay_ms = 0; int max_delay_ms = 0; { rtc::CritScope lock(&send_critsect_); @@ -1034,27 +1011,7 @@ void RTPSender::UpdateDelayStatistics(int64_t capture_time_ms, int64_t now_ms) { } { rtc::CritScope cs(&statistics_crit_); - // Compute the max and average of the recent capture-to-send delays. - // The time complexity of the current approach depends on the distribution - // of the delay values. This could be done more efficiently. - - RTC_DCHECK(ValidIterator()); - // Remove elements older than kSendSideDelayWindowMs. - auto lower_bound = - send_delays_.lower_bound(now_ms - kSendSideDelayWindowMs); - for (auto it = send_delays_.begin(); it != lower_bound; ++it) { - if (max_delay_it_ == it) { - max_delay_it_ = send_delays_.end(); - } - sum_delays_ms_ -= it->second; - } - send_delays_.erase(send_delays_.begin(), lower_bound); - if (max_delay_it_ == send_delays_.end()) { - // Removed the previous max. Need to recompute. - RecomputeMaxSendDelay(); - } - - // Add the new element. + // TODO(holmer): Compute this iteratively instead. RTC_DCHECK_GE(now_ms, static_cast(0)); RTC_DCHECK_LE(now_ms, std::numeric_limits::max() / 2); RTC_DCHECK_GE(capture_time_ms, static_cast(0)); @@ -1063,42 +1020,23 @@ void RTPSender::UpdateDelayStatistics(int64_t capture_time_ms, int64_t now_ms) { RTC_DCHECK_GE(diff_ms, static_cast(0)); RTC_DCHECK_LE(diff_ms, static_cast(std::numeric_limits::max())); - int new_send_delay = rtc::dchecked_cast(now_ms - capture_time_ms); - SendDelayMap::iterator it; - bool inserted; - std::tie(it, inserted) = - send_delays_.insert(std::make_pair(now_ms, new_send_delay)); - if (!inserted) { - // TODO(terelius): If we have multiple delay measurements during the same - // millisecond then we keep the most recent one. It is not clear that this - // is the right decision, but it preserves an earlier behavior. - int previous_send_delay = it->second; - sum_delays_ms_ -= previous_send_delay; - it->second = new_send_delay; - if (max_delay_it_ == it && new_send_delay < previous_send_delay) { - RecomputeMaxSendDelay(); - } + send_delays_[now_ms] = diff_ms; + send_delays_.erase( + send_delays_.begin(), + send_delays_.lower_bound(now_ms - kSendSideDelayWindowMs)); + int num_delays = 0; + for (auto it = send_delays_.upper_bound(now_ms - kSendSideDelayWindowMs); + it != send_delays_.end(); ++it) { + max_delay_ms = std::max(max_delay_ms, it->second); + avg_delay_ms += it->second; + ++num_delays; } - if (max_delay_it_ == send_delays_.end() || - it->second >= max_delay_it_->second) { - max_delay_it_ = it; - } - sum_delays_ms_ += new_send_delay; - - size_t num_delays = send_delays_.size(); - RTC_DCHECK(max_delay_it_ != send_delays_.end()); - max_delay_ms = rtc::dchecked_cast(max_delay_it_->second); - int64_t avg_ms = (sum_delays_ms_ + num_delays / 2) / num_delays; - RTC_DCHECK_GE(avg_ms, static_cast(0)); - RTC_DCHECK_LE(avg_ms, - static_cast(std::numeric_limits::max())); - avg_delay_ms = - rtc::dchecked_cast((sum_delays_ms_ + num_delays / 2) / num_delays); - - RTC_DCHECK(ValidIterator()); + if (num_delays == 0) + return; + avg_delay_ms = (avg_delay_ms + num_delays / 2) / num_delays; } - send_side_delay_observer_->SendSideDelayUpdated(avg_delay_ms, max_delay_ms, - ssrc); + send_side_delay_observer_->SendSideDelayUpdated( + rtc::dchecked_cast(avg_delay_ms), max_delay_ms, ssrc); } void RTPSender::UpdateOnSendPacket(int packet_id, diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index fa57abe484..f19fd9d40e 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -241,8 +241,6 @@ class RTPSender { const PacketOptions& options, const PacedPacketInfo& pacing_info); - bool ValidIterator() RTC_EXCLUSIVE_LOCKS_REQUIRED(statistics_crit_); - void RecomputeMaxSendDelay() RTC_EXCLUSIVE_LOCKS_REQUIRED(statistics_crit_); void UpdateDelayStatistics(int64_t capture_time_ms, int64_t now_ms); void UpdateOnSendPacket(int packet_id, int64_t capture_time_ms, @@ -300,8 +298,6 @@ class RTPSender { // Statistics rtc::CriticalSection statistics_crit_; SendDelayMap send_delays_ RTC_GUARDED_BY(statistics_crit_); - SendDelayMap::const_iterator max_delay_it_ RTC_GUARDED_BY(statistics_crit_); - int64_t sum_delays_ms_ RTC_GUARDED_BY(statistics_crit_); FrameCounts frame_counts_ RTC_GUARDED_BY(statistics_crit_); StreamDataCounters rtp_stats_ RTC_GUARDED_BY(statistics_crit_); StreamDataCounters rtx_rtp_stats_ RTC_GUARDED_BY(statistics_crit_); diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 5f1435e1ca..3c7a6d2df5 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -139,11 +139,6 @@ class MockTransportSequenceNumberAllocator MOCK_METHOD0(AllocateSequenceNumber, uint16_t()); }; -class MockSendSideDelayObserver : public SendSideDelayObserver { - public: - MOCK_METHOD3(SendSideDelayUpdated, void(int, int, uint32_t)); -}; - class MockSendPacketObserver : public SendPacketObserver { public: MOCK_METHOD3(OnSendPacket, void(uint16_t, int64_t, uint32_t)); @@ -496,71 +491,6 @@ TEST_P(RtpSenderTestWithoutPacer, NoAllocationIfNotRegistered) { SendGenericPayload(); } -TEST_P(RtpSenderTestWithoutPacer, OnSendSideDelayUpdated) { - testing::StrictMock send_side_delay_observer_; - rtp_sender_.reset( - new RTPSender(false, &fake_clock_, &transport_, nullptr, nullptr, nullptr, - nullptr, nullptr, nullptr, &send_side_delay_observer_, - &mock_rtc_event_log_, nullptr, nullptr, nullptr, false)); - rtp_sender_->SetSSRC(kSsrc); - - const uint8_t kPayloadType = 127; - const uint32_t kCaptureTimeMsToRtpTimestamp = 90; // 90 kHz clock - char payload_name[RTP_PAYLOAD_NAME_SIZE] = "GENERIC"; - RTPVideoHeader video_header; - EXPECT_EQ(0, rtp_sender_->RegisterPayload(payload_name, kPayloadType, - 1000 * kCaptureTimeMsToRtpTimestamp, - 0, 1500)); - - // Send packet with 10 ms send-side delay. The average and max should be 10 - // ms. - EXPECT_CALL(send_side_delay_observer_, SendSideDelayUpdated(10, 10, kSsrc)) - .Times(1); - int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); - fake_clock_.AdvanceTimeMilliseconds(10); - EXPECT_TRUE(rtp_sender_->SendOutgoingData( - kVideoFrameKey, kPayloadType, - capture_time_ms * kCaptureTimeMsToRtpTimestamp, capture_time_ms, - kPayloadData, sizeof(kPayloadData), nullptr, &video_header, nullptr, - kDefaultExpectedRetransmissionTimeMs)); - - // Send another packet with 20 ms delay. The average - // and max should be 15 and 20 ms respectively. - EXPECT_CALL(send_side_delay_observer_, SendSideDelayUpdated(15, 20, kSsrc)) - .Times(1); - fake_clock_.AdvanceTimeMilliseconds(10); - EXPECT_TRUE(rtp_sender_->SendOutgoingData( - kVideoFrameKey, kPayloadType, - capture_time_ms * kCaptureTimeMsToRtpTimestamp, capture_time_ms, - kPayloadData, sizeof(kPayloadData), nullptr, &video_header, nullptr, - kDefaultExpectedRetransmissionTimeMs)); - - // Send another packet at the same time, which replaces the last packet. - // Since this packet has 0 ms delay, the average is now 5 ms and max is 10 ms. - // TODO(terelius): Is is not clear that this is the right behavior. - EXPECT_CALL(send_side_delay_observer_, SendSideDelayUpdated(5, 10, kSsrc)) - .Times(1); - capture_time_ms = fake_clock_.TimeInMilliseconds(); - EXPECT_TRUE(rtp_sender_->SendOutgoingData( - kVideoFrameKey, kPayloadType, - capture_time_ms * kCaptureTimeMsToRtpTimestamp, capture_time_ms, - kPayloadData, sizeof(kPayloadData), nullptr, &video_header, nullptr, - kDefaultExpectedRetransmissionTimeMs)); - - // Send a packet 1 second later. The earlier packets should have timed - // out, so both max and average should be the delay of this packet. - fake_clock_.AdvanceTimeMilliseconds(1000); - capture_time_ms = fake_clock_.TimeInMilliseconds(); - fake_clock_.AdvanceTimeMilliseconds(1); - EXPECT_CALL(send_side_delay_observer_, SendSideDelayUpdated(1, 1, kSsrc)) - .Times(1); - EXPECT_TRUE(rtp_sender_->SendOutgoingData( - kVideoFrameKey, kPayloadType, - capture_time_ms * kCaptureTimeMsToRtpTimestamp, capture_time_ms, - kPayloadData, sizeof(kPayloadData), nullptr, &video_header, nullptr, - kDefaultExpectedRetransmissionTimeMs)); -} - TEST_P(RtpSenderTestWithoutPacer, OnSendPacketUpdated) { EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( kRtpExtensionTransportSequenceNumber,