Injectable audio encoders: WebRtcVoiceEngine and company

webrtc/call/audio_send_stream.cc

Index: webrtc/call/audio_send_stream.cc
diff --git a/webrtc/call/audio_send_stream.cc b/webrtc/call/audio_send_stream.cc
index 8b6dd9e416fc025f619aefb139e9732f334e2cb5..28638cac2cf98b422f0c50d069ea73d9cd7da99f 100644
--- a/webrtc/call/audio_send_stream.cc
+++ b/webrtc/call/audio_send_stream.cc
@@ -12,21 +12,6 @@
 
 #include <string>
 
-namespace {
-
-std::string ToString(const webrtc::CodecInst& codec_inst) {
-  std::stringstream ss;
-  ss << "{pltype: " << codec_inst.pltype;
-  ss << ", plname: \"" << codec_inst.plname << "\"";
-  ss << ", plfreq: " << codec_inst.plfreq;
-  ss << ", pacsize: " << codec_inst.pacsize;
-  ss << ", channels: " << codec_inst.channels;
-  ss << ", rate: " << codec_inst.rate;
-  ss << '}';
-  return ss.str();
-}
-}  // namespace
-
 namespace webrtc {
 
 AudioSendStream::Stats::Stats() = default;
@@ -70,24 +55,18 @@ std::string AudioSendStream::Config::Rtp::ToString() const {
   return ss.str();
 }
 
-AudioSendStream::Config::SendCodecSpec::SendCodecSpec() {
-  webrtc::CodecInst empty_inst = {0};
-  codec_inst = empty_inst;
-  codec_inst.pltype = -1;
-}
+AudioSendStream::Config::SendCodecSpec::SendCodecSpec()
+    : format("", 0, 0) {}

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

Why have a default constructor for this type? rtc::Optional<SendCodecSpec> seems
like a more natural choice.

But we usually default-construct these and then set one field at a time, don't
we... ick.

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

That's the big problem, currently. Eventually, the SendCodecSpec could be
created from, say, an SdpAudioFormat and optionally a CNG payload id, and then
the rest of the fields could be filled in. In the current implementation, the
WebRtcAudioSendStream wrapper uses a SendCodecSpec and gets called repeatedly
with different parts of the information, and not necessarily in a reasonable
order. I'll fix that at some point, but not right now.

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

Seems I didn't really understand the flow previously. Now it seems to me
creating a stream before a SendCodecSpec can be constructed is completely
reasonable. I've made it optional and made its constructor better.

+
+AudioSendStream::Config::SendCodecSpec::~SendCodecSpec() = default;
 
 std::string AudioSendStream::Config::SendCodecSpec::ToString() const {
   std::stringstream ss;
   ss << "{nack_enabled: " << (nack_enabled ? "true" : "false");
   ss << ", transport_cc_enabled: " << (transport_cc_enabled ? "true" : "false");
-  ss << ", enable_codec_fec: " << (enable_codec_fec ? "true" : "false");
-  ss << ", enable_opus_dtx: " << (enable_opus_dtx ? "true" : "false");
-  ss << ", opus_max_playback_rate: " << opus_max_playback_rate;
   ss << ", cng_payload_type: " << cng_payload_type;
-  ss << ", cng_plfreq: " << cng_plfreq;
-  ss << ", min_ptime: " << min_ptime_ms;
-  ss << ", max_ptime: " << max_ptime_ms;
-  ss << ", codec_inst: " << ::ToString(codec_inst);
+  ss << ", payload_type: " << payload_type;
+  ss << ", format: " << format;
   ss << '}';
   return ss.str();
 }
@@ -96,12 +75,9 @@ bool AudioSendStream::Config::SendCodecSpec::operator==(
     const AudioSendStream::Config::SendCodecSpec& rhs) const {
   if (nack_enabled == rhs.nack_enabled &&
       transport_cc_enabled == rhs.transport_cc_enabled &&
-      enable_codec_fec == rhs.enable_codec_fec &&
-      enable_opus_dtx == rhs.enable_opus_dtx &&
-      opus_max_playback_rate == rhs.opus_max_playback_rate &&
       cng_payload_type == rhs.cng_payload_type &&
-      cng_plfreq == rhs.cng_plfreq && max_ptime_ms == rhs.max_ptime_ms &&
-      min_ptime_ms == rhs.min_ptime_ms && codec_inst == rhs.codec_inst) {
+      payload_type == rhs.payload_type && format == rhs.format &&
+      target_bitrate_bps == rhs.target_bitrate_bps) {
     return true;
   }
   return false;