Injectable audio encoders: WebRtcVoiceEngine and company

webrtc/audio/audio_send_stream.cc

Index: webrtc/audio/audio_send_stream.cc
diff --git a/webrtc/audio/audio_send_stream.cc b/webrtc/audio/audio_send_stream.cc
index 438d1cc78a5aca5d7657b6368bfbac03fa5aed8e..118bd0794fdb1b478f6e4e29c2321b320131985a 100644
--- a/webrtc/audio/audio_send_stream.cc
+++ b/webrtc/audio/audio_send_stream.cc
@@ -11,14 +11,18 @@
 #include "webrtc/audio/audio_send_stream.h"
 
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "webrtc/audio/audio_state.h"
 #include "webrtc/audio/conversion.h"
 #include "webrtc/audio/scoped_voe_interface.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/event.h"
+#include "webrtc/base/function_view.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/task_queue.h"
+#include "webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h"
 #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
 #include "webrtc/modules/congestion_controller/include/congestion_controller.h"
 #include "webrtc/modules/pacing/paced_sender.h"
@@ -30,16 +34,19 @@
 
 namespace webrtc {
 
-namespace {
-
-constexpr char kOpusCodecName[] = "opus";
+namespace internal {
 
-bool IsCodec(const webrtc::CodecInst& codec, const char* ref_name) {
-  return (STR_CASE_CMP(codec.plname, ref_name) == 0);
+namespace {
+void CallEncoder(const std::unique_ptr<voe::ChannelProxy>& channel_proxy,
+                 rtc::FunctionView<void(AudioEncoder*)> lambda) {
+  channel_proxy->ModifyEncoder(
+      [&lambda](std::unique_ptr<AudioEncoder>* encoder_ptr) {

[kwiberg-webrtc, 2017/04/06 10:13:29]

Or just [&]. That way, you make it very plain that the lambda is run
immediately, since capturing everything is very forbidden if the lambda outlives
the current scope.

[ossu, 2017/04/06 11:14:24]

I'm all for [&] capture. In this case, capturing the lambda by reference is just
as forbidden if it outlives the current scope. I'll shorten it.

+        RTC_DCHECK(encoder_ptr);
+        lambda(encoder_ptr->get());
+      });
+}
 }

[the sun, 2017/04/04 23:02:54]

nit: // namespace

[kwiberg-webrtc, 2017/04/06 10:13:29]

clang-format will fix this for you nowadays...

[ossu, 2017/04/06 10:15:21]

Acknowledged.

-}  // namespace
 
-namespace internal {
 AudioSendStream::AudioSendStream(
     const webrtc::AudioSendStream::Config& config,
     const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
@@ -52,6 +59,8 @@ AudioSendStream::AudioSendStream(
     : worker_queue_(worker_queue),
       config_(config),
       audio_state_(audio_state),
+      event_log_(event_log),
+      packet_router_(packet_router),
       bitrate_allocator_(bitrate_allocator),
       congestion_controller_(congestion_controller) {
   LOG(LS_INFO) << "AudioSendStream: " << config_.ToString();
@@ -61,7 +70,7 @@ AudioSendStream::AudioSendStream(
 
   VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine());

[the sun, 2017/04/04 23:02:53]

Can you call Reconfigure() from here (or the some internal non-virtual helper
which both calls) to avoid code duplication?

[ossu, 2017/04/06 10:15:21]

I considered it but I don't think I'll be able to make it more readable while
trying to avoid duplication. For proper operation, I'd need to make config_
optional, so that Reconfigure could configure from nothing, as well as from some
other state. I'm not sure it's worth it considering most of this is a stop-gap
before we can stop using Channel altogether.

If you think it's important, I'll take another look at it.

   channel_proxy_ = voe_impl->GetChannelProxy(config_.voe_channel_id);
-  channel_proxy_->SetRtcEventLog(event_log);
+  channel_proxy_->SetRtcEventLog(event_log_);
   channel_proxy_->SetRtcpRttStats(rtcp_rtt_stats);
   channel_proxy_->SetRTCPStatus(true);
   channel_proxy_->SetLocalSSRC(config.rtp.ssrc);
@@ -88,7 +97,7 @@ AudioSendStream::AudioSendStream(
   channel_proxy_->RegisterSenderCongestionControlObjects(
       congestion_controller->pacer(), congestion_controller, packet_router,
       bandwidth_observer_.get());
-  if (!SetupSendCodec()) {
+  if (config_.send_codec_spec && !SetupSendCodec(config_)) {
     LOG(LS_ERROR) << "Failed to set up send codec state.";
   }
 }
@@ -102,17 +111,81 @@ AudioSendStream::~AudioSendStream() {
   channel_proxy_->SetRtcpRttStats(nullptr);
 }
 
+void AudioSendStream::Reconfigure(

[the sun, 2017/04/04 23:02:53]

Many of the lazy-updated attributes here amount to very little work inside
voe::Channel:
SetRTCP_CNAME() - a string copy
DeRegisterExternalTransport() - a memory write
RegisterExternalTransport(new_config.send_transport) - a memory write
SetSendAudioLevelIndicationStatus() - 4 memory writes to arrays
...

I think we should make this code as simple as possible now and optimize later,
and where the optimizations should really be done - in voe::Channel and the
RtpRtcp module.

[ossu, 2017/04/06 10:15:21]

Very little work, and locks.

Do you want me to remove the checks for change before the calls? What would that
accomplish, other than us taking a handful of unnecessary locks?

[the sun, 2017/04/06 20:33:47]

I think avoiding the duplicated code between here and the ctor wins in this
case. To me it looks like you could basically take the code in the ctor and put
it here, then call Reconfigure() from the ctor, and that'd be it. (In reality
you'll need to put it in a utility function called from both places, because of
the issues of calling virtual functions from the ctor.) Poof, 70 lines gone. And
we take as many locks as we do now, although quite rarely.

+    const webrtc::AudioSendStream::Config& new_config) {
+  LOG(LS_INFO) << "AudioSendStream::Reconfigure: " << new_config.ToString();
+  // TODO(ossu): Really enforce SSRC here?
+  RTC_CHECK_EQ(config_.rtp.ssrc, new_config.rtp.ssrc);
+  if (new_config.rtp.c_name != config_.rtp.c_name) {
+    channel_proxy_->SetRTCP_CNAME(new_config.rtp.c_name);
+  }
+  if (new_config.rtp.nack.rtp_history_ms != config_.rtp.nack.rtp_history_ms) {
+    // TODO(solenberg): Config NACK history window (which is a packet count),
+    // using the actual packet size for the configured codec.
+    channel_proxy_->SetNACKStatus(config_.rtp.nack.rtp_history_ms != 0,
+                                  config_.rtp.nack.rtp_history_ms / 20);
+  }
+
+  if (new_config.send_transport != config_.send_transport) {
+    channel_proxy_->DeRegisterExternalTransport();
+    channel_proxy_->RegisterExternalTransport(new_config.send_transport);
+  }
+
+  // RFC 5285: Each distinct extension MUST have a unique ID. The value 0 is

[the sun, 2017/04/04 23:02:54]

This is already guaranteed by WVoMC::SetSendParameters(). Treat that as the API
layer and just DCHECK in this layer.

[ossu, 2017/04/06 10:15:21]

I've added it as an explanation to why I can use 0 as a "not configured" value
here. I'll make that clearer in the comment.

+  // reserved for padding and MUST NOT be used as a local identifier.
+  struct ExtensionIds {
+    int audio_level = 0;
+    int transport_sequence_number = 0;
+  };
+
+  auto find_extension_ids = [](const std::vector<RtpExtension>& extensions) {
+    ExtensionIds ids;
+    for (const auto& extension : extensions) {
+      if (extension.uri == RtpExtension::kAudioLevelUri) {
+        ids.audio_level = extension.id;
+      } else if (extension.uri == RtpExtension::kTransportSequenceNumberUri) {
+        ids.transport_sequence_number = extension.id;
+      }
+    }
+    return ids;
+  };
+
+  const ExtensionIds old_ids = find_extension_ids(config_.rtp.extensions);
+  const ExtensionIds new_ids = find_extension_ids(new_config.rtp.extensions);
+  // Audio level indication
+  if (new_ids.audio_level != old_ids.audio_level) {
+    channel_proxy_->SetSendAudioLevelIndicationStatus(new_ids.audio_level != 0,
+                                                      new_ids.audio_level);
+  }
+  // Transport sequence number
+  if (new_ids.transport_sequence_number != old_ids.transport_sequence_number) {
+    channel_proxy_->ResetCongestionControlObjects();
+
+    if (new_ids.transport_sequence_number != 0) {
+      channel_proxy_->EnableSendTransportSequenceNumber(
+          new_ids.transport_sequence_number);
+      congestion_controller_->EnablePeriodicAlrProbing(true);
+      bandwidth_observer_.reset(congestion_controller_->GetBitrateController()
+                                    ->CreateRtcpBandwidthObserver());
+    } else {
+      bandwidth_observer_.reset();
+    }
+
+    channel_proxy_->RegisterSenderCongestionControlObjects(
+        congestion_controller_->pacer(), congestion_controller_, packet_router_,
+        bandwidth_observer_.get());
+  }
+
+  ReconfigureSendCodec(new_config);
+  ReconfigureBitrateObserver(new_config);
+
+  config_ = new_config;
+}
+
 void AudioSendStream::Start() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   if (config_.min_bitrate_bps != -1 && config_.max_bitrate_bps != -1) {
-    RTC_DCHECK_GE(config_.max_bitrate_bps, config_.min_bitrate_bps);
-    rtc::Event thread_sync_event(false /* manual_reset */, false);
-    worker_queue_->PostTask([this, &thread_sync_event] {
-      bitrate_allocator_->AddObserver(this, config_.min_bitrate_bps,
-                                      config_.max_bitrate_bps, 0, true);
-      thread_sync_event.Set();
-    });
-    thread_sync_event.Wait(rtc::Event::kForever);
+    ConfigureBitrateObserver(config_.min_bitrate_bps, config_.max_bitrate_bps);
   }
 
   ScopedVoEInterface<VoEBase> base(voice_engine());
@@ -124,12 +197,7 @@ void AudioSendStream::Start() {
 
 void AudioSendStream::Stop() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
-  rtc::Event thread_sync_event(false /* manual_reset */, false);
-  worker_queue_->PostTask([this, &thread_sync_event] {
-    bitrate_allocator_->RemoveObserver(this);
-    thread_sync_event.Set();
-  });
-  thread_sync_event.Wait(rtc::Event::kForever);
+  RemoveBitrateObserver();
 
   ScopedVoEInterface<VoEBase> base(voice_engine());
   int error = base->StopSend(config_.voe_channel_id);
@@ -169,11 +237,10 @@ webrtc::AudioSendStream::Stats AudioSendStream::GetStats() const {
   //                  implementation.
   stats.aec_quality_min = -1;
 
-  webrtc::CodecInst codec_inst = {0};
-  if (channel_proxy_->GetSendCodec(&codec_inst)) {
-    RTC_DCHECK_NE(codec_inst.pltype, -1);
-    stats.codec_name = codec_inst.plname;
-    stats.codec_payload_type = rtc::Optional<int>(codec_inst.pltype);
+  if (config_.send_codec_spec) {
+    const auto& spec = *config_.send_codec_spec;
+    stats.codec_name = spec.format.name;
+    stats.codec_payload_type = rtc::Optional<int>(spec.payload_type);
 
     // Get data from the last remote RTCP report.
     for (const auto& block : channel_proxy_->GetRemoteRTCPReportBlocks()) {
@@ -182,10 +249,10 @@ webrtc::AudioSendStream::Stats AudioSendStream::GetStats() const {
         stats.packets_lost = block.cumulative_num_packets_lost;
         stats.fraction_lost = Q8ToFloat(block.fraction_lost);
         stats.ext_seqnum = block.extended_highest_sequence_number;
-        // Convert samples to milliseconds.
-        if (codec_inst.plfreq / 1000 > 0) {
+        // Convert timestamps to milliseconds.
+        if (spec.format.clockrate_hz / 1000 > 0) {
           stats.jitter_ms =
-              block.interarrival_jitter / (codec_inst.plfreq / 1000);
+              block.interarrival_jitter / (spec.format.clockrate_hz / 1000);
         }
         break;
       }
@@ -267,116 +334,192 @@ VoiceEngine* AudioSendStream::voice_engine() const {
 }
 
 // Apply current codec settings to a single voe::Channel used for sending.
-bool AudioSendStream::SetupSendCodec() {
-  // Disable VAD and FEC unless we know the other side wants them.
-  channel_proxy_->SetVADStatus(false);
-  channel_proxy_->SetCodecFECStatus(false);
-
-  // We disable audio network adaptor here. This will on one hand make sure that
-  // audio network adaptor is disabled by default, and on the other allow audio
-  // network adaptor to be reconfigured, since SetReceiverFrameLengthRange can
-  // be only called when audio network adaptor is disabled.
-  channel_proxy_->DisableAudioNetworkAdaptor();
-
-  const auto& send_codec_spec = config_.send_codec_spec;
-
-  // We set the codec first, since the below extra configuration is only applied
-  // to the "current" codec.
-
-  // If codec is already configured, we do not it again.
-  // TODO(minyue): check if this check is really needed, or can we move it into
-  // |codec->SetSendCodec|.
-  webrtc::CodecInst current_codec = {0};
-  if (!channel_proxy_->GetSendCodec(&current_codec) ||
-      (send_codec_spec.codec_inst != current_codec)) {
-    if (!channel_proxy_->SetSendCodec(send_codec_spec.codec_inst)) {
-      LOG(LS_WARNING) << "SetSendCodec() failed.";
-      return false;
+bool AudioSendStream::SetupSendCodec(const Config& config) {
+  RTC_DCHECK(config.send_codec_spec);
+  // Explicitly hide config_ here, so we don't accidentally setup a send codec
+  // with old parameters.
+  auto setup_encoder = [](const Config& config, RtcEventLog* event_log) {
+    const auto& spec = *config.send_codec_spec;
+    std::unique_ptr<AudioEncoder> encoder =
+        config.encoder_factory->MakeAudioEncoder(spec.payload_type,
+                                                 spec.format);
+
+    if (!encoder) {
+      LOG(LS_ERROR) << "Unable to create encoder for " << spec.format;
+      return encoder;
     }
-  }
-
-  // Codec internal FEC. Treat any failure as fatal internal error.
-  if (send_codec_spec.enable_codec_fec) {
-    if (!channel_proxy_->SetCodecFECStatus(true)) {
-      LOG(LS_WARNING) << "SetCodecFECStatus() failed.";
-      return false;
+    // If a bitrate has been specified for the codec, use it over the
+    // codec's default.
+    if (spec.target_bitrate_bps) {
+      encoder->OnReceivedTargetAudioBitrate(*spec.target_bitrate_bps);
     }
-  }
 
-  // DTX and maxplaybackrate are only set if current codec is Opus.
-  if (IsCodec(send_codec_spec.codec_inst, kOpusCodecName)) {
-    if (!channel_proxy_->SetOpusDtx(send_codec_spec.enable_opus_dtx)) {
-      LOG(LS_WARNING) << "SetOpusDtx() failed.";
-      return false;
-    }
-
-    // If opus_max_playback_rate <= 0, the default maximum playback rate
-    // (48 kHz) will be used.
-    if (send_codec_spec.opus_max_playback_rate > 0) {
-      if (!channel_proxy_->SetOpusMaxPlaybackRate(
-              send_codec_spec.opus_max_playback_rate)) {
-        LOG(LS_WARNING) << "SetOpusMaxPlaybackRate() failed.";
-        return false;
+    // Enable ANA if configured (currently only used by Opus).
+    if (config.audio_network_adaptor_config) {
+      if (encoder->EnableAudioNetworkAdaptor(
+              *config.audio_network_adaptor_config, event_log,
+              Clock::GetRealTimeClock())) {
+        LOG(LS_INFO) << "Audio network adaptor enabled on SSRC "
+                     << config.rtp.ssrc;
+      } else {
+        RTC_NOTREACHED();
       }
     }
 
-    if (config_.audio_network_adaptor_config) {
-      // Audio network adaptor is only allowed for Opus currently.
-      // |SetReceiverFrameLengthRange| needs to be called before
-      // |EnableAudioNetworkAdaptor|.
-      channel_proxy_->SetReceiverFrameLengthRange(send_codec_spec.min_ptime_ms,
-                                                  send_codec_spec.max_ptime_ms);
-      channel_proxy_->EnableAudioNetworkAdaptor(
-          *config_.audio_network_adaptor_config);
-      LOG(LS_INFO) << "Audio network adaptor enabled on SSRC "
-                   << config_.rtp.ssrc;
+    // Wrap the encoder in a an AudioEncoderCNG, if VAD is enabled.
+    if (spec.cng_payload_type) {
+      AudioEncoderCng::Config cng_config;
+      cng_config.num_channels = encoder->NumChannels();
+      cng_config.payload_type = *spec.cng_payload_type;
+      cng_config.speech_encoder = std::move(encoder);
+      cng_config.vad_mode = Vad::kVadNormal;
+      encoder.reset(new AudioEncoderCng(std::move(cng_config)));
     }
+
+    return encoder;
+  };
+
+  auto encoder = setup_encoder(config, event_log_);
+  if (!encoder) {
+    return false;
   }
+  channel_proxy_->SetEncoder(config.send_codec_spec->payload_type,
+                             std::move(encoder));
+  return true;
+}
 
-  // Set the CN payloadtype and the VAD status.
-  if (send_codec_spec.cng_payload_type != -1) {
-    // The CN payload type for 8000 Hz clockrate is fixed at 13.
-    if (send_codec_spec.cng_plfreq != 8000) {
-      webrtc::PayloadFrequencies cn_freq;
-      switch (send_codec_spec.cng_plfreq) {
-        case 16000:
-          cn_freq = webrtc::kFreq16000Hz;
-          break;
-        case 32000:
-          cn_freq = webrtc::kFreq32000Hz;
-          break;
-        default:
-          RTC_NOTREACHED();
-          return false;
-      }
-      if (!channel_proxy_->SetSendCNPayloadType(
-          send_codec_spec.cng_payload_type, cn_freq)) {
-        LOG(LS_WARNING) << "SetSendCNPayloadType() failed.";
-        // TODO(ajm): This failure condition will be removed from VoE.
-        // Restore the return here when we update to a new enough webrtc.
-        //
-        // Not returning false because the SetSendCNPayloadType will fail if
-        // the channel is already sending.
-        // This can happen if the remote description is applied twice, for
-        // example in the case of ROAP on top of JSEP, where both side will
-        // send the offer.
-      }
-    }
+bool AudioSendStream::ReconfigureSendCodec(const Config& new_config) {
+  if (new_config.send_codec_spec == config_.send_codec_spec) {
+    return true;
+  }
 
-    // Only turn on VAD if we have a CN payload type that matches the
-    // clockrate for the codec we are going to use.
-    if (send_codec_spec.cng_plfreq == send_codec_spec.codec_inst.plfreq &&
-        send_codec_spec.codec_inst.channels == 1) {
-      // TODO(minyue): If CN frequency == 48000 Hz is allowed, consider the
-      // interaction between VAD and Opus FEC.
-      if (!channel_proxy_->SetVADStatus(true)) {
-        LOG(LS_WARNING) << "SetVADStatus() failed.";
-        return false;
-      }
-    }
+  // If we have no encoder, or the format or payload type's changed, create a
+  // new encoder.
+  if (!config_.send_codec_spec ||
+      new_config.send_codec_spec->format != config_.send_codec_spec->format ||
+      new_config.send_codec_spec->payload_type !=
+          config_.send_codec_spec->payload_type) {
+    return SetupSendCodec(new_config);
+  }
+
+  if (!new_config.send_codec_spec) {
+    // TODO(ossu): Double-check this!
+    LOG(LS_ERROR) << "Cannot replace the current encoder with no encoder";
+    RTC_NOTREACHED();
+    return false;
   }
+
+  const rtc::Optional<int>& new_target_bitrate_bps =
+      new_config.send_codec_spec->target_bitrate_bps;
+  // If a bitrate has been specified for the codec, use it over the
+  // codec's default.
+  if (new_target_bitrate_bps &&
+      new_target_bitrate_bps != config_.send_codec_spec->target_bitrate_bps) {
+    CallEncoder(channel_proxy_, [&](AudioEncoder* encoder) {
+      encoder->OnReceivedTargetAudioBitrate(*new_target_bitrate_bps);
+    });
+  }
+
+  ReconfigureANA(new_config);
+  ReconfigureCNG(new_config);
+
   return true;
 }
 
+void AudioSendStream::ReconfigureANA(const Config& new_config) {
+  if (new_config.audio_network_adaptor_config ==
+      config_.audio_network_adaptor_config) {
+    return;
+  }
+  if (new_config.audio_network_adaptor_config) {
+    CallEncoder(channel_proxy_, [&](AudioEncoder* encoder) {
+      if (encoder->EnableAudioNetworkAdaptor(
+              *new_config.audio_network_adaptor_config, event_log_,
+              Clock::GetRealTimeClock())) {
+        LOG(LS_INFO) << "Audio network adaptor enabled on SSRC "
+                     << new_config.rtp.ssrc;
+      } else {
+        RTC_NOTREACHED();
+      }
+    });
+  } else {
+    CallEncoder(channel_proxy_, [&](AudioEncoder* encoder) {
+      encoder->DisableAudioNetworkAdaptor();
+    });
+    LOG(LS_INFO) << "Audio network adaptor disabled on SSRC "
+                 << new_config.rtp.ssrc;
+  }
+}
+
+void AudioSendStream::ReconfigureCNG(const Config& new_config) {
+  if (new_config.send_codec_spec->cng_payload_type ==
+      config_.send_codec_spec->cng_payload_type) {
+    return;
+  }
+
+  // Wrap or unwrap the encoder in an AudioEncoderCNG.
+  channel_proxy_->ModifyEncoder(
+      [&](std::unique_ptr<AudioEncoder>* encoder_ptr) {
+        std::unique_ptr<AudioEncoder> old_encoder(std::move(*encoder_ptr));
+        auto sub_encoders = old_encoder->ReclaimContainedEncoders();

[kwiberg-webrtc, 2017/04/06 10:13:29]

Eugh. I'm having second thoughts about having this method in AudioEncoder. I
*really* wish we could use dynamic_cast instead...

How about subclassing AudioEncoder, and putting a corresponding method in that
subclass instead? Such a subclass could promise to have exactly one sub-encoder,
which would make life much easier. It could also be non-public.

For the case of no CN, we'd need a forwarding wrapper, though. We could avoid
that by making a separate interface for the reclamation function, but then we
have to tackle lifetime issues instead.

[ossu, 2017/04/06 11:14:24]

Yeah, it's a bit nasty, but slightly less so in this case, since we know there'd
be exactly one level of nesting.

I think we should eventually move away from wrapping encoders like this, instead
driving the components separately, perhaps with VAD extracted from
AudioEncoderCNG and working as a switch between the two.

Regardless, I'm not going to redo our AudioEncoder hierarchy in this CL. I'd
love to do it later, though! :)

+        if (!sub_encoders.empty()) {
+          // Replace enc with its sub encoder. We need to put the sub
+          // encoder in a temporary first, since otherwise the old value
+          // of enc would be destroyed before the new value got assigned,
+          // which would be bad since the new value is a part of the old
+          // value.
+          auto tmp = std::move(sub_encoders[0]);
+          old_encoder = std::move(tmp);
+        }
+        if (new_config.send_codec_spec->cng_payload_type) {
+          AudioEncoderCng::Config config;
+          config.speech_encoder = std::move(old_encoder);
+          config.num_channels = config.speech_encoder->NumChannels();
+          config.payload_type = *new_config.send_codec_spec->cng_payload_type;
+          config.vad_mode = Vad::kVadNormal;
+          encoder_ptr->reset(new AudioEncoderCng(std::move(config)));
+        } else {
+          *encoder_ptr = std::move(old_encoder);
+        }
+      });
+}
+
+void AudioSendStream::ReconfigureBitrateObserver(
+    const webrtc::AudioSendStream::Config& new_config) {
+  if (config_.min_bitrate_bps == new_config.min_bitrate_bps &&
+      config_.max_bitrate_bps == new_config.max_bitrate_bps) {
+    return;
+  }
+
+  if (new_config.min_bitrate_bps != -1 && new_config.max_bitrate_bps != -1) {

[ossu, 2017/04/04 15:36:38]

Do I need to check if we're sending here first? The bitrate observer is usually
configured just as we're about to start sending. Is it important that it's not
active for a long duration without us also sending?

+    ConfigureBitrateObserver(config_.min_bitrate_bps, config_.max_bitrate_bps);
+  } else {
+    RemoveBitrateObserver();
+  }
+}
+
+void AudioSendStream::ConfigureBitrateObserver(int min_bitrate_bps,
+                                               int max_bitrate_bps) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_GE(max_bitrate_bps, min_bitrate_bps);
+  rtc::Event thread_sync_event(false /* manual_reset */, false);
+  worker_queue_->PostTask([&] {
+    bitrate_allocator_->AddObserver(this, min_bitrate_bps, max_bitrate_bps, 0,
+                                    true);
+    thread_sync_event.Set();
+  });
+  thread_sync_event.Wait(rtc::Event::kForever);
+}
+
+void AudioSendStream::RemoveBitrateObserver() {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  rtc::Event thread_sync_event(false /* manual_reset */, false);
+  worker_queue_->PostTask([this, &thread_sync_event] {
+    bitrate_allocator_->RemoveObserver(this);
+    thread_sync_event.Set();
+  });
+  thread_sync_event.Wait(rtc::Event::kForever);
+}
+
 }  // namespace internal
 }  // namespace webrtc