From 28a44564c93b12839618dc0da2e2541ec6a0db23 Mon Sep 17 00:00:00 2001 From: Per Date: Wed, 4 May 2016 17:12:51 +0200 Subject: [PATCH] Revert "Revert of Remove SendPacer from ViEEncoder (patchset #13 id:240001 of https://codereview.webrtc.org/1917793002/ )" This reverts commit 825eb58d59940a4c3c9837595c4b3b07059c93ca. This Relands the cl reviewed in https://codereview.webrtc.org/1917793002/ patchset #1 is a pure reland. patchset #2 fix an overflow in BitrateProber that caused WebRtcVideoChannel2BaseTest.TwoStreamsSendAndReceive to fail. Original cl description: Remove SendPacer from ViEEncoder This CL moves the logic where the ViEEncoder pause if the pacer is full to the BitrateController. If the queue is full, the controller reports a bitrate of zero to Call (and BitrateAllocator) R=stefan@webrtc.org TBR=mflodman@webrtc.org BUG=webrtc:5687 Review URL: https://codereview.webrtc.org/1947873002 . Cr-Commit-Position: refs/heads/master@{#12630} --- webrtc/audio/audio_receive_stream_unittest.cc | 2 +- webrtc/audio/audio_send_stream_unittest.cc | 4 +- webrtc/call/bitrate_allocator.cc | 24 +- webrtc/call/bitrate_allocator.h | 4 +- webrtc/call/bitrate_allocator_unittest.cc | 13 + webrtc/call/call.cc | 11 +- .../bitrate_controller_impl.cc | 18 +- .../bitrate_controller_impl.h | 17 +- .../bitrate_controller_unittest.cc | 9 +- .../include/bitrate_controller.h | 14 +- .../include/mock/mock_bitrate_controller.h | 2 + .../congestion_controller.cc | 111 +++++- .../congestion_controller_unittest.cc | 111 ++++++ .../include/congestion_controller.h | 44 ++- .../include/mock/mock_congestion_controller.h | 14 +- webrtc/modules/modules.gyp | 1 + webrtc/modules/pacing/bitrate_prober.cc | 6 +- webrtc/modules/pacing/bitrate_prober.h | 6 +- .../modules/pacing/mock/mock_paced_sender.h | 3 +- webrtc/modules/pacing/paced_sender.cc | 63 +-- webrtc/modules/pacing/paced_sender.h | 38 +- .../modules/pacing/paced_sender_unittest.cc | 364 ++++++++---------- webrtc/modules/pacing/packet_router.h | 2 +- .../remote_bitrate_estimator.gypi | 1 + .../test/estimators/send_side.h | 1 - .../test/packet_sender.cc | 11 +- .../test/packet_sender.h | 2 +- .../video/encoder_state_feedback_unittest.cc | 13 +- webrtc/video/video_send_stream.cc | 12 +- webrtc/video/vie_encoder.cc | 15 +- webrtc/video/vie_encoder.h | 7 +- 31 files changed, 577 insertions(+), 366 deletions(-) create mode 100644 webrtc/modules/congestion_controller/congestion_controller_unittest.cc diff --git a/webrtc/audio/audio_receive_stream_unittest.cc b/webrtc/audio/audio_receive_stream_unittest.cc index 300ab20054..a5318d5150 100644 --- a/webrtc/audio/audio_receive_stream_unittest.cc +++ b/webrtc/audio/audio_receive_stream_unittest.cc @@ -157,7 +157,7 @@ struct ConfigHelper { private: SimulatedClock simulated_clock_; PacketRouter packet_router_; - testing::NiceMock bitrate_observer_; + testing::NiceMock bitrate_observer_; testing::NiceMock remote_bitrate_observer_; MockCongestionController congestion_controller_; MockRemoteBitrateEstimator remote_bitrate_estimator_; diff --git a/webrtc/audio/audio_send_stream_unittest.cc b/webrtc/audio/audio_send_stream_unittest.cc index 24efaada10..a94034c649 100644 --- a/webrtc/audio/audio_send_stream_unittest.cc +++ b/webrtc/audio/audio_send_stream_unittest.cc @@ -16,7 +16,7 @@ #include "webrtc/audio/audio_send_stream.h" #include "webrtc/audio/audio_state.h" #include "webrtc/audio/conversion.h" -#include "webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h" +#include "webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h" #include "webrtc/modules/congestion_controller/include/congestion_controller.h" #include "webrtc/modules/pacing/paced_sender.h" #include "webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h" @@ -161,7 +161,7 @@ struct ConfigHelper { rtc::scoped_refptr audio_state_; AudioSendStream::Config stream_config_; testing::StrictMock* channel_proxy_ = nullptr; - testing::NiceMock bitrate_observer_; + testing::NiceMock bitrate_observer_; testing::NiceMock remote_bitrate_observer_; CongestionController congestion_controller_; }; diff --git a/webrtc/call/bitrate_allocator.cc b/webrtc/call/bitrate_allocator.cc index 097378f02a..4c8d2a0c44 100644 --- a/webrtc/call/bitrate_allocator.cc +++ b/webrtc/call/bitrate_allocator.cc @@ -54,7 +54,9 @@ BitrateAllocator::ObserverBitrateMap BitrateAllocator::AllocateBitrates() { uint32_t sum_min_bitrates = 0; for (const auto& observer : bitrate_observers_) sum_min_bitrates += observer.second.min_bitrate; - if (last_bitrate_bps_ <= sum_min_bitrates) + if (last_bitrate_bps_ == 0) + return ZeroRateAllocation(); + else if (last_bitrate_bps_ <= sum_min_bitrates) return LowRateAllocation(last_bitrate_bps_); else return NormalRateAllocation(last_bitrate_bps_, sum_min_bitrates); @@ -104,18 +106,6 @@ void BitrateAllocator::RemoveObserver(BitrateAllocatorObserver* observer) { } } -void BitrateAllocator::GetMinMaxBitrateSumBps(int* min_bitrate_sum_bps, - int* max_bitrate_sum_bps) const { - *min_bitrate_sum_bps = 0; - *max_bitrate_sum_bps = 0; - - rtc::CritScope lock(&crit_sect_); - for (const auto& observer : bitrate_observers_) { - *min_bitrate_sum_bps += observer.second.min_bitrate; - *max_bitrate_sum_bps += observer.second.max_bitrate; - } -} - BitrateAllocator::BitrateObserverConfList::iterator BitrateAllocator::FindObserverConfigurationPair( const BitrateAllocatorObserver* observer) { @@ -170,6 +160,14 @@ BitrateAllocator::ObserverBitrateMap BitrateAllocator::NormalRateAllocation( return allocation; } +BitrateAllocator::ObserverBitrateMap BitrateAllocator::ZeroRateAllocation() { + ObserverBitrateMap allocation; + // Zero bitrate to all observers. + for (const auto& observer : bitrate_observers_) + allocation[observer.first] = 0; + return allocation; +} + BitrateAllocator::ObserverBitrateMap BitrateAllocator::LowRateAllocation( uint32_t bitrate) { ObserverBitrateMap allocation; diff --git a/webrtc/call/bitrate_allocator.h b/webrtc/call/bitrate_allocator.h index 404a312dad..fc88b783b5 100644 --- a/webrtc/call/bitrate_allocator.h +++ b/webrtc/call/bitrate_allocator.h @@ -60,9 +60,6 @@ class BitrateAllocator { void RemoveObserver(BitrateAllocatorObserver* observer); - void GetMinMaxBitrateSumBps(int* min_bitrate_sum_bps, - int* max_bitrate_sum_bps) const; - // This method controls the behavior when the available bitrate is lower than // the minimum bitrate, or the sum of minimum bitrates. // When true, the bitrate will never be set lower than the minimum bitrate(s). @@ -97,6 +94,7 @@ class BitrateAllocator { uint32_t sum_min_bitrates) EXCLUSIVE_LOCKS_REQUIRED(crit_sect_); + ObserverBitrateMap ZeroRateAllocation() EXCLUSIVE_LOCKS_REQUIRED(crit_sect_); ObserverBitrateMap LowRateAllocation(uint32_t bitrate) EXCLUSIVE_LOCKS_REQUIRED(crit_sect_); diff --git a/webrtc/call/bitrate_allocator_unittest.cc b/webrtc/call/bitrate_allocator_unittest.cc index 6e0cdd4d78..63149acbe3 100644 --- a/webrtc/call/bitrate_allocator_unittest.cc +++ b/webrtc/call/bitrate_allocator_unittest.cc @@ -96,6 +96,12 @@ TEST_F(BitrateAllocatorTest, TwoBitrateObserversOneRtcpObserver) { allocator_->OnNetworkChanged(1500000, 0, 50); EXPECT_EQ(600000u, bitrate_observer_1.last_bitrate_); EXPECT_EQ(600000u, bitrate_observer_2.last_bitrate_); + + // Verify that if the bandwidth estimate is set to zero, the allocated rate is + // zero. + allocator_->OnNetworkChanged(0, 0, 50); + EXPECT_EQ(0u, bitrate_observer_1.last_bitrate_); + EXPECT_EQ(0u, bitrate_observer_2.last_bitrate_); } class BitrateAllocatorTestNoEnforceMin : public ::testing::Test { @@ -171,6 +177,13 @@ TEST_F(BitrateAllocatorTestNoEnforceMin, ThreeBitrateObservers) { EXPECT_EQ(0u, bitrate_observer_2.last_bitrate_); EXPECT_EQ(0u, bitrate_observer_3.last_bitrate_); + allocator_->OnNetworkChanged(0, 0, 0); + // Verify that zero estimated bandwidth, means that that all gets zero, + // regardless of set min bitrate. + EXPECT_EQ(0u, bitrate_observer_1.last_bitrate_); + EXPECT_EQ(0u, bitrate_observer_2.last_bitrate_); + EXPECT_EQ(0u, bitrate_observer_3.last_bitrate_); + allocator_->RemoveObserver(&bitrate_observer_1); allocator_->RemoveObserver(&bitrate_observer_2); allocator_->RemoveObserver(&bitrate_observer_3); diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index 82ca630ad2..f7c66db4b2 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -52,8 +52,9 @@ const int Call::Config::kDefaultStartBitrateBps = 300000; namespace internal { -class Call : public webrtc::Call, public PacketReceiver, - public BitrateObserver { +class Call : public webrtc::Call, + public PacketReceiver, + public CongestionController::Observer { public: explicit Call(const Call::Config& config); virtual ~Call(); @@ -699,10 +700,8 @@ void Call::OnNetworkChanged(uint32_t target_bitrate_bps, uint8_t fraction_loss, pacer_bitrate_sum_kbits_ += pacer_bitrate_bps / 1000; ++num_bitrate_updates_; } - congestion_controller_->UpdatePacerBitrate( - target_bitrate_bps / 1000, - PacedSender::kDefaultPaceMultiplier * pacer_bitrate_bps / 1000, - pad_up_to_bitrate_bps / 1000); + congestion_controller_->SetAllocatedSendBitrate(allocated_bitrate_bps, + pad_up_to_bitrate_bps); } void Call::ConfigureSync(const std::string& sync_group) { diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc index 3c0d37c3b0..09652d8419 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc @@ -83,6 +83,10 @@ BitrateController* BitrateController::CreateBitrateController( return new BitrateControllerImpl(clock, observer); } +BitrateController* BitrateController::CreateBitrateController(Clock* clock) { + return new BitrateControllerImpl(clock, nullptr); +} + BitrateControllerImpl::BitrateControllerImpl(Clock* clock, BitrateObserver* observer) : clock_(clock), @@ -94,8 +98,8 @@ BitrateControllerImpl::BitrateControllerImpl(Clock* clock, last_fraction_loss_(0), last_rtt_ms_(0), last_reserved_bitrate_bps_(0) { - // This calls the observer_, which means that the observer provided by the - // user must be ready to accept a bitrate update when it constructs the + // This calls the observer_ if set, which means that the observer provided by + // the user must be ready to accept a bitrate update when it constructs the // controller. We do this to avoid having to keep synchronized initial values // in both the controller and the allocator. MaybeTriggerOnNetworkChanged(); @@ -199,11 +203,15 @@ void BitrateControllerImpl::OnReceivedRtcpReceiverReport( } void BitrateControllerImpl::MaybeTriggerOnNetworkChanged() { - uint32_t bitrate; + if (!observer_) + return; + + uint32_t bitrate_bps; uint8_t fraction_loss; int64_t rtt; - if (GetNetworkParameters(&bitrate, &fraction_loss, &rtt)) - observer_->OnNetworkChanged(bitrate, fraction_loss, rtt); + + if (GetNetworkParameters(&bitrate_bps, &fraction_loss, &rtt)) + observer_->OnNetworkChanged(bitrate_bps, fraction_loss, rtt); } bool BitrateControllerImpl::GetNetworkParameters(uint32_t* bitrate, diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h index a9661212d1..5a61379ce0 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h @@ -28,6 +28,8 @@ namespace webrtc { class BitrateControllerImpl : public BitrateController { public: + // TODO(perkj): BitrateObserver has been deprecated and is not used in WebRTC. + // |observer| is left for project that is not yet updated. BitrateControllerImpl(Clock* clock, BitrateObserver* observer); virtual ~BitrateControllerImpl() {} @@ -50,6 +52,11 @@ class BitrateControllerImpl : public BitrateController { void SetEventLog(RtcEventLog* event_log) override; + // Returns true if the parameters have changed since the last call. + bool GetNetworkParameters(uint32_t* bitrate, + uint8_t* fraction_loss, + int64_t* rtt) override; + int64_t TimeUntilNextProcess() override; void Process() override; @@ -64,20 +71,16 @@ class BitrateControllerImpl : public BitrateController { int number_of_packets, int64_t now_ms); + // Deprecated void MaybeTriggerOnNetworkChanged(); - // Returns true if the parameters have changed since the last call. - bool GetNetworkParameters(uint32_t* bitrate, - uint8_t* fraction_loss, - int64_t* rtt); - void OnNetworkChanged(uint32_t bitrate, uint8_t fraction_loss, // 0 - 255. int64_t rtt) EXCLUSIVE_LOCKS_REQUIRED(critsect_); // Used by process thread. - Clock* clock_; - BitrateObserver* observer_; + Clock* const clock_; + BitrateObserver* const observer_; int64_t last_bitrate_update_ms_; rtc::CriticalSection critsect_; diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc b/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc index 3f467ef8a4..4f92a3884b 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc @@ -14,11 +14,16 @@ #include "testing/gtest/include/gtest/gtest.h" #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" +#include "webrtc/modules/pacing/mock/mock_paced_sender.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" -using webrtc::RtcpBandwidthObserver; -using webrtc::BitrateObserver; +using ::testing::Exactly; +using ::testing::Return; + using webrtc::BitrateController; +using webrtc::BitrateObserver; +using webrtc::PacedSender; +using webrtc::RtcpBandwidthObserver; uint8_t WeightedLoss(int num_packets1, uint8_t fraction_loss1, int num_packets2, uint8_t fraction_loss2) { diff --git a/webrtc/modules/bitrate_controller/include/bitrate_controller.h b/webrtc/modules/bitrate_controller/include/bitrate_controller.h index d6cbc02d18..a61cf6a7a7 100644 --- a/webrtc/modules/bitrate_controller/include/bitrate_controller.h +++ b/webrtc/modules/bitrate_controller/include/bitrate_controller.h @@ -18,6 +18,7 @@ #include #include "webrtc/modules/include/module.h" +#include "webrtc/modules/pacing/paced_sender.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" namespace webrtc { @@ -26,6 +27,8 @@ class CriticalSectionWrapper; class RtcEventLog; struct PacketInfo; +// Deprecated +// TODO(perkj): Remove BitrateObserver when no implementations use it. class BitrateObserver { // Observer class for bitrate changes announced due to change in bandwidth // estimate or due to bitrate allocation changes. Fraction loss and rtt is @@ -46,10 +49,15 @@ class BitrateController : public Module { // estimation and divide the available bitrate between all its registered // BitrateObservers. public: - static const int kDefaultStartBitrateKbps = 300; + static const int kDefaultStartBitratebps = 300000; + // Deprecated: + // TODO(perkj): BitrateObserver has been deprecated and is not used in WebRTC. + // Remove this method once other other projects does not use it. static BitrateController* CreateBitrateController(Clock* clock, BitrateObserver* observer); + static BitrateController* CreateBitrateController(Clock* clock); + virtual ~BitrateController() {} virtual RtcpBandwidthObserver* CreateRtcpBandwidthObserver() = 0; @@ -71,6 +79,10 @@ class BitrateController : public Module { virtual bool AvailableBandwidth(uint32_t* bandwidth) const = 0; virtual void SetReservedBitrate(uint32_t reserved_bitrate_bps) = 0; + + virtual bool GetNetworkParameters(uint32_t* bitrate, + uint8_t* fraction_loss, + int64_t* rtt) = 0; }; } // namespace webrtc #endif // WEBRTC_MODULES_BITRATE_CONTROLLER_INCLUDE_BITRATE_CONTROLLER_H_ diff --git a/webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h b/webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h index 5290b01106..da6169e748 100644 --- a/webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h +++ b/webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h @@ -39,6 +39,8 @@ class MockBitrateController : public BitrateController { MOCK_METHOD1(SetEventLog, void(RtcEventLog* event_log)); MOCK_CONST_METHOD1(AvailableBandwidth, bool(uint32_t* bandwidth)); MOCK_METHOD1(SetReservedBitrate, void(uint32_t reserved_bitrate_bps)); + MOCK_METHOD3(GetNetworkParameters, + bool(uint32_t* bitrate, uint8_t* fraction_loss, int64_t* rtt)); MOCK_METHOD0(Process, void()); MOCK_METHOD0(TimeUntilNextProcess, int64_t()); diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 14a73fe1cd..d557843be2 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -20,7 +20,6 @@ #include "webrtc/base/socket.h" #include "webrtc/base/thread_annotations.h" #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" -#include "webrtc/modules/pacing/paced_sender.h" #include "webrtc/modules/remote_bitrate_estimator/include/send_time_history.h" #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h" #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h" @@ -141,28 +140,76 @@ CongestionController::CongestionController( BitrateObserver* bitrate_observer, RemoteBitrateObserver* remote_bitrate_observer) : clock_(clock), + observer_(nullptr), + packet_router_(new PacketRouter()), pacer_(new PacedSender(clock_, - &packet_router_, - BitrateController::kDefaultStartBitrateKbps, - PacedSender::kDefaultPaceMultiplier * - BitrateController::kDefaultStartBitrateKbps, - 0)), + packet_router_.get(), + BitrateController::kDefaultStartBitratebps)), + remote_bitrate_estimator_( + new WrappingBitrateEstimator(remote_bitrate_observer, clock_)), + bitrate_controller_( + BitrateController::CreateBitrateController(clock_, bitrate_observer)), + remote_estimator_proxy_(clock_, packet_router_.get()), + transport_feedback_adapter_(bitrate_controller_.get(), clock_), + min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), + send_queue_is_full_(false) { + Init(); +} + +CongestionController::CongestionController( + Clock* clock, + Observer* observer, + RemoteBitrateObserver* remote_bitrate_observer) + : clock_(clock), + observer_(observer), + packet_router_(new PacketRouter()), + pacer_(new PacedSender(clock_, + packet_router_.get(), + BitrateController::kDefaultStartBitratebps)), + remote_bitrate_estimator_( + new WrappingBitrateEstimator(remote_bitrate_observer, clock_)), + bitrate_controller_(BitrateController::CreateBitrateController(clock_)), + remote_estimator_proxy_(clock_, packet_router_.get()), + transport_feedback_adapter_(bitrate_controller_.get(), clock_), + min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), + send_queue_is_full_(false) { + Init(); +} + +CongestionController::CongestionController( + Clock* clock, + Observer* observer, + RemoteBitrateObserver* remote_bitrate_observer, + std::unique_ptr packet_router, + std::unique_ptr pacer) + : clock_(clock), + observer_(observer), + packet_router_(std::move(packet_router)), + pacer_(std::move(pacer)), remote_bitrate_estimator_( new WrappingBitrateEstimator(remote_bitrate_observer, clock_)), // Constructed last as this object calls the provided callback on // construction. - bitrate_controller_( - BitrateController::CreateBitrateController(clock_, bitrate_observer)), - remote_estimator_proxy_(clock_, &packet_router_), + bitrate_controller_(BitrateController::CreateBitrateController(clock_)), + remote_estimator_proxy_(clock_, packet_router_.get()), transport_feedback_adapter_(bitrate_controller_.get(), clock_), - min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps) { + min_bitrate_bps_(RemoteBitrateEstimator::kDefaultMinBitrateBps), + send_queue_is_full_(false) { + Init(); +} + +CongestionController::~CongestionController() {} + +void CongestionController::Init() { transport_feedback_adapter_.SetBitrateEstimator( new RemoteBitrateEstimatorAbsSendTime(&transport_feedback_adapter_)); transport_feedback_adapter_.GetBitrateEstimator()->SetMinBitrate( min_bitrate_bps_); -} - -CongestionController::~CongestionController() { + // This calls the observer_, which means that the observer provided by the + // user must be ready to accept a bitrate update when it constructs the + // controller. We do this to avoid having to keep synchronized initial values + // in both the controller and the allocator. + MaybeTriggerOnNetworkChanged(); } @@ -189,6 +236,7 @@ void CongestionController::SetBweBitrates(int min_bitrate_bps, min_bitrate_bps_ = min_bitrate_bps; transport_feedback_adapter_.GetBitrateEstimator()->SetMinBitrate( min_bitrate_bps_); + MaybeTriggerOnNetworkChanged(); } BitrateController* CongestionController::GetBitrateController() const { @@ -209,10 +257,9 @@ CongestionController::GetTransportFeedbackObserver() { return &transport_feedback_adapter_; } -void CongestionController::UpdatePacerBitrate(int bitrate_kbps, - int max_bitrate_kbps, - int min_bitrate_kbps) { - pacer_->UpdateBitrate(bitrate_kbps, max_bitrate_kbps, min_bitrate_kbps); +void CongestionController::SetAllocatedSendBitrate(int allocated_bitrate_bps, + int padding_bitrate_bps) { + pacer_->SetAllocatedSendBitrate(allocated_bitrate_bps, padding_bitrate_bps); } int64_t CongestionController::GetPacerQueuingDelayMs() const { @@ -245,6 +292,36 @@ int64_t CongestionController::TimeUntilNextProcess() { void CongestionController::Process() { bitrate_controller_->Process(); remote_bitrate_estimator_->Process(); + MaybeTriggerOnNetworkChanged(); +} + +void CongestionController::MaybeTriggerOnNetworkChanged() { + // TODO(perkj): |observer_| can be nullptr if the ctor that accepts a + // BitrateObserver is used. Remove this check once the ctor is removed. + if (!observer_) + return; + + uint32_t bitrate_bps; + uint8_t fraction_loss; + int64_t rtt; + bool network_changed = bitrate_controller_->GetNetworkParameters( + &bitrate_bps, &fraction_loss, &rtt); + if (network_changed) + pacer_->SetEstimatedBitrate(bitrate_bps); + bool send_queue_is_full = + pacer_->ExpectedQueueTimeMs() > PacedSender::kMaxQueueLengthMs; + bitrate_bps = send_queue_is_full ? 0 : bitrate_bps; + if ((network_changed && !send_queue_is_full) || + UpdateSendQueueStatus(send_queue_is_full)) { + observer_->OnNetworkChanged(bitrate_bps, fraction_loss, rtt); + } +} + +bool CongestionController::UpdateSendQueueStatus(bool send_queue_is_full) { + rtc::CritScope cs(&critsect_); + bool result = send_queue_is_full_ != send_queue_is_full; + send_queue_is_full_ = send_queue_is_full; + return result; } } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc new file mode 100644 index 0000000000..a86e43329b --- /dev/null +++ b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc @@ -0,0 +1,111 @@ +/* + * Copyright (c) 2016 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 "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "webrtc/modules/pacing/mock/mock_paced_sender.h" +#include "webrtc/modules/congestion_controller/include/congestion_controller.h" +#include "webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h" +#include "webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_observer.h" +#include "webrtc/system_wrappers/include/clock.h" + +using testing::_; +using testing::NiceMock; +using testing::Return; +using testing::SaveArg; +using testing::StrictMock; + +namespace webrtc { +namespace test { + +class CongestionControllerTest : public ::testing::Test { + protected: + CongestionControllerTest() : clock_(123456) {} + ~CongestionControllerTest() override {} + + void SetUp() override { + EXPECT_CALL(observer_, OnNetworkChanged(_, _, _)) + .WillOnce(SaveArg<0>(&initial_bitrate_bps_)); + + pacer_ = new NiceMock(); + std::unique_ptr pacer(pacer_); // Passes ownership. + std::unique_ptr packet_router(new PacketRouter()); + controller_.reset( + new CongestionController(&clock_, &observer_, &remote_bitrate_observer_, + std::move(packet_router), std::move(pacer))); + EXPECT_GT(initial_bitrate_bps_, 0u); + bandwidth_observer_.reset( + controller_->GetBitrateController()->CreateRtcpBandwidthObserver()); + } + + SimulatedClock clock_; + StrictMock observer_; + NiceMock* pacer_; + NiceMock remote_bitrate_observer_; + std::unique_ptr bandwidth_observer_; + std::unique_ptr controller_; + uint32_t initial_bitrate_bps_ = 0; +}; + +TEST_F(CongestionControllerTest, OnNetworkChanged) { + // Test no change. + clock_.AdvanceTimeMilliseconds(25); + controller_->Process(); + + EXPECT_CALL(observer_, OnNetworkChanged(initial_bitrate_bps_ * 2, _, _)); + bandwidth_observer_->OnReceivedEstimatedBitrate(initial_bitrate_bps_ * 2); + clock_.AdvanceTimeMilliseconds(25); + controller_->Process(); + + EXPECT_CALL(observer_, OnNetworkChanged(initial_bitrate_bps_, _, _)); + bandwidth_observer_->OnReceivedEstimatedBitrate(initial_bitrate_bps_); + clock_.AdvanceTimeMilliseconds(25); + controller_->Process(); +} + +TEST_F(CongestionControllerTest, OnSendQueueFull) { + EXPECT_CALL(*pacer_, ExpectedQueueTimeMs()) + .WillOnce(Return(PacedSender::kMaxQueueLengthMs + 1)); + + EXPECT_CALL(observer_, OnNetworkChanged(0, _, _)); + controller_->Process(); + + // Let the pacer not be full next time the controller checks. + EXPECT_CALL(*pacer_, ExpectedQueueTimeMs()) + .WillOnce(Return(PacedSender::kMaxQueueLengthMs - 1)); + + EXPECT_CALL(observer_, OnNetworkChanged(initial_bitrate_bps_, _, _)); + controller_->Process(); +} + +TEST_F(CongestionControllerTest, OnSendQueueFullAndEstimateChange) { + EXPECT_CALL(*pacer_, ExpectedQueueTimeMs()) + .WillOnce(Return(PacedSender::kMaxQueueLengthMs + 1)); + EXPECT_CALL(observer_, OnNetworkChanged(0, _, _)); + controller_->Process(); + + // Receive new estimate but let the queue still be full. + bandwidth_observer_->OnReceivedEstimatedBitrate(initial_bitrate_bps_ * 2); + EXPECT_CALL(*pacer_, ExpectedQueueTimeMs()) + .WillOnce(Return(PacedSender::kMaxQueueLengthMs + 1)); + clock_.AdvanceTimeMilliseconds(25); + controller_->Process(); + + // Let the pacer not be full next time the controller checks. + // |OnNetworkChanged| should be called with the new estimate. + EXPECT_CALL(*pacer_, ExpectedQueueTimeMs()) + .WillOnce(Return(PacedSender::kMaxQueueLengthMs - 1)); + EXPECT_CALL(observer_, OnNetworkChanged(initial_bitrate_bps_ * 2, _, _)); + clock_.AdvanceTimeMilliseconds(25); + controller_->Process(); +} + +} // namespace test +} // namespace webrtc diff --git a/webrtc/modules/congestion_controller/include/congestion_controller.h b/webrtc/modules/congestion_controller/include/congestion_controller.h index 284070cc21..a3b672e3bc 100644 --- a/webrtc/modules/congestion_controller/include/congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/congestion_controller.h @@ -19,6 +19,7 @@ #include "webrtc/modules/include/module.h" #include "webrtc/modules/include/module_common_types.h" #include "webrtc/modules/pacing/packet_router.h" +#include "webrtc/modules/pacing/paced_sender.h" #include "webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h" #include "webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h" @@ -31,7 +32,6 @@ namespace webrtc { class BitrateController; class BitrateObserver; class Clock; -class PacedSender; class ProcessThread; class RemoteBitrateEstimator; class RemoteBitrateObserver; @@ -39,9 +39,33 @@ class TransportFeedbackObserver; class CongestionController : public CallStatsObserver, public Module { public: + // Observer class for bitrate changes announced due to change in bandwidth + // estimate or due to that the send pacer is full. Fraction loss and rtt is + // also part of this callback to allow the observer to optimize its settings + // for different types of network environments. The bitrate does not include + // packet headers and is measured in bits per second. + class Observer { + public: + virtual void OnNetworkChanged(uint32_t bitrate_bps, + uint8_t fraction_loss, // 0 - 255. + int64_t rtt_ms) = 0; + + protected: + virtual ~Observer() {} + }; + // Deprecated + // TODO(perkj): Remove once no other clients use this ctor. CongestionController(Clock* clock, BitrateObserver* bitrate_observer, RemoteBitrateObserver* remote_bitrate_observer); + CongestionController(Clock* clock, + Observer* observer, + RemoteBitrateObserver* remote_bitrate_observer); + CongestionController(Clock* clock, + Observer* observer, + RemoteBitrateObserver* remote_bitrate_observer, + std::unique_ptr packet_router, + std::unique_ptr pacer); virtual ~CongestionController(); virtual void SetBweBitrates(int min_bitrate_bps, @@ -53,12 +77,11 @@ class CongestionController : public CallStatsObserver, public Module { bool send_side_bwe); virtual int64_t GetPacerQueuingDelayMs() const; virtual PacedSender* pacer() { return pacer_.get(); } - virtual PacketRouter* packet_router() { return &packet_router_; } + virtual PacketRouter* packet_router() { return packet_router_.get(); } virtual TransportFeedbackObserver* GetTransportFeedbackObserver(); - virtual void UpdatePacerBitrate(int bitrate_kbps, - int max_bitrate_kbps, - int min_bitrate_kbps); + void SetAllocatedSendBitrate(int allocated_bitrate_bps, + int padding_bitrate_bps); virtual void OnSentPacket(const rtc::SentPacket& sent_packet); @@ -70,14 +93,23 @@ class CongestionController : public CallStatsObserver, public Module { void Process() override; private: + void Init(); + void MaybeTriggerOnNetworkChanged(); + // Updates |send_queue_is_full_|. Returns true if |send_queue_is_full_| + // has changed. + bool UpdateSendQueueStatus(bool send_queue_is_full); + Clock* const clock_; + Observer* const observer_; + const std::unique_ptr packet_router_; const std::unique_ptr pacer_; const std::unique_ptr remote_bitrate_estimator_; const std::unique_ptr bitrate_controller_; - PacketRouter packet_router_; RemoteEstimatorProxy remote_estimator_proxy_; TransportFeedbackAdapter transport_feedback_adapter_; int min_bitrate_bps_; + rtc::CriticalSection critsect_; + bool send_queue_is_full_ GUARDED_BY(critsect_); RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(CongestionController); }; diff --git a/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h b/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h index c5b2412845..20955ea81a 100644 --- a/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h @@ -19,14 +19,20 @@ namespace webrtc { namespace test { +class MockCongestionObserver : public CongestionController::Observer { + public: + MOCK_METHOD3(OnNetworkChanged, + void(uint32_t bitrate_bps, + uint8_t fraction_loss, + int64_t rtt_ms)); +}; + class MockCongestionController : public CongestionController { public: MockCongestionController(Clock* clock, - BitrateObserver* bitrate_observer, + Observer* observer, RemoteBitrateObserver* remote_bitrate_observer) - : CongestionController(clock, - bitrate_observer, - remote_bitrate_observer) {} + : CongestionController(clock, observer, remote_bitrate_observer) {} MOCK_METHOD3(SetBweBitrates, void(int min_bitrate_bps, int start_bitrate_bps, diff --git a/webrtc/modules/modules.gyp b/webrtc/modules/modules.gyp index 921697e528..4365c653f0 100644 --- a/webrtc/modules/modules.gyp +++ b/webrtc/modules/modules.gyp @@ -270,6 +270,7 @@ 'audio_processing/vad/voice_activity_detector_unittest.cc', 'bitrate_controller/bitrate_controller_unittest.cc', 'bitrate_controller/send_side_bandwidth_estimation_unittest.cc', + 'congestion_controller/congestion_controller_unittest.cc', 'media_file/media_file_unittest.cc', 'module_common_types_unittest.cc', 'pacing/bitrate_prober_unittest.cc', diff --git a/webrtc/modules/pacing/bitrate_prober.cc b/webrtc/modules/pacing/bitrate_prober.cc index fbd9b81741..ca09ee2611 100644 --- a/webrtc/modules/pacing/bitrate_prober.cc +++ b/webrtc/modules/pacing/bitrate_prober.cc @@ -21,7 +21,7 @@ namespace webrtc { namespace { -int ComputeDeltaFromBitrate(size_t packet_size, int bitrate_bps) { +int ComputeDeltaFromBitrate(size_t packet_size, uint32_t bitrate_bps) { assert(bitrate_bps > 0); // Compute the time delta needed to send packet_size bytes at bitrate_bps // bps. Result is in milliseconds. @@ -52,7 +52,7 @@ bool BitrateProber::IsProbing() const { return probing_state_ == kProbing; } -void BitrateProber::OnIncomingPacket(int bitrate_bps, +void BitrateProber::OnIncomingPacket(uint32_t bitrate_bps, size_t packet_size, int64_t now_ms) { // Don't initialize probing unless we have something large enough to start @@ -66,7 +66,7 @@ void BitrateProber::OnIncomingPacket(int bitrate_bps, const int kMaxNumProbes = 2; const int kPacketsPerProbe = 5; const float kProbeBitrateMultipliers[kMaxNumProbes] = {3, 6}; - int bitrates_bps[kMaxNumProbes]; + uint32_t bitrates_bps[kMaxNumProbes]; std::stringstream bitrate_log; bitrate_log << "Start probing for bandwidth, bitrates:"; for (int i = 0; i < kMaxNumProbes; ++i) { diff --git a/webrtc/modules/pacing/bitrate_prober.h b/webrtc/modules/pacing/bitrate_prober.h index 84fbc522fc..0749ce4891 100644 --- a/webrtc/modules/pacing/bitrate_prober.h +++ b/webrtc/modules/pacing/bitrate_prober.h @@ -34,7 +34,9 @@ class BitrateProber { // Initializes a new probing session if the prober is allowed to probe. Does // not initialize the prober unless the packet size is large enough to probe // with. - void OnIncomingPacket(int bitrate_bps, size_t packet_size, int64_t now_ms); + void OnIncomingPacket(uint32_t bitrate_bps, + size_t packet_size, + int64_t now_ms); // Returns the number of milliseconds until the next packet should be sent to // get accurate probing. @@ -55,7 +57,7 @@ class BitrateProber { // Probe bitrate per packet. These are used to compute the delta relative to // the previous probe packet based on the size and time when that packet was // sent. - std::list probe_bitrates_; + std::list probe_bitrates_; size_t packet_size_last_send_; int64_t time_last_send_ms_; }; diff --git a/webrtc/modules/pacing/mock/mock_paced_sender.h b/webrtc/modules/pacing/mock/mock_paced_sender.h index c710dbcbea..c0dd4887c8 100644 --- a/webrtc/modules/pacing/mock/mock_paced_sender.h +++ b/webrtc/modules/pacing/mock/mock_paced_sender.h @@ -22,7 +22,7 @@ namespace webrtc { class MockPacedSender : public PacedSender { public: - MockPacedSender() : PacedSender(Clock::GetRealTimeClock(), NULL, 0, 0, 0) {} + MockPacedSender() : PacedSender(Clock::GetRealTimeClock(), nullptr, 0) {} MOCK_METHOD6(SendPacket, bool(Priority priority, uint32_t ssrc, uint16_t sequence_number, @@ -31,6 +31,7 @@ class MockPacedSender : public PacedSender { bool retransmission)); MOCK_CONST_METHOD0(QueueInMs, int64_t()); MOCK_CONST_METHOD0(QueueInPackets, int()); + MOCK_CONST_METHOD0(ExpectedQueueTimeMs, int64_t()); }; } // namespace webrtc diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc index b56d28510f..418c115c0c 100644 --- a/webrtc/modules/pacing/paced_sender.cc +++ b/webrtc/modules/pacing/paced_sender.cc @@ -246,20 +246,21 @@ const int64_t PacedSender::kMaxQueueLengthMs = 2000; const float PacedSender::kDefaultPaceMultiplier = 2.5f; PacedSender::PacedSender(Clock* clock, - Callback* callback, - int bitrate_kbps, - int max_bitrate_kbps, - int min_bitrate_kbps) + PacketSender* packet_sender, + int estimated_bitrate_bps) : clock_(clock), - callback_(callback), + packet_sender_(packet_sender), critsect_(CriticalSectionWrapper::CreateCriticalSection()), paused_(false), probing_enabled_(true), - media_budget_(new paced_sender::IntervalBudget(max_bitrate_kbps)), - padding_budget_(new paced_sender::IntervalBudget(min_bitrate_kbps)), + media_budget_(new paced_sender::IntervalBudget( + estimated_bitrate_bps / 1000 * kDefaultPaceMultiplier)), + padding_budget_(new paced_sender::IntervalBudget(0)), prober_(new BitrateProber()), - bitrate_bps_(1000 * bitrate_kbps), - max_bitrate_kbps_(max_bitrate_kbps), + estimated_bitrate_bps_(estimated_bitrate_bps), + min_send_bitrate_kbps_(0u), + pacing_bitrate_kbps_(estimated_bitrate_bps / 1000 * + kDefaultPaceMultiplier), time_last_update_us_(clock->TimeInMicroseconds()), packets_(new paced_sender::PacketQueue(clock)), packet_counter_(0) { @@ -283,16 +284,24 @@ void PacedSender::SetProbingEnabled(bool enabled) { probing_enabled_ = enabled; } -void PacedSender::UpdateBitrate(int bitrate_kbps, - int max_bitrate_kbps, - int min_bitrate_kbps) { +void PacedSender::SetEstimatedBitrate(uint32_t bitrate_bps) { + LOG(LS_INFO) << "SetNetWorkEstimateTargetBitrate, bitrate " << bitrate_bps; + CriticalSectionScoped cs(critsect_.get()); - // Don't set media bitrate here as it may be boosted in order to meet max - // queue time constraint. Just update max_bitrate_kbps_ and let media_budget_ - // be updated in Process(). - padding_budget_->set_target_rate_kbps(min_bitrate_kbps); - bitrate_bps_ = 1000 * bitrate_kbps; - max_bitrate_kbps_ = max_bitrate_kbps; + estimated_bitrate_bps_ = bitrate_bps; + pacing_bitrate_kbps_ = + std::max(min_send_bitrate_kbps_, estimated_bitrate_bps_ / 1000) * + kDefaultPaceMultiplier; +} + +void PacedSender::SetAllocatedSendBitrate(int allocated_bitrate, + int padding_bitrate) { + CriticalSectionScoped cs(critsect_.get()); + min_send_bitrate_kbps_ = allocated_bitrate / 1000; + pacing_bitrate_kbps_ = + std::max(min_send_bitrate_kbps_, estimated_bitrate_bps_ / 1000) * + kDefaultPaceMultiplier; + padding_budget_->set_target_rate_kbps(padding_bitrate / 1000); } void PacedSender::InsertPacket(RtpPacketSender::Priority priority, @@ -306,7 +315,7 @@ void PacedSender::InsertPacket(RtpPacketSender::Priority priority, if (probing_enabled_ && !prober_->IsProbing()) prober_->SetEnabled(true); int64_t now_ms = clock_->TimeInMilliseconds(); - prober_->OnIncomingPacket(bitrate_bps_, bytes, now_ms); + prober_->OnIncomingPacket(estimated_bitrate_bps_, bytes, now_ms); if (capture_time_ms < 0) capture_time_ms = now_ms; @@ -318,8 +327,9 @@ void PacedSender::InsertPacket(RtpPacketSender::Priority priority, int64_t PacedSender::ExpectedQueueTimeMs() const { CriticalSectionScoped cs(critsect_.get()); - RTC_DCHECK_GT(max_bitrate_kbps_, 0); - return static_cast(packets_->SizeInBytes() * 8 / max_bitrate_kbps_); + RTC_DCHECK_GT(pacing_bitrate_kbps_, 0u); + return static_cast(packets_->SizeInBytes() * 8 / + pacing_bitrate_kbps_); } size_t PacedSender::QueueSizePackets() const { @@ -360,7 +370,7 @@ void PacedSender::Process() { CriticalSectionScoped cs(critsect_.get()); int64_t elapsed_time_ms = (now_us - time_last_update_us_ + 500) / 1000; time_last_update_us_ = now_us; - int target_bitrate_kbps = max_bitrate_kbps_; + int target_bitrate_kbps = pacing_bitrate_kbps_; // TODO(holmer): Remove the !paused_ check when issue 5307 has been fixed. if (!paused_ && elapsed_time_ms > 0) { size_t queue_size_bytes = packets_->SizeInBytes(); @@ -425,10 +435,9 @@ bool PacedSender::SendPacket(const paced_sender::Packet& packet) { if (paused_ && packet.priority != kHighPriority) return false; critsect_->Leave(); - const bool success = callback_->TimeToSendPacket(packet.ssrc, - packet.sequence_number, - packet.capture_time_ms, - packet.retransmission); + const bool success = packet_sender_->TimeToSendPacket( + packet.ssrc, packet.sequence_number, packet.capture_time_ms, + packet.retransmission); critsect_->Enter(); if (success) { @@ -447,7 +456,7 @@ bool PacedSender::SendPacket(const paced_sender::Packet& packet) { void PacedSender::SendPadding(size_t padding_needed) { critsect_->Leave(); - size_t bytes_sent = callback_->TimeToSendPadding(padding_needed); + size_t bytes_sent = packet_sender_->TimeToSendPadding(padding_needed); critsect_->Enter(); if (bytes_sent > 0) { diff --git a/webrtc/modules/pacing/paced_sender.h b/webrtc/modules/pacing/paced_sender.h index 16569b0404..52b63e53bc 100644 --- a/webrtc/modules/pacing/paced_sender.h +++ b/webrtc/modules/pacing/paced_sender.h @@ -33,7 +33,7 @@ class PacketQueue; class PacedSender : public Module, public RtpPacketSender { public: - class Callback { + class PacketSender { public: // Note: packets sent as a result of a callback should not pass by this // module again. @@ -48,7 +48,7 @@ class PacedSender : public Module, public RtpPacketSender { virtual size_t TimeToSendPadding(size_t bytes) = 0; protected: - virtual ~Callback() {} + virtual ~PacketSender() {} }; // Expected max pacer delay in ms. If ExpectedQueueTimeMs() is higher than @@ -68,10 +68,8 @@ class PacedSender : public Module, public RtpPacketSender { static const size_t kMinProbePacketSize = 200; PacedSender(Clock* clock, - Callback* callback, - int bitrate_kbps, - int max_bitrate_kbps, - int min_bitrate_kbps); + PacketSender* packet_sender, + int target_bitrate_bps); virtual ~PacedSender(); @@ -86,14 +84,19 @@ class PacedSender : public Module, public RtpPacketSender { // effect. void SetProbingEnabled(bool enabled); - // Set target bitrates for the pacer. - // We will pace out bursts of packets at a bitrate of |max_bitrate_kbps|. - // |bitrate_kbps| is our estimate of what we are allowed to send on average. - // Padding packets will be utilized to reach |min_bitrate| unless enough media - // packets are available. - void UpdateBitrate(int bitrate_kbps, - int max_bitrate_kbps, - int min_bitrate_kbps); + // Sets the estimated capacity of the network. + // |bitrate_bps| is our estimate of what we are allowed to send on average. + // We will pace out bursts of packets at a bitrate of + // |bitrate_bps| * kDefaultPaceMultiplier. + void SetEstimatedBitrate(uint32_t bitrate_bps); + + // Sets the bitrate that has been allocated for encoders. + // |allocated_bitrate| might be higher that the estimated available network + // bitrate and if so, the pacer will send with |allocated_bitrate|. + // Padding packets will be utilized to reach |padding_bitrate| unless enough + // media packets are available. + void SetAllocatedSendBitrate(int allocated_bitrate_bps, + int padding_bitrate_bps); // Returns true if we send the packet now, else it will add the packet // information to the queue and call TimeToSendPacket when it's time to send. @@ -134,7 +137,7 @@ class PacedSender : public Module, public RtpPacketSender { void SendPadding(size_t padding_needed) EXCLUSIVE_LOCKS_REQUIRED(critsect_); Clock* const clock_; - Callback* const callback_; + PacketSender* const packet_sender_; std::unique_ptr critsect_; bool paused_ GUARDED_BY(critsect_); @@ -152,8 +155,9 @@ class PacedSender : public Module, public RtpPacketSender { std::unique_ptr prober_ GUARDED_BY(critsect_); // Actual configured bitrates (media_budget_ may temporarily be higher in // order to meet pace time constraint). - int bitrate_bps_ GUARDED_BY(critsect_); - int max_bitrate_kbps_ GUARDED_BY(critsect_); + uint32_t estimated_bitrate_bps_ GUARDED_BY(critsect_); + uint32_t min_send_bitrate_kbps_ GUARDED_BY(critsect_); + uint32_t pacing_bitrate_kbps_ GUARDED_BY(critsect_); int64_t time_last_update_us_ GUARDED_BY(critsect_); diff --git a/webrtc/modules/pacing/paced_sender_unittest.cc b/webrtc/modules/pacing/paced_sender_unittest.cc index 941c81335b..15bb462949 100644 --- a/webrtc/modules/pacing/paced_sender_unittest.cc +++ b/webrtc/modules/pacing/paced_sender_unittest.cc @@ -22,10 +22,9 @@ using testing::Return; namespace webrtc { namespace test { -static const int kTargetBitrate = 800; -static const float kPaceMultiplier = 1.5f; +static const int kTargetBitrateBps = 800000; -class MockPacedSenderCallback : public PacedSender::Callback { +class MockPacedSenderCallback : public PacedSender::PacketSender { public: MOCK_METHOD4(TimeToSendPacket, bool(uint32_t ssrc, @@ -36,7 +35,7 @@ class MockPacedSenderCallback : public PacedSender::Callback { size_t(size_t bytes)); }; -class PacedSenderPadding : public PacedSender::Callback { +class PacedSenderPadding : public PacedSender::PacketSender { public: PacedSenderPadding() : padding_sent_(0) {} @@ -60,7 +59,7 @@ class PacedSenderPadding : public PacedSender::Callback { size_t padding_sent_; }; -class PacedSenderProbing : public PacedSender::Callback { +class PacedSenderProbing : public PacedSender::PacketSender { public: PacedSenderProbing(const std::list& expected_deltas, Clock* clock) : prev_packet_time_ms_(-1), @@ -108,11 +107,7 @@ class PacedSenderTest : public ::testing::Test { PacedSenderTest() : clock_(123456) { srand(0); // Need to initialize PacedSender after we initialize clock. - send_bucket_.reset(new PacedSender(&clock_, - &callback_, - kTargetBitrate, - kPaceMultiplier * kTargetBitrate, - 0)); + send_bucket_.reset(new PacedSender(&clock_, &callback_, kTargetBitrateBps)); // Default to bitrate probing disabled for testing purposes. Probing tests // have to enable probing, either by creating a new PacedSender instance or // by calling SetProbingEnabled(true). @@ -141,29 +136,21 @@ class PacedSenderTest : public ::testing::Test { TEST_F(PacedSenderTest, QueuePacket) { uint32_t ssrc = 12345; uint16_t sequence_number = 1234; - // Due to the multiplicative factor we can send 3 packets not 2 packets. - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); + // Due to the multiplicative factor we can send 5 packets during a send + // interval. (network capacity * multiplier / (8 bits per byte * + // (packet size * #send intervals per second) + const size_t packets_to_send = + kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200); + for (size_t i = 0; i < packets_to_send; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); + } + int64_t queued_packet_timestamp = clock_.TimeInMilliseconds(); send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, sequence_number, queued_packet_timestamp, 250, false); + EXPECT_EQ(packets_to_send + 1, send_bucket_->QueueSizePackets()); send_bucket_->Process(); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); EXPECT_CALL(callback_, TimeToSendPadding(_)).Times(0); @@ -171,86 +158,79 @@ TEST_F(PacedSenderTest, QueuePacket) { EXPECT_EQ(1, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(1); EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); - EXPECT_CALL( - callback_, - TimeToSendPacket(ssrc, sequence_number++, queued_packet_timestamp, false)) + EXPECT_EQ(1u, send_bucket_->QueueSizePackets()); + EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number, + queued_packet_timestamp, false)) .Times(1) .WillRepeatedly(Return(true)); send_bucket_->Process(); sequence_number++; - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); + EXPECT_EQ(0u, send_bucket_->QueueSizePackets()); + + // We can send packets_to_send -1 packets of size 250 during the current + // interval since one packet has already been sent. + for (size_t i = 0; i < packets_to_send - 1; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); + } send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, clock_.TimeInMilliseconds(), 250, false); + EXPECT_EQ(packets_to_send, send_bucket_->QueueSizePackets()); send_bucket_->Process(); + EXPECT_EQ(1u, send_bucket_->QueueSizePackets()); } TEST_F(PacedSenderTest, PaceQueuedPackets) { uint32_t ssrc = 12345; uint16_t sequence_number = 1234; - // Due to the multiplicative factor we can send 3 packets not 2 packets. - for (int i = 0; i < 3; ++i) { - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); + // Due to the multiplicative factor we can send 5 packets during a send + // interval. (network capacity * multiplier / (8 bits per byte * + // (packet size * #send intervals per second) + const size_t packets_to_send_per_interval = + kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200); + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); } - for (int j = 0; j < 30; ++j) { + + for (size_t j = 0; j < packets_to_send_per_interval * 10; ++j) { send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, clock_.TimeInMilliseconds(), 250, false); } + EXPECT_EQ(packets_to_send_per_interval + packets_to_send_per_interval * 10, + send_bucket_->QueueSizePackets()); send_bucket_->Process(); + EXPECT_EQ(packets_to_send_per_interval * 10, + send_bucket_->QueueSizePackets()); EXPECT_CALL(callback_, TimeToSendPadding(_)).Times(0); for (int k = 0; k < 10; ++k) { EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, _, false)) - .Times(3) + .Times(packets_to_send_per_interval) .WillRepeatedly(Return(true)); EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); send_bucket_->Process(); } + EXPECT_EQ(0u, send_bucket_->QueueSizePackets()); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); + EXPECT_EQ(0u, send_bucket_->QueueSizePackets()); send_bucket_->Process(); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); + + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); + } send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, sequence_number, clock_.TimeInMilliseconds(), 250, false); send_bucket_->Process(); + EXPECT_EQ(1u, send_bucket_->QueueSizePackets()); } TEST_F(PacedSenderTest, PaceQueuedPacketsWithDuplicates) { @@ -258,18 +238,18 @@ TEST_F(PacedSenderTest, PaceQueuedPacketsWithDuplicates) { uint16_t sequence_number = 1234; uint16_t queued_sequence_number; - // Due to the multiplicative factor we can send 3 packets not 2 packets. - for (int i = 0; i < 3; ++i) { - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); + // Due to the multiplicative factor we can send 5 packets during a send + // interval. (network capacity * multiplier / (8 bits per byte * + // (packet size * #send intervals per second) + const size_t packets_to_send_per_interval = + kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200); + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); } queued_sequence_number = sequence_number; - for (int j = 0; j < 30; ++j) { + for (size_t j = 0; j < packets_to_send_per_interval * 10; ++j) { // Send in duplicate packets. send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, sequence_number, clock_.TimeInMilliseconds(), @@ -284,7 +264,7 @@ TEST_F(PacedSenderTest, PaceQueuedPacketsWithDuplicates) { EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); - for (int i = 0; i < 3; ++i) { + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { EXPECT_CALL(callback_, TimeToSendPacket(ssrc, queued_sequence_number++, _, false)) .Times(1) @@ -297,28 +277,16 @@ TEST_F(PacedSenderTest, PaceQueuedPacketsWithDuplicates) { clock_.AdvanceTimeMilliseconds(5); EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); send_bucket_->Process(); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); + + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); + } send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, clock_.TimeInMilliseconds(), 250, false); send_bucket_->Process(); + EXPECT_EQ(1u, send_bucket_->QueueSizePackets()); } TEST_F(PacedSenderTest, CanQueuePacketsWithSameSequenceNumberOnDifferentSsrcs) { @@ -348,33 +316,33 @@ TEST_F(PacedSenderTest, Padding) { uint32_t ssrc = 12345; uint16_t sequence_number = 1234; - send_bucket_->UpdateBitrate( - kTargetBitrate, kPaceMultiplier * kTargetBitrate, kTargetBitrate); - // Due to the multiplicative factor we can send 3 packets not 2 packets. - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - clock_.TimeInMilliseconds(), - 250, - false); + send_bucket_->SetEstimatedBitrate(kTargetBitrateBps); + send_bucket_->SetAllocatedSendBitrate(kTargetBitrateBps, kTargetBitrateBps); + + // Due to the multiplicative factor we can send 5 packets during a send + // interval. (network capacity * multiplier / (8 bits per byte * + // (packet size * #send intervals per second) + const size_t packets_to_send_per_interval = + kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200); + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); + } // No padding is expected since we have sent too much already. EXPECT_CALL(callback_, TimeToSendPadding(_)).Times(0); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); send_bucket_->Process(); + EXPECT_EQ(0u, send_bucket_->QueueSizePackets()); + + // 5 milliseconds later should not send padding since we filled the buffers + // initially. + EXPECT_CALL(callback_, TimeToSendPadding(250)).Times(0); + EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); + clock_.AdvanceTimeMilliseconds(5); + EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); + send_bucket_->Process(); // 5 milliseconds later we have enough budget to send some padding. EXPECT_CALL(callback_, TimeToSendPadding(250)).Times(1). @@ -391,8 +359,9 @@ TEST_F(PacedSenderTest, VerifyPaddingUpToBitrate) { int64_t capture_time_ms = 56789; const int kTimeStep = 5; const int64_t kBitrateWindow = 100; - send_bucket_->UpdateBitrate( - kTargetBitrate, kPaceMultiplier * kTargetBitrate, kTargetBitrate); + send_bucket_->SetEstimatedBitrate(kTargetBitrateBps); + send_bucket_->SetAllocatedSendBitrate(kTargetBitrateBps, kTargetBitrateBps); + int64_t start_time = clock_.TimeInMilliseconds(); while (clock_.TimeInMilliseconds() - start_time < kBitrateWindow) { SendAndExpectPacket(PacedSender::kNormalPriority, @@ -415,11 +384,11 @@ TEST_F(PacedSenderTest, VerifyAverageBitrateVaryingMediaPayload) { const int kTimeStep = 5; const int64_t kBitrateWindow = 10000; PacedSenderPadding callback; - send_bucket_.reset(new PacedSender( - &clock_, &callback, kTargetBitrate, kPaceMultiplier * kTargetBitrate, 0)); + send_bucket_.reset(new PacedSender(&clock_, &callback, kTargetBitrateBps)); send_bucket_->SetProbingEnabled(false); - send_bucket_->UpdateBitrate( - kTargetBitrate, kPaceMultiplier * kTargetBitrate, kTargetBitrate); + send_bucket_->SetEstimatedBitrate(kTargetBitrateBps); + send_bucket_->SetAllocatedSendBitrate(kTargetBitrateBps, kTargetBitrateBps); + int64_t start_time = clock_.TimeInMilliseconds(); size_t media_bytes = 0; while (clock_.TimeInMilliseconds() - start_time < kBitrateWindow) { @@ -432,9 +401,10 @@ TEST_F(PacedSenderTest, VerifyAverageBitrateVaryingMediaPayload) { clock_.AdvanceTimeMilliseconds(kTimeStep); send_bucket_->Process(); } - EXPECT_NEAR(kTargetBitrate, + EXPECT_NEAR(kTargetBitrateBps / 1000, static_cast(8 * (media_bytes + callback.padding_sent()) / - kBitrateWindow), 1); + kBitrateWindow), + 1); } TEST_F(PacedSenderTest, Priority) { @@ -444,50 +414,41 @@ TEST_F(PacedSenderTest, Priority) { int64_t capture_time_ms = 56789; int64_t capture_time_ms_low_priority = 1234567; - // Due to the multiplicative factor we can send 3 packets not 2 packets. - SendAndExpectPacket(PacedSender::kLowPriority, - ssrc, - sequence_number++, - capture_time_ms, - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - capture_time_ms, - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - capture_time_ms, - 250, - false); + // Due to the multiplicative factor we can send 5 packets during a send + // interval. (network capacity * multiplier / (8 bits per byte * + // (packet size * #send intervals per second) + const size_t packets_to_send_per_interval = + kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200); + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); + } send_bucket_->Process(); + EXPECT_EQ(0u, send_bucket_->QueueSizePackets()); // Expect normal and low priority to be queued and high to pass through. send_bucket_->InsertPacket(PacedSender::kLowPriority, ssrc_low_priority, sequence_number++, capture_time_ms_low_priority, 250, false); - send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, - sequence_number++, capture_time_ms, 250, false); - send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, - sequence_number++, capture_time_ms, 250, false); - send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, - sequence_number++, capture_time_ms, 250, false); + + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { + send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, + sequence_number++, capture_time_ms, 250, false); + } send_bucket_->InsertPacket(PacedSender::kHighPriority, ssrc, sequence_number++, capture_time_ms, 250, false); // Expect all high and normal priority to be sent out first. EXPECT_CALL(callback_, TimeToSendPadding(_)).Times(0); EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, capture_time_ms, false)) - .Times(4) + .Times(packets_to_send_per_interval + 1) .WillRepeatedly(Return(true)); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); send_bucket_->Process(); + EXPECT_EQ(1u, send_bucket_->QueueSizePackets()); EXPECT_CALL(callback_, TimeToSendPacket( @@ -513,23 +474,30 @@ TEST_F(PacedSenderTest, HighPrioDoesntAffectBudget) { capture_time_ms, 250, false); } send_bucket_->Process(); - // Low prio packets does affect the budget, so we should only be able to send - // 3 at once, the 4th should be queued. - for (int i = 0; i < 3; ++i) { + // Low prio packets does affect the budget. + // Due to the multiplicative factor we can send 5 packets during a send + // interval. (network capacity * multiplier / (8 bits per byte * + // (packet size * #send intervals per second) + const size_t packets_to_send_per_interval = + kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200); + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { SendAndExpectPacket(PacedSender::kLowPriority, ssrc, sequence_number++, - capture_time_ms, 250, false); + clock_.TimeInMilliseconds(), 250, false); } send_bucket_->InsertPacket(PacedSender::kLowPriority, ssrc, sequence_number, capture_time_ms, 250, false); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); send_bucket_->Process(); + EXPECT_EQ(1u, send_bucket_->QueueSizePackets()); EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number++, capture_time_ms, false)) - .Times(1); + .Times(1) + .WillRepeatedly(Return(true)); EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); clock_.AdvanceTimeMilliseconds(5); send_bucket_->Process(); + EXPECT_EQ(0u, send_bucket_->QueueSizePackets()); } TEST_F(PacedSenderTest, Pause) { @@ -540,25 +508,16 @@ TEST_F(PacedSenderTest, Pause) { EXPECT_EQ(0, send_bucket_->QueueInMs()); - // Due to the multiplicative factor we can send 3 packets not 2 packets. - SendAndExpectPacket(PacedSender::kLowPriority, - ssrc, - sequence_number++, - capture_time_ms, - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - capture_time_ms, - 250, - false); - SendAndExpectPacket(PacedSender::kNormalPriority, - ssrc, - sequence_number++, - capture_time_ms, - 250, - false); + // Due to the multiplicative factor we can send 5 packets during a send + // interval. (network capacity * multiplier / (8 bits per byte * + // (packet size * #send intervals per second) + const size_t packets_to_send_per_interval = + kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200); + for (size_t i = 0; i < packets_to_send_per_interval; ++i) { + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), 250, false); + } + send_bucket_->Process(); send_bucket_->Pause(); @@ -668,18 +627,18 @@ TEST_F(PacedSenderTest, ExpectedQueueTimeMs) { uint16_t sequence_number = 1234; const size_t kNumPackets = 60; const size_t kPacketSize = 1200; - const int32_t kMaxBitrate = kPaceMultiplier * 30; + const int32_t kMaxBitrate = PacedSender::kDefaultPaceMultiplier * 30000; EXPECT_EQ(0, send_bucket_->ExpectedQueueTimeMs()); - send_bucket_->UpdateBitrate(30, kMaxBitrate, 0); + send_bucket_->SetEstimatedBitrate(30000); for (size_t i = 0; i < kNumPackets; ++i) { SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, clock_.TimeInMilliseconds(), kPacketSize, false); } - // Queue in ms = 1000 * (bytes in queue) / (kbit per second * 1000 / 8) + // Queue in ms = 1000 * (bytes in queue) *8 / (bits per second) int64_t queue_in_ms = - static_cast(kNumPackets * kPacketSize * 8 / kMaxBitrate); + static_cast(1000 * kNumPackets * kPacketSize * 8 / kMaxBitrate); EXPECT_EQ(queue_in_ms, send_bucket_->ExpectedQueueTimeMs()); int64_t time_start = clock_.TimeInMilliseconds(); @@ -697,7 +656,7 @@ TEST_F(PacedSenderTest, ExpectedQueueTimeMs) { // Allow for aliasing, duration should be within one pack of max time limit. EXPECT_NEAR(duration, PacedSender::kMaxQueueLengthMs, - static_cast(kPacketSize * 8 / kMaxBitrate)); + static_cast(1000 * kPacketSize * 8 / kMaxBitrate)); } TEST_F(PacedSenderTest, QueueTimeGrowsOverTime) { @@ -705,7 +664,7 @@ TEST_F(PacedSenderTest, QueueTimeGrowsOverTime) { uint16_t sequence_number = 1234; EXPECT_EQ(0, send_bucket_->QueueInMs()); - send_bucket_->UpdateBitrate(30, kPaceMultiplier * 30, 0); + send_bucket_->SetEstimatedBitrate(30000); SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number, @@ -723,25 +682,22 @@ TEST_F(PacedSenderTest, ProbingWithInitialFrame) { const int kNumPackets = 11; const int kNumDeltas = kNumPackets - 1; const size_t kPacketSize = 1200; - const int kInitialBitrateKbps = 300; + const int kInitialBitrateBps = 300000; uint32_t ssrc = 12346; uint16_t sequence_number = 1234; + const int expected_deltas[kNumDeltas] = {10, 10, 10, 10, 10, 5, 5, 5, 5, 5}; std::list expected_deltas_list(expected_deltas, expected_deltas + kNumDeltas); PacedSenderProbing callback(expected_deltas_list, &clock_); - send_bucket_.reset( - new PacedSender(&clock_, - &callback, - kInitialBitrateKbps, - kPaceMultiplier * kInitialBitrateKbps, - 0)); + send_bucket_.reset(new PacedSender(&clock_, &callback, kInitialBitrateBps)); for (int i = 0; i < kNumPackets; ++i) { send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, clock_.TimeInMilliseconds(), kPacketSize, false); } + while (callback.packets_sent() < kNumPackets) { int time_until_process = send_bucket_->TimeUntilNextProcess(); if (time_until_process <= 0) { @@ -756,15 +712,14 @@ TEST_F(PacedSenderTest, ProbingWithTooSmallInitialFrame) { const int kNumPackets = 11; const int kNumDeltas = kNumPackets - 1; const size_t kPacketSize = 1200; - const int kInitialBitrateKbps = 300; + const int kInitialBitrateBps = 300000; uint32_t ssrc = 12346; uint16_t sequence_number = 1234; const int expected_deltas[kNumDeltas] = {10, 10, 10, 10, 10, 5, 5, 5, 5, 5}; std::list expected_deltas_list(expected_deltas, expected_deltas + kNumDeltas); PacedSenderProbing callback(expected_deltas_list, &clock_); - send_bucket_.reset(new PacedSender(&clock_, &callback, kInitialBitrateKbps, - kPaceMultiplier * kInitialBitrateKbps, 0)); + send_bucket_.reset(new PacedSender(&clock_, &callback, kInitialBitrateBps)); for (int i = 0; i < kNumPackets - 5; ++i) { send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, @@ -839,21 +794,21 @@ TEST_F(PacedSenderTest, PaddingOveruse) { uint16_t sequence_number = 1234; const size_t kPacketSize = 1200; - // Min bitrate 0 => no padding, padding budget will stay at 0. - send_bucket_->UpdateBitrate(60, 90, 0); + send_bucket_->SetEstimatedBitrate(60000); + send_bucket_->SetAllocatedSendBitrate(60000, 0); + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, clock_.TimeInMilliseconds(), kPacketSize, false); send_bucket_->Process(); // Add 30kbit padding. When increasing budget, media budget will increase from - // negative (overuse) while padding budget will increase form 0. + // negative (overuse) while padding budget will increase from 0. clock_.AdvanceTimeMilliseconds(5); - send_bucket_->UpdateBitrate(60, 90, 30); - - send_bucket_->InsertPacket(PacedSender::kHighPriority, ssrc, - sequence_number++, clock_.TimeInMilliseconds(), - kPacketSize, false); + send_bucket_->SetAllocatedSendBitrate(60000, 30000); + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, + clock_.TimeInMilliseconds(), kPacketSize, false); + EXPECT_LT(5u, send_bucket_->ExpectedQueueTimeMs()); // Don't send padding if queue is non-empty, even if padding budget > 0. EXPECT_CALL(callback_, TimeToSendPadding(_)).Times(0); send_bucket_->Process(); @@ -864,9 +819,8 @@ TEST_F(PacedSenderTest, AverageQueueTime) { uint16_t sequence_number = 1234; const size_t kPacketSize = 1200; const int kBitrateBps = 10 * kPacketSize * 8; // 10 packets per second. - const int kBitrateKbps = (kBitrateBps + 500) / 1000; - send_bucket_->UpdateBitrate(kBitrateKbps, kBitrateKbps, kBitrateKbps); + send_bucket_->SetEstimatedBitrate(kBitrateBps); EXPECT_EQ(0, send_bucket_->AverageQueueTimeMs()); diff --git a/webrtc/modules/pacing/packet_router.h b/webrtc/modules/pacing/packet_router.h index 635b931225..a6039fddd8 100644 --- a/webrtc/modules/pacing/packet_router.h +++ b/webrtc/modules/pacing/packet_router.h @@ -30,7 +30,7 @@ class TransportFeedback; // PacketRouter routes outgoing data to the correct sending RTP module, based // on the simulcast layer in RTPVideoHeader. -class PacketRouter : public PacedSender::Callback, +class PacketRouter : public PacedSender::PacketSender, public TransportSequenceNumberAllocator { public: PacketRouter(); diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi index 32663d729b..7b20cf7f9d 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi @@ -62,6 +62,7 @@ 'type': 'static_library', 'dependencies': [ '<(DEPTH)/testing/gtest.gyp:gtest', + '<(DEPTH)/testing/gmock.gyp:gmock', ], 'sources': [ 'test/bwe.cc', diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h index e6a05684cc..51e29e34c2 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h @@ -14,7 +14,6 @@ #include #include -#include "webrtc/base/constructormagic.h" #include "webrtc/modules/remote_bitrate_estimator/include/send_time_history.h" #include "webrtc/modules/remote_bitrate_estimator/test/bwe.h" diff --git a/webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc b/webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc index 3bcbc0a071..00e7a8b157 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc @@ -157,12 +157,7 @@ PacedVideoSender::PacedVideoSender(PacketProcessorListener* listener, VideoSource* source, BandwidthEstimatorType estimator) : VideoSender(listener, source, estimator), - pacer_(&clock_, - this, - source->bits_per_second() / 1000, - PacedSender::kDefaultPaceMultiplier * source->bits_per_second() / - 1000, - 0) { + pacer_(&clock_, this, source->bits_per_second()) { modules_.push_back(&pacer_); } @@ -310,9 +305,7 @@ void PacedVideoSender::OnNetworkChanged(uint32_t target_bitrate_bps, uint8_t fraction_lost, int64_t rtt) { VideoSender::OnNetworkChanged(target_bitrate_bps, fraction_lost, rtt); - pacer_.UpdateBitrate( - target_bitrate_bps / 1000, - PacedSender::kDefaultPaceMultiplier * target_bitrate_bps / 1000, 0); + pacer_.SetEstimatedBitrate(target_bitrate_bps); } const int kNoLimit = std::numeric_limits::max(); diff --git a/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h b/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h index 5ed4a3bc38..1280138461 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h +++ b/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h @@ -100,7 +100,7 @@ class VideoSender : public PacketSender, public BitrateObserver { RTC_DISALLOW_COPY_AND_ASSIGN(VideoSender); }; -class PacedVideoSender : public VideoSender, public PacedSender::Callback { +class PacedVideoSender : public VideoSender, public PacedSender::PacketSender { public: PacedVideoSender(PacketProcessorListener* listener, VideoSource* source, diff --git a/webrtc/video/encoder_state_feedback_unittest.cc b/webrtc/video/encoder_state_feedback_unittest.cc index fff5ca82d8..7dda826011 100644 --- a/webrtc/video/encoder_state_feedback_unittest.cc +++ b/webrtc/video/encoder_state_feedback_unittest.cc @@ -25,14 +25,12 @@ namespace webrtc { class MockVieEncoder : public ViEEncoder { public: - explicit MockVieEncoder(ProcessThread* process_thread, PacedSender* pacer) + explicit MockVieEncoder(ProcessThread* process_thread) : ViEEncoder(1, std::vector(), process_thread, nullptr, - nullptr, - nullptr, - pacer) {} + nullptr) {} ~MockVieEncoder() {} MOCK_METHOD1(OnReceivedIntraFrameRequest, @@ -47,12 +45,7 @@ TEST(VieKeyRequestTest, CreateAndTriggerRequests) { static const uint32_t kSsrc = 1234; NiceMock process_thread; PacketRouter router; - PacedSender pacer(Clock::GetRealTimeClock(), &router, - BitrateController::kDefaultStartBitrateKbps, - PacedSender::kDefaultPaceMultiplier * - BitrateController::kDefaultStartBitrateKbps, - 0); - MockVieEncoder encoder(&process_thread, &pacer); + MockVieEncoder encoder(&process_thread); EncoderStateFeedback encoder_state_feedback; encoder_state_feedback.Init(std::vector(1, kSsrc), &encoder); diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index f27a858a27..9ffe8a34c0 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -379,9 +379,7 @@ VideoSendStream::VideoSendStream( config_.rtp.ssrcs, module_process_thread_, &stats_proxy_, - config.pre_encode_callback, - &overuse_detector_, - congestion_controller_->pacer()), + &overuse_detector_), video_sender_(vie_encoder_.video_sender()), bandwidth_observer_(congestion_controller_->GetBitrateController() ->CreateRtcpBandwidthObserver()), @@ -581,8 +579,14 @@ void VideoSendStream::EncoderProcess() { } VideoFrame frame; - if (input_.GetVideoFrame(&frame)) + if (input_.GetVideoFrame(&frame)) { + // TODO(perkj): |pre_encode_callback| is only used by tests. Tests should + // register as a sink to the VideoSource instead. + if (config_.pre_encode_callback) { + config_.pre_encode_callback->OnFrame(frame); + } vie_encoder_.EncodeVideoFrame(frame); + } } vie_encoder_.DeRegisterExternalEncoder(config_.encoder_settings.payload_type); } diff --git a/webrtc/video/vie_encoder.cc b/webrtc/video/vie_encoder.cc index 79b79d9726..91fc1f0db2 100644 --- a/webrtc/video/vie_encoder.cc +++ b/webrtc/video/vie_encoder.cc @@ -57,9 +57,7 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores, const std::vector& ssrcs, ProcessThread* module_process_thread, SendStatisticsProxy* stats_proxy, - rtc::VideoSinkInterface* pre_encode_callback, - OveruseFrameDetector* overuse_detector, - PacedSender* pacer) + OveruseFrameDetector* overuse_detector) : number_of_cores_(number_of_cores), ssrcs_(ssrcs), vp_(VideoProcessing::Create()), @@ -70,9 +68,7 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores, qm_callback_.get(), this), stats_proxy_(stats_proxy), - pre_encode_callback_(pre_encode_callback), overuse_detector_(overuse_detector), - pacer_(pacer), time_of_last_frame_activity_ms_(0), encoder_config_(), min_transmit_bitrate_bps_(0), @@ -245,8 +241,7 @@ bool ViEEncoder::EncoderPaused() const { if (encoder_paused_) { return true; } - if (pacer_->ExpectedQueueTimeMs() > PacedSender::kMaxQueueLengthMs) { - // Too much data in pacer queue, drop frame. + if (video_suspended_ || last_observed_bitrate_bps_ == 0) { return true; } return !network_is_transmitting_; @@ -295,10 +290,6 @@ void ViEEncoder::EncodeVideoFrame(const VideoFrame& video_frame) { } } - if (pre_encode_callback_) { - pre_encode_callback_->OnFrame(*frame_to_send); - } - if (codec_type == webrtc::kVideoCodecVP8) { webrtc::CodecSpecificInfo codec_specific_info; codec_specific_info.codecType = webrtc::kVideoCodecVP8; @@ -435,7 +426,7 @@ void ViEEncoder::OnReceivedIntraFrameRequest(uint32_t ssrc) { void ViEEncoder::OnBitrateUpdated(uint32_t bitrate_bps, uint8_t fraction_lost, int64_t round_trip_time_ms) { - LOG(LS_VERBOSE) << "OnBitrateUpdated, bitrate" << bitrate_bps + LOG(LS_VERBOSE) << "OnBitrateUpdated, bitrate " << bitrate_bps << " packet loss " << static_cast(fraction_lost) << " rtt " << round_trip_time_ms; video_sender_.SetChannelParameters(bitrate_bps, fraction_lost, diff --git a/webrtc/video/vie_encoder.h b/webrtc/video/vie_encoder.h index 9369f9faa3..17ad30be71 100644 --- a/webrtc/video/vie_encoder.h +++ b/webrtc/video/vie_encoder.h @@ -61,10 +61,7 @@ class ViEEncoder : public VideoEncoderRateObserver, const std::vector& ssrcs, ProcessThread* module_process_thread, SendStatisticsProxy* stats_proxy, - // TODO(nisse): Used only for tests, delete? - rtc::VideoSinkInterface* pre_encode_callback, - OveruseFrameDetector* overuse_detector, - PacedSender* pacer); + OveruseFrameDetector* overuse_detector); ~ViEEncoder(); vcm::VideoSender* video_sender(); @@ -139,9 +136,7 @@ class ViEEncoder : public VideoEncoderRateObserver, rtc::CriticalSection data_cs_; SendStatisticsProxy* const stats_proxy_; - rtc::VideoSinkInterface* const pre_encode_callback_; OveruseFrameDetector* const overuse_detector_; - PacedSender* const pacer_; // The time we last received an input frame or encoded frame. This is used to // track when video is stopped long enough that we also want to stop sending