Opus implementation of the AudioEncoderFactoryTemplate API

webrtc/api/audio_codecs/opus/audio_encoder_opus_config.cc

+/*
+ *  Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/api/audio_codecs/opus/audio_encoder_opus_config.h"
+
+namespace webrtc {
+
+namespace {
+
+#if defined(WEBRTC_ANDROID) || defined(WEBRTC_IOS) || defined(WEBRTC_ARCH_ARM)
+// If we are on Android, iOS and/or ARM, use a lower complexity setting by

[ossu, 2017/06/16 09:53:53]

Is this comment copied from somewhere? Otherwise, using a lower complexity to
save complexity doesn't really say much. :)
"use a lower complexity setting by default, to save CPU time.", maybe?

[kwiberg-webrtc, 2017/06/16 12:44:14]

Yes, it's a copy. :-) And yes, it's a tautology---"complexity" just means the
amount of computing to be done.

Fixed.

+// default, to save encoder complexity.
+constexpr int kDefaultComplexity = 5;
+#else
+constexpr int kDefaultComplexity = 9;
+#endif
+
+constexpr int kDefaultLowRateComplexity =
+    WEBRTC_OPUS_VARIABLE_COMPLEXITY ? 9 : kDefaultComplexity;
+
+}  // namespace
+
+constexpr int AudioEncoderOpusConfig::kDefaultFrameSizeMs;
+constexpr int AudioEncoderOpusConfig::kMinBitrateBps;
+constexpr int AudioEncoderOpusConfig::kMaxBitrateBps;
+
+AudioEncoderOpusConfig::AudioEncoderOpusConfig()
+    : frame_size_ms(kDefaultFrameSizeMs),

[ossu, 2017/06/16 09:53:53]

Would it be preferable to have these directly in the header instead? That way,
if you want to customize the Opus codec by implementing the static interface
yourself, you'd easily see what the defaults were.

[the sun, 2017/06/16 12:44:10]

+1

[kwiberg-webrtc, 2017/06/16 12:44:14]

The defaults used to be in the header, but I moved them here to reduce clutter
(this is a problem especially for the #ifdeffed stuff). I figured that anyone
who wants to see the defaults can just look here.

[ossu, 2017/06/16 13:18:27]

Alright. I guess if you're going to modify the parameters to an encoder, looking
in the .cc file shouldn't be too much of a hindrance. :)

+      num_channels(1),
+      application(ApplicationMode::kVoip),
+      bitrate_bps(32000),
+      fec_enabled(false),
+      cbr_enabled(false),
+      max_playback_rate_hz(48000),
+      complexity(kDefaultComplexity),
+      low_rate_complexity(kDefaultLowRateComplexity),
+      complexity_threshold_bps(12500),
+      complexity_threshold_window_bps(1500),
+      dtx_enabled(false),
+      uplink_bandwidth_update_interval_ms(200) {}
+AudioEncoderOpusConfig::AudioEncoderOpusConfig(const AudioEncoderOpusConfig&) =
+    default;
+AudioEncoderOpusConfig::~AudioEncoderOpusConfig() = default;
+AudioEncoderOpusConfig& AudioEncoderOpusConfig::operator=(
+    const AudioEncoderOpusConfig&) = default;
+
+bool AudioEncoderOpusConfig::IsOk() const {
+  if (frame_size_ms <= 0 || frame_size_ms % 10 != 0)

[ossu, 2017/06/16 09:53:53]

Is this strictly true, though? Eh, nvm: is this how it's implemented right now?
:)

According to RTC 7587: "Possible values are 3, 5, 10, 20, 40, 60, or an
arbitrary
      multiple of an Opus frame size rounded up to the next full integer
      value, up to a maximum value of 120, as defined in Section 4."
Not sure we could reasonably deal with < 10ms, though, since that's the block
size used internally.

[kwiberg-webrtc, 2017/06/16 12:44:14]

Supporting < 10 ms frames would be difficult, yes.

I have a feeling that you're right, and this condition is a bit too permissive.
However, I've just copied it from the old Opus AudioEncoder config struct;
changing it should probably be a separate CL.

[ossu, 2017/06/16 13:18:27]

Agreed.

+    return false;
+  if (num_channels != 1 && num_channels != 2)
+    return false;
+  if (bitrate_bps < kMinBitrateBps || bitrate_bps > kMaxBitrateBps)
+    return false;
+  if (complexity < 0 || complexity > 10)
+    return false;
+  if (low_rate_complexity < 0 || low_rate_complexity > 10)
+    return false;
+  return true;
+}
+}  // namespace webrtc