Reland "Detach RemoteEstimatorProxy from RemoteBitrateEstimator interface"

This reverts commit 6769e95bbc.

Reason for revert: downstream code adjusted

Original change's description:
> 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}

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}

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_;

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<uint32_t> 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<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(

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 {

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 @@ 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<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 @@ 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<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(

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 @@ 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<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 @@ 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.

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) {