diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h index fe30865c53..77bd4cc66b 100644 --- a/modules/congestion_controller/include/receive_side_congestion_controller.h +++ b/modules/congestion_controller/include/receive_side_congestion_controller.h @@ -47,11 +47,6 @@ class ReceiveSideCongestionController : public CallStatsObserver { const RTPHeader& header); void SetSendPeriodicFeedback(bool send_periodic_feedback); - // TODO(nisse): Delete these methods, design a more specific interface. - [[deprecated]] virtual RemoteBitrateEstimator* GetRemoteBitrateEstimator( - bool send_side_bwe); - [[deprecated]] virtual const RemoteBitrateEstimator* - GetRemoteBitrateEstimator(bool send_side_bwe) const; // Implements CallStatsObserver. void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; @@ -122,6 +117,7 @@ class ReceiveSideCongestionController : public CallStatsObserver { int min_bitrate_bps_; }; + Clock& clock_; const FieldTrialBasedConfig field_trial_config_; RembThrottler remb_throttler_; WrappingBitrateEstimator remote_bitrate_estimator_; diff --git a/modules/congestion_controller/receive_side_congestion_controller.cc b/modules/congestion_controller/receive_side_congestion_controller.cc index 3f337f84c7..92ca62b7fc 100644 --- a/modules/congestion_controller/receive_side_congestion_controller.cc +++ b/modules/congestion_controller/receive_side_congestion_controller.cc @@ -124,10 +124,10 @@ ReceiveSideCongestionController::ReceiveSideCongestionController( RemoteEstimatorProxy::TransportFeedbackSender feedback_sender, RembThrottler::RembSender remb_sender, NetworkStateEstimator* network_state_estimator) - : remb_throttler_(std::move(remb_sender), clock), + : clock_(*clock), + remb_throttler_(std::move(remb_sender), clock), remote_bitrate_estimator_(&remb_throttler_, clock), - remote_estimator_proxy_(clock, - std::move(feedback_sender), + remote_estimator_proxy_(std::move(feedback_sender), &field_trial_config_, network_state_estimator) {} @@ -148,25 +148,6 @@ void ReceiveSideCongestionController::SetSendPeriodicFeedback( remote_estimator_proxy_.SetSendPeriodicFeedback(send_periodic_feedback); } -RemoteBitrateEstimator* -ReceiveSideCongestionController::GetRemoteBitrateEstimator(bool send_side_bwe) { - if (send_side_bwe) { - return &remote_estimator_proxy_; - } else { - return &remote_bitrate_estimator_; - } -} - -const RemoteBitrateEstimator* -ReceiveSideCongestionController::GetRemoteBitrateEstimator( - bool send_side_bwe) const { - if (send_side_bwe) { - return &remote_estimator_proxy_; - } else { - return &remote_bitrate_estimator_; - } -} - DataRate ReceiveSideCongestionController::LatestReceiveSideEstimate() const { std::vector unused_ssrcs; uint32_t bitrate_bps = 0; @@ -199,18 +180,16 @@ void ReceiveSideCongestionController::Process() { } TimeDelta ReceiveSideCongestionController::MaybeProcess() { + Timestamp now = clock_.CurrentTime(); int64_t time_until_rbe_ms = remote_bitrate_estimator_.TimeUntilNextProcess(); if (time_until_rbe_ms <= 0) { remote_bitrate_estimator_.Process(); time_until_rbe_ms = remote_bitrate_estimator_.TimeUntilNextProcess(); } - int64_t time_until_rep_ms = remote_estimator_proxy_.TimeUntilNextProcess(); - if (time_until_rep_ms <= 0) { - remote_estimator_proxy_.Process(); - time_until_rep_ms = remote_estimator_proxy_.TimeUntilNextProcess(); - } - int64_t time_until_next_ms = std::min(time_until_rbe_ms, time_until_rep_ms); - return TimeDelta::Millis(std::max(time_until_next_ms, 0)); + TimeDelta time_until_rbe = TimeDelta::Millis(time_until_rbe_ms); + TimeDelta time_until_rep = remote_estimator_proxy_.Process(now); + TimeDelta time_until = std::min(time_until_rbe, time_until_rep); + return std::max(time_until, TimeDelta::Zero()); } void ReceiveSideCongestionController::SetMaxDesiredReceiveBitrate( diff --git a/modules/congestion_controller/remb_throttler.h b/modules/congestion_controller/remb_throttler.h index 2f610c1df9..85292cbc09 100644 --- a/modules/congestion_controller/remb_throttler.h +++ b/modules/congestion_controller/remb_throttler.h @@ -16,7 +16,7 @@ #include "api/units/data_rate.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" -#include "modules/remote_bitrate_estimator/remote_estimator_proxy.h" +#include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "rtc_base/synchronization/mutex.h" namespace webrtc { diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index 4691fa6283..0215b64155 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -16,6 +16,7 @@ #include #include "api/units/data_size.h" +#include "modules/rtp_rtcp/source/rtcp_packet/remote_estimate.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" @@ -46,22 +47,20 @@ TimeDelta GetAbsoluteSendTimeDelta(uint32_t new_sendtime, } // namespace RemoteEstimatorProxy::RemoteEstimatorProxy( - Clock* clock, TransportFeedbackSender feedback_sender, const FieldTrialsView* key_value_config, NetworkStateEstimator* network_state_estimator) - : clock_(clock), - feedback_sender_(std::move(feedback_sender)), + : feedback_sender_(std::move(feedback_sender)), send_config_(key_value_config), - last_process_time_ms_(-1), + last_process_time_(Timestamp::MinusInfinity()), network_state_estimator_(network_state_estimator), media_ssrc_(0), feedback_packet_count_(0), packet_overhead_(DataSize::Zero()), - send_interval_ms_(send_config_.default_interval->ms()), + send_interval_(send_config_.default_interval.Get()), send_periodic_feedback_(true), previous_abs_send_time_(0), - abs_send_timestamp_(clock->CurrentTime()) { + abs_send_timestamp_(Timestamp::Zero()) { RTC_LOG(LS_INFO) << "Maximum interval between transport feedback RTCP messages (ms): " << send_config_.max_interval->ms(); @@ -151,32 +150,19 @@ void RemoteEstimatorProxy::IncomingPacket(Packet packet) { } } -bool RemoteEstimatorProxy::LatestEstimate(std::vector* ssrcs, - unsigned int* bitrate_bps) const { - return false; -} - -int64_t RemoteEstimatorProxy::TimeUntilNextProcess() { +TimeDelta RemoteEstimatorProxy::Process(Timestamp now) { MutexLock lock(&lock_); if (!send_periodic_feedback_) { - // Wait a day until next process. - return 24 * 60 * 60 * 1000; - } else if (last_process_time_ms_ != -1) { - int64_t now = clock_->TimeInMilliseconds(); - if (now - last_process_time_ms_ < send_interval_ms_) - return last_process_time_ms_ + send_interval_ms_ - now; + return TimeDelta::PlusInfinity(); } - return 0; -} - -void RemoteEstimatorProxy::Process() { - MutexLock lock(&lock_); - if (!send_periodic_feedback_) { - return; + Timestamp next_process_time = last_process_time_ + send_interval_; + if (now >= next_process_time) { + last_process_time_ = now; + SendPeriodicFeedbacks(); + return send_interval_; } - last_process_time_ms_ = clock_->TimeInMilliseconds(); - SendPeriodicFeedbacks(); + return next_process_time - now; } void RemoteEstimatorProxy::OnBitrateChanged(int bitrate_bps) { @@ -185,18 +171,23 @@ void RemoteEstimatorProxy::OnBitrateChanged(int bitrate_bps) { // TwccReport size at 50ms interval is 24 byte. // TwccReport size at 250ms interval is 36 byte. // AverageTwccReport = (TwccReport(50ms) + TwccReport(250ms)) / 2 - constexpr int kTwccReportSize = 20 + 8 + 10 + 30; - const double kMinTwccRate = - kTwccReportSize * 8.0 * 1000.0 / send_config_.max_interval->ms(); - const double kMaxTwccRate = - kTwccReportSize * 8.0 * 1000.0 / send_config_.min_interval->ms(); + constexpr DataSize kTwccReportSize = DataSize::Bytes(20 + 8 + 10 + 30); + const DataRate kMinTwccRate = + kTwccReportSize / send_config_.max_interval.Get(); // Let TWCC reports occupy 5% of total bandwidth. + DataRate twcc_bitrate = + DataRate::BitsPerSec(send_config_.bandwidth_fraction * bitrate_bps); + + // Check upper send_interval bound by checking bitrate to avoid overflow when + // dividing by small bitrate, in particular avoid dividing by zero bitrate. + TimeDelta send_interval = twcc_bitrate <= kMinTwccRate + ? send_config_.max_interval.Get() + : std::max(kTwccReportSize / twcc_bitrate, + send_config_.min_interval.Get()); + MutexLock lock(&lock_); - send_interval_ms_ = static_cast( - 0.5 + kTwccReportSize * 8.0 * 1000.0 / - rtc::SafeClamp(send_config_.bandwidth_fraction * bitrate_bps, - kMinTwccRate, kMaxTwccRate)); + send_interval_ = send_interval; } void RemoteEstimatorProxy::SetSendPeriodicFeedback( diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/modules/remote_bitrate_estimator/remote_estimator_proxy.h index 6aba92fec0..509ad0ba02 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h @@ -18,37 +18,33 @@ #include "absl/types/optional.h" #include "api/field_trials_view.h" +#include "api/rtp_headers.h" #include "api/transport/network_control.h" #include "api/units/data_size.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" -#include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "modules/remote_bitrate_estimator/packet_arrival_map.h" +#include "modules/rtp_rtcp/source/rtcp_packet.h" +#include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "rtc_base/experiments/field_trial_parser.h" #include "rtc_base/numerics/sequence_number_util.h" #include "rtc_base/synchronization/mutex.h" namespace webrtc { -class Clock; -namespace rtcp { -class TransportFeedback; -} - // Class used when send-side BWE is enabled: This proxy is instantiated on the // receive side. It buffers a number of receive timestamps and then sends // transport feedback messages back too the send side. -class RemoteEstimatorProxy : public RemoteBitrateEstimator { +class RemoteEstimatorProxy { public: // Used for sending transport feedback messages when send side // BWE is used. using TransportFeedbackSender = std::function> packets)>; - RemoteEstimatorProxy(Clock* clock, - TransportFeedbackSender feedback_sender, + RemoteEstimatorProxy(TransportFeedbackSender feedback_sender, const FieldTrialsView* key_value_config, NetworkStateEstimator* network_state_estimator); - ~RemoteEstimatorProxy() override; + ~RemoteEstimatorProxy(); struct Packet { Timestamp arrival_time; @@ -62,14 +58,12 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { void IncomingPacket(int64_t arrival_time_ms, size_t payload_size, - const RTPHeader& header) override; - void RemoveStream(uint32_t ssrc) override {} - bool LatestEstimate(std::vector* ssrcs, - unsigned int* bitrate_bps) const override; - void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override {} - void SetMinBitrate(int min_bitrate_bps) override {} - int64_t TimeUntilNextProcess() override; - void Process() override; + const RTPHeader& header); + + // Sends periodic feedback if it is time to send it. + // Returns time until next call to Process should be made. + TimeDelta Process(Timestamp now); + void OnBitrateChanged(int bitrate); void SetSendPeriodicFeedback(bool send_periodic_feedback); void SetTransportOverhead(DataSize overhead_per_packet); @@ -116,10 +110,9 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { int64_t end_sequence_number_exclusive, bool is_periodic_update) RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_); - Clock* const clock_; const TransportFeedbackSender feedback_sender_; const TransportWideFeedbackConfig send_config_; - int64_t last_process_time_ms_; + Timestamp last_process_time_; Mutex lock_; // `network_state_estimator_` may be null. @@ -137,7 +130,7 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { // Packet arrival times, by sequence number. PacketArrivalTimeMap packet_arrival_times_ RTC_GUARDED_BY(&lock_); - int64_t send_interval_ms_ RTC_GUARDED_BY(&lock_); + TimeDelta send_interval_ RTC_GUARDED_BY(&lock_); bool send_periodic_feedback_ RTC_GUARDED_BY(&lock_); // Unwraps absolute send times. diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc index c2360c0594..57db595293 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc @@ -79,8 +79,7 @@ class RemoteEstimatorProxyTest : public ::testing::Test { public: RemoteEstimatorProxyTest() : clock_(0), - proxy_(&clock_, - feedback_sender_.AsStdFunction(), + proxy_(feedback_sender_.AsStdFunction(), &field_trial_config_, &network_state_estimator_) {} @@ -98,7 +97,7 @@ class RemoteEstimatorProxyTest : public ::testing::Test { void Process() { clock_.AdvanceTime(kDefaultSendInterval); - proxy_.Process(); + proxy_.Process(clock_.CurrentTime()); } FieldTrialBasedConfig field_trial_config_; @@ -425,41 +424,32 @@ TEST_F(RemoteEstimatorProxyTest, RemovesTimestampsOutOfScope) { Process(); } -TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsZeroBeforeFirstProcess) { - EXPECT_EQ(0, proxy_.TimeUntilNextProcess()); -} - TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsDefaultOnUnkownBitrate) { - Process(); - EXPECT_EQ(proxy_.TimeUntilNextProcess(), kDefaultSendInterval.ms()); + EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), kDefaultSendInterval); } TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsMinIntervalOn300kbps) { - Process(); proxy_.OnBitrateChanged(300'000); - EXPECT_EQ(proxy_.TimeUntilNextProcess(), kMinSendInterval.ms()); + EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), kMinSendInterval); } TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsMaxIntervalOn0kbps) { - Process(); // TimeUntilNextProcess should be limited by `kMaxSendIntervalMs` when // bitrate is small. We choose 0 bps as a special case, which also tests // erroneous behaviors like division-by-zero. proxy_.OnBitrateChanged(0); - EXPECT_EQ(proxy_.TimeUntilNextProcess(), kMaxSendInterval.ms()); + EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), kMaxSendInterval); } TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsMaxIntervalOn20kbps) { - Process(); proxy_.OnBitrateChanged(20'000); - EXPECT_EQ(proxy_.TimeUntilNextProcess(), kMaxSendInterval.ms()); + EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), kMaxSendInterval); } TEST_F(RemoteEstimatorProxyTest, TwccReportsUse5PercentOfAvailableBandwidth) { - Process(); - proxy_.OnBitrateChanged(80000); + proxy_.OnBitrateChanged(80'000); // 80kbps * 0.05 = TwccReportSize(68B * 8b/B) * 1000ms / SendInterval(136ms) - EXPECT_EQ(136, proxy_.TimeUntilNextProcess()); + EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), TimeDelta::Millis(136)); } ////////////////////////////////////////////////////////////////////////////// @@ -467,9 +457,9 @@ TEST_F(RemoteEstimatorProxyTest, TwccReportsUse5PercentOfAvailableBandwidth) { // by the sender. ////////////////////////////////////////////////////////////////////////////// typedef RemoteEstimatorProxyTest RemoteEstimatorProxyOnRequestTest; -TEST_F(RemoteEstimatorProxyOnRequestTest, TimeUntilNextProcessIsHigh) { +TEST_F(RemoteEstimatorProxyOnRequestTest, DisablesPeriodicProcess) { proxy_.SetSendPeriodicFeedback(false); - EXPECT_GE(proxy_.TimeUntilNextProcess(), 60 * 60 * 1000); + EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), TimeDelta::PlusInfinity()); } TEST_F(RemoteEstimatorProxyOnRequestTest, ProcessDoesNotSendFeedback) {