Injectable audio encoders: BuiltinAudioEncoderFactory

webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc

Index: webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
diff --git a/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc b/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
index bac963f4e8e2de9ff047ac78d889e9b187c6469e..f92033c064466a587cc1c8ce61e4cd2aa5b66114 100644
--- a/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
+++ b/webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
@@ -10,13 +10,17 @@
 
 #include "webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h"
 
+#include <stdlib.h>
+
 #include <algorithm>
 #include <iterator>
 
+#include "webrtc/base/arraysize.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/numerics/exp_filter.h"
 #include "webrtc/base/safe_conversions.h"
+#include "webrtc/base/string_to_number.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/common_types.h"
 #include "webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h"
@@ -28,39 +32,41 @@ namespace webrtc {
 
 namespace {
 
-constexpr int kSampleRateHz = 48000;
+// 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 API allows a min bitrate of 500bps, but Opus documentation suggests
-// a minimum bitrate of 6kbps.
-constexpr int kMinBitrateBps = 6000;
+// bitrate should be in the range between 6000 and 510000.

[the sun, 2017/03/14 20:48:29]

nit: "between" excludes 6000 and 510000 - but below (line 149) you include them

[kwiberg-webrtc, 2017/03/15 10:09:14]

Well, I'd say it *sometimes* excludes the endpoints, but that there's no hard
rule---which is at least as bad, of course.

I would say "in the range [6000, 510000]" or maybe "between 6000 and 510000,
inclusive". The mathematical notation has the advantage of being both short and
precise, and is probably well-known enough.

[ossu, 2017/03/15 11:26:52]

I don't interpret it as excluding. Quick, pick a number between one and five!
I'll make the comment clearer, though, by explicitly mentioning it being
inclusive.

[the sun, 2017/03/15 11:57:40]

I'd actually say "from 1 to 5". :) Use the interval notation - that's
unambiguous! 

[kwiberg-webrtc, 2017/03/15 13:33:18]

Exactly.

[kwiberg-webrtc, 2017/03/15 13:33:18]

"[...] the bitrate should be an integer n such that 6000 ≤ n ≤ 510000". :-)

[ossu, 2017/03/16 18:03:58]

This has got to be the biggest bike-shed in quite a while! :)

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

Well, fortunately I was just pulling your leg. I think we reached consensus on
the [6000, 510000] notation. :-)

+const int kOpusMinBitrateBps = 6000;
+const int kOpusMaxBitrateBps = 510000;

[ossu, 2017/03/14 20:25:11]

I removed the old kMin/MaxBitrateBps and replaced with these, that originally
come from webrtcvoiceengine.cc. This means I also changed the max to 510 kbps,
instead of 512 kbps. From a quick web search, I can only find references to 510
kbps as being the max for Opus.

[kwiberg-webrtc, 2017/03/15 13:33:18]

Make these five constexpr?

[hlundin-webrtc, 2017/03/16 12:16:31]

Looks good. This is in line with the documentation in opus source:
https://git.xiph.org/?p=opus.git;a=blob;f=include/opus.h;h=5be73ddf4e5dd2e0cd....

 
-constexpr int kMaxBitrateBps = 512000;
+constexpr int kSampleRateHz = 48000;
 
+// These two lists must be sorted from low to high
 #if WEBRTC_OPUS_SUPPORT_120MS_PTIME
-constexpr int kSupportedFrameLengths[] = {20, 60, 120};
+constexpr int kANASupportedFrameLengths[] = {20, 60, 120};
+constexpr int kOpusSupportedFrameLengths[] = {10, 20, 40, 60, 120};
 #else
-constexpr int kSupportedFrameLengths[] = {20, 60};
+constexpr int kANASupportedFrameLengths[] = {20, 60};
+constexpr int kOpusSupportedFrameLengths[] = {10, 20, 40, 60};
 #endif
 
 // PacketLossFractionSmoother uses an exponential filter with a time constant
 // of -1.0 / ln(0.9999) = 10000 ms.
 constexpr float kAlphaForPacketLossFractionSmoother = 0.9999f;
 
-AudioEncoderOpus::Config CreateConfig(const CodecInst& codec_inst) {
-  AudioEncoderOpus::Config config;
-  config.frame_size_ms = rtc::CheckedDivExact(codec_inst.pacsize, 48);
-  config.num_channels = codec_inst.channels;
-  config.bitrate_bps = rtc::Optional<int>(codec_inst.rate);
-  config.payload_type = codec_inst.pltype;
-  config.application = config.num_channels == 1 ? AudioEncoderOpus::kVoip
-                                                : AudioEncoderOpus::kAudio;
-  config.supported_frame_lengths_ms.push_back(config.frame_size_ms);
-#if WEBRTC_OPUS_VARIABLE_COMPLEXITY
-  config.low_rate_complexity = 9;
-#endif
-  return config;
-}
-
 // Optimize the loss rate to configure Opus. Basically, optimized loss rate is
 // the input loss rate rounded down to various levels, because a robustly good
 // audio quality is achieved by lowering the packet loss down.
@@ -101,8 +107,200 @@ float OptimizePacketLossRate(float new_loss_rate, float old_loss_rate) {
   }
 }
 
+rtc::Optional<std::string> GetFormatParameter(const SdpAudioFormat& format,
+                                              const std::string& param) {

[kwiberg-webrtc, 2017/03/15 13:33:18]

Change this to return a const char* that points to the string?

[ossu, 2017/03/16 18:03:57]

I looked at it, but it makes a number of things less nice, like having to
explicitly type std::string("1") in the comparisons, or do nasty strcmp calls.
Ideally, we'd have a StringPiece like what chromium does, to ferry around string
data with very low cost.

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

Hmm. std::string::operator== accepts const char* arguments. Are you talking
about comparing two const char*?
Yes.

+  auto it = format.parameters.find(param);
+  return (it == format.parameters.end())
+             ? rtc::Optional<std::string>()
+             : rtc::Optional<std::string>(it->second);
+}
+
+template <typename T>
+rtc::Optional<T> GetFormatParameter(const SdpAudioFormat& format,
+                                    const std::string& param) {
+  return rtc::StringToNumber<T>(GetFormatParameter(format, param).value_or(""));
+};
+
+int CalculateDefaultBitrate(int max_playback_rate, int num_channels) {
+  int bitrate = 0;

[kwiberg-webrtc, 2017/03/15 13:33:18]

Ick. Default value that's never used, mutable variable... lambda?

[ossu, 2017/03/16 18:03:57]

It's not that bad :)

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

Hmm.

  const int bitrate =
      num_channels * (max_playback_rate <= 8000
                          ? kOpusBitrateNbBps
                          : max_playback_rate <= 16000 ? kOpusBitrateWbBps
                                                       : kOpusBitrateFbBps);

If you don't like that either, I've been toying with the idea of defining a
function that returns a value unchanged, but msan-poisoned. So this line would
become something like

  int bitrate = rtc::MsanUninitialized(0);

which would behave exactly like the current code in normal builds, but
additionally (1) clearly signal that the author thinks the value will always be
overwritten, and (2) let MSan verify this.

+  if (max_playback_rate <= 8000) {
+    bitrate = kOpusBitrateNbBps * num_channels;
+  } else if (max_playback_rate <= 16000) {
+    bitrate = kOpusBitrateWbBps * num_channels;
+  } else {
+    bitrate = kOpusBitrateFbBps * num_channels;
+  }
+  RTC_DCHECK_GE(bitrate, kOpusMinBitrateBps);
+  RTC_DCHECK_LE(bitrate, kOpusMaxBitrateBps);
+  return bitrate;
+}
+
+// Get the maxaveragebitrate parameter in string-form, so we can properly figure
+// out how invalid it is and accurately log invalid values.
+int CalculateBitrate(int max_playback_rate_hz,
+                     int num_channels,
+                     rtc::Optional<std::string> bitrate_param) {

[kwiberg-webrtc, 2017/03/15 13:33:18]

Make bitrate_param a const char*?

[ossu, 2017/03/16 18:03:58]

I've kept this as-is since it'll interface directly with GetFormatParameter,
above.

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

Acknowledged.

+  const int default_bitrate =
+      CalculateDefaultBitrate(max_playback_rate_hz, num_channels);
+
+  if (bitrate_param) {

[kwiberg-webrtc, 2017/03/15 13:33:18]

Invert condition + early return?

[ossu, 2017/03/16 18:03:58]

No. I like this flow. There's only one way we can return the default bitrate,
and that's if we've no usable maxaveragebitrate. The nesting is only a couple of
levels.

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

Acknowledged.

+    const auto bitrate = rtc::StringToNumber<int>(*bitrate_param);
+    if (bitrate) {

[kwiberg-webrtc, 2017/03/15 13:33:18]

Invert condition + early return?

[ossu, 2017/03/16 18:03:58]

No, see above.

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

Acknowledged.

+      if (*bitrate >= kOpusMinBitrateBps && *bitrate <= kOpusMaxBitrateBps) {
+        return *bitrate;
+      }
+
+      const int new_bitrate = (*bitrate < kOpusMinBitrateBps)
+                                  ? kOpusMinBitrateBps
+                                  : kOpusMaxBitrateBps;
+      LOG(LS_WARNING) << "Invalid maxaveragebitrate " << *bitrate
+                      << " clamped to " << new_bitrate;
+      return new_bitrate;

[kwiberg-webrtc, 2017/03/15 13:33:18]

Don't we have at least one library function to do clamping?

[ossu, 2017/03/16 18:03:57]

None that I can find. We probably should have one that looks like std::clamp.
Many places could be cleaned up by this.

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

You're right, we actually don't appear to have one. :-(

+    }
+
+    LOG(LS_WARNING) << "Invalid maxaveragebitrate \"" << *bitrate_param
+                    << "\" replaced by default bitrate " << default_bitrate;
+  }
+
+  return default_bitrate;
+}
+
+rtc::Optional<int> GetChannelCount(const SdpAudioFormat& format) {
+  const auto param = GetFormatParameter(format, "stereo");
+  if (!param || param == "0") {
+    return rtc::Optional<int>(1);
+  } else if (param == "1") {
+    return rtc::Optional<int>(2);
+  }
+  return rtc::Optional<int>();
+}
+
+rtc::Optional<int> GetMaxPlaybackRate(const SdpAudioFormat& format) {
+  const auto param = GetFormatParameter(format, "maxplaybackrate");
+  if (!param) {
+    return rtc::Optional<int>(48000);
+  }
+  const auto parsed = rtc::StringToNumber<int>(*param);
+  if (parsed && *parsed >= 8000) {
+    return rtc::Optional<int>(*parsed);
+  }
+  return rtc::Optional<int>();
+}
+
 }  // namespace
 
+rtc::Optional<AudioFormatInfo> AudioEncoderOpus::QueryAudioFormat(
+    const SdpAudioFormat& format) {
+  if (STR_CASE_CMP(format.name.c_str(), GetPayloadName()) == 0 &&
+      format.clockrate_hz == 48000 && format.num_channels == 2) {

[kwiberg-webrtc, 2017/03/15 13:33:18]

Invert condition + early return?

[ossu, 2017/03/16 18:03:58]

As with the other one, I prefer having the error/fallback return at the end,
rather than spread throughout the code.

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

Acknowledged.

+
+    const rtc::Optional<int> num_channels = GetChannelCount(format);
+    const rtc::Optional<int> max_playback_rate = GetMaxPlaybackRate(format);
+    if (num_channels && max_playback_rate) {
+      const int bitrate =
+          CalculateBitrate(*max_playback_rate, *num_channels,
+                           GetFormatParameter(format, "maxaveragebitrate"));
+      AudioFormatInfo info(48000, *num_channels, bitrate, kOpusMinBitrateBps,
+                           kOpusMaxBitrateBps);
+      info.allow_comfort_noise = false;
+      info.supports_network_adaption = true;
+
+      return rtc::Optional<AudioFormatInfo>(info);
+    }
+  }
+  return rtc::Optional<AudioFormatInfo>();
+}
+
+AudioEncoderOpus::Config AudioEncoderOpus::CreateConfig(
+    const CodecInst& codec_inst) {
+  AudioEncoderOpus::Config config;
+  config.frame_size_ms = rtc::CheckedDivExact(codec_inst.pacsize, 48);
+  config.num_channels = codec_inst.channels;
+  config.bitrate_bps = rtc::Optional<int>(codec_inst.rate);
+  config.payload_type = codec_inst.pltype;
+  config.application = config.num_channels == 1 ? AudioEncoderOpus::kVoip
+                                                : AudioEncoderOpus::kAudio;
+  config.supported_frame_lengths_ms.push_back(config.frame_size_ms);
+#if WEBRTC_OPUS_VARIABLE_COMPLEXITY
+  config.low_rate_complexity = 9;
+#endif
+  return config;
+}
+
+AudioEncoderOpus::Config AudioEncoderOpus::CreateConfig(
+    int payload_type,
+    const SdpAudioFormat& format) {
+  AudioEncoderOpus::Config config;
+
+  // Normally, the first two parameters should already have been checked buy

[kwiberg-webrtc, 2017/03/15 13:33:18]

by

[ossu, 2017/03/16 18:03:57]

Acknowledged.

+  // QueryAudioFormat At this point, we might as well fall back to something

[kwiberg-webrtc, 2017/03/15 13:33:18]

.

[ossu, 2017/03/16 18:03:58]

Acknowledged.

+  // reasonable.
+  config.num_channels = GetChannelCount(format).value_or(1);
+  config.max_playback_rate_hz = GetMaxPlaybackRate(format).value_or(48000);
+  config.fec_enabled = (GetFormatParameter(format, "useinbandfec") == "1");
+  config.dtx_enabled = (GetFormatParameter(format, "usedtx") == "1");
+  config.bitrate_bps = rtc::Optional<int>(
+      CalculateBitrate(config.max_playback_rate_hz, config.num_channels,
+                       GetFormatParameter(format, "maxaveragebitrate")));
+  config.payload_type = payload_type;
+  config.application = config.num_channels == 1 ? AudioEncoderOpus::kVoip
+                                                : AudioEncoderOpus::kAudio;
+#if WEBRTC_OPUS_VARIABLE_COMPLEXITY
+  config.low_rate_complexity = 9;
+#endif
+
+  constexpr int kMinSupportedFrameLength = kOpusSupportedFrameLengths[0];
+  constexpr int kMaxSupportedFrameLength =
+      kOpusSupportedFrameLengths[arraysize(kOpusSupportedFrameLengths) - 1];

[kwiberg-webrtc, 2017/03/15 13:33:18]

Maybe

  *(std::end(kOpusSupportedFrameLengths) - 1)

[ossu, 2017/03/16 18:03:57]

Tried it but the compiler claims thats's not constexpr. Looks like it might be
in newer versions of the standard.

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

Ah well.

+
+  const auto ptime = GetFormatParameter<int>(format, "ptime");
+  if (ptime) {
+    // Pick the next highest supported frame length from
+    // kOpusSupportedFrameLengths. Default to the largest, if we find none.
+    config.frame_size_ms = kMaxSupportedFrameLength;
+    for (const int supported_frame_length : kOpusSupportedFrameLengths) {
+      if (supported_frame_length >= *ptime) {
+        config.frame_size_ms = supported_frame_length;
+        break;
+      }
+    }
+  }
+
+  // For now, minptime and maxptime are only used with ANA. If ptime is outside
+  // of this range, it will get adjusted once ANA takes hold. Ideally, we'd now

[kwiberg-webrtc, 2017/03/15 13:33:18]

know

[ossu, 2017/03/16 18:03:57]

Maan.. I was typing in a hurry last night!

+  // if ANA was to be used when setting up the config, and adjust accordingly.
+  const int min_frame_length_ms =
+      std::min(std::max(GetFormatParameter<int>(format, "minptime")
+                            .value_or(kMinSupportedFrameLength),
+                        kMinSupportedFrameLength),
+               kMaxSupportedFrameLength);
+  const int max_frame_length_ms =
+      std::min(std::max(GetFormatParameter<int>(format, "maxptime")
+                            .value_or(kMaxSupportedFrameLength),
+                        kMinSupportedFrameLength),
+               kMaxSupportedFrameLength);
+  if (min_frame_length_ms <= max_frame_length_ms) {

[kwiberg-webrtc, 2017/03/15 13:33:18]

This conditional is just an optimization; since this isn't performance critical,
remove it.

[ossu, 2017/03/16 18:03:57]

Acknowledged.

+    for (const int frame_length_ms : kANASupportedFrameLengths) {
+      if (frame_length_ms >= min_frame_length_ms &&
+          frame_length_ms <= max_frame_length_ms) {
+        config.supported_frame_lengths_ms.push_back(frame_length_ms);
+      }
+    }
+  }
+
+  // As a fallback, just pick the whole set of supported frame lengths.
+  if (config.supported_frame_lengths_ms.empty()) {
+    for (const int frame_length_ms : kANASupportedFrameLengths) {
+      config.supported_frame_lengths_ms.push_back(frame_length_ms);

[kwiberg-webrtc, 2017/03/15 13:33:18]

std::vector::assign?

[ossu, 2017/03/16 18:03:58]

I find it a bit less clear what's going on (for-loops are common,
std::vector::assign isn't) but it might be slightly faster - not that that
should matter at this point. They're both a bit messy.

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

Acknowledged.

+    }
+  }
+
+  RTC_DCHECK(std::is_sorted(config.supported_frame_lengths_ms.begin(),
+                            config.supported_frame_lengths_ms.end()));
+
+  return config;
+}
+
 class AudioEncoderOpus::PacketLossFractionSmoother {
  public:
   explicit PacketLossFractionSmoother(const Clock* clock)
@@ -147,7 +345,7 @@ bool AudioEncoderOpus::Config::IsOk() const {
   if (num_channels != 1 && num_channels != 2)
     return false;
   if (bitrate_bps &&
-      (*bitrate_bps < kMinBitrateBps || *bitrate_bps > kMaxBitrateBps))
+      (*bitrate_bps < kOpusMinBitrateBps || *bitrate_bps > kOpusMaxBitrateBps))
     return false;
   if (complexity < 0 || complexity > 10)
     return false;
@@ -208,6 +406,10 @@ AudioEncoderOpus::AudioEncoderOpus(
 AudioEncoderOpus::AudioEncoderOpus(const CodecInst& codec_inst)
     : AudioEncoderOpus(CreateConfig(codec_inst), nullptr) {}
 
+AudioEncoderOpus::AudioEncoderOpus(int payload_type,
+                                   const SdpAudioFormat& format)
+    : AudioEncoderOpus(CreateConfig(payload_type, format), nullptr) {}
+
 AudioEncoderOpus::~AudioEncoderOpus() {
   RTC_CHECK_EQ(0, WebRtcOpus_EncoderFree(inst_));
 }
@@ -335,8 +537,8 @@ void AudioEncoderOpus::OnReceivedUplinkBandwidth(
     const int overhead_bps = static_cast<int>(
         *overhead_bytes_per_packet_ * 8 * 100 / Num10MsFramesInNextPacket());
     SetTargetBitrate(std::min(
-        kMaxBitrateBps,
-        std::max(kMinBitrateBps, target_audio_bitrate_bps - overhead_bps)));
+        kOpusMaxBitrateBps,
+        std::max(kOpusMinBitrateBps, target_audio_bitrate_bps - overhead_bps)));
   } else {
     SetTargetBitrate(target_audio_bitrate_bps);
   }
@@ -367,8 +569,8 @@ void AudioEncoderOpus::SetReceiverFrameLengthRange(int min_frame_length_ms,
   RTC_DCHECK(!audio_network_adaptor_);
 
   config_.supported_frame_lengths_ms.clear();
-  std::copy_if(std::begin(kSupportedFrameLengths),
-               std::end(kSupportedFrameLengths),
+  std::copy_if(std::begin(kANASupportedFrameLengths),
+               std::end(kANASupportedFrameLengths),
                std::back_inserter(config_.supported_frame_lengths_ms),
                [&](int frame_length_ms) {
                  return frame_length_ms >= min_frame_length_ms &&
@@ -507,8 +709,8 @@ void AudioEncoderOpus::SetProjectedPacketLossRate(float fraction) {
 }
 
 void AudioEncoderOpus::SetTargetBitrate(int bits_per_second) {
-  config_.bitrate_bps = rtc::Optional<int>(
-      std::max(std::min(bits_per_second, kMaxBitrateBps), kMinBitrateBps));
+  config_.bitrate_bps = rtc::Optional<int>(std::max(
+      std::min(bits_per_second, kOpusMaxBitrateBps), kOpusMinBitrateBps));
   RTC_DCHECK(config_.IsOk());
   RTC_CHECK_EQ(0, WebRtcOpus_SetBitRate(inst_, config_.GetBitrateBps()));
   const auto new_complexity = config_.GetNewComplexity();
@@ -549,7 +751,7 @@ AudioEncoderOpus::DefaultAudioNetworkAdaptorCreator(
       config,
       ControllerManagerImpl::Create(
           config_string, NumChannels(), supported_frame_lengths_ms(),
-          kMinBitrateBps, num_channels_to_encode_, next_frame_length_ms_,
+          kOpusMinBitrateBps, num_channels_to_encode_, next_frame_length_ms_,
           GetTargetBitrate(), config_.fec_enabled, GetDtx(), clock)));
 }