Refactor how padding is calculated.

webrtc/call/bitrate_allocator.cc

Index: webrtc/call/bitrate_allocator.cc
diff --git a/webrtc/call/bitrate_allocator.cc b/webrtc/call/bitrate_allocator.cc
index 3672ef520ca3422bc372d2108b75abf9b5390666..efff2a50ed78e36aee8a1271df3d4fdc6e131146 100644
--- a/webrtc/call/bitrate_allocator.cc
+++ b/webrtc/call/bitrate_allocator.cc
@@ -24,17 +24,18 @@ namespace webrtc {
 const int kTransmissionMaxBitrateMultiplier = 2;
 const int kDefaultBitrateBps = 300000;
 
-BitrateAllocator::BitrateAllocator()
-    : bitrate_observer_configs_(),
+BitrateAllocator::BitrateAllocator(LimitObserver* limit_observer)
+    : limit_observer_(limit_observer),
+      bitrate_observer_configs_(),
       enforce_min_bitrate_(true),
       last_bitrate_bps_(kDefaultBitrateBps),
       last_non_zero_bitrate_bps_(kDefaultBitrateBps),
       last_fraction_loss_(0),
       last_rtt_(0) {}
 
-uint32_t BitrateAllocator::OnNetworkChanged(uint32_t bitrate,
-                                            uint8_t fraction_loss,
-                                            int64_t rtt) {
+void BitrateAllocator::OnNetworkChanged(uint32_t bitrate,
+                                        uint8_t fraction_loss,
+                                        int64_t rtt) {
   rtc::CritScope lock(&crit_sect_);
   last_bitrate_bps_ = bitrate;
   last_non_zero_bitrate_bps_ =
@@ -42,18 +43,16 @@ uint32_t BitrateAllocator::OnNetworkChanged(uint32_t bitrate,
   last_fraction_loss_ = fraction_loss;
   last_rtt_ = rtt;
 
-  uint32_t allocated_bitrate_bps = 0;
   ObserverAllocation allocation = AllocateBitrates(bitrate);
   for (const auto& kv : allocation) {
     kv.first->OnBitrateUpdated(kv.second, last_fraction_loss_, last_rtt_);
-    allocated_bitrate_bps += kv.second;
   }
-  return allocated_bitrate_bps;
 }
 
 int BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer,
                                   uint32_t min_bitrate_bps,
                                   uint32_t max_bitrate_bps,
+                                  uint32_t pad_up_bitrate_bps,
                                   bool enforce_min_bitrate) {
   rtc::CritScope lock(&crit_sect_);
   // TODO(mflodman): Enforce this per observer.
@@ -70,10 +69,12 @@ int BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer,
     // Update current configuration.
     it->min_bitrate_bps = min_bitrate_bps;
     it->max_bitrate_bps = max_bitrate_bps;
+    it->pad_up_bitrate_bps = pad_up_bitrate_bps;
   } else {
     // Add new settings.
-    bitrate_observer_configs_.push_back(ObserverConfig(
-        observer, min_bitrate_bps, max_bitrate_bps, enforce_min_bitrate));
+    bitrate_observer_configs_.push_back(
+        ObserverConfig(observer, min_bitrate_bps, max_bitrate_bps,
+                       pad_up_bitrate_bps, enforce_min_bitrate));
   }
 
   int new_observer_bitrate_bps = 0;
@@ -94,15 +95,49 @@ int BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer,
     observer->OnBitrateUpdated(0, last_fraction_loss_, last_rtt_);
     new_observer_bitrate_bps = allocation[observer];
   }
+
+  UpdateAllocationLimits();
+
   return new_observer_bitrate_bps;
 }
 
+void BitrateAllocator::NotifyObserverInactive(

[mflodman, 2016/06/08 10:18:31]

Can we in this case just remove the observer instead?

Or in another way, if it is inactive, wouldn't it be better to remove it from
the allocation?

[perkj_webrtc, 2016/06/08 15:35:27]

Seems like that is what stefan suggests too.

+    BitrateAllocatorObserver* observer,
+    bool inactive) {
+  rtc::CritScope lock(&crit_sect_);
+  auto iter = std::find_if(
+      bitrate_observer_configs_.begin(), bitrate_observer_configs_.end(),
+      [observer](const ObserverConfig& c) { return c.observer == observer; });
+  RTC_DCHECK(iter != bitrate_observer_configs_.end());
+  iter->active = !inactive;
+
+  UpdateAllocationLimits();
+}
+
+void BitrateAllocator::UpdateAllocationLimits() {
+  uint32_t total_requested_padding_bitrate = 0;
+  uint32_t total_requested_min_bitrate = 0;
+
+  for (const auto& config : bitrate_observer_configs_) {
+    if (enforce_min_bitrate_) {
+      total_requested_min_bitrate += config.min_bitrate_bps;
+    }
+    if (config.active)
+      total_requested_padding_bitrate += config.pad_up_bitrate_bps;
+  }
+  crit_sect_.Leave();

[stefan-webrtc, 2016/06/08 09:25:13]

I don't like this... Can we make sure we can OnAllocationLimitsChanged() after
the crit is no longer active? You should be able to scope the crit in
AddObserver() so that it is no longer active once you call
OnAllocationLimitsChanged at the end. Same for NotifyObserverInactive.

[perkj_webrtc, 2016/06/08 15:35:27]

Yes, but with code duplication. Do you have a suggestion?

+  limit_observer_->OnAllocationLimitsChanged(total_requested_min_bitrate,
+                                             total_requested_padding_bitrate);
+  crit_sect_.Enter();
+}
+
 void BitrateAllocator::RemoveObserver(BitrateAllocatorObserver* observer) {
   rtc::CritScope lock(&crit_sect_);
   auto it = FindObserverConfig(observer);
   if (it != bitrate_observer_configs_.end()) {
     bitrate_observer_configs_.erase(it);
   }
+  UpdateAllocationLimits();
 }
 
 void BitrateAllocator::EnforceMinBitrate(bool enforce_min_bitrate) {