BWE allocation strategy

webrtc/rtc_base/bitrateallocationstrategy.h

+/*

[Taylor Brandstetter, 2017/09/11 16:55:07]

Not sure if this has been discussed before, but does this belong in rtc_base/ or
call/?

[alexnarest, 2017/09/14 13:08:34]

I thought rtc_base/ is relevant because this provides both API and basic
implementation.

[Taylor Brandstetter, 2017/09/15 01:26:21]

Something I forgot from the round of comments I just sent: Since this is exposed
from PeerConnectionInterface, it should actually go in api/. Our end goal is
make api/ self-contained (not include anything from other directories, even
rtc_base).

[nisse-webrtc, 2017/09/15 07:05:44]

Agreed. When moving to api/, be extra careful about good naming. In particular,
|enforce_min_bitrate| is a bit confusing. Maybe something like |can_pause| (with
opposite meaning) would be clearer? If this affects code before this cl (I think
it does), it may make things easier to do it in a separate cl, to be landed
first.

If the other reviewrs agree, it's also ok with me to leave file in rtc_base in
this cl, and do a followup cl moving to api/ and tweaking the naming and other
api cleanup.

+ *  Copyright 2017 The WebRTC Project Authors. All rights reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_RTC_BASE_BITRATEALLOCATIONSTRATEGY_H_
+#define WEBRTC_RTC_BASE_BITRATEALLOCATIONSTRATEGY_H_
+
+#include <map>
+#include <memory>
+#include <string>
+#include <vector>
+#include "webrtc/rtc_base/array_view.h"
+#include "webrtc/rtc_base/checks.h"
+#include "webrtc/rtc_base/refcount.h"
+#include "webrtc/rtc_base/refcountedobject.h"
+
+namespace rtc {
+
+// Plugable strategy allows configuration of bitrate allocation per media track.

[Taylor Brandstetter, 2017/09/11 16:55:07]

Would be nice to have a more detailed explanation here.

[alexnarest, 2017/09/14 13:08:33]

Done.

+class BitrateAllocationStrategy {
+ public:

[Taylor Brandstetter, 2017/09/11 16:55:07]

Can you document the arguments to TrackConfig's constructor? I assume
"enforce_min_bitrate" means "must allocate at least min_bitrate_bps", and
"track_id" is a MediaStreamTrack ID, but it would be help if this were
clarified.

[alexnarest, 2017/09/14 13:08:34]

Done.

+  struct TrackConfig {
+    TrackConfig(uint32_t min_bitrate_bps,
+                uint32_t max_bitrate_bps,
+                bool enforce_min_bitrate,
+                std::string track_id)
+        : min_bitrate_bps(min_bitrate_bps),
+          max_bitrate_bps(max_bitrate_bps),
+          enforce_min_bitrate(enforce_min_bitrate),
+          track_id(track_id) {}
+    TrackConfig(const TrackConfig& track_config) = default;
+    virtual ~TrackConfig() = default;
+    TrackConfig() {}
+
+    uint32_t min_bitrate_bps;
+    uint32_t max_bitrate_bps;
+    bool enforce_min_bitrate;
+    std::string track_id;
+  };
+
+  std::vector<uint32_t> SetAllBitratesToMinimum(
+      const ArrayView<const TrackConfig*> track_configs);
+  std::vector<uint32_t> DistributeBitratesEvenly(
+      const ArrayView<const TrackConfig*> track_configs,
+      uint32_t available_bitrate);

[Taylor Brandstetter, 2017/09/11 16:55:07]

If these methods aren't virtual, do they need to be here, or could they be
implemented in Call internally? Or, if you want to re-use them elsewhere, they
could just be BitrateAllocationStrategy subclasses. 

[alexnarest, 2017/09/14 13:08:34]

These are utility methods. Future strategies will use them similarly to
AudioPriorityBitrateAllocationStrategy. 

[Taylor Brandstetter, 2017/09/15 01:24:41]

I understand. I just think it would be cleaner if they were standalone
functions, or subclasses of BitrateAllocationStrategy, so that
BitrateAllocationStrategy can be a pure virtual interface. I don't see the
reason for them being member functions.

[nisse-webrtc, 2017/09/15 07:05:44]

Are they intended as internal utility functions, or do we really want to
advertise them to applications?

I think it would be more appropriate with functions or static methods (a static
method is just a function which is grouped with the class for namespace
purposes). 

+
+  virtual std::vector<uint32_t> AllocateBitrates(

[Taylor Brandstetter, 2017/09/11 16:55:06]

Some comments around this would be helpful. Some questions that come to mind:
What happens if a subclass allocates more than available_bitrate? Or less than
it? Or violates the minimum, when enforce_min_bitrate is true?

[alexnarest, 2017/09/14 13:08:33]

Done.

+      uint32_t available_bitrate,
+      const ArrayView<const TrackConfig*> track_configs) = 0;
+
+  virtual ~BitrateAllocationStrategy() = default;
+};
+
+// Simple allocation strategy giving priority to audio at low bitrates.

[Taylor Brandstetter, 2017/09/11 16:55:06]

Can this comment elaborate a bit more? I'd summarize it as "prioritize audio
bitrate 100% until sufficient_audio_bitrate is reached, then follow normal
bitrate allocation algorithm".

[alexnarest, 2017/09/14 13:08:34]

Done.

+class AudioPriorityBitrateAllocationStrategy
+    : public BitrateAllocationStrategy {
+ public:
+  AudioPriorityBitrateAllocationStrategy(std::string audio_track_id,
+                                         uint32_t sufficient_audio_bitrate);
+  std::vector<uint32_t> AllocateBitrates(
+      uint32_t available_bitrate,
+      const ArrayView<const TrackConfig*> track_configs) override;
+
+ private:
+  std::string audio_track_id_;
+  uint32_t sufficient_audio_bitrate_;
+};
+}  // namespace rtc
+
+#endif  // WEBRTC_RTC_BASE_BITRATEALLOCATIONSTRATEGY_H_