From e47aee3b864fe5a4f964d405a7f6f3ac8c49f174 Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Wed, 17 Apr 2019 15:27:32 +0200 Subject: [PATCH] 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 Reviewed-by: Stefan Holmer Reviewed-by: Sebastian Jansson Commit-Queue: Daniel Lee Cr-Commit-Position: refs/heads/master@{#27667} --- audio/audio_send_stream.cc | 60 ++++++++---- audio/audio_send_stream.h | 9 ++ audio/audio_send_stream_unittest.cc | 98 +++++++++++++++++++ media/engine/webrtc_voice_engine.cc | 25 ++++- media/engine/webrtc_voice_engine_unittest.cc | 65 +----------- .../experiments/audio_allocation_settings.cc | 36 +++---- .../experiments/audio_allocation_settings.h | 21 ++-- 7 files changed, 194 insertions(+), 120 deletions(-) diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index 283fd9a603..c8730d7402 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -446,18 +446,12 @@ void AudioSendStream::DeliverRtcp(const uint8_t* packet, size_t length) { } uint32_t AudioSendStream::OnBitrateUpdated(BitrateAllocationUpdate update) { - // 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(), 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; + // 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); channel_send_->OnBitrateAllocation(update); @@ -799,12 +793,14 @@ void AudioSendStream::ReconfigureBitrateObserver( 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{ - static_cast(config_.min_bitrate_bps), - static_cast(config_.max_bitrate_bps), 0, - allocation_settings_.DefaultPriorityBitrate().bps(), true, - config_.track_id, config_.bitrate_priority}); + this, + MediaStreamAllocationConfig{ + constraints.min.bps(), constraints.max.bps(), 0, + allocation_settings_.DefaultPriorityBitrate().bps(), true, + config_.track_id, config_.bitrate_priority}); } void AudioSendStream::RemoveBitrateObserver() { @@ -819,6 +815,36 @@ void AudioSendStream::RemoveBitrateObserver() { 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 8bee3524d5..9796e80c90 100644 --- a/audio/audio_send_stream.h +++ b/audio/audio_send_stream.h @@ -104,6 +104,11 @@ class AudioSendStream final : public webrtc::AudioSendStream, private: class TimedTransport; + // Constraints including overhead. + struct TargetAudioBitrateConstraints { + DataRate min; + DataRate max; + }; internal::AudioState* audio_state(); const internal::AudioState* audio_state() const; @@ -126,6 +131,10 @@ class AudioSendStream final : public webrtc::AudioSendStream, 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 701df1c9ee..61acc96c16 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -73,6 +73,13 @@ const AudioCodecSpec kCodecSpecs[] = { {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, @@ -483,6 +490,97 @@ TEST(AudioSendStreamTest, DoesNotPassHigherBitrateThanMaxBitrate) { 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 9110d551e6..b0c5b7b3cf 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -977,10 +977,27 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream config_.send_codec_spec && absl::EqualsIgnoreCase(config_.send_codec_spec->format.name, kOpusCodecName); - 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); + 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; + } } } diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index 4ddcd43377..a76186d897 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -2405,72 +2405,11 @@ class WebRtcVoiceEngineWithSendSideBweWithOverheadTest public: WebRtcVoiceEngineWithSendSideBweWithOverheadTest() : WebRtcVoiceEngineTestFake( - "WebRTC-Audio-SendSideBwe/Enabled/WebRTC-SendSideBwe-WithOverhead/" + "WebRTC-Audio-SendSideBwe/Enabled/WebRTC-Audio-Allocation/" + "min:6000bps,max:32000bps/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 a601cce340..6579531da6 100644 --- a/rtc_base/experiments/audio_allocation_settings.cc +++ b/rtc_base/experiments/audio_allocation_settings.cc @@ -12,9 +12,6 @@ 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 @@ -23,8 +20,8 @@ AudioAllocationSettings::AudioAllocationSettings() allocate_audio_without_feedback_("Enabled"), force_no_audio_feedback_("Enabled"), send_side_bwe_with_overhead_("Enabled"), - default_min_bitrate_("min", DataRate::bps(kOpusMinBitrateBps)), - default_max_bitrate_("max", DataRate::bps(kOpusBitrateFbBps)), + min_bitrate_("min"), + max_bitrate_("max"), priority_bitrate_("prio", DataRate::Zero()) { ParseFieldTrial({&audio_send_side_bwe_}, field_trial::FindFullName("WebRTC-Audio-SendSideBwe")); @@ -35,9 +32,8 @@ AudioAllocationSettings::AudioAllocationSettings() ParseFieldTrial({&send_side_bwe_with_overhead_}, field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead")); - ParseFieldTrial( - {&default_min_bitrate_, &default_max_bitrate_, &priority_bitrate_}, - field_trial::FindFullName("WebRTC-Audio-Allocation")); + ParseFieldTrial({&min_bitrate_, &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. @@ -100,25 +96,15 @@ bool AudioAllocationSettings::IncludeAudioInAllocationOnReconfigure( return true; } -int AudioAllocationSettings::MinBitrateBps() const { - return default_min_bitrate_->bps() + min_overhead_bps_; +bool AudioAllocationSettings::IncludeOverheadInAudioAllocation() const { + return send_side_bwe_with_overhead_; } -int AudioAllocationSettings::MaxBitrateBps( - absl::optional 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_; +absl::optional AudioAllocationSettings::MinBitrate() const { + return min_bitrate_.GetOptional(); +} +absl::optional AudioAllocationSettings::MaxBitrate() const { + return max_bitrate_.GetOptional(); } 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 32e11df46e..c0999735ee 100644 --- a/rtc_base/experiments/audio_allocation_settings.h +++ b/rtc_base/experiments/audio_allocation_settings.h @@ -60,14 +60,13 @@ class AudioAllocationSettings { 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, 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 rtp_parameter_max_bitrate_bps) const; + // Returns the min bitrate for audio rate allocation. + absl::optional MinBitrate() const; + // Returns the max bitrate for audio rate allocation. + absl::optional MaxBitrate() 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. @@ -79,10 +78,10 @@ class AudioAllocationSettings { FieldTrialFlag force_no_audio_feedback_; FieldTrialFlag send_side_bwe_with_overhead_; int min_overhead_bps_ = 0; - // Default bitrates to use as range if there's no user configured - // bitrate range but audio bitrate allocation is enabled. - FieldTrialParameter default_min_bitrate_; - FieldTrialParameter default_max_bitrate_; + // Field Trial configured bitrates to use as overrides over default/user + // configured bitrate range when audio bitrate allocation is enabled. + FieldTrialOptional min_bitrate_; + FieldTrialOptional max_bitrate_; FieldTrialParameter priority_bitrate_; }; } // namespace webrtc