Fixes bug in AudioPriorityBitrateAllocationStrategy field trial.
Previously the rate limits weren't properly applied. This is fixed by
working on mutable copies of the TrackConfig.
Bug: webrtc:9718
Change-Id: I7438c59efa5d7e70fa3ce5e466e2c53a5a7ea9e2
Reviewed-on: https://webrtc-review.googlesource.com/c/107636
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25367}diff --git a/call/bitrate_allocator.cc b/call/bitrate_allocator.cc
index 64d1a486..c13b7dc 100644
--- a/call/bitrate_allocator.cc
+++ b/call/bitrate_allocator.cc
@@ -301,14 +301,14 @@
return ObserverAllocation();
if (bitrate_allocation_strategy_ != nullptr) {
- std::vector<const rtc::BitrateAllocationStrategy::TrackConfig*>
- track_configs(bitrate_observer_configs_.size());
- int i = 0;
- for (const auto& c : bitrate_observer_configs_) {
- track_configs[i++] = &c;
- }
+ // Note: This intentionally causes slicing, we only copy the fields in
+ // ObserverConfig that are inherited from TrackConfig.
+ std::vector<rtc::BitrateAllocationStrategy::TrackConfig> track_configs(
+ bitrate_observer_configs_.begin(), bitrate_observer_configs_.end());
+
std::vector<uint32_t> track_allocations =
- bitrate_allocation_strategy_->AllocateBitrates(bitrate, track_configs);
+ bitrate_allocation_strategy_->AllocateBitrates(
+ bitrate, std::move(track_configs));
// The strategy should return allocation for all tracks.
RTC_CHECK(track_allocations.size() == bitrate_observer_configs_.size());
ObserverAllocation allocation;
diff --git a/rtc_base/bitrateallocationstrategy.cc b/rtc_base/bitrateallocationstrategy.cc
index 3c41207..46e6674 100644
--- a/rtc_base/bitrateallocationstrategy.cc
+++ b/rtc_base/bitrateallocationstrategy.cc
@@ -43,31 +43,31 @@
const int kTransmissionMaxBitrateMultiplier = 2;
std::vector<uint32_t> BitrateAllocationStrategy::SetAllBitratesToMinimum(
- const ArrayView<const TrackConfig*> track_configs) {
+ const std::vector<BitrateAllocationStrategy::TrackConfig>& track_configs) {
std::vector<uint32_t> track_allocations;
- for (const auto* track_config : track_configs) {
- track_allocations.push_back(track_config->min_bitrate_bps);
+ for (const auto& track_config : track_configs) {
+ track_allocations.push_back(track_config.min_bitrate_bps);
}
return track_allocations;
}
std::vector<uint32_t> BitrateAllocationStrategy::DistributeBitratesEvenly(
- const ArrayView<const TrackConfig*> track_configs,
+ const std::vector<BitrateAllocationStrategy::TrackConfig>& track_configs,
uint32_t available_bitrate) {
std::vector<uint32_t> track_allocations =
SetAllBitratesToMinimum(track_configs);
uint32_t sum_min_bitrates = 0;
uint32_t sum_max_bitrates = 0;
- for (const auto* track_config : track_configs) {
- sum_min_bitrates += track_config->min_bitrate_bps;
- sum_max_bitrates += track_config->max_bitrate_bps;
+ for (const auto& track_config : track_configs) {
+ sum_min_bitrates += track_config.min_bitrate_bps;
+ sum_max_bitrates += track_config.max_bitrate_bps;
}
if (sum_min_bitrates >= available_bitrate) {
return track_allocations;
} else if (available_bitrate >= sum_max_bitrates) {
auto track_allocations_it = track_allocations.begin();
- for (const auto* track_config : track_configs) {
- *track_allocations_it++ = track_config->max_bitrate_bps;
+ for (const auto& track_config : track_configs) {
+ *track_allocations_it++ = track_config.max_bitrate_bps;
}
return track_allocations;
} else {
@@ -76,11 +76,10 @@
// lowest max_bitrate_bps. Remainder of available bitrate split evenly among
// remaining tracks.
std::multimap<uint32_t, size_t> max_bitrate_sorted_configs;
- for (const TrackConfig** track_configs_it = track_configs.begin();
- track_configs_it != track_configs.end(); ++track_configs_it) {
+ for (const auto& track_config : track_configs) {
max_bitrate_sorted_configs.insert(
- std::make_pair((*track_configs_it)->max_bitrate_bps,
- track_configs_it - track_configs.begin()));
+ std::make_pair(track_config.max_bitrate_bps,
+ &track_config - &track_configs.front()));
}
uint32_t total_available_increase = available_bitrate - sum_min_bitrates;
int processed_configs = 0;
@@ -89,8 +88,8 @@
total_available_increase /
(static_cast<uint32_t>(track_configs.size() - processed_configs));
uint32_t consumed_increase =
- std::min(track_configs[track_config_pair.second]->max_bitrate_bps -
- track_configs[track_config_pair.second]->min_bitrate_bps,
+ std::min(track_configs[track_config_pair.second].max_bitrate_bps -
+ track_configs[track_config_pair.second].min_bitrate_bps,
available_increase);
track_allocations[track_config_pair.second] += consumed_increase;
total_available_increase -= consumed_increase;
@@ -99,7 +98,6 @@
return track_allocations;
}
}
-
AudioPriorityBitrateAllocationStrategy::AudioPriorityBitrateAllocationStrategy(
std::string audio_track_id,
uint32_t sufficient_audio_bitrate)
@@ -112,45 +110,32 @@
std::vector<uint32_t> AudioPriorityBitrateAllocationStrategy::AllocateBitrates(
uint32_t available_bitrate,
- const ArrayView<const TrackConfig*> track_configs) {
- absl::optional<TrackConfig> audio_track_config;
+ std::vector<BitrateAllocationStrategy::TrackConfig> track_configs) {
+ TrackConfig* audio_track_config = nullptr;
size_t audio_config_index = 0;
uint32_t sum_min_bitrates = 0;
uint32_t sum_max_bitrates = 0;
- for (const auto*& track_config : track_configs) {
- if (track_config->track_id == audio_track_id_) {
+ for (auto& track_config : track_configs) {
+ if (track_config.track_id == audio_track_id_) {
audio_config_index = &track_config - &track_configs[0];
- audio_track_config = *track_config;
+ audio_track_config = &track_config;
if (config_.min_rate)
audio_track_config->min_bitrate_bps = config_.min_rate->bps();
if (config_.max_rate)
audio_track_config->max_bitrate_bps = config_.max_rate->bps();
- sum_min_bitrates += audio_track_config->min_bitrate_bps;
- sum_max_bitrates += audio_track_config->max_bitrate_bps;
- } else {
- sum_min_bitrates += track_config->min_bitrate_bps;
- sum_max_bitrates += track_config->max_bitrate_bps;
}
+ sum_min_bitrates += track_config.min_bitrate_bps;
+ sum_max_bitrates += track_config.max_bitrate_bps;
}
if (sum_max_bitrates < available_bitrate) {
// Allow non audio streams to go above max upto
// kTransmissionMaxBitrateMultiplier * max_bitrate_bps
- size_t track_configs_size = track_configs.size();
- std::vector<TrackConfig> increased_track_configs(track_configs_size);
- std::vector<const TrackConfig*> increased_track_configs_ptr(
- track_configs_size);
- for (unsigned long i = 0; i < track_configs_size; i++) {
- increased_track_configs[i] = (*track_configs[i]);
- increased_track_configs_ptr[i] = &increased_track_configs[i];
- if (track_configs[i]->track_id != audio_track_id_) {
- increased_track_configs[i].max_bitrate_bps =
- track_configs[i]->max_bitrate_bps *
- kTransmissionMaxBitrateMultiplier;
- }
+ for (auto& track_config : track_configs) {
+ if (&track_config != audio_track_config)
+ track_config.max_bitrate_bps *= kTransmissionMaxBitrateMultiplier;
}
- return DistributeBitratesEvenly(increased_track_configs_ptr,
- available_bitrate);
+ return DistributeBitratesEvenly(track_configs, available_bitrate);
}
if (!audio_track_config) {
return DistributeBitratesEvenly(track_configs, available_bitrate);
@@ -172,9 +157,7 @@
// Setting audio track minimum to safe_sufficient_audio_bitrate will
// allow using DistributeBitratesEvenly to allocate at least sufficient
// bitrate for audio and the rest evenly.
- TrackConfig sufficient_track_config(*track_configs[audio_config_index]);
- sufficient_track_config.min_bitrate_bps = safe_sufficient_audio_bitrate;
- track_configs[audio_config_index] = &sufficient_track_config;
+ audio_track_config->min_bitrate_bps = safe_sufficient_audio_bitrate;
std::vector<uint32_t> track_allocations =
DistributeBitratesEvenly(track_configs, available_bitrate);
return track_allocations;
diff --git a/rtc_base/bitrateallocationstrategy.h b/rtc_base/bitrateallocationstrategy.h
index e68ea74..13a4eee 100644
--- a/rtc_base/bitrateallocationstrategy.h
+++ b/rtc_base/bitrateallocationstrategy.h
@@ -57,10 +57,12 @@
std::string track_id;
};
+ // These are only used by AudioPriorityBitrateAllocationStrategy. They are
+ // exposed here to they can be unit tested.
static std::vector<uint32_t> SetAllBitratesToMinimum(
- const ArrayView<const TrackConfig*> track_configs);
+ const std::vector<BitrateAllocationStrategy::TrackConfig>& track_configs);
static std::vector<uint32_t> DistributeBitratesEvenly(
- const ArrayView<const TrackConfig*> track_configs,
+ const std::vector<BitrateAllocationStrategy::TrackConfig>& track_configs,
uint32_t available_bitrate);
// Strategy is expected to allocate all available_bitrate up to the sum of
@@ -75,7 +77,7 @@
// available_bitrate decrease.
virtual std::vector<uint32_t> AllocateBitrates(
uint32_t available_bitrate,
- const ArrayView<const TrackConfig*> track_configs) = 0;
+ std::vector<BitrateAllocationStrategy::TrackConfig> track_configs) = 0;
virtual ~BitrateAllocationStrategy() = default;
};
@@ -105,7 +107,8 @@
uint32_t sufficient_audio_bitrate);
std::vector<uint32_t> AllocateBitrates(
uint32_t available_bitrate,
- const ArrayView<const TrackConfig*> track_configs) override;
+ std::vector<BitrateAllocationStrategy::TrackConfig> track_configs)
+ override;
private:
webrtc::AudioPriorityConfig config_;
diff --git a/rtc_base/bitrateallocationstrategy_unittest.cc b/rtc_base/bitrateallocationstrategy_unittest.cc
index bfc41f5..f4c7ee7 100644
--- a/rtc_base/bitrateallocationstrategy_unittest.cc
+++ b/rtc_base/bitrateallocationstrategy_unittest.cc
@@ -43,11 +43,8 @@
BitrateAllocationStrategy::TrackConfig(min_other_bitrate,
max_other_bitrate, false, "")};
- std::vector<const rtc::BitrateAllocationStrategy::TrackConfig*>
- track_config_ptrs = MakeTrackConfigPtrsVector(track_configs);
-
std::vector<uint32_t> allocations =
- BitrateAllocationStrategy::SetAllBitratesToMinimum(track_config_ptrs);
+ BitrateAllocationStrategy::SetAllBitratesToMinimum(track_configs);
EXPECT_EQ(min_audio_bitrate, allocations[0]);
EXPECT_EQ(min_video_bitrate, allocations[1]);
EXPECT_EQ(min_other_bitrate, allocations[2]);
@@ -76,11 +73,8 @@
BitrateAllocationStrategy::TrackConfig(min_other_bitrate,
max_other_bitrate, false, "")};
- std::vector<const rtc::BitrateAllocationStrategy::TrackConfig*>
- track_config_ptrs = MakeTrackConfigPtrsVector(track_configs);
-
std::vector<uint32_t> allocations =
- BitrateAllocationStrategy::DistributeBitratesEvenly(track_config_ptrs,
+ BitrateAllocationStrategy::DistributeBitratesEvenly(track_configs,
available_bitrate);
EXPECT_EQ(min_audio_bitrate + even_bitrate_increase, allocations[0]);
EXPECT_EQ(min_video_bitrate + even_bitrate_increase, allocations[1]);
@@ -108,11 +102,7 @@
BitrateAllocationStrategy::TrackConfig(min_other_bitrate,
max_other_bitrate, false, "")};
- std::vector<const rtc::BitrateAllocationStrategy::TrackConfig*>
- track_config_ptrs = MakeTrackConfigPtrsVector(track_configs);
-
- return allocation_strategy.AllocateBitrates(available_bitrate,
- track_config_ptrs);
+ return allocation_strategy.AllocateBitrates(available_bitrate, track_configs);
}
// Test that when the available bitrate is less than the sum of the minimum
