Remove locks in SendSideBandwidthEstimation since those are only accessed while owning locks in

BitrateControllerImpl (excluding AvailableBandwidth).

 + Refactor BitrateController logic around LowRate allocation so access to SendSideBandwidthEstimation
is clear.
 + Refactor NormalRateAllocation away from OnNetworkChange.
 + Annotate BitrateController locks.

R=henrik.lundin@webrtc.org, stefan@webrtc.org
BUG=3065

Review URL: https://webrtc-codereview.appspot.com/10129004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@5749 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/bitrate_controller/bitrate_controller_impl.cc

@@ -18,7 +18,8 @@
 
 namespace webrtc {
 
-class RtcpBandwidthObserverImpl : public RtcpBandwidthObserver {
+class BitrateControllerImpl::RtcpBandwidthObserverImpl
+    : public RtcpBandwidthObserver {
  public:
   explicit RtcpBandwidthObserverImpl(BitrateControllerImpl* owner)
       : owner_(owner) {
@@ -76,90 +77,14 @@ class RtcpBandwidthObserverImpl : public RtcpBandwidthObserver {
   BitrateControllerImpl* owner_;
 };
 
-class LowRateStrategy {
- public:
-  LowRateStrategy(
-      SendSideBandwidthEstimation* bandwidth_estimation,
-      BitrateControllerImpl::BitrateObserverConfList* bitrate_observers)
-      : bandwidth_estimation_(bandwidth_estimation),
-        bitrate_observers_(bitrate_observers) {}
-
-  virtual ~LowRateStrategy() {}
-
-  virtual void LowRateAllocation(uint32_t bitrate,
-                                 uint8_t fraction_loss,
-                                 uint32_t rtt,
-                                 uint32_t sum_min_bitrates) = 0;
-
- protected:
-  SendSideBandwidthEstimation* bandwidth_estimation_;
-  BitrateControllerImpl::BitrateObserverConfList* bitrate_observers_;
-};
-
-class EnforceMinRateStrategy : public LowRateStrategy {
- public:
-  EnforceMinRateStrategy(
-      SendSideBandwidthEstimation* bandwidth_estimation,
-      BitrateControllerImpl::BitrateObserverConfList* bitrate_observers)
-      : LowRateStrategy(bandwidth_estimation, bitrate_observers) {}
-
-  void LowRateAllocation(uint32_t bitrate,
-                         uint8_t fraction_loss,
-                         uint32_t rtt,
-                         uint32_t sum_min_bitrates) {
-    // Min bitrate to all observers.
-    BitrateControllerImpl::BitrateObserverConfList::iterator it;
-    for (it = bitrate_observers_->begin(); it != bitrate_observers_->end();
-        ++it) {
-      it->first->OnNetworkChanged(it->second->min_bitrate_, fraction_loss,
-                                  rtt);
-    }
-    // Set sum of min to current send bitrate.
-    bandwidth_estimation_->SetSendBitrate(sum_min_bitrates);
-  }
-};
-
-class NoEnforceMinRateStrategy : public LowRateStrategy {
- public:
-  NoEnforceMinRateStrategy(
-      SendSideBandwidthEstimation* bandwidth_estimation,
-      BitrateControllerImpl::BitrateObserverConfList* bitrate_observers)
-      : LowRateStrategy(bandwidth_estimation, bitrate_observers) {}
-
-  void LowRateAllocation(uint32_t bitrate,
-                         uint8_t fraction_loss,
-                         uint32_t rtt,
-                         uint32_t sum_min_bitrates) {
-    // Allocate up to |min_bitrate_| to one observer at a time, until
-    // |bitrate| is depleted.
-    uint32_t remainder = bitrate;
-    BitrateControllerImpl::BitrateObserverConfList::iterator it;
-    for (it = bitrate_observers_->begin(); it != bitrate_observers_->end();
-        ++it) {
-      uint32_t allocation = std::min(remainder, it->second->min_bitrate_);
-      it->first->OnNetworkChanged(allocation, fraction_loss, rtt);
-      remainder -= allocation;
-    }
-    // Set |bitrate| to current send bitrate.
-    bandwidth_estimation_->SetSendBitrate(bitrate);
-  }
-};
-
 BitrateController* BitrateController::CreateBitrateController(
     bool enforce_min_bitrate) {
   return new BitrateControllerImpl(enforce_min_bitrate);
 }
 
 BitrateControllerImpl::BitrateControllerImpl(bool enforce_min_bitrate)
-    : critsect_(CriticalSectionWrapper::CreateCriticalSection()) {
+    : critsect_(CriticalSectionWrapper::CreateCriticalSection()),
-  if (enforce_min_bitrate) {
+      enforce_min_bitrate_(enforce_min_bitrate) {}
-    low_rate_strategy_.reset(new EnforceMinRateStrategy(
-        &bandwidth_estimation_, &bitrate_observers_));
-  } else {
-    low_rate_strategy_.reset(new NoEnforceMinRateStrategy(
-        &bandwidth_estimation_, &bitrate_observers_));
-  }
-}
 
 BitrateControllerImpl::~BitrateControllerImpl() {
   BitrateObserverConfList::iterator it =
@@ -240,13 +165,7 @@ void BitrateControllerImpl::RemoveBitrateObserver(BitrateObserver* observer) {
 
 void BitrateControllerImpl::EnforceMinBitrate(bool enforce_min_bitrate) {
   CriticalSectionScoped cs(critsect_);
-  if (enforce_min_bitrate) {
+  enforce_min_bitrate_ = enforce_min_bitrate;
-    low_rate_strategy_.reset(new EnforceMinRateStrategy(
-        &bandwidth_estimation_, &bitrate_observers_));
-  } else {
-    low_rate_strategy_.reset(new NoEnforceMinRateStrategy(
-        &bandwidth_estimation_, &bitrate_observers_));
-  }
 }
 
 void BitrateControllerImpl::SetBweMinBitrate(uint32_t min_bitrate) {
@@ -281,28 +200,34 @@ void BitrateControllerImpl::OnReceivedRtcpReceiverReport(
   }
 }
 
-// We have the lock here.
 void BitrateControllerImpl::OnNetworkChanged(const uint32_t bitrate,
                                              const uint8_t fraction_loss,
                                              const uint32_t rtt) {
   // Sanity check.
-  uint32_t number_of_observers = bitrate_observers_.size();
+  if (bitrate_observers_.empty())
-  if (number_of_observers == 0) {
     return;
-  }
+
   uint32_t sum_min_bitrates = 0;
   BitrateObserverConfList::iterator it;
   for (it = bitrate_observers_.begin(); it != bitrate_observers_.end(); ++it) {
     sum_min_bitrates += it->second->min_bitrate_;
   }
-  if (bitrate <= sum_min_bitrates) {
+  if (bitrate <= sum_min_bitrates)
-    return low_rate_strategy_->LowRateAllocation(bitrate, fraction_loss, rtt,
+    return LowRateAllocation(bitrate, fraction_loss, rtt, sum_min_bitrates);
-                                                 sum_min_bitrates);
+  else
+    return NormalRateAllocation(bitrate, fraction_loss, rtt, sum_min_bitrates);
 }
+
+void BitrateControllerImpl::NormalRateAllocation(uint32_t bitrate,
+                                                 uint8_t fraction_loss,
+                                                 uint32_t rtt,
+                                                 uint32_t sum_min_bitrates) {
+  uint32_t number_of_observers = bitrate_observers_.size();
   uint32_t bitrate_per_observer = (bitrate - sum_min_bitrates) /
       number_of_observers;
   // Use map to sort list based on max bitrate.
   ObserverSortingMap list_max_bitrates;
+  BitrateObserverConfList::iterator it;
   for (it = bitrate_observers_.begin(); it != bitrate_observers_.end(); ++it) {
     list_max_bitrates.insert(std::pair<uint32_t, ObserverConfiguration*>(
         it->second->max_bitrate_,
@@ -333,7 +258,37 @@ void BitrateControllerImpl::OnNetworkChanged(const uint32_t bitrate,
   }
 }
 
+void BitrateControllerImpl::LowRateAllocation(uint32_t bitrate,
+                                              uint8_t fraction_loss,
+                                              uint32_t rtt,
+                                              uint32_t sum_min_bitrates) {
+  if (enforce_min_bitrate_) {
+    // Min bitrate to all observers.
+    BitrateControllerImpl::BitrateObserverConfList::iterator it;
+    for (it = bitrate_observers_.begin(); it != bitrate_observers_.end();
+         ++it) {
+      it->first->OnNetworkChanged(it->second->min_bitrate_, fraction_loss, rtt);
+    }
+    // Set sum of min to current send bitrate.
+    bandwidth_estimation_.SetSendBitrate(sum_min_bitrates);
+  } else {
+    // Allocate up to |min_bitrate_| to one observer at a time, until
+    // |bitrate| is depleted.
+    uint32_t remainder = bitrate;
+    BitrateControllerImpl::BitrateObserverConfList::iterator it;
+    for (it = bitrate_observers_.begin(); it != bitrate_observers_.end();
+         ++it) {
+      uint32_t allocation = std::min(remainder, it->second->min_bitrate_);
+      it->first->OnNetworkChanged(allocation, fraction_loss, rtt);
+      remainder -= allocation;
+    }
+    // Set |bitrate| to current send bitrate.
+    bandwidth_estimation_.SetSendBitrate(bitrate);
+  }
+}
+
 bool BitrateControllerImpl::AvailableBandwidth(uint32_t* bandwidth) const {
+  CriticalSectionScoped cs(critsect_);
   return bandwidth_estimation_.AvailableBandwidth(bandwidth);
 }
 

webrtc/modules/bitrate_controller/bitrate_controller_impl.h

@@ -27,12 +27,28 @@
 
 namespace webrtc {
 
-class RtcpBandwidthObserverImpl;
-class LowRateStrategy;
-
 class BitrateControllerImpl : public BitrateController {
  public:
-  friend class RtcpBandwidthObserverImpl;
+  explicit BitrateControllerImpl(bool enforce_min_bitrate);
+  virtual ~BitrateControllerImpl();
+
+  virtual bool AvailableBandwidth(uint32_t* bandwidth) const OVERRIDE;
+
+  virtual RtcpBandwidthObserver* CreateRtcpBandwidthObserver() OVERRIDE;
+
+  virtual void SetBitrateObserver(BitrateObserver* observer,
+                                  const uint32_t start_bitrate,
+                                  const uint32_t min_bitrate,
+                                  const uint32_t max_bitrate) OVERRIDE;
+
+  virtual void RemoveBitrateObserver(BitrateObserver* observer) OVERRIDE;
+
+  virtual void EnforceMinBitrate(bool enforce_min_bitrate) OVERRIDE;
+
+  virtual void SetBweMinBitrate(uint32_t min_bitrate) OVERRIDE;
+
+ private:
+  class RtcpBandwidthObserverImpl;
 
   struct BitrateConfiguration {
     BitrateConfiguration(uint32_t start_bitrate,
@@ -59,25 +75,7 @@ class BitrateControllerImpl : public BitrateController {
       BitrateObserverConfiguration;
   typedef std::list<BitrateObserverConfiguration> BitrateObserverConfList;
 
-  explicit BitrateControllerImpl(bool enforce_min_bitrate);
-  virtual ~BitrateControllerImpl();
 
-  virtual bool AvailableBandwidth(uint32_t* bandwidth) const OVERRIDE;
-
-  virtual RtcpBandwidthObserver* CreateRtcpBandwidthObserver() OVERRIDE;
-
-  virtual void SetBitrateObserver(BitrateObserver* observer,
-                                  const uint32_t start_bitrate,
-                                  const uint32_t min_bitrate,
-                                  const uint32_t max_bitrate) OVERRIDE;
-
-  virtual void RemoveBitrateObserver(BitrateObserver* observer) OVERRIDE;
-
-  virtual void EnforceMinBitrate(bool enforce_min_bitrate) OVERRIDE;
-
-  virtual void SetBweMinBitrate(uint32_t min_bitrate) OVERRIDE;
-
- private:
   // Called by BitrateObserver's direct from the RTCP module.
   void OnReceivedEstimatedBitrate(const uint32_t bitrate);
 
@@ -86,18 +84,32 @@ class BitrateControllerImpl : public BitrateController {
                                     const int number_of_packets,
                                     const uint32_t now_ms);
 
-  typedef std::multimap<uint32_t, ObserverConfiguration*> ObserverSortingMap;
-
-  BitrateObserverConfList::iterator
-      FindObserverConfigurationPair(const BitrateObserver* observer);
   void OnNetworkChanged(const uint32_t bitrate,
                         const uint8_t fraction_loss,  // 0 - 255.
-                        const uint32_t rtt);
+                        const uint32_t rtt)
+      EXCLUSIVE_LOCKS_REQUIRED(*critsect_);
+
+  void NormalRateAllocation(uint32_t bitrate,
+                            uint8_t fraction_loss,
+                            uint32_t rtt,
+                            uint32_t sum_min_bitrates)
+      EXCLUSIVE_LOCKS_REQUIRED(*critsect_);
+
+  void LowRateAllocation(uint32_t bitrate,
+                         uint8_t fraction_loss,
+                         uint32_t rtt,
+                         uint32_t sum_min_bitrates)
+      EXCLUSIVE_LOCKS_REQUIRED(*critsect_);
+
+  typedef std::multimap<uint32_t, ObserverConfiguration*> ObserverSortingMap;
+
+  BitrateObserverConfList::iterator FindObserverConfigurationPair(
+      const BitrateObserver* observer) EXCLUSIVE_LOCKS_REQUIRED(*critsect_);
 
   CriticalSectionWrapper* critsect_;
-  SendSideBandwidthEstimation bandwidth_estimation_;
+  SendSideBandwidthEstimation bandwidth_estimation_ GUARDED_BY(*critsect_);
-  BitrateObserverConfList bitrate_observers_;
+  BitrateObserverConfList bitrate_observers_ GUARDED_BY(*critsect_);
-  scoped_ptr<LowRateStrategy> low_rate_strategy_;
+  bool enforce_min_bitrate_ GUARDED_BY(*critsect_);
 };
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_BITRATE_CONTROLLER_BITRATE_CONTROLLER_IMPL_H_

webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc

@@ -17,8 +17,7 @@
 namespace webrtc {
 
 SendSideBandwidthEstimation::SendSideBandwidthEstimation()
-    : critsect_(CriticalSectionWrapper::CreateCriticalSection()),
+    : accumulate_lost_packets_Q8_(0),
-      accumulate_lost_packets_Q8_(0),
       accumulate_expected_packets_(0),
       bitrate_(0),
       min_bitrate_configured_(0),
@@ -27,21 +26,16 @@ SendSideBandwidthEstimation::SendSideBandwidthEstimation()
       last_round_trip_time_(0),
       bwe_incoming_(0),
       time_last_increase_(0),
-      time_last_decrease_(0) {
+      time_last_decrease_(0) {}
-}
 
-SendSideBandwidthEstimation::~SendSideBandwidthEstimation() {
+SendSideBandwidthEstimation::~SendSideBandwidthEstimation() {}
-    delete critsect_;
-}
 
 void SendSideBandwidthEstimation::SetSendBitrate(const uint32_t bitrate) {
-  CriticalSectionScoped cs(critsect_);
   bitrate_ = bitrate;
 }
 
 void SendSideBandwidthEstimation::SetMinMaxBitrate(const uint32_t min_bitrate,
                                                    const uint32_t max_bitrate) {
-  CriticalSectionScoped cs(critsect_);
   min_bitrate_configured_ = min_bitrate;
   if (max_bitrate == 0) {
     // no max configured use 1Gbit/s
@@ -52,7 +46,6 @@ void SendSideBandwidthEstimation::SetMinMaxBitrate(const uint32_t min_bitrate,
 }
 
 void SendSideBandwidthEstimation::SetMinBitrate(uint32_t min_bitrate) {
-  CriticalSectionScoped cs(critsect_);
   min_bitrate_configured_ = min_bitrate;
 }
 
@@ -62,8 +55,6 @@ bool SendSideBandwidthEstimation::UpdateBandwidthEstimate(
     uint8_t* fraction_lost,
     uint16_t* rtt) {
   *new_bitrate = 0;
-  CriticalSectionScoped cs(critsect_);
-
   bwe_incoming_ = bandwidth;
 
   if (bitrate_ == 0) {
@@ -86,8 +77,6 @@ bool SendSideBandwidthEstimation::UpdatePacketLoss(
     const uint32_t now_ms,
     uint8_t* loss,
     uint32_t* new_bitrate) {
-  CriticalSectionScoped cs(critsect_);
-
   if (bitrate_ == 0) {
     // SendSideBandwidthEstimation off
     return false;
@@ -130,7 +119,6 @@ bool SendSideBandwidthEstimation::UpdatePacketLoss(
 
 bool SendSideBandwidthEstimation::AvailableBandwidth(
     uint32_t* bandwidth) const {
-  CriticalSectionScoped cs(critsect_);
   if (bitrate_ == 0) {
     return false;
   }

webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h

@@ -56,8 +56,6 @@ class SendSideBandwidthEstimation {
   enum { kLimitNumPackets = 20 };
   enum { kAvgPacketSizeBytes = 1000 };
 
-  CriticalSectionWrapper* critsect_;
-
   // incoming filters
   int accumulate_lost_packets_Q8_;
   int accumulate_expected_packets_;