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}
diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h
index 77bd4cc..fe30865 100644
--- a/modules/congestion_controller/include/receive_side_congestion_controller.h
+++ b/modules/congestion_controller/include/receive_side_congestion_controller.h
@@ -47,6 +47,11 @@
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;
@@ -117,7 +122,6 @@
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 92ca62b..3f337f8 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)
- : clock_(*clock),
- remb_throttler_(std::move(remb_sender), clock),
+ : remb_throttler_(std::move(remb_sender), clock),
remote_bitrate_estimator_(&remb_throttler_, clock),
- remote_estimator_proxy_(std::move(feedback_sender),
+ remote_estimator_proxy_(clock,
+ std::move(feedback_sender),
&field_trial_config_,
network_state_estimator) {}
@@ -148,6 +148,25 @@
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;
@@ -180,16 +199,18 @@
}
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();
}
- 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());
+ 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));
}
void ReceiveSideCongestionController::SetMaxDesiredReceiveBitrate(
diff --git a/modules/congestion_controller/remb_throttler.h b/modules/congestion_controller/remb_throttler.h
index 85292cb..2f610c1 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/include/remote_bitrate_estimator.h"
+#include "modules/remote_bitrate_estimator/remote_estimator_proxy.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 0215b64..4691fa6 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
@@ -16,7 +16,6 @@
#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"
@@ -47,20 +46,22 @@
} // namespace
RemoteEstimatorProxy::RemoteEstimatorProxy(
+ Clock* clock,
TransportFeedbackSender feedback_sender,
const FieldTrialsView* key_value_config,
NetworkStateEstimator* network_state_estimator)
- : feedback_sender_(std::move(feedback_sender)),
+ : clock_(clock),
+ feedback_sender_(std::move(feedback_sender)),
send_config_(key_value_config),
- last_process_time_(Timestamp::MinusInfinity()),
+ last_process_time_ms_(-1),
network_state_estimator_(network_state_estimator),
media_ssrc_(0),
feedback_packet_count_(0),
packet_overhead_(DataSize::Zero()),
- send_interval_(send_config_.default_interval.Get()),
+ send_interval_ms_(send_config_.default_interval->ms()),
send_periodic_feedback_(true),
previous_abs_send_time_(0),
- abs_send_timestamp_(Timestamp::Zero()) {
+ abs_send_timestamp_(clock->CurrentTime()) {
RTC_LOG(LS_INFO)
<< "Maximum interval between transport feedback RTCP messages (ms): "
<< send_config_.max_interval->ms();
@@ -150,19 +151,32 @@
}
}
-TimeDelta RemoteEstimatorProxy::Process(Timestamp now) {
+bool RemoteEstimatorProxy::LatestEstimate(std::vector<unsigned int>* ssrcs,
+ unsigned int* bitrate_bps) const {
+ return false;
+}
+
+int64_t RemoteEstimatorProxy::TimeUntilNextProcess() {
MutexLock lock(&lock_);
if (!send_periodic_feedback_) {
- return TimeDelta::PlusInfinity();
+ // 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;
}
- Timestamp next_process_time = last_process_time_ + send_interval_;
- if (now >= next_process_time) {
- last_process_time_ = now;
- SendPeriodicFeedbacks();
- return send_interval_;
- }
+ return 0;
+}
- return next_process_time - now;
+void RemoteEstimatorProxy::Process() {
+ MutexLock lock(&lock_);
+ if (!send_periodic_feedback_) {
+ return;
+ }
+ last_process_time_ms_ = clock_->TimeInMilliseconds();
+
+ SendPeriodicFeedbacks();
}
void RemoteEstimatorProxy::OnBitrateChanged(int bitrate_bps) {
@@ -171,23 +185,18 @@
// TwccReport size at 50ms interval is 24 byte.
// TwccReport size at 250ms interval is 36 byte.
// AverageTwccReport = (TwccReport(50ms) + TwccReport(250ms)) / 2
- constexpr DataSize kTwccReportSize = DataSize::Bytes(20 + 8 + 10 + 30);
- const DataRate kMinTwccRate =
- kTwccReportSize / send_config_.max_interval.Get();
+ 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();
// 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_ = send_interval;
+ send_interval_ms_ = static_cast<int>(
+ 0.5 + kTwccReportSize * 8.0 * 1000.0 /
+ rtc::SafeClamp(send_config_.bandwidth_fraction * bitrate_bps,
+ kMinTwccRate, kMaxTwccRate));
}
void RemoteEstimatorProxy::SetSendPeriodicFeedback(
diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/modules/remote_bitrate_estimator/remote_estimator_proxy.h
index 509ad0b..6aba92f 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h
@@ -18,33 +18,37 @@
#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 {
+class RemoteEstimatorProxy : public RemoteBitrateEstimator {
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(TransportFeedbackSender feedback_sender,
+ RemoteEstimatorProxy(Clock* clock,
+ TransportFeedbackSender feedback_sender,
const FieldTrialsView* key_value_config,
NetworkStateEstimator* network_state_estimator);
- ~RemoteEstimatorProxy();
+ ~RemoteEstimatorProxy() override;
struct Packet {
Timestamp arrival_time;
@@ -58,12 +62,14 @@
void IncomingPacket(int64_t arrival_time_ms,
size_t payload_size,
- 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);
-
+ 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;
void OnBitrateChanged(int bitrate);
void SetSendPeriodicFeedback(bool send_periodic_feedback);
void SetTransportOverhead(DataSize overhead_per_packet);
@@ -110,9 +116,10 @@
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_;
- Timestamp last_process_time_;
+ int64_t last_process_time_ms_;
Mutex lock_;
// `network_state_estimator_` may be null.
@@ -130,7 +137,7 @@
// Packet arrival times, by sequence number.
PacketArrivalTimeMap packet_arrival_times_ RTC_GUARDED_BY(&lock_);
- TimeDelta send_interval_ RTC_GUARDED_BY(&lock_);
+ int64_t send_interval_ms_ 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 57db595..c2360c0 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
@@ -79,7 +79,8 @@
public:
RemoteEstimatorProxyTest()
: clock_(0),
- proxy_(feedback_sender_.AsStdFunction(),
+ proxy_(&clock_,
+ feedback_sender_.AsStdFunction(),
&field_trial_config_,
&network_state_estimator_) {}
@@ -97,7 +98,7 @@
void Process() {
clock_.AdvanceTime(kDefaultSendInterval);
- proxy_.Process(clock_.CurrentTime());
+ proxy_.Process();
}
FieldTrialBasedConfig field_trial_config_;
@@ -424,32 +425,41 @@
Process();
}
+TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsZeroBeforeFirstProcess) {
+ EXPECT_EQ(0, proxy_.TimeUntilNextProcess());
+}
+
TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsDefaultOnUnkownBitrate) {
- EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), kDefaultSendInterval);
+ Process();
+ EXPECT_EQ(proxy_.TimeUntilNextProcess(), kDefaultSendInterval.ms());
}
TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsMinIntervalOn300kbps) {
+ Process();
proxy_.OnBitrateChanged(300'000);
- EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), kMinSendInterval);
+ EXPECT_EQ(proxy_.TimeUntilNextProcess(), kMinSendInterval.ms());
}
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_.Process(clock_.CurrentTime()), kMaxSendInterval);
+ EXPECT_EQ(proxy_.TimeUntilNextProcess(), kMaxSendInterval.ms());
}
TEST_F(RemoteEstimatorProxyTest, TimeUntilNextProcessIsMaxIntervalOn20kbps) {
+ Process();
proxy_.OnBitrateChanged(20'000);
- EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), kMaxSendInterval);
+ EXPECT_EQ(proxy_.TimeUntilNextProcess(), kMaxSendInterval.ms());
}
TEST_F(RemoteEstimatorProxyTest, TwccReportsUse5PercentOfAvailableBandwidth) {
- proxy_.OnBitrateChanged(80'000);
+ Process();
+ proxy_.OnBitrateChanged(80000);
// 80kbps * 0.05 = TwccReportSize(68B * 8b/B) * 1000ms / SendInterval(136ms)
- EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), TimeDelta::Millis(136));
+ EXPECT_EQ(136, proxy_.TimeUntilNextProcess());
}
//////////////////////////////////////////////////////////////////////////////
@@ -457,9 +467,9 @@
// by the sender.
//////////////////////////////////////////////////////////////////////////////
typedef RemoteEstimatorProxyTest RemoteEstimatorProxyOnRequestTest;
-TEST_F(RemoteEstimatorProxyOnRequestTest, DisablesPeriodicProcess) {
+TEST_F(RemoteEstimatorProxyOnRequestTest, TimeUntilNextProcessIsHigh) {
proxy_.SetSendPeriodicFeedback(false);
- EXPECT_EQ(proxy_.Process(clock_.CurrentTime()), TimeDelta::PlusInfinity());
+ EXPECT_GE(proxy_.TimeUntilNextProcess(), 60 * 60 * 1000);
}
TEST_F(RemoteEstimatorProxyOnRequestTest, ProcessDoesNotSendFeedback) {