BWE allocation strategy

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 2eb40b0670813c008b1b594541fc1e12147e47e9..eecd852da227135c1b4f2b73f2a9666a39a246b4 100644
--- a/webrtc/call/bitrate_allocator.cc
+++ b/webrtc/call/bitrate_allocator.cc
@@ -16,6 +16,7 @@
 
 #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
 #include "webrtc/rtc_base/checks.h"
+#include "webrtc/rtc_base/helpers.h"
 #include "webrtc/rtc_base/logging.h"
 #include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/system_wrappers/include/metrics.h"
@@ -56,7 +57,8 @@ BitrateAllocator::BitrateAllocator(LimitObserver* limit_observer)
       clock_(Clock::GetRealTimeClock()),
       last_bwe_log_time_(0),
       total_requested_padding_bitrate_(0),
-      total_requested_min_bitrate_(0) {
+      total_requested_min_bitrate_(0),
+      bitrate_allocation_strategy_(nullptr) {
   sequenced_checker_.Detach();
 }
 
@@ -86,36 +88,36 @@ void BitrateAllocator::OnNetworkChanged(uint32_t target_bitrate_bps,
 
   ObserverAllocation allocation = AllocateBitrates(target_bitrate_bps);
 
-  for (auto& config : bitrate_observer_configs_) {
-    uint32_t allocated_bitrate = allocation[config.observer];
-    uint32_t protection_bitrate = config.observer->OnBitrateUpdated(
-        allocated_bitrate, last_fraction_loss_, last_rtt_,
-        last_bwe_period_ms_);
+  for (const auto& track_config : bitrate_observer_configs_) {
+    ObserverConfig* config = static_cast<ObserverConfig*>(track_config.get());

[nisse-webrtc, 2017/09/05 11:03:46]

It not so nice to have these static casts everywhere bitrate_observer_configs_
is accessed. If it is safe, then you should be able to change the type of
|bitrate_observer_configs_| to a std::vector<std::unique_ptr<ObserverConfig>>.

Hmm, or would you need to convert it to a
std::vector<std::unique_ptr<TrackConfig>> elsewhere? If there's a problem with
that, I think we should consult one one of our C++ experts to see if we can find
a reasonable way to avoid the type casts.

[alexnarest, 2017/09/05 13:37:45]

TrackConfig is visible at BitrateAllocationStrategy and does not have observer
field. ObserverConfig inherits TrackConfig to reuse common fields and add
observer. The collection bitrate_observer_configs_ is passed to
BitrateAllocationStrategy as it is. This is the reason the collection is
TrackConfig based.
I'm not aware of a way to downcast without a cast.

We could hide cast into some utility class that would implement the conversion
but it still would cast. I do not think this would be nice to hide the cast.

[kwiberg-webrtc, 2017/09/06 08:49:46]

+1 to what nisse@ said about downcasting. It's really nice to not have to do it.
Also, having a unique_ptr<BitrateAllocationStrategy::TrackConfig> owning an
ObserverConfig is not safe, since BitrateAllocationStrategy::TrackConfig's
destructor is not virtual; the unique_ptr will destroy just the base class, not
the derived class. Currently, it happens to be the case that the fields added by
ObserverConfig are all trivially destructible so that it works anyway, but as
soon as a non-trivially destructible field is added, its destructor will
silently not get called.

I would propose that bitrate_observer_configs_ should be e.g. an
std::vector<ObserverConfig>; you can then *up*cast pointers to its elements to
BitrateAllocationStrategy::TrackConfig* when you need to.

[nisse-webrtc, 2017/09/06 09:04:02]

And we'd then need to change the signature of the AllocateBitrates method. I see
two reasonable options:

1. Take an ArrayView<const TrackConfig*>. Has the drawback that we'd 
   need to construct an array or vector of pointers, either for each
   call, or cached in some way. But at least we wouldn't need to copy
   the objects.

2. A pointer to an interface a method GetTrackConfig(int index), 
   which could be implemented by returning a pointer directly into
   the std::Vector<ObserverConfig>.

There's also a third alternative: keep a std::vector<TrackConfig>, pass a
reference to it as an ArrayView<TrackConfig>, and use a *separate* vector for
storing the additional per-track information. 

[alexnarest, 2017/09/08 17:09:23]

I'm not fully agree with the tradeoff between performance (creation of the array
of pointers on every call) and code beauty (static_cast) but will accept your
suggestion because in my view the potential impact is small either way. 

+    uint32_t allocated_bitrate = allocation[config->observer];
+    uint32_t protection_bitrate = config->observer->OnBitrateUpdated(
+        allocated_bitrate, last_fraction_loss_, last_rtt_, last_bwe_period_ms_);
 
-    if (allocated_bitrate == 0 && config.allocated_bitrate_bps > 0) {
+    if (allocated_bitrate == 0 && config->allocated_bitrate_bps > 0) {
       if (target_bitrate_bps > 0)
         ++num_pause_events_;
       // The protection bitrate is an estimate based on the ratio between media
       // and protection used before this observer was muted.
       uint32_t predicted_protection_bps =
-          (1.0 - config.media_ratio) * config.min_bitrate_bps;
-      LOG(LS_INFO) << "Pausing observer " << config.observer
-                   << " with configured min bitrate " << config.min_bitrate_bps
+          (1.0 - config->media_ratio) * config->min_bitrate_bps;
+      LOG(LS_INFO) << "Pausing observer " << config->observer
+                   << " with configured min bitrate " << config->min_bitrate_bps
                    << " and current estimate of " << target_bitrate_bps
                    << " and protection bitrate " << predicted_protection_bps;
-    } else if (allocated_bitrate > 0 && config.allocated_bitrate_bps == 0) {
+    } else if (allocated_bitrate > 0 && config->allocated_bitrate_bps == 0) {
       if (target_bitrate_bps > 0)
         ++num_pause_events_;
-      LOG(LS_INFO) << "Resuming observer " << config.observer
-                   << ", configured min bitrate " << config.min_bitrate_bps
+      LOG(LS_INFO) << "Resuming observer " << config->observer
+                   << ", configured min bitrate " << config->min_bitrate_bps
                    << ", current allocation " << allocated_bitrate
                    << " and protection bitrate " << protection_bitrate;
     }
 
     // Only update the media ratio if the observer got an allocation.
     if (allocated_bitrate > 0)
-      config.media_ratio = MediaRatio(allocated_bitrate, protection_bitrate);
-    config.allocated_bitrate_bps = allocated_bitrate;
+      config->media_ratio = MediaRatio(allocated_bitrate, protection_bitrate);
+    config->allocated_bitrate_bps = allocated_bitrate;
   }
   UpdateAllocationLimits();
 }
@@ -131,28 +133,31 @@ void BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer,
 
   // Update settings if the observer already exists, create a new one otherwise.
   if (it != bitrate_observer_configs_.end()) {
-    it->min_bitrate_bps = min_bitrate_bps;
-    it->max_bitrate_bps = max_bitrate_bps;
-    it->pad_up_bitrate_bps = pad_up_bitrate_bps;
-    it->enforce_min_bitrate = enforce_min_bitrate;
+    ObserverConfig* observer_config = static_cast<ObserverConfig*>(it->get());
+    observer_config->min_bitrate_bps = min_bitrate_bps;
+    observer_config->max_bitrate_bps = max_bitrate_bps;
+    observer_config->pad_up_bitrate_bps = pad_up_bitrate_bps;
+    observer_config->enforce_min_bitrate = enforce_min_bitrate;
+    observer_config->track_id = track_id;
   } else {
-    bitrate_observer_configs_.push_back(
-        ObserverConfig(observer, min_bitrate_bps, max_bitrate_bps,
-                       pad_up_bitrate_bps, enforce_min_bitrate, track_id));
+    bitrate_observer_configs_.emplace_back(
+        new ObserverConfig(observer, min_bitrate_bps, max_bitrate_bps,
+                           pad_up_bitrate_bps, enforce_min_bitrate, track_id));
   }
 
   ObserverAllocation allocation;
   if (last_bitrate_bps_ > 0) {
     // Calculate a new allocation and update all observers.
     allocation = AllocateBitrates(last_bitrate_bps_);
-    for (auto& config : bitrate_observer_configs_) {
-      uint32_t allocated_bitrate = allocation[config.observer];
-      uint32_t protection_bitrate = config.observer->OnBitrateUpdated(
+    for (auto& track_config : bitrate_observer_configs_) {
+      ObserverConfig* config = static_cast<ObserverConfig*>(track_config.get());
+      uint32_t allocated_bitrate = allocation[config->observer];
+      uint32_t protection_bitrate = config->observer->OnBitrateUpdated(
           allocated_bitrate, last_fraction_loss_, last_rtt_,
           last_bwe_period_ms_);
-      config.allocated_bitrate_bps = allocated_bitrate;
+      config->allocated_bitrate_bps = allocated_bitrate;
       if (allocated_bitrate > 0)
-        config.media_ratio = MediaRatio(allocated_bitrate, protection_bitrate);
+        config->media_ratio = MediaRatio(allocated_bitrate, protection_bitrate);
     }
   } else {
     // Currently, an encoder is not allowed to produce frames.
@@ -170,13 +175,14 @@ void BitrateAllocator::UpdateAllocationLimits() {
   uint32_t total_requested_padding_bitrate = 0;
   uint32_t total_requested_min_bitrate = 0;
 
-  for (const auto& config : bitrate_observer_configs_) {
-    uint32_t stream_padding = config.pad_up_bitrate_bps;
-    if (config.enforce_min_bitrate) {
-      total_requested_min_bitrate += config.min_bitrate_bps;
-    } else if (config.allocated_bitrate_bps == 0) {
+  for (const auto& track_config : bitrate_observer_configs_) {
+    ObserverConfig* config = static_cast<ObserverConfig*>(track_config.get());
+    uint32_t stream_padding = config->pad_up_bitrate_bps;
+    if (config->enforce_min_bitrate) {
+      total_requested_min_bitrate += config->min_bitrate_bps;
+    } else if (config->allocated_bitrate_bps == 0) {
       stream_padding =
-          std::max(MinBitrateWithHysteresis(config), stream_padding);
+          std::max(MinBitrateWithHysteresis(*config), stream_padding);
     }
     total_requested_padding_bitrate += stream_padding;
   }
@@ -210,26 +216,35 @@ void BitrateAllocator::RemoveObserver(BitrateAllocatorObserver* observer) {
 int BitrateAllocator::GetStartBitrate(BitrateAllocatorObserver* observer) {
   RTC_DCHECK_CALLED_SEQUENTIALLY(&sequenced_checker_);
   const auto& it = FindObserverConfig(observer);
+  ObserverConfig* config = static_cast<ObserverConfig*>(it->get());
   if (it == bitrate_observer_configs_.end()) {
     // This observer hasn't been added yet, just give it its fair share.
     return last_non_zero_bitrate_bps_ /
            static_cast<int>((bitrate_observer_configs_.size() + 1));
-  } else if (it->allocated_bitrate_bps == -1) {
+  } else if (config->allocated_bitrate_bps == -1) {
     // This observer hasn't received an allocation yet, so do the same.
     return last_non_zero_bitrate_bps_ /
            static_cast<int>(bitrate_observer_configs_.size());
   } else {
     // This observer already has an allocation.
-    return it->allocated_bitrate_bps;
+    return config->allocated_bitrate_bps;
   }
 }
 
-BitrateAllocator::ObserverConfigs::iterator
+void BitrateAllocator::SetBitrateAllocationStrategy(
+    rtc::scoped_refptr<rtc::BitrateAllocationStrategy>
+        bitrate_allocation_strategy) {
+  RTC_DCHECK_CALLED_SEQUENTIALLY(&sequenced_checker_);
+  bitrate_allocation_strategy_ = bitrate_allocation_strategy;
+}
+
+rtc::BitrateAllocationStrategy::TrackConfigs::iterator
 BitrateAllocator::FindObserverConfig(const BitrateAllocatorObserver* observer) {
   RTC_DCHECK_CALLED_SEQUENTIALLY(&sequenced_checker_);
   for (auto it = bitrate_observer_configs_.begin();
        it != bitrate_observer_configs_.end(); ++it) {
-    if (it->observer == observer)
+    ObserverConfig* config = static_cast<ObserverConfig*>(it->get());
+    if (config->observer == observer)
       return it;
   }
   return bitrate_observer_configs_.end();
@@ -241,14 +256,30 @@ BitrateAllocator::ObserverAllocation BitrateAllocator::AllocateBitrates(
   if (bitrate_observer_configs_.empty())
     return ObserverAllocation();
 
+  if (bitrate_allocation_strategy_ != nullptr) {
+    rtc::BitrateAllocationStrategy::TrackAllocations track_allocations =
+        bitrate_allocation_strategy_->AllocateBitrates(
+            bitrate, bitrate_observer_configs_);

[nisse-webrtc, 2017/09/05 11:03:46]

You could consider using ArrayView as argument type for AllocateBitrates (see
rtc_base/array_view.h). Not sure whether or not that would help with the typing,
though.

[alexnarest, 2017/09/05 13:37:45]

ArrayView does not implement the needed conversion

+    // The strategy should return allocation for all tracks.
+    RTC_CHECK(track_allocations.size() == bitrate_observer_configs_.size());
+    ObserverAllocation allocation;
+    auto track_allocations_it = track_allocations.begin();
+    for (const auto& track_config : bitrate_observer_configs_) {
+      ObserverConfig* observer_config =
+          static_cast<ObserverConfig*>(track_config.get());
+      allocation[observer_config->observer] = *track_allocations_it++;

[stefan-webrtc, 2017/09/05 10:42:47]

Increment on a separate line for better readability.

[alexnarest, 2017/09/05 13:37:45]

git CL format does not like it on the separate line and LINT Is getting
confused, so I'll keep it as format likes it.

+    }
+    return allocation;
+  }
+
   if (bitrate == 0)
     return ZeroRateAllocation();
 
   uint32_t sum_min_bitrates = 0;
   uint32_t sum_max_bitrates = 0;
   for (const auto& observer_config : bitrate_observer_configs_) {
-    sum_min_bitrates += observer_config.min_bitrate_bps;
-    sum_max_bitrates += observer_config.max_bitrate_bps;
+    sum_min_bitrates += observer_config->min_bitrate_bps;
+    sum_max_bitrates += observer_config->max_bitrate_bps;
   }
 
   // Not enough for all observers to get an allocation, allocate according to:
@@ -268,8 +299,11 @@ BitrateAllocator::ObserverAllocation BitrateAllocator::AllocateBitrates(
 BitrateAllocator::ObserverAllocation BitrateAllocator::ZeroRateAllocation() {
   RTC_DCHECK_CALLED_SEQUENTIALLY(&sequenced_checker_);
   ObserverAllocation allocation;
-  for (const auto& observer_config : bitrate_observer_configs_)
-    allocation[observer_config.observer] = 0;
+  for (const auto& track_config : bitrate_observer_configs_) {
+    ObserverConfig* observer_config =
+        static_cast<ObserverConfig*>(track_config.get());
+    allocation[observer_config->observer] = 0;
+  }
   return allocation;
 }
 
@@ -280,25 +314,29 @@ BitrateAllocator::ObserverAllocation BitrateAllocator::LowRateAllocation(
   // Start by allocating bitrate to observers enforcing a min bitrate, hence
   // remaining_bitrate might turn negative.
   int64_t remaining_bitrate = bitrate;
-  for (const auto& observer_config : bitrate_observer_configs_) {
+  for (const auto& track_config : bitrate_observer_configs_) {
+    ObserverConfig* observer_config =
+        static_cast<ObserverConfig*>(track_config.get());
     int32_t allocated_bitrate = 0;
-    if (observer_config.enforce_min_bitrate)
-      allocated_bitrate = observer_config.min_bitrate_bps;
+    if (observer_config->enforce_min_bitrate)
+      allocated_bitrate = observer_config->min_bitrate_bps;
 
-    allocation[observer_config.observer] = allocated_bitrate;
+    allocation[observer_config->observer] = allocated_bitrate;
     remaining_bitrate -= allocated_bitrate;
   }
 
   // Allocate bitrate to all previously active streams.
   if (remaining_bitrate > 0) {
-    for (const auto& observer_config : bitrate_observer_configs_) {
-      if (observer_config.enforce_min_bitrate ||
-          LastAllocatedBitrate(observer_config) == 0)
+    for (const auto& track_config : bitrate_observer_configs_) {
+      ObserverConfig* observer_config =
+          static_cast<ObserverConfig*>(track_config.get());
+      if (observer_config->enforce_min_bitrate ||
+          LastAllocatedBitrate(*observer_config) == 0)
         continue;
 
-      uint32_t required_bitrate = MinBitrateWithHysteresis(observer_config);
+      uint32_t required_bitrate = MinBitrateWithHysteresis(*observer_config);
       if (remaining_bitrate >= required_bitrate) {
-        allocation[observer_config.observer] = required_bitrate;
+        allocation[observer_config->observer] = required_bitrate;
         remaining_bitrate -= required_bitrate;
       }
     }
@@ -306,14 +344,16 @@ BitrateAllocator::ObserverAllocation BitrateAllocator::LowRateAllocation(
 
   // Allocate bitrate to previously paused streams.
   if (remaining_bitrate > 0) {
-    for (const auto& observer_config : bitrate_observer_configs_) {
-      if (LastAllocatedBitrate(observer_config) != 0)
+    for (const auto& track_config : bitrate_observer_configs_) {
+      ObserverConfig* observer_config =
+          static_cast<ObserverConfig*>(track_config.get());
+      if (LastAllocatedBitrate(*observer_config) != 0)
         continue;
 
       // Add a hysteresis to avoid toggling.
-      uint32_t required_bitrate = MinBitrateWithHysteresis(observer_config);
+      uint32_t required_bitrate = MinBitrateWithHysteresis(*observer_config);
       if (remaining_bitrate >= required_bitrate) {
-        allocation[observer_config.observer] = required_bitrate;
+        allocation[observer_config->observer] = required_bitrate;
         remaining_bitrate -= required_bitrate;
       }
     }
@@ -332,8 +372,11 @@ BitrateAllocator::ObserverAllocation BitrateAllocator::NormalRateAllocation(
     uint32_t sum_min_bitrates) {
   RTC_DCHECK_CALLED_SEQUENTIALLY(&sequenced_checker_);
   ObserverAllocation allocation;
-  for (const auto& observer_config : bitrate_observer_configs_)
-    allocation[observer_config.observer] = observer_config.min_bitrate_bps;
+  for (const auto& track_config : bitrate_observer_configs_) {
+    ObserverConfig* observer_config =
+        static_cast<ObserverConfig*>(track_config.get());
+    allocation[observer_config->observer] = observer_config->min_bitrate_bps;
+  }
 
   bitrate -= sum_min_bitrates;
   if (bitrate > 0)
@@ -348,9 +391,11 @@ BitrateAllocator::ObserverAllocation BitrateAllocator::MaxRateAllocation(
   RTC_DCHECK_CALLED_SEQUENTIALLY(&sequenced_checker_);
   ObserverAllocation allocation;
 
-  for (const auto& observer_config : bitrate_observer_configs_) {
-    allocation[observer_config.observer] = observer_config.max_bitrate_bps;
-    bitrate -= observer_config.max_bitrate_bps;
+  for (const auto& track_config : bitrate_observer_configs_) {
+    ObserverConfig* observer_config =
+        static_cast<ObserverConfig*>(track_config.get());
+    allocation[observer_config->observer] = observer_config->max_bitrate_bps;
+    bitrate -= observer_config->max_bitrate_bps;
   }
   DistributeBitrateEvenly(bitrate, true, kTransmissionMaxBitrateMultiplier,
                           &allocation);
@@ -394,11 +439,13 @@ void BitrateAllocator::DistributeBitrateEvenly(uint32_t bitrate,
   RTC_DCHECK_EQ(allocation->size(), bitrate_observer_configs_.size());
 
   ObserverSortingMap list_max_bitrates;
-  for (const auto& observer_config : bitrate_observer_configs_) {
+  for (const auto& track_config : bitrate_observer_configs_) {
+    ObserverConfig* observer_config =
+        static_cast<ObserverConfig*>(track_config.get());
     if (include_zero_allocations ||
-        allocation->at(observer_config.observer) != 0) {
-      list_max_bitrates.insert(std::pair<uint32_t, const ObserverConfig*>(
-          observer_config.max_bitrate_bps, &observer_config));
+        allocation->at(observer_config->observer) != 0) {
+      list_max_bitrates.insert(std::pair<uint32_t, ObserverConfig*>(
+          observer_config->max_bitrate_bps, observer_config));
     }
   }
   auto it = list_max_bitrates.begin();
@@ -430,9 +477,11 @@ bool BitrateAllocator::EnoughBitrateForAllObservers(uint32_t bitrate,
   uint32_t extra_bitrate_per_observer =
       (bitrate - sum_min_bitrates) /
       static_cast<uint32_t>(bitrate_observer_configs_.size());
-  for (const auto& observer_config : bitrate_observer_configs_) {
-    if (observer_config.min_bitrate_bps + extra_bitrate_per_observer <
-        MinBitrateWithHysteresis(observer_config)) {
+  for (const auto& track_config : bitrate_observer_configs_) {
+    ObserverConfig* observer_config =
+        static_cast<ObserverConfig*>(track_config.get());
+    if (observer_config->min_bitrate_bps + extra_bitrate_per_observer <
+        MinBitrateWithHysteresis(*observer_config)) {
       return false;
     }
   }