Revert "Detach RemoteEstimatorProxy from RemoteBitrateEstimator interface"

This reverts commit 08c7e75892.

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}

modules/congestion_controller/include/receive_side_congestion_controller.h

@@ -47,6 +47,11 @@ 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;
@@ -117,7 +122,6 @@ class ReceiveSideCongestionController : public CallStatsObserver {
     int min_bitrate_bps_;
   };
 
-  Clock& clock_;
   const FieldTrialBasedConfig field_trial_config_;
   RembThrottler remb_throttler_;
   WrappingBitrateEstimator remote_bitrate_estimator_;

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)
-    : 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 @@ 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<uint32_t> unused_ssrcs;
   uint32_t bitrate_bps = 0;
@@ -180,16 +199,18 @@ 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();
   }
-  TimeDelta time_until_rbe = TimeDelta::Millis(time_until_rbe_ms);
+  int64_t time_until_rep_ms = remote_estimator_proxy_.TimeUntilNextProcess();
-  TimeDelta time_until_rep = remote_estimator_proxy_.Process(now);
+  if (time_until_rep_ms <= 0) {
-  TimeDelta time_until = std::min(time_until_rbe, time_until_rep);
+    remote_estimator_proxy_.Process();
-  return std::max(time_until, TimeDelta::Zero());
+    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(

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 {

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 @@ TimeDelta GetAbsoluteSendTimeDelta(uint32_t new_sendtime,
 }  // 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 @@ void RemoteEstimatorProxy::IncomingPacket(Packet packet) {
   }
 }
 
-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;
-  Timestamp next_process_time = last_process_time_ + send_interval_;
+  } else if (last_process_time_ms_ != -1) {
-  if (now >= next_process_time) {
+    int64_t now = clock_->TimeInMilliseconds();
-    last_process_time_ = now;
+    if (now - last_process_time_ms_ < send_interval_ms_)
-    SendPeriodicFeedbacks();
+      return last_process_time_ms_ + send_interval_ms_ - now;
-    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 @@ 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 DataSize kTwccReportSize = DataSize::Bytes(20 + 8 + 10 + 30);
+  constexpr int kTwccReportSize = 20 + 8 + 10 + 30;
-  const DataRate kMinTwccRate =
+  const double kMinTwccRate =
-      kTwccReportSize / send_config_.max_interval.Get();
+      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(

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 @@ class RemoteEstimatorProxy {
 
   void IncomingPacket(int64_t arrival_time_ms,
                       size_t payload_size,
-                      const RTPHeader& header);
+                      const RTPHeader& header) override;
-
+  void RemoveStream(uint32_t ssrc) override {}
-  // Sends periodic feedback if it is time to send it.
+  bool LatestEstimate(std::vector<unsigned int>* ssrcs,
-  // Returns time until next call to Process should be made.
+                      unsigned int* bitrate_bps) const override;
-  TimeDelta Process(Timestamp now);
+  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 @@ class RemoteEstimatorProxy {
       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 @@ class RemoteEstimatorProxy {
   // 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.

modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc

@@ -79,7 +79,8 @@ class RemoteEstimatorProxyTest : public ::testing::Test {
  public:
   RemoteEstimatorProxyTest()
       : clock_(0),
-        proxy_(feedback_sender_.AsStdFunction(),
+        proxy_(&clock_,
+               feedback_sender_.AsStdFunction(),
                &field_trial_config_,
                &network_state_estimator_) {}
 
@@ -97,7 +98,7 @@ class RemoteEstimatorProxyTest : public ::testing::Test {
 
   void Process() {
     clock_.AdvanceTime(kDefaultSendInterval);
-    proxy_.Process(clock_.CurrentTime());
+    proxy_.Process();
   }
 
   FieldTrialBasedConfig field_trial_config_;
@@ -424,32 +425,41 @@ TEST_F(RemoteEstimatorProxyTest, RemovesTimestampsOutOfScope) {
   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 @@ TEST_F(RemoteEstimatorProxyTest, TwccReportsUse5PercentOfAvailableBandwidth) {
 // 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) {