Revert "Ensure that we always set values for min and max audio bitrate."
This reverts commit e47aee3b864fe5a4f964d405a7f6f3ac8c49f174.
Reason for revert: Breaks downstream project
Original change's description:
> Ensure that we always set values for min and max audio bitrate.
>
> Use (in order from lowest to highest precedence):
> -- fixed 32000bps
> -- fixed target bitrate from codec
> -- explicit values from the rtp encoding parameters
> -- Final precedence is given to field trial values from
> WebRTC-Audio-Allocation
>
> Bug: webrtc:10487
> Change-Id: I7e289f209a927785572058b6fbfdf60fa14edf05
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/126229
> Reviewed-by: Minyue Li <minyue@google.com>
> Reviewed-by: Stefan Holmer <stefan@webrtc.org>
> Reviewed-by: Sebastian Jansson <srte@webrtc.org>
> Commit-Queue: Daniel Lee <dklee@google.com>
> Cr-Commit-Position: refs/heads/master@{#27667}
TBR=solenberg@webrtc.org,stefan@webrtc.org,srte@webrtc.org,crodbro@webrtc.org,minyue@webrtc.org,minyue@google.com,dklee@google.com
Change-Id: Ie975cf40e65105d1e4cfab417b220b6bfc34592b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:10487
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/133481
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Commit-Queue: Daniel Lee <dklee@google.com>
Cr-Commit-Position: refs/heads/master@{#27670}diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index c8730d7..283fd9a 100644
--- a/audio/audio_send_stream.cc
+++ b/audio/audio_send_stream.cc
@@ -446,12 +446,18 @@
}
uint32_t AudioSendStream::OnBitrateUpdated(BitrateAllocationUpdate update) {
- // Pick a target bitrate between the constraints. Overrules the allocator if
- // it 1) allocated a bitrate of zero to disable the stream or 2) allocated a
- // higher than max to allow for e.g. extra FEC. For now we ignore these the
- // exceptions.
- auto constraints = GetMinMaxBitrateConstraints();
- update.target_bitrate.Clamp(constraints.min, constraints.max);
+ // A send stream may be allocated a bitrate of zero if the allocator decides
+ // to disable it. For now we ignore this decision and keep sending on min
+ // bitrate.
+ if (update.target_bitrate.IsZero()) {
+ update.target_bitrate = DataRate::bps(config_.min_bitrate_bps);
+ }
+ RTC_DCHECK_GE(update.target_bitrate.bps<int>(), config_.min_bitrate_bps);
+ // The bitrate allocator might allocate an higher than max configured bitrate
+ // if there is room, to allow for, as example, extra FEC. Ignore that for now.
+ const DataRate max_bitrate = DataRate::bps(config_.max_bitrate_bps);
+ if (update.target_bitrate > max_bitrate)
+ update.target_bitrate = max_bitrate;
channel_send_->OnBitrateAllocation(update);
@@ -793,14 +799,12 @@
void AudioSendStream::ConfigureBitrateObserver() {
// This either updates the current observer or adds a new observer.
// TODO(srte): Add overhead compensation here.
- auto constraints = GetMinMaxBitrateConstraints();
-
bitrate_allocator_->AddObserver(
- this,
- MediaStreamAllocationConfig{
- constraints.min.bps<uint32_t>(), constraints.max.bps<uint32_t>(), 0,
- allocation_settings_.DefaultPriorityBitrate().bps(), true,
- config_.track_id, config_.bitrate_priority});
+ this, MediaStreamAllocationConfig{
+ static_cast<uint32_t>(config_.min_bitrate_bps),
+ static_cast<uint32_t>(config_.max_bitrate_bps), 0,
+ allocation_settings_.DefaultPriorityBitrate().bps(), true,
+ config_.track_id, config_.bitrate_priority});
}
void AudioSendStream::RemoveBitrateObserver() {
@@ -815,36 +819,6 @@
thread_sync_event.Wait(rtc::Event::kForever);
}
-AudioSendStream::TargetAudioBitrateConstraints
-AudioSendStream::GetMinMaxBitrateConstraints() const {
- TargetAudioBitrateConstraints constraints{
- DataRate::bps(config_.min_bitrate_bps),
- DataRate::bps(config_.max_bitrate_bps)};
-
- // If bitrates were explicitly overriden via field trial, use those values.
- if (allocation_settings_.MinBitrate())
- constraints.min = *allocation_settings_.MinBitrate();
- if (allocation_settings_.MaxBitrate())
- constraints.max = *allocation_settings_.MaxBitrate();
-
- RTC_DCHECK_GE(constraints.min.bps(), 0);
- RTC_DCHECK_GE(constraints.max.bps(), 0);
- RTC_DCHECK_GE(constraints.max.bps(), constraints.min.bps());
-
- // TODO(srte,dklee): Replace these with proper overhead calculations.
- if (allocation_settings_.IncludeOverheadInAudioAllocation()) {
- // OverheadPerPacket = Ipv4(20B) + UDP(8B) + SRTP(10B) + RTP(12)
- const DataSize kOverheadPerPacket = DataSize::bytes(20 + 8 + 10 + 12);
- const TimeDelta kMaxFrameLength = TimeDelta::ms(60); // Based on Opus spec
- const DataRate kMinOverhead = kOverheadPerPacket / kMaxFrameLength;
- constraints.min += kMinOverhead;
- // TODO(dklee): This is obviously overly conservative to avoid exceeding max
- // bitrate. Carefully reconsider the logic when addressing todo above.
- constraints.max += kMinOverhead;
- }
- return constraints;
-}
-
void AudioSendStream::RegisterCngPayloadType(int payload_type,
int clockrate_hz) {
channel_send_->RegisterCngPayloadType(payload_type, clockrate_hz);
diff --git a/audio/audio_send_stream.h b/audio/audio_send_stream.h
index 9796e80..8bee352 100644
--- a/audio/audio_send_stream.h
+++ b/audio/audio_send_stream.h
@@ -104,11 +104,6 @@
private:
class TimedTransport;
- // Constraints including overhead.
- struct TargetAudioBitrateConstraints {
- DataRate min;
- DataRate max;
- };
internal::AudioState* audio_state();
const internal::AudioState* audio_state() const;
@@ -131,10 +126,6 @@
void ConfigureBitrateObserver() RTC_RUN_ON(worker_queue_);
void RemoveBitrateObserver();
- // Returns bitrate constraints, maybe including overhead when enabled by
- // field trial.
- TargetAudioBitrateConstraints GetMinMaxBitrateConstraints() const;
-
// Sets per-packet overhead on encoded (for ANA) based on current known values
// of transport and packetization overheads.
void UpdateOverheadForEncoder()
diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc
index 61acc96..701df1c 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -73,13 +73,6 @@
{kOpusFormat, {48000, 1, 32000, 6000, 510000}},
{kG722Format, {16000, 1, 64000}}};
-// TODO(dklee): This mirrors calculation in audio_send_stream.cc, which
-// should be made more precise in the future. This can be changed when that
-// logic is more accurate.
-const DataSize kOverheadPerPacket = DataSize::bytes(20 + 8 + 10 + 12);
-const TimeDelta kMaxFrameLength = TimeDelta::ms(60);
-const DataRate kOverheadRate = kOverheadPerPacket / kMaxFrameLength;
-
class MockLimitObserver : public BitrateAllocator::LimitObserver {
public:
MOCK_METHOD3(OnAllocationLimitsChanged,
@@ -490,97 +483,6 @@
send_stream->OnBitrateUpdated(update);
}
-TEST(AudioSendStreamTest, SSBweTargetInRangeRespected) {
- ScopedFieldTrials field_trials("WebRTC-Audio-SendSideBwe/Enabled/");
- ConfigHelper helper(true, true);
- auto send_stream = helper.CreateAudioSendStream();
- EXPECT_CALL(*helper.channel_send(),
- OnBitrateAllocation(Field(
- &BitrateAllocationUpdate::target_bitrate,
- Eq(DataRate::bps(helper.config().max_bitrate_bps - 5000)))));
- BitrateAllocationUpdate update;
- update.target_bitrate = DataRate::bps(helper.config().max_bitrate_bps - 5000);
- send_stream->OnBitrateUpdated(update);
-}
-
-TEST(AudioSendStreamTest, SSBweFieldTrialMinRespected) {
- ScopedFieldTrials field_trials(
- "WebRTC-Audio-SendSideBwe/Enabled/"
- "WebRTC-Audio-Allocation/min:6kbps,max:64kbps/");
- ConfigHelper helper(true, true);
- auto send_stream = helper.CreateAudioSendStream();
- EXPECT_CALL(
- *helper.channel_send(),
- OnBitrateAllocation(Field(&BitrateAllocationUpdate::target_bitrate,
- Eq(DataRate::kbps(6)))));
- BitrateAllocationUpdate update;
- update.target_bitrate = DataRate::kbps(1);
- send_stream->OnBitrateUpdated(update);
-}
-
-TEST(AudioSendStreamTest, SSBweFieldTrialMaxRespected) {
- ScopedFieldTrials field_trials(
- "WebRTC-Audio-SendSideBwe/Enabled/"
- "WebRTC-Audio-Allocation/min:6kbps,max:64kbps/");
- ConfigHelper helper(true, true);
- auto send_stream = helper.CreateAudioSendStream();
- EXPECT_CALL(
- *helper.channel_send(),
- OnBitrateAllocation(Field(&BitrateAllocationUpdate::target_bitrate,
- Eq(DataRate::kbps(64)))));
- BitrateAllocationUpdate update;
- update.target_bitrate = DataRate::kbps(128);
- send_stream->OnBitrateUpdated(update);
-}
-
-TEST(AudioSendStreamTest, SSBweWithOverhead) {
- ScopedFieldTrials field_trials(
- "WebRTC-Audio-SendSideBwe/Enabled/"
- "WebRTC-SendSideBwe-WithOverhead/Enabled/");
- ConfigHelper helper(true, true);
- auto send_stream = helper.CreateAudioSendStream();
- const DataRate bitrate =
- DataRate::bps(helper.config().max_bitrate_bps) + kOverheadRate;
- EXPECT_CALL(*helper.channel_send(),
- OnBitrateAllocation(Field(
- &BitrateAllocationUpdate::target_bitrate, Eq(bitrate))));
- BitrateAllocationUpdate update;
- update.target_bitrate = bitrate;
- send_stream->OnBitrateUpdated(update);
-}
-
-TEST(AudioSendStreamTest, SSBweWithOverheadMinRespected) {
- ScopedFieldTrials field_trials(
- "WebRTC-Audio-SendSideBwe/Enabled/"
- "WebRTC-SendSideBwe-WithOverhead/Enabled/"
- "WebRTC-Audio-Allocation/min:6kbps,max:64kbps/");
- ConfigHelper helper(true, true);
- auto send_stream = helper.CreateAudioSendStream();
- const DataRate bitrate = DataRate::kbps(6) + kOverheadRate;
- EXPECT_CALL(*helper.channel_send(),
- OnBitrateAllocation(Field(
- &BitrateAllocationUpdate::target_bitrate, Eq(bitrate))));
- BitrateAllocationUpdate update;
- update.target_bitrate = DataRate::kbps(1);
- send_stream->OnBitrateUpdated(update);
-}
-
-TEST(AudioSendStreamTest, SSBweWithOverheadMaxRespected) {
- ScopedFieldTrials field_trials(
- "WebRTC-Audio-SendSideBwe/Enabled/"
- "WebRTC-SendSideBwe-WithOverhead/Enabled/"
- "WebRTC-Audio-Allocation/min:6kbps,max:64kbps/");
- ConfigHelper helper(true, true);
- auto send_stream = helper.CreateAudioSendStream();
- const DataRate bitrate = DataRate::kbps(64) + kOverheadRate;
- EXPECT_CALL(*helper.channel_send(),
- OnBitrateAllocation(Field(
- &BitrateAllocationUpdate::target_bitrate, Eq(bitrate))));
- BitrateAllocationUpdate update;
- update.target_bitrate = DataRate::kbps(128);
- send_stream->OnBitrateUpdated(update);
-}
-
TEST(AudioSendStreamTest, ProbingIntervalOnBitrateUpdated) {
ConfigHelper helper(false, true);
auto send_stream = helper.CreateAudioSendStream();
diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc
index b0c5b7b..9110d55 100644
--- a/media/engine/webrtc_voice_engine.cc
+++ b/media/engine/webrtc_voice_engine.cc
@@ -977,27 +977,10 @@
config_.send_codec_spec &&
absl::EqualsIgnoreCase(config_.send_codec_spec->format.name,
kOpusCodecName);
- if (is_opus) {
- // The order of precedence (from lowest to highest is)
- // - a reasonable default of 32kbps min/max
- // - fixed target bitrate from codec spec
- // - bitrate configured in the rtp_parameter encodings settings
- const int kDefaultBitrateBps = 32000;
- config_.min_bitrate_bps = kDefaultBitrateBps;
- config_.max_bitrate_bps = kDefaultBitrateBps;
-
- if (config_.send_codec_spec &&
- config_.send_codec_spec->target_bitrate_bps) {
- config_.min_bitrate_bps = *config_.send_codec_spec->target_bitrate_bps;
- config_.max_bitrate_bps = *config_.send_codec_spec->target_bitrate_bps;
- }
-
- if (rtp_parameters_.encodings[0].min_bitrate_bps) {
- config_.min_bitrate_bps = *rtp_parameters_.encodings[0].min_bitrate_bps;
- }
- if (rtp_parameters_.encodings[0].max_bitrate_bps) {
- config_.max_bitrate_bps = *rtp_parameters_.encodings[0].max_bitrate_bps;
- }
+ if (is_opus && allocation_settings_.ConfigureRateAllocationRange()) {
+ config_.min_bitrate_bps = allocation_settings_.MinBitrateBps();
+ config_.max_bitrate_bps = allocation_settings_.MaxBitrateBps(
+ rtp_parameters_.encodings[0].max_bitrate_bps);
}
}
diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc
index a76186d..4ddcd43 100644
--- a/media/engine/webrtc_voice_engine_unittest.cc
+++ b/media/engine/webrtc_voice_engine_unittest.cc
@@ -2405,11 +2405,72 @@
public:
WebRtcVoiceEngineWithSendSideBweWithOverheadTest()
: WebRtcVoiceEngineTestFake(
- "WebRTC-Audio-SendSideBwe/Enabled/WebRTC-Audio-Allocation/"
- "min:6000bps,max:32000bps/WebRTC-SendSideBwe-WithOverhead/"
+ "WebRTC-Audio-SendSideBwe/Enabled/WebRTC-SendSideBwe-WithOverhead/"
"Enabled/") {}
};
+TEST_F(WebRtcVoiceEngineWithSendSideBweWithOverheadTest, MinAndMaxBitrate) {
+ EXPECT_TRUE(SetupSendStream());
+ cricket::AudioSendParameters parameters;
+ parameters.codecs.push_back(kOpusCodec);
+ SetSendParameters(parameters);
+ const int initial_num = call_.GetNumCreatedSendStreams();
+ EXPECT_EQ(initial_num, call_.GetNumCreatedSendStreams());
+
+ // OverheadPerPacket = Ipv4(20B) + UDP(8B) + SRTP(10B) + RTP(12)
+ constexpr int kOverheadPerPacket = 20 + 8 + 10 + 12;
+ constexpr int kOpusMaxPtimeMs = WEBRTC_OPUS_SUPPORT_120MS_PTIME ? 120 : 60;
+ constexpr int kMinOverheadBps =
+ kOverheadPerPacket * 8 * 1000 / kOpusMaxPtimeMs;
+
+ constexpr int kOpusMinBitrateBps = 6000;
+ EXPECT_EQ(kOpusMinBitrateBps + kMinOverheadBps,
+ GetSendStreamConfig(kSsrcX).min_bitrate_bps);
+ constexpr int kOpusBitrateFbBps = 32000;
+ EXPECT_EQ(kOpusBitrateFbBps + kMinOverheadBps,
+ GetSendStreamConfig(kSsrcX).max_bitrate_bps);
+
+ parameters.options.audio_network_adaptor = true;
+ parameters.options.audio_network_adaptor_config = {"1234"};
+ SetSendParameters(parameters);
+
+ constexpr int kMinOverheadWithAnaBps =
+ kOverheadPerPacket * 8 * 1000 / kOpusMaxPtimeMs;
+
+ EXPECT_EQ(kOpusMinBitrateBps + kMinOverheadWithAnaBps,
+ GetSendStreamConfig(kSsrcX).min_bitrate_bps);
+
+ EXPECT_EQ(kOpusBitrateFbBps + kMinOverheadWithAnaBps,
+ GetSendStreamConfig(kSsrcX).max_bitrate_bps);
+}
+
+// This test is similar to
+// WebRtcVoiceEngineTestFake.SetRtpSendParameterUpdatesMaxBitrate but with an
+// additional field trial.
+TEST_F(WebRtcVoiceEngineWithSendSideBweWithOverheadTest,
+ SetRtpSendParameterUpdatesMaxBitrate) {
+ EXPECT_TRUE(SetupSendStream());
+ cricket::AudioSendParameters send_parameters;
+ send_parameters.codecs.push_back(kOpusCodec);
+ SetSendParameters(send_parameters);
+
+ webrtc::RtpParameters rtp_parameters = channel_->GetRtpSendParameters(kSsrcX);
+ // Expect empty on parameters.encodings[0].max_bitrate_bps;
+ EXPECT_FALSE(rtp_parameters.encodings[0].max_bitrate_bps);
+
+ constexpr int kMaxBitrateBps = 6000;
+ rtp_parameters.encodings[0].max_bitrate_bps = kMaxBitrateBps;
+ EXPECT_TRUE(channel_->SetRtpSendParameters(kSsrcX, rtp_parameters).ok());
+
+ const int max_bitrate = GetSendStreamConfig(kSsrcX).max_bitrate_bps;
+#if WEBRTC_OPUS_SUPPORT_120MS_PTIME
+ constexpr int kMinOverhead = 3333;
+#else
+ constexpr int kMinOverhead = 6666;
+#endif
+ EXPECT_EQ(max_bitrate, kMaxBitrateBps + kMinOverhead);
+}
+
// Test that we can set the outgoing SSRC properly.
// SSRC is set in SetupSendStream() by calling AddSendStream.
TEST_F(WebRtcVoiceEngineTestFake, SetSendSsrc) {
diff --git a/rtc_base/experiments/audio_allocation_settings.cc b/rtc_base/experiments/audio_allocation_settings.cc
index 6579531..a601cce 100644
--- a/rtc_base/experiments/audio_allocation_settings.cc
+++ b/rtc_base/experiments/audio_allocation_settings.cc
@@ -12,6 +12,9 @@
namespace webrtc {
namespace {
+// For SendSideBwe, Opus bitrate should be in the range between 6000 and 32000.
+const int kOpusMinBitrateBps = 6000;
+const int kOpusBitrateFbBps = 32000;
// OverheadPerPacket = Ipv4(20B) + UDP(8B) + SRTP(10B) + RTP(12)
constexpr int kOverheadPerPacket = 20 + 8 + 10 + 12;
} // namespace
@@ -20,8 +23,8 @@
allocate_audio_without_feedback_("Enabled"),
force_no_audio_feedback_("Enabled"),
send_side_bwe_with_overhead_("Enabled"),
- min_bitrate_("min"),
- max_bitrate_("max"),
+ default_min_bitrate_("min", DataRate::bps(kOpusMinBitrateBps)),
+ default_max_bitrate_("max", DataRate::bps(kOpusBitrateFbBps)),
priority_bitrate_("prio", DataRate::Zero()) {
ParseFieldTrial({&audio_send_side_bwe_},
field_trial::FindFullName("WebRTC-Audio-SendSideBwe"));
@@ -32,8 +35,9 @@
ParseFieldTrial({&send_side_bwe_with_overhead_},
field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead"));
- ParseFieldTrial({&min_bitrate_, &max_bitrate_, &priority_bitrate_},
- field_trial::FindFullName("WebRTC-Audio-Allocation"));
+ ParseFieldTrial(
+ {&default_min_bitrate_, &default_max_bitrate_, &priority_bitrate_},
+ field_trial::FindFullName("WebRTC-Audio-Allocation"));
// TODO(mflodman): Keep testing this and set proper values.
// Note: This is an early experiment currently only supported by Opus.
@@ -96,15 +100,25 @@
return true;
}
-bool AudioAllocationSettings::IncludeOverheadInAudioAllocation() const {
- return send_side_bwe_with_overhead_;
+int AudioAllocationSettings::MinBitrateBps() const {
+ return default_min_bitrate_->bps() + min_overhead_bps_;
}
-absl::optional<DataRate> AudioAllocationSettings::MinBitrate() const {
- return min_bitrate_.GetOptional();
-}
-absl::optional<DataRate> AudioAllocationSettings::MaxBitrate() const {
- return max_bitrate_.GetOptional();
+int AudioAllocationSettings::MaxBitrateBps(
+ absl::optional<int> rtp_parameter_max_bitrate_bps) const {
+ // We assume that the max is a hard limit on the payload bitrate, so we add
+ // min_overhead_bps to it to ensure that, when overhead is deducted, the
+ // payload rate never goes beyond the limit. Note: this also means that if a
+ // higher overhead is forced, we cannot reach the limit.
+ // TODO(minyue): Reconsider this when the signaling to BWE is done
+ // through a dedicated API.
+
+ // This means that when RtpParameters is reset, we may change the
+ // encoder's bit rate immediately (through ReconfigureAudioSendStream()),
+ // meanwhile change the cap to the output of BWE.
+ if (rtp_parameter_max_bitrate_bps)
+ return *rtp_parameter_max_bitrate_bps + min_overhead_bps_;
+ return default_max_bitrate_->bps() + min_overhead_bps_;
}
DataRate AudioAllocationSettings::DefaultPriorityBitrate() const {
DataRate max_overhead = DataRate::Zero();
diff --git a/rtc_base/experiments/audio_allocation_settings.h b/rtc_base/experiments/audio_allocation_settings.h
index c099973..32e11df 100644
--- a/rtc_base/experiments/audio_allocation_settings.h
+++ b/rtc_base/experiments/audio_allocation_settings.h
@@ -60,13 +60,14 @@
int max_bitrate_bps,
bool has_dscp,
int transport_seq_num_extension_header_id) const;
- // Returns true if we should include packet overhead in audio allocation.
- bool IncludeOverheadInAudioAllocation() const;
- // Returns the min bitrate for audio rate allocation.
- absl::optional<DataRate> MinBitrate() const;
- // Returns the max bitrate for audio rate allocation.
- absl::optional<DataRate> MaxBitrate() const;
+ // Returns the min bitrate for audio rate allocation, potentially including
+ // overhead.
+ int MinBitrateBps() const;
+ // Returns the max bitrate for audio rate allocation, potentially including
+ // overhead. |rtp_parameter_max_bitrate_bps| max bitrate as configured in rtp
+ // parameters, excluding overhead.
+ int MaxBitrateBps(absl::optional<int> rtp_parameter_max_bitrate_bps) const;
// Indicates the default priority bitrate for audio streams. The bitrate
// allocator will prioritize audio until it reaches this bitrate and will
// divide bitrate evently between audio and video above this bitrate.
@@ -78,10 +79,10 @@
FieldTrialFlag force_no_audio_feedback_;
FieldTrialFlag send_side_bwe_with_overhead_;
int min_overhead_bps_ = 0;
- // Field Trial configured bitrates to use as overrides over default/user
- // configured bitrate range when audio bitrate allocation is enabled.
- FieldTrialOptional<DataRate> min_bitrate_;
- FieldTrialOptional<DataRate> max_bitrate_;
+ // Default bitrates to use as range if there's no user configured
+ // bitrate range but audio bitrate allocation is enabled.
+ FieldTrialParameter<DataRate> default_min_bitrate_;
+ FieldTrialParameter<DataRate> default_max_bitrate_;
FieldTrialParameter<DataRate> priority_bitrate_;
};
} // namespace webrtc
