Injectable audio encoders: WebRtcVoiceEngine and company

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 eaeea5725db1adb8dc9d5fc687e9bb5f84f10c10..58929dfabc2eb35ca5dfe7e4d79e1fcfe7a0c800 100644
--- a/webrtc/media/engine/webrtcvoiceengine.cc
+++ b/webrtc/media/engine/webrtcvoiceengine.cc
@@ -37,6 +37,7 @@
 #include "webrtc/media/engine/webrtcvoe.h"
 #include "webrtc/modules/audio_coding/acm2/rent_a_codec.h"
 #include "webrtc/modules/audio_mixer/audio_mixer_impl.h"
+#include "webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 #include "webrtc/system_wrappers/include/field_trial.h"
 #include "webrtc/system_wrappers/include/trace.h"
@@ -72,27 +73,9 @@ constexpr int kNackRtpHistoryMs = 5000;
 #error "Set WEBRTC_INTELLIGIBILITY_ENHANCER to either 0 or 1"
 #endif
 
-// Codec parameters for Opus.
-// draft-spittka-payload-rtp-opus-03
-
-// Recommended bitrates:
-// 8-12 kb/s for NB speech,
-// 16-20 kb/s for WB speech,
-// 28-40 kb/s for FB speech,
-// 48-64 kb/s for FB mono music, and
-// 64-128 kb/s for FB stereo music.
-// The current implementation applies the following values to mono signals,
-// and multiplies them by 2 for stereo.
-const int kOpusBitrateNbBps = 12000;
-const int kOpusBitrateWbBps = 20000;
-const int kOpusBitrateFbBps = 32000;
-
-// Opus bitrate should be in the range between 6000 and 510000.
+// For SendSideBwe, Opus bitrate should be in the range between 6000 and 32000.

[the sun, 2017/03/16 08:48:19]

32000? 

[ossu, 2017/03/20 18:19:48]

Yup, seems so. I've just rearranged the code a bit and added an explanatory
comment. Perhaps I should remove the comment and move the constants close to
their use instead?

 const int kOpusMinBitrateBps = 6000;
-const int kOpusMaxBitrateBps = 510000;
-
-// iSAC bitrate should be <= 56000.
-const int kIsacMaxBitrateBps = 56000;
+const int kOpusBitrateFbBps = 32000;
 
 // Default audio dscp value.
 // See http://tools.ietf.org/html/rfc2474 for details.
@@ -133,8 +116,15 @@ bool ValidateStreamParams(const StreamParams& sp) {
 // Dumps an AudioCodec in RFC 2327-ish format.
 std::string ToString(const AudioCodec& codec) {
   std::stringstream ss;
-  ss << codec.name << "/" << codec.clockrate << "/" << codec.channels
-     << " (" << codec.id << ")";
+  ss << codec.name << "/" << codec.clockrate << "/" << codec.channels;
+  if (!codec.params.empty()) {
+    ss << " {";
+        for (const auto& param : codec.params) {
+          ss << " " << param.first << "=" << param.second;
+        }

[kwiberg-webrtc, 2017/03/01 12:26:47]

Broken indentation. Also, isn't there a second parameter map?

[ossu, 2017/03/02 01:30:28]

Wow, what happened here? :)
I guess you're talking about the feedback params? Yes, there is. It would
probably make sense to print them as well.

+    ss << " }";
+  }
+  ss << " (" << codec.id << ")";
   return ss.str();
 }
 
@@ -180,12 +170,6 @@ bool VerifyUniquePayloadTypes(const std::vector<AudioCodec>& codecs) {
   return it == payload_types.end();
 }
 
-// Return true if codec.params[feature] == "1", false otherwise.
-bool IsCodecFeatureEnabled(const AudioCodec& codec, const char* feature) {
-  int value;
-  return codec.GetParam(feature, &value) && value == 1;
-}
-
 rtc::Optional<std::string> GetAudioNetworkAdaptorConfig(
     const AudioOptions& options) {
   if (options.audio_network_adaptor && *options.audio_network_adaptor &&
@@ -197,85 +181,6 @@ rtc::Optional<std::string> GetAudioNetworkAdaptorConfig(
   return rtc::Optional<std::string>();
 }
 
-// Returns integer parameter params[feature] if it is defined. Returns
-// |default_value| otherwise.
-int GetCodecFeatureInt(const AudioCodec& codec,
-                       const char* feature,
-                       int default_value) {
-  int value = 0;
-  if (codec.GetParam(feature, &value)) {
-    return value;
-  }
-  return default_value;
-}
-
-// Use params[kCodecParamMaxAverageBitrate] if it is defined, use codec.bitrate
-// otherwise. If the value (either from params or codec.bitrate) <=0, use the
-// default configuration. If the value is beyond feasible bit rate of Opus,
-// clamp it. Returns the Opus bit rate for operation.
-int GetOpusBitrate(const AudioCodec& codec, int max_playback_rate) {
-  int bitrate = 0;
-  bool use_param = true;
-  if (!codec.GetParam(kCodecParamMaxAverageBitrate, &bitrate)) {
-    bitrate = codec.bitrate;
-    use_param = false;
-  }
-  if (bitrate <= 0) {
-    if (max_playback_rate <= 8000) {
-      bitrate = kOpusBitrateNbBps;
-    } else if (max_playback_rate <= 16000) {
-      bitrate = kOpusBitrateWbBps;
-    } else {
-      bitrate = kOpusBitrateFbBps;
-    }
-
-    if (IsCodecFeatureEnabled(codec, kCodecParamStereo)) {
-      bitrate *= 2;
-    }
-  } else if (bitrate < kOpusMinBitrateBps || bitrate > kOpusMaxBitrateBps) {
-    bitrate = (bitrate < kOpusMinBitrateBps) ? kOpusMinBitrateBps
-                                             : kOpusMaxBitrateBps;
-    std::string rate_source =
-        use_param ? "Codec parameter \"maxaveragebitrate\"" :
-            "Supplied Opus bitrate";
-    LOG(LS_WARNING) << rate_source
-                    << " is invalid and is replaced by: "
-                    << bitrate;
-  }
-  return bitrate;
-}
-
-void GetOpusConfig(const AudioCodec& codec,
-                   webrtc::CodecInst* voe_codec,
-                   bool* enable_codec_fec,
-                   int* max_playback_rate,
-                   bool* enable_codec_dtx,
-                   int* min_ptime_ms,
-                   int* max_ptime_ms) {
-  *enable_codec_fec = IsCodecFeatureEnabled(codec, kCodecParamUseInbandFec);
-  *enable_codec_dtx = IsCodecFeatureEnabled(codec, kCodecParamUseDtx);
-  *max_playback_rate = GetCodecFeatureInt(codec, kCodecParamMaxPlaybackRate,
-                                          kOpusDefaultMaxPlaybackRate);
-  *max_ptime_ms =
-      GetCodecFeatureInt(codec, kCodecParamMaxPTime, kOpusDefaultMaxPTime);
-  *min_ptime_ms =
-      GetCodecFeatureInt(codec, kCodecParamMinPTime, kOpusDefaultMinPTime);
-  if (*max_ptime_ms < *min_ptime_ms) {
-    // If min ptime or max ptime defined by codec parameter is wrong, we use
-    // the default values.
-    *max_ptime_ms = kOpusDefaultMaxPTime;
-    *min_ptime_ms = kOpusDefaultMinPTime;
-  }
-
-  // If OPUS, change what we send according to the "stereo" codec
-  // parameter, and not the "channels" parameter.  We set
-  // voe_codec.channels to 2 if "stereo=1" and 1 otherwise.  If
-  // the bitrate is not specified, i.e. is <= zero, we set it to the
-  // appropriate default value for mono or stereo Opus.
-  voe_codec->channels = IsCodecFeatureEnabled(codec, kCodecParamStereo) ? 2 : 1;
-  voe_codec->rate = GetOpusBitrate(codec, *max_playback_rate);
-}
-
 webrtc::AudioState::Config MakeAudioStateConfig(
     VoEWrapper* voe_wrapper,
     rtc::scoped_refptr<webrtc::AudioMixer> audio_mixer) {
@@ -291,67 +196,14 @@ webrtc::AudioState::Config MakeAudioStateConfig(
 
 class WebRtcVoiceCodecs final {
  public:
-  // TODO(solenberg): Do this filtering once off-line, add a simple AudioCodec
-  // list and add a test which verifies VoE supports the listed codecs.
-  static std::vector<AudioCodec> SupportedSendCodecs() {
-    std::vector<AudioCodec> result;
-    // Iterate first over our preferred codecs list, so that the results are
-    // added in order of preference.
-    for (size_t i = 0; i < arraysize(kCodecPrefs); ++i) {
-      const CodecPref* pref = &kCodecPrefs[i];
-      for (webrtc::CodecInst voe_codec : webrtc::acm2::RentACodec::Database()) {
-        // Change the sample rate of G722 to 8000 to match SDP.
-        MaybeFixupG722(&voe_codec, 8000);
-        // Skip uncompressed formats.
-        if (IsCodec(voe_codec, kL16CodecName)) {
-          continue;
-        }
-
-        if (!IsCodec(voe_codec, pref->name) ||
-            pref->clockrate != voe_codec.plfreq ||
-            pref->channels != voe_codec.channels) {
-          // Not a match.
-          continue;
-        }
-
-        AudioCodec codec(pref->payload_type, voe_codec.plname, voe_codec.plfreq,
-                         voe_codec.rate, voe_codec.channels);
-        LOG(LS_INFO) << "Adding supported codec: " << ToString(codec);
-        if (IsCodec(codec, kIsacCodecName)) {
-          // Indicate auto-bitrate in signaling.
-          codec.bitrate = 0;
-        }
-        if (IsCodec(codec, kOpusCodecName)) {
-          // Only add fmtp parameters that differ from the spec.
-          if (kPreferredMinPTime != kOpusDefaultMinPTime) {
-            codec.params[kCodecParamMinPTime] =
-                rtc::ToString(kPreferredMinPTime);
-          }
-          if (kPreferredMaxPTime != kOpusDefaultMaxPTime) {
-            codec.params[kCodecParamMaxPTime] =
-                rtc::ToString(kPreferredMaxPTime);
-          }
-          codec.SetParam(kCodecParamUseInbandFec, 1);
-          codec.AddFeedbackParam(
-              FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty));
-
-          // TODO(hellner): Add ptime, sprop-stereo, and stereo
-          // when they can be set to values other than the default.
-        }
-        result.push_back(codec);
-      }
-    }
-    return result;
-  }
-
-  static bool ToCodecInst(const AudioCodec& in,
-                          webrtc::CodecInst* out) {
+  static bool ToCodecInst(const AudioCodec& in, webrtc::CodecInst* out) {

[the sun, 2017/03/02 20:37:24]

So this is just needed for tests now? And it is static? If you don't want to
update the tests and get rid of it in this CL, could you at least move it into
webrtcvoiceengine_unittest.cc?

[ossu, 2017/03/02 20:43:27]

I'll try to clean up the tests, so we won't need this one at all. Wanted to get
the functional change up as a patch set so we could have a look at it and decide
if it was better than the other way around. I think so.

[ossu, 2017/03/20 18:19:48]

Cleaned up (or plain removed) all remaining tests using it. Turns out it's still
needed until https://codereview.webrtc.org/2686043006/ lands. I guess whoever
lands last gets to remove this function and all it depends on. :)

     for (webrtc::CodecInst voe_codec : webrtc::acm2::RentACodec::Database()) {
       // Change the sample rate of G722 to 8000 to match SDP.
       MaybeFixupG722(&voe_codec, 8000);
       AudioCodec codec(voe_codec.pltype, voe_codec.plname, voe_codec.plfreq,
                        voe_codec.rate, voe_codec.channels);
-      bool multi_rate = IsCodecMultiRate(voe_codec);
+      const bool multi_rate =
+          IsCodec(codec, kIsacCodecName) || IsCodec(codec, kOpusCodecName);
       // Allow arbitrary rates for ISAC to be specified.
       if (multi_rate) {
         // Set codec.bitrate to 0 so the check for codec.Matches() passes.
@@ -378,112 +230,6 @@ class WebRtcVoiceCodecs final {
     return false;
   }
 
-  static bool IsCodecMultiRate(const webrtc::CodecInst& codec) {
-    for (size_t i = 0; i < arraysize(kCodecPrefs); ++i) {
-      if (IsCodec(codec, kCodecPrefs[i].name) &&
-          kCodecPrefs[i].clockrate == codec.plfreq) {
-        return kCodecPrefs[i].is_multi_rate;
-      }
-    }
-    return false;
-  }
-
-  static int MaxBitrateBps(const webrtc::CodecInst& codec) {
-    for (size_t i = 0; i < arraysize(kCodecPrefs); ++i) {
-      if (IsCodec(codec, kCodecPrefs[i].name) &&
-          kCodecPrefs[i].clockrate == codec.plfreq) {
-        return kCodecPrefs[i].max_bitrate_bps;
-      }
-    }
-    return 0;
-  }
-
-  static rtc::ArrayView<const int> GetPacketSizesMs(
-      const webrtc::CodecInst& codec) {
-    for (size_t i = 0; i < arraysize(kCodecPrefs); ++i) {
-      if (IsCodec(codec, kCodecPrefs[i].name)) {
-        size_t num_packet_sizes = kMaxNumPacketSize;
-        for (int index = 0; index < kMaxNumPacketSize; index++) {
-          if (kCodecPrefs[i].packet_sizes_ms[index] == 0) {
-            num_packet_sizes = index;
-            break;
-          }
-        }
-        return rtc::ArrayView<const int>(kCodecPrefs[i].packet_sizes_ms,
-                                         num_packet_sizes);
-      }
-    }
-    return rtc::ArrayView<const int>();
-  }
-
-  // If the AudioCodec param kCodecParamPTime is set, then we will set it to
-  // codec pacsize if it's valid, or we will pick the next smallest value we
-  // support.
-  // TODO(Brave): Query supported packet sizes from ACM when the API is ready.
-  static bool SetPTimeAsPacketSize(webrtc::CodecInst* codec, int ptime_ms) {
-    for (const CodecPref& codec_pref : kCodecPrefs) {
-      if ((IsCodec(*codec, codec_pref.name) &&
-          codec_pref.clockrate == codec->plfreq) ||
-          IsCodec(*codec, kG722CodecName)) {
-        int packet_size_ms = SelectPacketSize(codec_pref, ptime_ms);
-        if (packet_size_ms) {
-          // Convert unit from milli-seconds to samples.
-          codec->pacsize = (codec->plfreq / 1000) * packet_size_ms;
-          return true;
-        }
-      }
-    }
-    return false;
-  }
-
-  static const AudioCodec* GetPreferredCodec(
-      const std::vector<AudioCodec>& codecs,
-      webrtc::CodecInst* out) {
-    RTC_DCHECK(out);
-    // Select the preferred send codec (the first non-telephone-event/CN codec).
-    for (const AudioCodec& codec : codecs) {
-      if (IsCodec(codec, kDtmfCodecName) || IsCodec(codec, kCnCodecName)) {
-        // Skip telephone-event/CN codecs - they will be handled later.
-        continue;
-      }
-
-      // We'll use the first codec in the list to actually send audio data.
-      // Be sure to use the payload type requested by the remote side.
-      // Ignore codecs we don't know about. The negotiation step should prevent
-      // this, but double-check to be sure.
-      if (!ToCodecInst(codec, out)) {
-        LOG(LS_WARNING) << "Unknown codec " << ToString(codec);
-        continue;
-      }
-      return &codec;
-    }
-    return nullptr;
-  }
-
- private:
-  static const int kMaxNumPacketSize = 6;
-  struct CodecPref {
-    const char* name;
-    int clockrate;
-    size_t channels;
-    int payload_type;
-    bool is_multi_rate;
-    int packet_sizes_ms[kMaxNumPacketSize];
-    int max_bitrate_bps;
-  };
-  // Note: keep the supported packet sizes in ascending order.
-  static const CodecPref kCodecPrefs[14];
-
-  static int SelectPacketSize(const CodecPref& codec_pref, int ptime_ms) {
-    int selected_packet_size_ms = codec_pref.packet_sizes_ms[0];
-    for (int packet_size_ms : codec_pref.packet_sizes_ms) {
-      if (packet_size_ms && packet_size_ms <= ptime_ms) {
-        selected_packet_size_ms = packet_size_ms;
-      }
-    }
-    return selected_packet_size_ms;
-  }
-
   // Changes RTP timestamp rate of G722. This is due to the "bug" in the RFC
   // which says that G722 should be advertised as 8 kHz although it is a 16 kHz
   // codec.
@@ -497,69 +243,40 @@ class WebRtcVoiceCodecs final {
   }
 };
 
-const WebRtcVoiceCodecs::CodecPref WebRtcVoiceCodecs::kCodecPrefs[14] = {
-#if WEBRTC_OPUS_SUPPORT_120MS_PTIME
-    {kOpusCodecName, 48000, 2, 111, true, {10, 20, 40, 60, 120},
-     kOpusMaxBitrateBps},
-#else
-    {kOpusCodecName, 48000, 2, 111, true, {10, 20, 40, 60}, kOpusMaxBitrateBps},
-#endif
-    {kIsacCodecName, 16000, 1, 103, true, {30, 60}, kIsacMaxBitrateBps},
-    {kIsacCodecName, 32000, 1, 104, true, {30}, kIsacMaxBitrateBps},
-    // G722 should be advertised as 8000 Hz because of the RFC "bug".
-    {kG722CodecName, 8000, 1, 9, false, {10, 20, 30, 40, 50, 60}},
-    {kIlbcCodecName, 8000, 1, 102, false, {20, 30, 40, 60}},
-    {kPcmuCodecName, 8000, 1, 0, false, {10, 20, 30, 40, 50, 60}},
-    {kPcmaCodecName, 8000, 1, 8, false, {10, 20, 30, 40, 50, 60}},
-    {kCnCodecName, 32000, 1, 106, false, {}},
-    {kCnCodecName, 16000, 1, 105, false, {}},
-    {kCnCodecName, 8000, 1, 13, false, {}},
-    {kDtmfCodecName, 48000, 1, 110, false, {}},
-    {kDtmfCodecName, 32000, 1, 112, false, {}},
-    {kDtmfCodecName, 16000, 1, 113, false, {}},
-    {kDtmfCodecName, 8000, 1, 126, false, {}}
-};
-
 // |max_send_bitrate_bps| is the bitrate from "b=" in SDP.
 // |rtp_max_bitrate_bps| is the bitrate from RtpSender::SetParameters.
 rtc::Optional<int> ComputeSendBitrate(int max_send_bitrate_bps,
                                       rtc::Optional<int> rtp_max_bitrate_bps,
-                                      const webrtc::CodecInst& codec_inst) {
+                                      const webrtc::AudioCodecSpec& spec) {
   // If application-configured bitrate is set, take minimum of that and SDP
   // bitrate.
   const int bps = rtp_max_bitrate_bps
                       ? MinPositive(max_send_bitrate_bps, *rtp_max_bitrate_bps)
                       : max_send_bitrate_bps;
-  const int codec_rate = codec_inst.rate;
-
   if (bps <= 0) {
-    return rtc::Optional<int>(codec_rate);
+    return rtc::Optional<int>(spec.info.default_bitrate_bps);
   }
 
-  if (codec_inst.pltype == -1) {
-    return rtc::Optional<int>(codec_rate);
-    ;
-  }
-
-  if (WebRtcVoiceCodecs::IsCodecMultiRate(codec_inst)) {
-    // If codec is multi-rate then just set the bitrate.
-    return rtc::Optional<int>(
-        std::min(bps, WebRtcVoiceCodecs::MaxBitrateBps(codec_inst)));
-  }
-
-  if (bps < codec_inst.rate) {
+  if (bps < spec.info.min_bitrate_bps) {
     // If codec is not multi-rate and |bps| is less than the fixed bitrate then
     // fail. If codec is not multi-rate and |bps| exceeds or equal the fixed
     // bitrate then ignore.
-    LOG(LS_ERROR) << "Failed to set codec " << codec_inst.plname
+    LOG(LS_ERROR) << "Failed to set codec " << spec.format.name
                   << " to bitrate " << bps << " bps"
-                  << ", requires at least " << codec_inst.rate << " bps.";
+                  << ", requires at least " << spec.info.min_bitrate_bps
+                  << " bps.";
     return rtc::Optional<int>();
   }
-  return rtc::Optional<int>(codec_rate);
+
+  if (spec.info.HasFixedBitrate()) {
+    return rtc::Optional<int>(spec.info.default_bitrate_bps);
+  } else {
+    // If codec is multi-rate then just set the bitrate.
+    return rtc::Optional<int>(std::min(bps, spec.info.max_bitrate_bps));
+  }
 }
 
-}  // namespace {
+}  // namespace
 
 bool WebRtcVoiceEngine::ToCodecInst(const AudioCodec& in,
                                     webrtc::CodecInst* out) {
@@ -580,7 +297,10 @@ WebRtcVoiceEngine::WebRtcVoiceEngine(
     const rtc::scoped_refptr<webrtc::AudioDecoderFactory>& decoder_factory,
     rtc::scoped_refptr<webrtc::AudioMixer> audio_mixer,
     VoEWrapper* voe_wrapper)
-    : adm_(adm), decoder_factory_(decoder_factory), voe_wrapper_(voe_wrapper) {
+    : adm_(adm),
+      encoder_factory_(webrtc::CreateBuiltinAudioEncoderFactory()),
+      decoder_factory_(decoder_factory),
+      voe_wrapper_(voe_wrapper) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "WebRtcVoiceEngine::WebRtcVoiceEngine";
   RTC_DCHECK(voe_wrapper);
@@ -590,13 +310,13 @@ WebRtcVoiceEngine::WebRtcVoiceEngine(
 
   // Load our audio codec list.
   LOG(LS_INFO) << "Supported send codecs in order of preference:";
-  send_codecs_ = WebRtcVoiceCodecs::SupportedSendCodecs();
+  send_codecs_ = CollectCodecs(encoder_factory_->GetSupportedEncoders());
   for (const AudioCodec& codec : send_codecs_) {
     LOG(LS_INFO) << ToString(codec);
   }
 
   LOG(LS_INFO) << "Supported recv codecs in order of preference:";
-  recv_codecs_ = CollectRecvCodecs();
+  recv_codecs_ = CollectCodecs(decoder_factory_->GetSupportedDecoders());
   for (const AudioCodec& codec : recv_codecs_) {
     LOG(LS_INFO) << ToString(codec);
   }
@@ -1148,11 +868,10 @@ webrtc::AudioProcessing* WebRtcVoiceEngine::apm() {
   return apm_;
 }
 
-AudioCodecs WebRtcVoiceEngine::CollectRecvCodecs() const {
+AudioCodecs WebRtcVoiceEngine::CollectCodecs(
+    const std::vector<webrtc::AudioCodecSpec>& specs) const {
   PayloadTypeMapper mapper;
   AudioCodecs out;
-  const std::vector<webrtc::AudioCodecSpec>& specs =
-      decoder_factory_->GetSupportedDecoders();
 
   // Only generate CN payload types for these clockrates:
   std::map<int, bool, std::greater<int>> generate_cn = {{ 8000,  false },
@@ -1237,7 +956,8 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
       int max_send_bitrate_bps,
       const rtc::Optional<std::string>& audio_network_adaptor_config,
       webrtc::Call* call,
-      webrtc::Transport* send_transport)
+      webrtc::Transport* send_transport,
+      const rtc::scoped_refptr<webrtc::AudioEncoderFactory>& encoder_factory)
       : voe_audio_transport_(voe_audio_transport),
         call_(call),
         config_(send_transport),
@@ -1249,11 +969,13 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
     // TODO(solenberg): Once we're not using FakeWebRtcVoiceEngine anymore:
     // RTC_DCHECK(voe_audio_transport);
     RTC_DCHECK(call);
+    RTC_DCHECK(encoder_factory);
     config_.rtp.ssrc = ssrc;
     config_.rtp.c_name = c_name;
     config_.voe_channel_id = ch;
     config_.rtp.extensions = extensions;
     config_.audio_network_adaptor_config = audio_network_adaptor_config;
+    config_.encoder_factory = encoder_factory;
     rtp_parameters_.encodings[0].ssrc = rtc::Optional<uint32_t>(ssrc);
     RecreateAudioSendStream(send_codec_spec);
   }
@@ -1271,13 +993,25 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
     config_.rtp.nack.rtp_history_ms =
         send_codec_spec_.nack_enabled ? kNackRtpHistoryMs : 0;
     config_.send_codec_spec = send_codec_spec_;
-    auto send_rate = ComputeSendBitrate(
-        max_send_bitrate_bps_, rtp_parameters_.encodings[0].max_bitrate_bps,
-        send_codec_spec.codec_inst);
-    if (send_rate) {
-      // Apply a send rate that abides by |max_send_bitrate_bps_| and
-      // |rtp_parameters_| when possible. Otherwise use the codec rate.
-      config_.send_codec_spec.codec_inst.rate = *send_rate;
+    if (send_codec_spec_.format.name != "") {

[kwiberg-webrtc, 2017/03/01 12:26:47]

One of these again...

[ossu, 2017/03/02 01:30:28]

Yes. This is the most important one. There are two factors that conspire to
this:
1. AudioSendStream should never allow an invalid or nonexistent format in its
SendCodecSpec, so an rtc::Optional doesn't make sense for the interface.
2. WebRtcAudioSendStream keeps a SendCodecSpec internally and builds it up
during repeated calls to RecreateAudioSendStream. Sometimes, the
WebRtcAudioSendStream is created without a valid SendCodecSpec.

Clearly, this should be fixed, but not in this CL. For the time being, I'd
either have to re-engineer WebRtcAudioSendStream to use a similar, but slightly
different format for building up the SendCodecSpec (maybe ok) or I could just
leave this check in place for now, and deal with it when cleaning up the way
SendStreams are initialized. WDYT? 

[kwiberg-webrtc, 2017/03/02 02:17:55]

Dealing with it when cleaning up sounds OK.

[ossu, 2017/03/20 18:19:48]

SendCodecSpec is now optional. No more string comparison!

+      auto info =
+          config_.encoder_factory->QueryAudioFormat(send_codec_spec_.format);
+      RTC_DCHECK(info);
+      // If a specific target bitrate has been set for the stream, use that as
+      // the new default bitrate when computing send bitrate.
+      if (send_codec_spec_.target_bitrate_bps) {
+        info->default_bitrate_bps =
+            std::max(info->min_bitrate_bps,
+                     std::min(info->max_bitrate_bps,
+                              *send_codec_spec.target_bitrate_bps));
+      }
+
+      audio_codec_spec_.emplace(
+          webrtc::AudioCodecSpec{send_codec_spec_.format, *info});
+
+      config_.send_codec_spec.target_bitrate_bps = ComputeSendBitrate(
+          max_send_bitrate_bps_, rtp_parameters_.encodings[0].max_bitrate_bps,
+          *audio_codec_spec_);
     }
     RecreateAudioSendStream();
   }
@@ -1301,18 +1035,20 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
 
   bool SetMaxSendBitrate(int bps) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    RTC_DCHECK(audio_codec_spec_);
     auto send_rate =
         ComputeSendBitrate(bps, rtp_parameters_.encodings[0].max_bitrate_bps,
-                           send_codec_spec_.codec_inst);
+                           *audio_codec_spec_);
+
     if (!send_rate) {
       return false;
     }
 
     max_send_bitrate_bps_ = bps;
 
-    if (config_.send_codec_spec.codec_inst.rate != *send_rate) {
-      // Recreate AudioSendStream with new bit rate.
-      config_.send_codec_spec.codec_inst.rate = *send_rate;
+    if (send_rate != config_.send_codec_spec.target_bitrate_bps) {
+      // TODO(ossu): Should not recreate stream!
+      config_.send_codec_spec.target_bitrate_bps = send_rate;
       RecreateAudioSendStream();
     }
     return true;
@@ -1429,19 +1165,24 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
     if (!ValidateRtpParameters(parameters)) {
       return false;
     }
-    auto send_rate = ComputeSendBitrate(max_send_bitrate_bps_,
-                                        parameters.encodings[0].max_bitrate_bps,
-                                        send_codec_spec_.codec_inst);
-    if (!send_rate) {
-      return false;
+
+    rtc::Optional<int> send_rate;
+    if (audio_codec_spec_) {
+      send_rate = ComputeSendBitrate(max_send_bitrate_bps_,
+                                     parameters.encodings[0].max_bitrate_bps,
+                                     *audio_codec_spec_);
+      if (!send_rate) {
+        return false;
+      }
     }
 
     rtp_parameters_ = parameters;
 
     // parameters.encodings[0].encodings[0].max_bitrate_bps could have changed.
-    if (config_.send_codec_spec.codec_inst.rate != *send_rate) {
+    if (send_rate && send_rate != config_.send_codec_spec.target_bitrate_bps) {
+      // TODO(ossu): Should not recreate stream!
       // Recreate AudioSendStream with new bit rate.
-      config_.send_codec_spec.codec_inst.rate = *send_rate;
+      config_.send_codec_spec.target_bitrate_bps = send_rate;
       RecreateAudioSendStream();
     } else {
       // parameters.encodings[0].active could have changed.
@@ -1476,38 +1217,27 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
       // TODO(mflodman): Keep testing this and set proper values.
       // Note: This is an early experiment currently only supported by Opus.
       if (send_side_bwe_with_overhead_) {
-        auto packet_sizes_ms = WebRtcVoiceCodecs::GetPacketSizesMs(
-            config_.send_codec_spec.codec_inst);
-        if (!packet_sizes_ms.empty()) {
-          int max_packet_size_ms =
-              *std::max_element(packet_sizes_ms.begin(), packet_sizes_ms.end());
-          int min_packet_size_ms =
-              *std::min_element(packet_sizes_ms.begin(), packet_sizes_ms.end());
-
-          // Audio network adaptor will just use 20ms and 60ms frame lengths.
-          // The adaptor will only be active for the Opus encoder.
-          if (config_.audio_network_adaptor_config &&
-              IsCodec(config_.send_codec_spec.codec_inst, kOpusCodecName)) {
-#if WEBRTC_OPUS_SUPPORT_120MS_PTIME
-            max_packet_size_ms = 120;
-#else
-            max_packet_size_ms = 60;
-#endif
-            min_packet_size_ms = 20;
-          }
-
-          // OverheadPerPacket = Ipv4(20B) + UDP(8B) + SRTP(10B) + RTP(12)
-          constexpr int kOverheadPerPacket = 20 + 8 + 10 + 12;
-
-          int min_overhead_bps =
-              kOverheadPerPacket * 8 * 1000 / max_packet_size_ms;
-
-          int max_overhead_bps =
-              kOverheadPerPacket * 8 * 1000 / min_packet_size_ms;
-
-          config_.min_bitrate_bps = kOpusMinBitrateBps + min_overhead_bps;
-          config_.max_bitrate_bps = kOpusBitrateFbBps + max_overhead_bps;
-        }
+        const bool is_opus_with_ana =
+            config_.audio_network_adaptor_config &&
+            !STR_CASE_CMP(config_.send_codec_spec.format.name.c_str(),
+                          kOpusCodecName);
+        const int max_packet_size_ms =
+            (is_opus_with_ana && WEBRTC_OPUS_SUPPORT_120MS_PTIME) ? 120 : 60;
+        // Audio network adaptor will just use 20ms and 60ms frame lengths.
+        // The adaptor will only be active for the Opus encoder.
+        const int min_packet_size_ms = is_opus_with_ana ? 20 : 10;
+
+        // OverheadPerPacket = Ipv4(20B) + UDP(8B) + SRTP(10B) + RTP(12)
+        constexpr int kOverheadPerPacket = 20 + 8 + 10 + 12;
+
+        int min_overhead_bps =
+            kOverheadPerPacket * 8 * 1000 / max_packet_size_ms;
+
+        int max_overhead_bps =
+            kOverheadPerPacket * 8 * 1000 / min_packet_size_ms;
+
+        config_.min_bitrate_bps = kOpusMinBitrateBps + min_overhead_bps;
+        config_.max_bitrate_bps = kOpusBitrateFbBps + max_overhead_bps;
       }
     }
     stream_ = call_->CreateAudioSendStream(config_);
@@ -1534,6 +1264,8 @@ class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
   int max_send_bitrate_bps_;
   webrtc::RtpParameters rtp_parameters_;
   webrtc::AudioSendStream::Config::SendCodecSpec send_codec_spec_;
+  // TODO(ossu): Worst. Names. Ever.
+  rtc::Optional<webrtc::AudioCodecSpec> audio_codec_spec_;
 
   RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(WebRtcAudioSendStream);
 };
@@ -1990,76 +1722,63 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs(
   {
     send_codec_spec.nack_enabled = send_codec_spec_.nack_enabled;
 
-    // Find send codec (the first non-telephone-event/CN codec).
-    const AudioCodec* codec = WebRtcVoiceCodecs::GetPreferredCodec(
-        codecs, &send_codec_spec.codec_inst);
-    if (!codec) {
-      LOG(LS_WARNING) << "Received empty list of codecs.";
-      return false;
-    }
-
-    send_codec_spec.transport_cc_enabled = HasTransportCc(*codec);
-    send_codec_spec.nack_enabled = HasNack(*codec);
-    bitrate_config_ = GetBitrateConfigForCodec(*codec);
-
-    // For Opus as the send codec, we are to determine inband FEC, maximum
-    // playback rate, and opus internal dtx.
-    if (IsCodec(*codec, kOpusCodecName)) {
-      GetOpusConfig(*codec, &send_codec_spec.codec_inst,
-                    &send_codec_spec.enable_codec_fec,
-                    &send_codec_spec.opus_max_playback_rate,
-                    &send_codec_spec.enable_opus_dtx,
-                    &send_codec_spec.min_ptime_ms,
-                    &send_codec_spec.max_ptime_ms);
-    }
+    const AudioCodec* voice_codec = nullptr;
+    for (const AudioCodec& codec : codecs) {
+      if (!(IsCodec(codec, kCnCodecName) ||
+            IsCodec(codec, kDtmfCodecName) ||
+            IsCodec(codec, kRedCodecName))) {
+        webrtc::SdpAudioFormat format(codec.name, codec.clockrate,
+                                      codec.channels,
+                                      CodecParameterMap(codec.params));
+
+        if (!engine()->encoder_factory_->IsSupportedEncoder(format)) {
+          LOG(LS_WARNING) << "Unknown codec " << ToString(codec);
+          continue;
+        }
 
-    // Set packet size if the AudioCodec param kCodecParamPTime is set.
-    int ptime_ms = 0;
-    if (codec->GetParam(kCodecParamPTime, &ptime_ms)) {
-      if (!WebRtcVoiceCodecs::SetPTimeAsPacketSize(
-          &send_codec_spec.codec_inst, ptime_ms)) {
-        LOG(LS_WARNING) << "Failed to set packet size for codec "
-                        << send_codec_spec.codec_inst.plname;
-        return false;
+        voice_codec = &codec;
+        send_codec_spec.payload_type = voice_codec->id;
+        send_codec_spec.format = format;
+        if (codec.bitrate > 0) {
+          send_codec_spec.target_bitrate_bps =
+              rtc::Optional<int>(codec.bitrate);
+        }
+        send_codec_spec.transport_cc_enabled = HasTransportCc(*voice_codec);
+        send_codec_spec.nack_enabled = HasNack(*voice_codec);
+        bitrate_config_ = GetBitrateConfigForCodec(*voice_codec);
+        break;
       }
     }
 
+    if (!voice_codec)
+      return false;
+
     // Loop through the codecs list again to find the CN codec.
     // TODO(solenberg): Break out into a separate function?
     for (const AudioCodec& codec : codecs) {
-      // Ignore codecs we don't know about. The negotiation step should prevent
-      // this, but double-check to be sure.
-      webrtc::CodecInst voe_codec = {0};
-      if (!WebRtcVoiceEngine::ToCodecInst(codec, &voe_codec)) {
-        LOG(LS_WARNING) << "Unknown codec " << ToString(codec);
-        continue;
-      }
-
-      if (IsCodec(codec, kCnCodecName)) {
+      if (IsCodec(codec, kCnCodecName) &&
+          codec.clockrate == voice_codec->clockrate) {
         // Turn voice activity detection/comfort noise on if supported.
         // Set the wideband CN payload type appropriately.
         // (narrowband always uses the static payload type 13).
-        int cng_plfreq = -1;
         switch (codec.clockrate) {
           case 8000:
           case 16000:
           case 32000:
-            cng_plfreq = codec.clockrate;
+            send_codec_spec.cng_payload_type = codec.id;
             break;
           default:
             LOG(LS_WARNING) << "CN frequency " << codec.clockrate
                             << " not supported.";
             continue;
         }
-        send_codec_spec.cng_payload_type = codec.id;
-        send_codec_spec.cng_plfreq = cng_plfreq;
         break;
       }
     }
 
     // Find the telephone-event PT exactly matching the preferred send codec.
     for (const AudioCodec& dtmf_codec : dtmf_codecs) {
-      if (dtmf_codec.clockrate == codec->clockrate) {
+      if (dtmf_codec.clockrate == voice_codec->clockrate) {
         dtmf_payload_type_ = rtc::Optional<int>(dtmf_codec.id);
         dtmf_payload_freq_ = dtmf_codec.clockrate;
         break;
@@ -2209,7 +1928,7 @@ bool WebRtcVoiceMediaChannel::AddSendStream(const StreamParams& sp) {
   WebRtcAudioSendStream* stream = new WebRtcAudioSendStream(
       channel, audio_transport, ssrc, sp.cname, send_codec_spec_,
       send_rtp_extensions_, max_send_bitrate_bps_, audio_network_adaptor_config,
-      call_, this);
+      call_, this, engine()->encoder_factory_);
   send_streams_.insert(std::make_pair(ssrc, stream));
 
   // At this point the stream's local SSRC has been updated. If it is the first