Make CongestionController::remote_bitrate_estimator_ a non-pointer.

webrtc/modules/congestion_controller/congestion_controller.cc

Index: webrtc/modules/congestion_controller/congestion_controller.cc
diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc
index 94b3d8c6da98bbaee3a070e8e9f2421044a758b9..b6b2ff7dfafd6576704eebb9ae59db80285be579 100644
--- a/webrtc/modules/congestion_controller/congestion_controller.cc
+++ b/webrtc/modules/congestion_controller/congestion_controller.cc
@@ -23,7 +23,6 @@
 #include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h"
 #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h"
 #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h"
-#include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 
 namespace webrtc {
 namespace {
@@ -46,107 +45,96 @@ static void ClampBitrates(int* bitrate_bps,
     *bitrate_bps = std::max(*min_bitrate_bps, *bitrate_bps);
 }
 
-class WrappingBitrateEstimator : public RemoteBitrateEstimator {
- public:
-  WrappingBitrateEstimator(RemoteBitrateObserver* observer, Clock* clock)
-      : observer_(observer),
-        clock_(clock),
-        crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
-        rbe_(new RemoteBitrateEstimatorSingleStream(observer_, clock_)),
-        using_absolute_send_time_(false),
-        packets_since_absolute_send_time_(0),
-        min_bitrate_bps_(congestion_controller::GetMinBitrateBps()) {}
-
-  virtual ~WrappingBitrateEstimator() {}
-
-  void IncomingPacket(int64_t arrival_time_ms,
-                      size_t payload_size,
-                      const RTPHeader& header) override {
-    CriticalSectionScoped cs(crit_sect_.get());
-    PickEstimatorFromHeader(header);
-    rbe_->IncomingPacket(arrival_time_ms, payload_size, header);
-  }
+}  // namespace
 
-  void Process() override {
-    CriticalSectionScoped cs(crit_sect_.get());
-    rbe_->Process();
-  }
+CongestionController::WrappingBitrateEstimator::WrappingBitrateEstimator(
+    RemoteBitrateObserver* observer, Clock* clock)
+    : observer_(observer),
+      clock_(clock),
+      rbe_(new RemoteBitrateEstimatorSingleStream(observer_, clock_)),
+      using_absolute_send_time_(false),
+      packets_since_absolute_send_time_(0),
+      min_bitrate_bps_(congestion_controller::GetMinBitrateBps()) {}
+
+void CongestionController::WrappingBitrateEstimator::IncomingPacket(
+    int64_t arrival_time_ms,
+    size_t payload_size,
+    const RTPHeader& header) {
+  rtc::CritScope cs(&crit_sect_);
+  PickEstimatorFromHeader(header);
+  rbe_->IncomingPacket(arrival_time_ms, payload_size, header);
+}
 
-  int64_t TimeUntilNextProcess() override {
-    CriticalSectionScoped cs(crit_sect_.get());
-    return rbe_->TimeUntilNextProcess();
-  }
+void CongestionController::WrappingBitrateEstimator::Process() {
+  rtc::CritScope cs(&crit_sect_);
+  rbe_->Process();
+}
 
-  void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override {
-    CriticalSectionScoped cs(crit_sect_.get());
-    rbe_->OnRttUpdate(avg_rtt_ms, max_rtt_ms);
-  }
+int64_t CongestionController::WrappingBitrateEstimator::TimeUntilNextProcess() {
+  rtc::CritScope cs(&crit_sect_);
+  return rbe_->TimeUntilNextProcess();
+}
 
-  void RemoveStream(unsigned int ssrc) override {
-    CriticalSectionScoped cs(crit_sect_.get());
-    rbe_->RemoveStream(ssrc);
-  }
+void CongestionController::WrappingBitrateEstimator::OnRttUpdate(
+    int64_t avg_rtt_ms, int64_t max_rtt_ms) {
+  rtc::CritScope cs(&crit_sect_);
+  rbe_->OnRttUpdate(avg_rtt_ms, max_rtt_ms);
+}
 
-  bool LatestEstimate(std::vector<unsigned int>* ssrcs,
-                      unsigned int* bitrate_bps) const override {
-    CriticalSectionScoped cs(crit_sect_.get());
-    return rbe_->LatestEstimate(ssrcs, bitrate_bps);
-  }
+void CongestionController::WrappingBitrateEstimator::RemoveStream(
+    unsigned int ssrc) {
+  rtc::CritScope cs(&crit_sect_);
+  rbe_->RemoveStream(ssrc);
+}
 
-  void SetMinBitrate(int min_bitrate_bps) override {
-    CriticalSectionScoped cs(crit_sect_.get());
-    rbe_->SetMinBitrate(min_bitrate_bps);
-    min_bitrate_bps_ = min_bitrate_bps;
-  }
+bool CongestionController::WrappingBitrateEstimator::LatestEstimate(
+    std::vector<unsigned int>* ssrcs,
+    unsigned int* bitrate_bps) const {
+  rtc::CritScope cs(&crit_sect_);
+  return rbe_->LatestEstimate(ssrcs, bitrate_bps);
+}
+
+void CongestionController::WrappingBitrateEstimator::SetMinBitrate(
+    int min_bitrate_bps) {
+  rtc::CritScope cs(&crit_sect_);
+  rbe_->SetMinBitrate(min_bitrate_bps);
+  min_bitrate_bps_ = min_bitrate_bps;
+}
 
- private:
-  void PickEstimatorFromHeader(const RTPHeader& header)
-      EXCLUSIVE_LOCKS_REQUIRED(crit_sect_.get()) {
-    if (header.extension.hasAbsoluteSendTime) {
-      // If we see AST in header, switch RBE strategy immediately.
-      if (!using_absolute_send_time_) {
-        LOG(LS_INFO) <<
-            "WrappingBitrateEstimator: Switching to absolute send time RBE.";
-        using_absolute_send_time_ = true;
+void CongestionController::WrappingBitrateEstimator::PickEstimatorFromHeader(
+    const RTPHeader& header) {
+  if (header.extension.hasAbsoluteSendTime) {
+    // If we see AST in header, switch RBE strategy immediately.
+    if (!using_absolute_send_time_) {
+      LOG(LS_INFO) <<
+          "WrappingBitrateEstimator: Switching to absolute send time RBE.";
+      using_absolute_send_time_ = true;
+      PickEstimator();
+    }
+    packets_since_absolute_send_time_ = 0;
+  } else {
+    // When we don't see AST, wait for a few packets before going back to TOF.
+    if (using_absolute_send_time_) {
+      ++packets_since_absolute_send_time_;
+      if (packets_since_absolute_send_time_ >= kTimeOffsetSwitchThreshold) {
+        LOG(LS_INFO) << "WrappingBitrateEstimator: Switching to transmission "
+                     << "time offset RBE.";
+        using_absolute_send_time_ = false;
         PickEstimator();
       }
-      packets_since_absolute_send_time_ = 0;
-    } else {
-      // When we don't see AST, wait for a few packets before going back to TOF.
-      if (using_absolute_send_time_) {
-        ++packets_since_absolute_send_time_;
-        if (packets_since_absolute_send_time_ >= kTimeOffsetSwitchThreshold) {
-          LOG(LS_INFO) << "WrappingBitrateEstimator: Switching to transmission "
-                       << "time offset RBE.";
-          using_absolute_send_time_ = false;
-          PickEstimator();
-        }
-      }
     }
   }
+}
 
-  // Instantiate RBE for Time Offset or Absolute Send Time extensions.
-  void PickEstimator() EXCLUSIVE_LOCKS_REQUIRED(crit_sect_.get()) {
-    if (using_absolute_send_time_) {
-      rbe_.reset(new RemoteBitrateEstimatorAbsSendTime(observer_, clock_));
-    } else {
-      rbe_.reset(new RemoteBitrateEstimatorSingleStream(observer_, clock_));
-    }
-    rbe_->SetMinBitrate(min_bitrate_bps_);
+// Instantiate RBE for Time Offset or Absolute Send Time extensions.
+void CongestionController::WrappingBitrateEstimator::PickEstimator() {
+  if (using_absolute_send_time_) {
+    rbe_.reset(new RemoteBitrateEstimatorAbsSendTime(observer_, clock_));
+  } else {
+    rbe_.reset(new RemoteBitrateEstimatorSingleStream(observer_, clock_));
   }
-
-  RemoteBitrateObserver* observer_;
-  Clock* const clock_;
-  std::unique_ptr<CriticalSectionWrapper> crit_sect_;
-  std::unique_ptr<RemoteBitrateEstimator> rbe_;
-  bool using_absolute_send_time_;
-  uint32_t packets_since_absolute_send_time_;
-  int min_bitrate_bps_;
-
-  RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(WrappingBitrateEstimator);
-};
-
-}  // namespace
+  rbe_->SetMinBitrate(min_bitrate_bps_);
+}
 
 CongestionController::CongestionController(
     Clock* clock,
@@ -174,8 +162,6 @@ CongestionController::CongestionController(
       observer_(observer),
       packet_router_(packet_router),
       pacer_(std::move(pacer)),
-      remote_bitrate_estimator_(
-          new WrappingBitrateEstimator(remote_bitrate_observer, clock_)),
       // Constructed last as this object calls the provided callback on

[nisse-webrtc, 2017/01/23 10:47:02]

This comment looks a bit scary, since bitrate_controller_ is far from last. If
the comment is still valid, can we clarify what other members
bitrate_controller_ depends on? Or should it just be deleted?

[nisse-webrtc, 2017/01/23 13:05:15]

I'm deleting this comment, BitrateController has an optional and deprecated
BitrateObserver which is invoked from the constructor if set, but in our case,
there's no such observer.

       // construction.
       bitrate_controller_(
@@ -183,6 +169,7 @@ CongestionController::CongestionController(
       probe_controller_(new ProbeController(pacer_.get(), clock_)),
       retransmission_rate_limiter_(
           new RateLimiter(clock, kRetransmitWindowSizeMs)),
+      remote_bitrate_estimator_(remote_bitrate_observer, clock_),
       remote_estimator_proxy_(clock_, packet_router_),
       transport_feedback_adapter_(clock_, bitrate_controller_.get()),
       min_bitrate_bps_(congestion_controller::GetMinBitrateBps()),
@@ -204,12 +191,9 @@ void CongestionController::OnReceivedPacket(int64_t arrival_time_ms,
   if (header.extension.hasTransportSequenceNumber) {
     remote_estimator_proxy_.IncomingPacket(arrival_time_ms, payload_size,
                                            header);
-    return;
-  }
-
-  // Receive-side BWE.
-  if (remote_bitrate_estimator_) {
-    remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size,
+  } else {
+    // Receive-side BWE.
+    remote_bitrate_estimator_.IncomingPacket(arrival_time_ms, payload_size,
                                               header);
   }
 }
@@ -226,8 +210,7 @@ void CongestionController::SetBweBitrates(int min_bitrate_bps,
                                  max_bitrate_bps);
   max_bitrate_bps_ = max_bitrate_bps;
 
-  if (remote_bitrate_estimator_)
-    remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps);
+  remote_bitrate_estimator_.SetMinBitrate(min_bitrate_bps);
   min_bitrate_bps_ = min_bitrate_bps;
   transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps_);
   MaybeTriggerOnNetworkChanged();
@@ -245,8 +228,7 @@ void CongestionController::ResetBweAndBitrates(int bitrate_bps,
   max_bitrate_bps_ = max_bitrate_bps;
   // TODO(honghaiz): Recreate this object once the remote bitrate estimator is
   // no longer exposed outside CongestionController.
-  if (remote_bitrate_estimator_)
-    remote_bitrate_estimator_->SetMinBitrate(min_bitrate_bps);
+  remote_bitrate_estimator_.SetMinBitrate(min_bitrate_bps);
 
   transport_feedback_adapter_.InitBwe();
   transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps);
@@ -263,7 +245,7 @@ RemoteBitrateEstimator* CongestionController::GetRemoteBitrateEstimator(
   if (send_side_bwe) {
     return &remote_estimator_proxy_;
   } else {
-    return remote_bitrate_estimator_.get();
+    return &remote_bitrate_estimator_;
   }
 }
 
@@ -322,18 +304,18 @@ void CongestionController::OnSentPacket(const rtc::SentPacket& sent_packet) {
 }
 
 void CongestionController::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) {
-  remote_bitrate_estimator_->OnRttUpdate(avg_rtt_ms, max_rtt_ms);
+  remote_bitrate_estimator_.OnRttUpdate(avg_rtt_ms, max_rtt_ms);
   transport_feedback_adapter_.OnRttUpdate(avg_rtt_ms, max_rtt_ms);
 }
 
 int64_t CongestionController::TimeUntilNextProcess() {
   return std::min(bitrate_controller_->TimeUntilNextProcess(),
-                  remote_bitrate_estimator_->TimeUntilNextProcess());
+                  remote_bitrate_estimator_.TimeUntilNextProcess());
 }
 
 void CongestionController::Process() {
   bitrate_controller_->Process();
-  remote_bitrate_estimator_->Process();
+  remote_bitrate_estimator_.Process();
   probe_controller_->Process();
   MaybeTriggerOnNetworkChanged();
 }