From ff0581672e76b309602f6e38ff1c63ac473af615 Mon Sep 17 00:00:00 2001 From: Sam Zackrisson Date: Tue, 20 Nov 2018 17:15:13 +0100 Subject: [PATCH] Delete the WebRTC.Call.TimeSendingAudioRtpPacketsInSeconds metric It never saw much use, and is blocking refactoring. Histograms.xml-side cleanup: https://chromium-review.googlesource.com/c/chromium/src/+/1344141 Bug: webrtc:7882 Change-Id: I112232a573fcd218dc7a51bfcdd7898847d14f18 Reviewed-on: https://webrtc-review.googlesource.com/c/111506 Commit-Queue: Niels Moller Commit-Queue: Sam Zackrisson Reviewed-by: Fredrik Solenberg Cr-Commit-Position: refs/heads/master@{#25780} --- audio/BUILD.gn | 3 -- audio/audio_send_stream.cc | 48 ++------------------- audio/audio_send_stream.h | 9 +--- audio/audio_send_stream_unittest.cc | 40 +----------------- audio/test/media_transport_test.cc | 3 +- audio/time_interval.cc | 56 ------------------------- audio/time_interval.h | 65 ----------------------------- audio/time_interval_unittest.cc | 48 --------------------- call/call.cc | 9 +--- 9 files changed, 7 insertions(+), 274 deletions(-) delete mode 100644 audio/time_interval.cc delete mode 100644 audio/time_interval.h delete mode 100644 audio/time_interval_unittest.cc diff --git a/audio/BUILD.gn b/audio/BUILD.gn index 37a7e07ac3..c045af6376 100644 --- a/audio/BUILD.gn +++ b/audio/BUILD.gn @@ -33,8 +33,6 @@ rtc_static_library("audio") { "null_audio_poller.h", "remix_resample.cc", "remix_resample.h", - "time_interval.cc", - "time_interval.h", "transport_feedback_packet_loss_tracker.cc", "transport_feedback_packet_loss_tracker.h", ] @@ -126,7 +124,6 @@ if (rtc_include_tests) { "remix_resample_unittest.cc", "test/audio_stats_test.cc", "test/media_transport_test.cc", - "time_interval_unittest.cc", "transport_feedback_packet_loss_tracker_unittest.cc", ] deps = [ diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index 3ac7e36a97..082c557d87 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -90,29 +90,6 @@ void UpdateEventLogStreamConfig(RtcEventLog* event_log, } // namespace -// Helper class to track the actively sending lifetime of this stream. -class AudioSendStream::TimedTransport : public Transport { - public: - TimedTransport(Transport* transport, TimeInterval* time_interval) - : transport_(transport), lifetime_(time_interval) {} - bool SendRtp(const uint8_t* packet, - size_t length, - const PacketOptions& options) { - if (lifetime_) { - lifetime_->Extend(); - } - return transport_->SendRtp(packet, length, options); - } - bool SendRtcp(const uint8_t* packet, size_t length) { - return transport_->SendRtcp(packet, length); - } - ~TimedTransport() {} - - private: - Transport* transport_; - TimeInterval* lifetime_; -}; - AudioSendStream::AudioSendStream( const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, @@ -122,8 +99,7 @@ AudioSendStream::AudioSendStream( BitrateAllocatorInterface* bitrate_allocator, RtcEventLog* event_log, RtcpRttStats* rtcp_rtt_stats, - const absl::optional& suspended_rtp_state, - TimeInterval* overall_call_lifetime) + const absl::optional& suspended_rtp_state) : AudioSendStream(config, audio_state, worker_queue, @@ -132,7 +108,6 @@ AudioSendStream::AudioSendStream( event_log, rtcp_rtt_stats, suspended_rtp_state, - overall_call_lifetime, voe::CreateChannelSend(worker_queue, module_process_thread, config.media_transport, @@ -152,7 +127,6 @@ AudioSendStream::AudioSendStream( RtcEventLog* event_log, RtcpRttStats* rtcp_rtt_stats, const absl::optional& suspended_rtp_state, - TimeInterval* overall_call_lifetime, std::unique_ptr channel_send) : worker_queue_(worker_queue), config_(Config(/*send_transport=*/nullptr, @@ -166,8 +140,7 @@ AudioSendStream::AudioSendStream( kPacketLossRateMinNumAckedPackets, kRecoverablePacketLossRateMinNumAckedPairs), rtp_rtcp_module_(nullptr), - suspended_rtp_state_(suspended_rtp_state), - overall_call_lifetime_(overall_call_lifetime) { + suspended_rtp_state_(suspended_rtp_state) { RTC_LOG(LS_INFO) << "AudioSendStream: " << config.rtp.ssrc; RTC_DCHECK(worker_queue_); RTC_DCHECK(audio_state_); @@ -178,7 +151,6 @@ AudioSendStream::AudioSendStream( // should be no rtp_transport, and below check should be strengthened to XOR // (either rtp_transport or media_transport but not both). RTC_DCHECK(rtp_transport || config.media_transport); - RTC_DCHECK(overall_call_lifetime_); rtp_rtcp_module_ = channel_send_->GetRtpRtcp(); RTC_DCHECK(rtp_rtcp_module_); @@ -202,10 +174,6 @@ AudioSendStream::~AudioSendStream() { channel_send_->RegisterTransport(nullptr); channel_send_->ResetSenderCongestionControlObjects(); } - // Lifetime can only be updated after deregistering - // |timed_send_transport_adapter_| in the underlying channel object to avoid - // data races in |active_lifetime_|. - overall_call_lifetime_->Extend(active_lifetime_); } const webrtc::AudioSendStream::Config& AudioSendStream::GetConfig() const { @@ -257,17 +225,7 @@ void AudioSendStream::ConfigureStream( } if (first_time || new_config.send_transport != old_config.send_transport) { - if (old_config.send_transport) { - channel_send->RegisterTransport(nullptr); - } - if (new_config.send_transport) { - stream->timed_send_transport_adapter_.reset(new TimedTransport( - new_config.send_transport, &stream->active_lifetime_)); - } else { - stream->timed_send_transport_adapter_.reset(nullptr); - } - channel_send->RegisterTransport( - stream->timed_send_transport_adapter_.get()); + channel_send->RegisterTransport(new_config.send_transport); } // Enable the frame encryptor if a new frame encryptor has been provided. diff --git a/audio/audio_send_stream.h b/audio/audio_send_stream.h index 73e657e5d1..bf94901ef0 100644 --- a/audio/audio_send_stream.h +++ b/audio/audio_send_stream.h @@ -15,7 +15,6 @@ #include #include "audio/channel_send.h" -#include "audio/time_interval.h" #include "audio/transport_feedback_packet_loss_tracker.h" #include "call/audio_send_stream.h" #include "call/audio_state.h" @@ -46,8 +45,7 @@ class AudioSendStream final : public webrtc::AudioSendStream, BitrateAllocatorInterface* bitrate_allocator, RtcEventLog* event_log, RtcpRttStats* rtcp_rtt_stats, - const absl::optional& suspended_rtp_state, - TimeInterval* overall_call_lifetime); + const absl::optional& suspended_rtp_state); // For unit tests, which need to supply a mock ChannelSend. AudioSendStream(const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, @@ -57,7 +55,6 @@ class AudioSendStream final : public webrtc::AudioSendStream, RtcEventLog* event_log, RtcpRttStats* rtcp_rtt_stats, const absl::optional& suspended_rtp_state, - TimeInterval* overall_call_lifetime, std::unique_ptr channel_send); ~AudioSendStream() override; @@ -144,10 +141,6 @@ class AudioSendStream final : public webrtc::AudioSendStream, RtpRtcp* rtp_rtcp_module_; absl::optional const suspended_rtp_state_; - std::unique_ptr timed_send_transport_adapter_; - TimeInterval active_lifetime_; - TimeInterval* overall_call_lifetime_ = nullptr; - // RFC 5285: Each distinct extension MUST have a unique ID. The value 0 is // reserved for padding and MUST NOT be used as a local identifier. // So it should be safe to use 0 here to indicate "not configured". diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc index 5d4966c522..13e588448e 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -14,7 +14,6 @@ #include "absl/memory/memory.h" #include "api/test/mock_frame_encryptor.h" -#include "api/units/time_delta.h" #include "audio/audio_send_stream.h" #include "audio/audio_state.h" #include "audio/conversion.h" @@ -28,12 +27,10 @@ #include "modules/rtp_rtcp/mocks/mock_rtcp_bandwidth_observer.h" #include "modules/rtp_rtcp/mocks/mock_rtcp_rtt_stats.h" #include "modules/rtp_rtcp/mocks/mock_rtp_rtcp.h" -#include "rtc_base/fakeclock.h" #include "rtc_base/task_queue.h" #include "test/gtest.h" #include "test/mock_audio_encoder.h" #include "test/mock_audio_encoder_factory.h" -#include "test/mock_transport.h" namespace webrtc { namespace test { @@ -168,7 +165,6 @@ struct ConfigHelper { new internal::AudioSendStream( stream_config_, audio_state_, &worker_queue_, &rtp_transport_, &bitrate_allocator_, &event_log_, &rtcp_rtt_stats_, absl::nullopt, - &active_lifetime_, std::unique_ptr(channel_send_))); } @@ -179,7 +175,6 @@ struct ConfigHelper { } MockChannelSend* channel_send() { return channel_send_; } RtpTransportControllerSendInterface* transport() { return &rtp_transport_; } - TimeInterval* active_lifetime() { return &active_lifetime_; } static void AddBweToConfig(AudioSendStream::Config* config) { config->rtp.extensions.push_back(RtpExtension( @@ -216,11 +211,7 @@ struct ConfigHelper { .Times(1); } EXPECT_CALL(*channel_send_, ResetSenderCongestionControlObjects()).Times(1); - { - ::testing::InSequence unregister_on_destruction; - EXPECT_CALL(*channel_send_, RegisterTransport(_)).Times(1); - EXPECT_CALL(*channel_send_, RegisterTransport(nullptr)).Times(1); - } + EXPECT_CALL(*channel_send_, RegisterTransport(nullptr)).Times(2); } void SetupMockForSetupSendCodec(bool expect_set_encoder_call) { @@ -300,7 +291,6 @@ struct ConfigHelper { testing::StrictMock* channel_send_ = nullptr; rtc::scoped_refptr audio_processing_; AudioProcessingStats audio_processing_stats_; - TimeInterval active_lifetime_; testing::StrictMock bandwidth_observer_; testing::NiceMock event_log_; testing::NiceMock rtp_transport_; @@ -561,33 +551,5 @@ TEST(AudioSendStreamTest, ReconfigureWithFrameEncryptor) { EXPECT_CALL(*helper.channel_send(), SetFrameEncryptor(Ne(nullptr))).Times(1); send_stream->Reconfigure(new_config); } - -// Checks that AudioSendStream logs the times at which RTP packets are sent -// through its interface. -TEST(AudioSendStreamTest, UpdateLifetime) { - ConfigHelper helper(false, true); - - MockTransport mock_transport; - helper.config().send_transport = &mock_transport; - - Transport* registered_transport; - ON_CALL(*helper.channel_send(), RegisterTransport(_)) - .WillByDefault(Invoke([®istered_transport](Transport* transport) { - registered_transport = transport; - })); - - rtc::ScopedFakeClock fake_clock; - constexpr int64_t kTimeBetweenSendRtpCallsMs = 100; - { - auto send_stream = helper.CreateAudioSendStream(); - EXPECT_CALL(mock_transport, SendRtp(_, _, _)).Times(2); - const PacketOptions options; - registered_transport->SendRtp(nullptr, 0, options); - fake_clock.AdvanceTime(TimeDelta::ms(kTimeBetweenSendRtpCallsMs)); - registered_transport->SendRtp(nullptr, 0, options); - } - EXPECT_TRUE(!helper.active_lifetime()->Empty()); - EXPECT_EQ(helper.active_lifetime()->Length(), kTimeBetweenSendRtpCallsMs); -} } // namespace test } // namespace webrtc diff --git a/audio/test/media_transport_test.cc b/audio/test/media_transport_test.cc index 91677dbd8b..b4af4a0881 100644 --- a/audio/test/media_transport_test.cc +++ b/audio/test/media_transport_test.cc @@ -118,11 +118,10 @@ TEST(AudioWithMediaTransport, DeliversAudio) { rtc::TaskQueue send_tq("audio send queue"); std::unique_ptr send_process_thread = ProcessThread::Create("audio send thread"); - TimeInterval life_time; webrtc::internal::AudioSendStream send_stream( send_config, audio_state, &send_tq, send_process_thread.get(), /*transport=*/nullptr, &bitrate_allocator, null_event_log.get(), - /*rtcp_rtt_stats=*/nullptr, absl::optional(), &life_time); + /*rtcp_rtt_stats=*/nullptr, absl::optional()); audio_device->Init(); // Starts thread. audio_device->RegisterAudioCallback(audio_state->audio_transport()); diff --git a/audio/time_interval.cc b/audio/time_interval.cc deleted file mode 100644 index cc103408a3..0000000000 --- a/audio/time_interval.cc +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "audio/time_interval.h" -#include "rtc_base/checks.h" -#include "rtc_base/timeutils.h" - -namespace webrtc { - -TimeInterval::TimeInterval() = default; -TimeInterval::~TimeInterval() = default; - -void TimeInterval::Extend() { - Extend(rtc::TimeMillis()); -} - -void TimeInterval::Extend(int64_t time) { - if (!interval_) { - interval_.emplace(time, time); - } else { - if (time < interval_->first) { - interval_->first = time; - } - if (time > interval_->last) { - interval_->last = time; - } - } -} - -void TimeInterval::Extend(const TimeInterval& other_interval) { - if (!other_interval.Empty()) { - Extend(other_interval.interval_->first); - Extend(other_interval.interval_->last); - } -} - -bool TimeInterval::Empty() const { - return !interval_; -} - -int64_t TimeInterval::Length() const { - RTC_DCHECK(interval_); - return interval_->last - interval_->first; -} - -TimeInterval::Interval::Interval(int64_t first, int64_t last) - : first(first), last(last) {} - -} // namespace webrtc diff --git a/audio/time_interval.h b/audio/time_interval.h deleted file mode 100644 index 79fe29d9d5..0000000000 --- a/audio/time_interval.h +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef AUDIO_TIME_INTERVAL_H_ -#define AUDIO_TIME_INTERVAL_H_ - -#include - -#include "absl/types/optional.h" - -namespace webrtc { - -// This class logs the first and last time its Extend() function is called. -// -// This class is not thread-safe; Extend() calls should only be made by a -// single thread at a time, such as within a lock or destructor. -// -// Example usage: -// // let x < y < z < u < v -// rtc::TimeInterval interval; -// ... // interval.Extend(); // at time x -// ... -// interval.Extend(); // at time y -// ... -// interval.Extend(); // at time u -// ... -// interval.Extend(z); // at time v -// ... -// if (!interval.Empty()) { -// int64_t active_time = interval.Length(); // returns (u - x) -// } -class TimeInterval { - public: - TimeInterval(); - ~TimeInterval(); - // Extend the interval with the current time. - void Extend(); - // Extend the interval with a given time. - void Extend(int64_t time); - // Take the convex hull with another interval. - void Extend(const TimeInterval& other_interval); - // True iff Extend has never been called. - bool Empty() const; - // Returns the time between the first and the last tick, in milliseconds. - int64_t Length() const; - - private: - struct Interval { - Interval(int64_t first, int64_t last); - - int64_t first, last; - }; - absl::optional interval_; -}; - -} // namespace webrtc - -#endif // AUDIO_TIME_INTERVAL_H_ diff --git a/audio/time_interval_unittest.cc b/audio/time_interval_unittest.cc deleted file mode 100644 index deff6e363d..0000000000 --- a/audio/time_interval_unittest.cc +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright 2017 The WebRTC Project Authors. All rights reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "audio/time_interval.h" -#include "api/units/time_delta.h" -#include "rtc_base/fakeclock.h" -#include "test/gtest.h" - -namespace webrtc { - -TEST(TimeIntervalTest, TimeInMs) { - rtc::ScopedFakeClock fake_clock; - TimeInterval interval; - interval.Extend(); - fake_clock.AdvanceTime(TimeDelta::ms(100)); - interval.Extend(); - EXPECT_EQ(interval.Length(), 100); -} - -TEST(TimeIntervalTest, Empty) { - TimeInterval interval; - EXPECT_TRUE(interval.Empty()); - interval.Extend(); - EXPECT_FALSE(interval.Empty()); - interval.Extend(200); - EXPECT_FALSE(interval.Empty()); -} - -TEST(TimeIntervalTest, MonotoneIncreasing) { - const size_t point_count = 7; - const int64_t interval_points[] = {3, 2, 5, 0, 4, 1, 6}; - const int64_t interval_differences[] = {0, 1, 3, 5, 5, 5, 6}; - TimeInterval interval; - EXPECT_TRUE(interval.Empty()); - for (size_t i = 0; i < point_count; ++i) { - interval.Extend(interval_points[i]); - EXPECT_EQ(interval_differences[i], interval.Length()); - } -} - -} // namespace webrtc diff --git a/call/call.cc b/call/call.cc index f40f5fa330..95f64e9249 100644 --- a/call/call.cc +++ b/call/call.cc @@ -22,7 +22,6 @@ #include "audio/audio_receive_stream.h" #include "audio/audio_send_stream.h" #include "audio/audio_state.h" -#include "audio/time_interval.h" #include "call/bitrate_allocator.h" #include "call/call.h" #include "call/flexfec_receive_stream_impl.h" @@ -347,7 +346,6 @@ class Call final : public webrtc::Call, absl::optional last_received_rtp_audio_ms_; absl::optional first_received_rtp_video_ms_; absl::optional last_received_rtp_video_ms_; - TimeInterval sent_rtp_audio_timer_ms_; rtc::CriticalSection last_bandwidth_bps_crit_; uint32_t last_bandwidth_bps_ RTC_GUARDED_BY(&last_bandwidth_bps_crit_); @@ -541,11 +539,6 @@ void Call::UpdateHistograms() { void Call::UpdateSendHistograms(int64_t first_sent_packet_ms) { if (first_sent_packet_ms == -1) return; - if (!sent_rtp_audio_timer_ms_.Empty()) { - RTC_HISTOGRAM_COUNTS_100000( - "WebRTC.Call.TimeSendingAudioRtpPacketsInSeconds", - sent_rtp_audio_timer_ms_.Length() / 1000); - } int64_t elapsed_sec = (clock_->TimeInMilliseconds() - first_sent_packet_ms) / 1000; if (elapsed_sec < metrics::kMinRunTimeInSeconds) @@ -649,7 +642,7 @@ webrtc::AudioSendStream* Call::CreateAudioSendStream( config, config_.audio_state, transport_send_ptr_->GetWorkerQueue(), module_process_thread_.get(), transport_send_ptr_, bitrate_allocator_.get(), event_log_, call_stats_.get(), - suspended_rtp_state, &sent_rtp_audio_timer_ms_); + suspended_rtp_state); { WriteLockScoped write_lock(*send_crit_); RTC_DCHECK(audio_send_ssrcs_.find(config.rtp.ssrc) ==