Injectable audio encoders: BuiltinAudioEncoderFactory

webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc

Index: webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc
diff --git a/webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc b/webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1d069007c7038cef25e5405acbd4fad866ac12f7
--- /dev/null
+++ b/webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc
@@ -0,0 +1,157 @@
+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.

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

+1

+ *
+ *  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/modules/audio_coding/codecs/builtin_audio_encoder_factory.h"
+
+#include <vector>
+
+#include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/base/optional.h"
+#include "webrtc/common_types.h"
+#include "webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h"
+#ifdef WEBRTC_CODEC_G722

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

Can you file an issue to deprecate these compiler flags?

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

How do we want this to work in future? Should our builtin one always include all
of these?
For those that want to pick and choose, they can create their own, but maybe
some are target dependent (like ISAC/ISAC_FIX)

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

Should we deprecate them? Isn't the idea to provide gn flags for basically every
built-in codec? If so, that would be implemented with preprocessor defines...

I'd love to have them defined to 1 or 0, though, instead of defined/undefined.

+#include "webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.h"
+#endif
+#ifdef WEBRTC_CODEC_ILBC
+#include "webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.h"
+#endif
+#ifdef WEBRTC_CODEC_ISACFX
+#include "webrtc/modules/audio_coding/codecs/isac/fix/include/audio_encoder_isacfix.h"
+#endif
+#ifdef WEBRTC_CODEC_ISAC
+#include "webrtc/modules/audio_coding/codecs/isac/main/include/audio_encoder_isac.h"
+#endif
+#ifdef WEBRTC_CODEC_OPUS
+#include "webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h"
+#endif
+#include "webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.h"
+
+namespace webrtc {
+
+namespace {
+
+struct NamedEncoderFactory {
+  const char *name;
+  rtc::Optional<AudioFormatInfo> (*QueryAudioFormat)(
+      const SdpAudioFormat& format);
+  std::unique_ptr<AudioEncoder> (*MakeAudioEncoder)(
+      int payload_type,
+      const SdpAudioFormat& format);
+
+  template <typename T>
+  static NamedEncoderFactory ForEncoder() {
+    auto constructor =
+        [](int payload_type, const SdpAudioFormat& format) {
+          auto opt_info = T::QueryAudioFormat(format);
+          if (opt_info) {

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

Why store the return value in a local variable?

+            return std::unique_ptr<AudioEncoder>(new T(payload_type, format));
+          }
+          return std::unique_ptr<AudioEncoder>();
+        };
+
+    return {T::GetPayloadName(), T::QueryAudioFormat, constructor};
+  }
+};
+
+NamedEncoderFactory encoder_factories[] = {
+#ifdef WEBRTC_CODEC_G722
+  NamedEncoderFactory::ForEncoder<AudioEncoderG722>(),
+#endif
+#ifdef WEBRTC_CODEC_ILBC
+  NamedEncoderFactory::ForEncoder<AudioEncoderIlbc>(),
+#endif
+#if defined(WEBRTC_CODEC_ISACFX)
+  NamedEncoderFactory::ForEncoder<AudioEncoderIsacFix>(),
+#elif defined(WEBRTC_CODEC_ISAC)
+  NamedEncoderFactory::ForEncoder<AudioEncoderIsac>(),
+#endif
+
+#ifdef WEBRTC_CODEC_OPUS
+  NamedEncoderFactory::ForEncoder<AudioEncoderOpus>(),
+#endif
+  NamedEncoderFactory::ForEncoder<AudioEncoderPcm16B>(),
+  NamedEncoderFactory::ForEncoder<AudioEncoderPcmA>(),
+  NamedEncoderFactory::ForEncoder<AudioEncoderPcmU>(),
+};
+}  // namespace
+
+class BuiltinAudioEncoderFactory : public AudioEncoderFactory {

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

Put this class too in the anonymous namespace?
Also, final? You even call one of your own virtual methods below...

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

It cannot be final, since it requires wrapping in rtc::RefCountedObject to
fulfill AddRef / Release.
Why does it matter that I'm calling one of my (possibly) virtual methods below?
Possibly avoiding an indirect call?

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

Ah, OK.
Yes---the primary benefit of "final" is to reassure the reader that there are no
subclasses, but the compiler can also use it to devirtualize function calls.

+ public:
+  BuiltinAudioEncoderFactory() {}

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

Is this necessary?

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

Probably not.

+
+  std::vector<AudioCodecSpec> GetSupportedEncoders() override {
+    return GetSupportedEncodersInternal();

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

Why delegate to a private method?

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

I believe I used the internal method at some point, since it returns a reference
rather than a copy. It's not necessary anymore, though.

+  }
+
+  bool IsSupportedEncoder(const SdpAudioFormat& format) override {
+    return !!QueryAudioFormat(format);

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

Ah, haha. What about just having one method then?

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

I'm considering whether there is enough semantic difference between the two to
keep them both. For correctness, they'll need to match 100% anyways, so I think
not.

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

+1 to not having two separate methods, both because it's one more than we need,
and because of the possibility of them not matching.

+  }
+
+  rtc::Optional<AudioFormatInfo> QueryAudioFormat(
+      const SdpAudioFormat& format) override {
+    for (const auto& ef : encoder_factories) {
+      if (STR_CASE_CMP(format.name.c_str(), ef.name) == 0) {
+        return ef.QueryAudioFormat(format);
+      }
+    }
+    return rtc::Optional<AudioFormatInfo>();
+  }
+
+  std::unique_ptr<AudioEncoder> MakeAudioEncoder(
+      int payload_type,
+      const SdpAudioFormat& format) override {
+    for (const auto& ef : encoder_factories) {
+      if (STR_CASE_CMP(format.name.c_str(), ef.name) == 0) {
+        return ef.MakeAudioEncoder(payload_type, format);
+      }
+    }
+    return nullptr;
+  }
+
+ private:
+  static const std::vector<AudioCodecSpec>& GetSupportedEncodersInternal() {
+    static const SdpAudioFormat desired_encoders[] = {
+      {"opus", 48000, 2, {{"minptime", "10"},
+                          {"useinbandfec", "1"}}},
+      {"isac", 16000, 1},
+      {"isac", 32000, 1},
+      {"G722", 8000, 1},
+      {"iLBC", 8000, 1},
+      {"PCMU", 8000, 1},
+      {"PCMA", 8000, 1},
+    };

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

Move this into the lambda?

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

I can't honestly tell you why it is reachable by the lambda at this point? Seems
like its staticness, but I'm unsure if this is correct or just happens to work.
I kind-of like it outside the lambda, though. I find it easier to read when
separate from the code that uses it.

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

Yeah, you're right, that's surprising---I wouldn't have thought it'd be
available.
OK. It's not as if bringing it inside the lambda would reduce its scope a whole
lot anyway...

+
+    // Although this looks a bit strange, it means specs need only be
+    // initialized once, and that that initialization is thread-safe.

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

That static variables are initialized once, on first use is sufficiently
well-known, I think. But if you must say something, just

  // This is initialized once, on first use.

will do. That it's thread safe goes without saying, because if it weren't, it
would be broken.

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

Sure, it would be broken if it weren't, but I'm not sure that's well known
enough that it doesn't warrant a comment. I think this is a relatively new
construct in our code-base, so if someone isn't sure, it's good they don't have
to spend time researching it. I'll try to slim down the comment a bit.

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

Acknowledged.

+    static std::vector<AudioCodecSpec> supported_encoders = [] {

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

const? For thread safety...

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

Is that more thread-safe?

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

Well, yes---initialization of mutable statics is thread safe, but no one is
taking care of protecting the mutating operations for you. Now, you don't happen
to actually make any mutating operations, but unless you say "const" here, the
compiler won't hold you to that.

+      std::vector<AudioCodecSpec> supported_encoders;

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

Call this something else, since shadowing is confusing? Maybe even something
short?

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

The shadowing here is intended, but perhaps unnecessary. My intention was that
nothing would manage to refer to the outer supported_encoders, but the empty
lambda capture should've seen to that. I've shortened the name a bit, though,
and moved it all up into GetSupportedEncoders.

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

Acknowledged.

+      for (const auto& format : desired_encoders) {
+        for (const auto& ef : encoder_factories) {
+          if (STR_CASE_CMP(format.name.c_str(), ef.name) == 0) {
+            auto opt_info = ef.QueryAudioFormat(format);
+            if (opt_info) {
+              supported_encoders.push_back({format, *opt_info});
+            }
+          }
+        }
+      }
+      return supported_encoders;
+    }();
+    return supported_encoders;
+  }
+};
+
+rtc::scoped_refptr<AudioEncoderFactory> CreateBuiltinAudioEncoderFactory() {
+  return rtc::scoped_refptr<AudioEncoderFactory>(
+      new rtc::RefCountedObject<BuiltinAudioEncoderFactory>());
+}
+

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

I'm missing a unit test for this class.

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

I'll add a few generic tests that could be re-used by other implementations,
like:
- Its list of supported encoders isn't empty
- The encoders it claims to support are all queryable (and the values make
sense)
- All those encoders are also constructable and can deal with simple calls, like
Reset, and the values from GetSampleRateHz etc. match those we get from querying
the encoder.

I'll also make specific test for this class, to ensure that the compilation
flags are picked up properly.

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

Acknowledged.

+}  // namespace webrtc