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
diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc
index f4f085c..3810406 100644
--- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc
+++ b/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 @@
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()) {
- if (enforce_min_bitrate) {
- low_rate_strategy_.reset(new EnforceMinRateStrategy(
- &bandwidth_estimation_, &bitrate_observers_));
- } else {
- low_rate_strategy_.reset(new NoEnforceMinRateStrategy(
- &bandwidth_estimation_, &bitrate_observers_));
- }
-}
+ : critsect_(CriticalSectionWrapper::CreateCriticalSection()),
+ enforce_min_bitrate_(enforce_min_bitrate) {}
BitrateControllerImpl::~BitrateControllerImpl() {
BitrateObserverConfList::iterator it =
@@ -240,13 +165,7 @@
void BitrateControllerImpl::EnforceMinBitrate(bool enforce_min_bitrate) {
CriticalSectionScoped cs(critsect_);
- if (enforce_min_bitrate) {
- low_rate_strategy_.reset(new EnforceMinRateStrategy(
- &bandwidth_estimation_, &bitrate_observers_));
- } else {
- low_rate_strategy_.reset(new NoEnforceMinRateStrategy(
- &bandwidth_estimation_, &bitrate_observers_));
- }
+ enforce_min_bitrate_ = enforce_min_bitrate;
}
void BitrateControllerImpl::SetBweMinBitrate(uint32_t min_bitrate) {
@@ -281,28 +200,34 @@
}
}
-// 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 (number_of_observers == 0) {
+ if (bitrate_observers_.empty())
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) {
- return low_rate_strategy_->LowRateAllocation(bitrate, fraction_loss, rtt,
- sum_min_bitrates);
- }
+ if (bitrate <= sum_min_bitrates)
+ return LowRateAllocation(bitrate, fraction_loss, rtt, 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::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);
}
diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h
index a5bba44..4e40997 100644
--- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h
+++ b/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 @@
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 @@
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_;
- BitrateObserverConfList bitrate_observers_;
- scoped_ptr<LowRateStrategy> low_rate_strategy_;
+ SendSideBandwidthEstimation bandwidth_estimation_ GUARDED_BY(*critsect_);
+ BitrateObserverConfList bitrate_observers_ GUARDED_BY(*critsect_);
+ bool enforce_min_bitrate_ GUARDED_BY(*critsect_);
};
} // namespace webrtc
#endif // WEBRTC_MODULES_BITRATE_CONTROLLER_BITRATE_CONTROLLER_IMPL_H_
diff --git a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
index e01ca40..a089590 100644
--- a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
+++ b/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 @@
last_round_trip_time_(0),
bwe_incoming_(0),
time_last_increase_(0),
- time_last_decrease_(0) {
-}
+ time_last_decrease_(0) {}
-SendSideBandwidthEstimation::~SendSideBandwidthEstimation() {
- delete critsect_;
-}
+SendSideBandwidthEstimation::~SendSideBandwidthEstimation() {}
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::SetMinBitrate(uint32_t min_bitrate) {
- CriticalSectionScoped cs(critsect_);
min_bitrate_configured_ = min_bitrate;
}
@@ -62,8 +55,6 @@
uint8_t* fraction_lost,
uint16_t* rtt) {
*new_bitrate = 0;
- CriticalSectionScoped cs(critsect_);
-
bwe_incoming_ = bandwidth;
if (bitrate_ == 0) {
@@ -86,8 +77,6 @@
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::AvailableBandwidth(
uint32_t* bandwidth) const {
- CriticalSectionScoped cs(critsect_);
if (bitrate_ == 0) {
return false;
}
diff --git a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h
index 1d4786c..79b9e2d 100644
--- a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h
+++ b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h
@@ -56,8 +56,6 @@
enum { kLimitNumPackets = 20 };
enum { kAvgPacketSizeBytes = 1000 };
- CriticalSectionWrapper* critsect_;
-
// incoming filters
int accumulate_lost_packets_Q8_;
int accumulate_expected_packets_;
