Reland "Detach RemoteEstimatorProxy from RemoteBitrateEstimator interface"
This reverts commit 6769e95bbc0e4f29c482ee1356b3e250df1fde3b.
Reason for revert: downstream code adjusted
Original change's description:
> Revert "Detach RemoteEstimatorProxy from RemoteBitrateEstimator interface"
>
> This reverts commit 08c7e7589218da1f26e9621e703dd790b4e7e7d7.
>
> Reason for revert: breaks downstream tests
>
> Original change's description:
> > Detach RemoteEstimatorProxy from RemoteBitrateEstimator interface
> >
> > Bug: None
> > Change-Id: I47b7c83320b0c7327c0d2ee59f7a0a30704cd331
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/266540
> > Reviewed-by: Philip Eliasson <philipel@webrtc.org>
> > Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
> > Cr-Commit-Position: refs/heads/main@{#37354}
>
> Bug: None
> Change-Id: Ia355be085890856141fc943432f6e2edef1c0900
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/267065
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
> Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#37361}
Bug: None
Change-Id: Ifaf8ff84a37a768b388b1f79c8c7829390d1905e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/267104
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37367}
diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h
index fe30865..77bd4cc 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 @@
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 @@
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 3f337f8..92ca62b 100644
--- a/modules/congestion_controller/receive_side_congestion_controller.cc
+++ b/modules/congestion_controller/receive_side_congestion_controller.cc
@@ -124,10 +124,10 @@
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 @@
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<uint32_t> unused_ssrcs;
uint32_t bitrate_bps = 0;
@@ -199,18 +180,16 @@
}
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<int64_t>(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 2f610c1..85292cb 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 4691fa6..0215b64 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
@@ -16,6 +16,7 @@
#include <utility>
#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 @@
} // 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 @@
}
}
-bool RemoteEstimatorProxy::LatestEstimate(std::vector<unsigned int>* 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 @@
// 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<int>(
- 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 6aba92f..509ad0b 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<void(
std::vector<std::unique_ptr<rtcp::RtcpPacket>> 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 @@
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<unsigned int>* 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 @@
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 @@
// 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 c2360c0..57db595 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 @@
public:
RemoteEstimatorProxyTest()
: clock_(0),
- proxy_(&clock_,
- feedback_sender_.AsStdFunction(),
+ proxy_(feedback_sender_.AsStdFunction(),
&field_trial_config_,
&network_state_estimator_) {}
@@ -98,7 +97,7 @@
void Process() {
clock_.AdvanceTime(kDefaultSendInterval);
- proxy_.Process();
+ proxy_.Process(clock_.CurrentTime());
}
FieldTrialBasedConfig field_trial_config_;
@@ -425,41 +424,32 @@
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 @@
// 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) {
