Wire up x-google-{min,start,max}-bitrate to WebRtcVoiceMediaChannel.

webrtc/media/engine/webrtcvoiceengine.cc

Index: webrtc/media/engine/webrtcvoiceengine.cc
diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc
index 96f9ed70d403c6c464cc9b015f83190630a0b331..69b9134d3a066322a19404e89db4d1e7155bdb5e 100644
--- a/webrtc/media/engine/webrtcvoiceengine.cc
+++ b/webrtc/media/engine/webrtcvoiceengine.cc
@@ -277,6 +277,32 @@ void GetOpusConfig(const AudioCodec& codec,
   voe_codec->rate = GetOpusBitrate(codec, *max_playback_rate);
 }
 
+webrtc::Call::Config::BitrateConfig GetBitrateConfigForCodec(
+    const AudioCodec& codec) {
+  webrtc::Call::Config::BitrateConfig config;
+  int bitrate_kbps;

[the sun, 2016/11/30 11:22:27]

nit: always default init

[stefan-webrtc, 2016/11/30 11:44:42]

Done.

+  if (codec.GetParam(kCodecParamMinBitrate, &bitrate_kbps) &&
+      bitrate_kbps > 0) {
+    config.min_bitrate_bps = bitrate_kbps * 1000;
+  } else {
+    config.min_bitrate_bps = 0;
+  }
+  if (codec.GetParam(kCodecParamStartBitrate, &bitrate_kbps) &&
+      bitrate_kbps > 0) {
+    config.start_bitrate_bps = bitrate_kbps * 1000;
+  } else {
+    // Do not reconfigure start bitrate unless it's specified and positive.
+    config.start_bitrate_bps = -1;

[the sun, 2016/11/30 11:22:27]

Change the optional fields in the config to use rtc::Optional

[stefan-webrtc, 2016/11/30 11:44:42]

I'd prefer making interface changes later.

+  }
+  if (codec.GetParam(kCodecParamMaxBitrate, &bitrate_kbps) &&
+      bitrate_kbps > 0) {
+    config.max_bitrate_bps = bitrate_kbps * 1000;
+  } else {
+    config.max_bitrate_bps = -1;
+  }
+  return config;
+}
+
 webrtc::AudioState::Config MakeAudioStateConfig(VoEWrapper* voe_wrapper) {
   webrtc::AudioState::Config config;
   config.voice_engine = voe_wrapper->engine();
@@ -1616,6 +1642,15 @@ bool WebRtcVoiceMediaChannel::SetSendParameters(
     return false;
   }
 
+  if (params.max_bandwidth_bps >= 0) {

[the sun, 2016/11/30 11:22:27]

Is this logic the same for video? Does it make sense to make a util?

[stefan-webrtc, 2016/11/30 11:44:43]

It is the same, not sure about making it a util though... There are many small
things in these files that are similar for video and audio.

I made the GetBitrateConfigForCodec() a util though, that seems more natural.

+    // Note that max_bandwidth_bps intentionally takes priority over the
+    // bitrate config for the codec. This allows FEC to be applied above the

[minyue-webrtc, 2016/11/29 22:10:55]

Not sure about the reasoning on FEC in the comment.

[stefan-webrtc, 2016/11/30 11:44:43]

I made it a bit more generic.

+    // codec target bitrate.
+    bitrate_config_.max_bitrate_bps =
+        params.max_bandwidth_bps == 0 ? -1 : params.max_bandwidth_bps;
+  }
+  call_->SetBitrateConfig(bitrate_config_);
+
   if (!ValidateRtpExtensions(params.extensions)) {
     return false;
   }
@@ -1933,6 +1968,13 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs(
                     &send_codec_spec.max_ptime_ms);
     }
 
+    bitrate_config_ = GetBitrateConfigForCodec(*codec);
+    if (send_codec_spec_set_ && send_codec_spec == send_codec_spec_) {

[minyue-webrtc, 2016/11/29 22:10:55]

I don't think this is the place we reset codec. please consider line 1255

[the sun, 2016/11/30 11:22:27]

I don't get the logic involving send_codec_spec_set_. IIUC it is there so that
we never reset bw when the same codec is set multiple times. Can it be
simplified by always clearing bitrate_config_.start_bitrate_bps after pushing
the config to call_ (as well as clearing it here - or preferably at line
2039-ish - when codec specs are identical)?

[stefan-webrtc, 2016/11/30 11:44:42]

I tried to the changes into WebRtcAudioSendStream instead, but it prevents us
from using cricket::AudioCodec to determine the BitrateConfig, which makes it
difficult to reuse the same function for doing that for both audio and video.

Since there's no clear benefit of having it in WebRtcAudioSendStream I would
prefer this way.

[stefan-webrtc, 2016/11/30 11:44:43]

I don't think the send_codec_spec_set_ check is needed, removed it. That I think
makes it more like you suggested. Also moved it down.

+      // If the codec isn't changing, set the start bitrate to -1 which means
+      // "unchanged" so that BWE isn't affected.
+      bitrate_config_.start_bitrate_bps = -1;
+    }
+
     // Set packet size if the AudioCodec param kCodecParamPTime is set.
     int ptime_ms = 0;
     if (codec->GetParam(kCodecParamPTime, &ptime_ms)) {
@@ -1990,6 +2032,7 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs(
   // Apply new settings to all streams.
   if (send_codec_spec_ != send_codec_spec) {
     send_codec_spec_ = std::move(send_codec_spec);
+    send_codec_spec_set_ = true;
     for (const auto& kv : send_streams_) {
       kv.second->RecreateAudioSendStream(send_codec_spec_);
     }