New interface (AudioDecoderFactory), with an implementation

webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc

+/*
+ *  Copyright (c) 2016 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/modules/audio_coding/codecs/builtin_audio_decoder_factory.h"
+
+#include <vector>
+
+#include "webrtc/base/checks.h"
+#include "webrtc/common_types.h"
+#include "webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h"
+#include "webrtc/modules/audio_coding/codecs/g711/audio_decoder_pcm.h"
+#ifdef WEBRTC_CODEC_G722
+#include "webrtc/modules/audio_coding/codecs/g722/audio_decoder_g722.h"
+#endif
+#ifdef WEBRTC_CODEC_ILBC
+#include "webrtc/modules/audio_coding/codecs/ilbc/audio_decoder_ilbc.h"
+#endif
+#ifdef WEBRTC_CODEC_ISACFX
+#include "webrtc/modules/audio_coding/codecs/isac/fix/include/audio_decoder_isacfix.h"
+#endif
+#ifdef WEBRTC_CODEC_ISAC
+#include "webrtc/modules/audio_coding/codecs/isac/main/include/audio_decoder_isac.h"
+#endif
+#ifdef WEBRTC_CODEC_OPUS
+#include "webrtc/modules/audio_coding/codecs/opus/audio_decoder_opus.h"
+#endif
+#include "webrtc/modules/audio_coding/codecs/pcm16b/audio_decoder_pcm16b.h"
+
+namespace webrtc {
+
+namespace {
+
+struct NamedDecoderConstructor {
+  const char* name;
+  std::unique_ptr<AudioDecoder> (*constructor)(int clockrate_hz,
+                                               int num_channels);
+};
+
+std::unique_ptr<AudioDecoder> Uniq(AudioDecoder* d) {

[hlundin-webrtc, 2016/04/27 14:35:56]

Did you mean to name it Unique, or were you paying tribute to the
linux/unix-inspired Uniq command?

[the sun, 2016/04/27 20:34:15]

I think the name is confusing because of that connotation too.

How about returning naked AudioDecoder*s from the constructor lambdas, since
you're already wrapping the result in a unique_ptr?

[kwiberg-webrtc, 2016/04/28 12:35:52]

I was initially going to name it U, but I foresaw lots of reviewer grief in that
future, so I chose a slightly longer name. :-) I'll change it to Unique.

Yes, I could return raw pointers, but (1) that would violate the rule about
passing ownership in raw pointers, if only locally, but (2) I'm planning to
replace all these lambdas with factory functions owned by each decoder, so the
violation would no longer be local and therefore much worse.

[the sun, 2016/04/28 13:02:42]

Ok, (2) makes it make sense.

+  return std::unique_ptr<AudioDecoder>(d);
+}
+
+// TODO(kwiberg): These factory functions should probably be moved to each
+// decoder.
+NamedDecoderConstructor decoder_constructors[] = {
+    {"pcmu",
+     [](int clockrate_hz, int num_channels) {

[the sun, 2016/04/27 20:34:14]

nit: Indent 1 looks weird - did git cl format do this?

[kwiberg-webrtc, 2016/04/28 12:35:52]

Yes. And it's not weird; it's how multiline array and struct literals are
usually formatted:

  {1,
   2,
   3}

+       return clockrate_hz == 8000 && num_channels >= 1
+                  ? Uniq(new AudioDecoderPcmU(num_channels))
+                  : nullptr;
+     }},
+    {"pcma",
+     [](int clockrate_hz, int num_channels) {
+       return clockrate_hz == 8000 && num_channels >= 1
+                  ? Uniq(new AudioDecoderPcmA(num_channels))
+                  : nullptr;
+     }},
+#ifdef WEBRTC_CODEC_ILBC
+    {"ilbc",
+     [](int clockrate_hz, int num_channels) {
+       return clockrate_hz == 8000 && num_channels == 1
+                  ? Uniq(new AudioDecoderIlbc)
+                  : nullptr;
+     }},
+#endif
+#if defined(WEBRTC_CODEC_ISACFX)
+    {"isac",
+     [](int clockrate_hz, int num_channels) {
+       return clockrate_hz == 16000 && num_channels == 1
+                  ? Uniq(new AudioDecoderIsacFix)
+                  : nullptr;
+     }},
+#elif defined(WEBRTC_CODEC_ISAC)
+    {"isac",
+     [](int clockrate_hz, int num_channels) {
+       return (clockrate_hz == 16000 || clockrate_hz == 32000) &&
+                      num_channels == 1
+                  ? Uniq(new AudioDecoderIsac)
+                  : nullptr;
+     }},
+#endif
+    {"l16",
+     [](int clockrate_hz, int num_channels) {

[the sun, 2016/04/27 20:34:15]

L16 isn't supported in WVoE. See:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

[ossu, 2016/04/28 08:35:54]

Good to know,  however isn't this just a straight mapping of the previous codec
support in audio_coding? The actual list of formats to use is provided below
(well, not provided yet, it seems).

[kwiberg-webrtc, 2016/04/28 12:35:52]

Interesting. But ossu@ is right, I've just tried to preserve what NetEq can
currently do.

[the sun, 2016/04/28 13:02:42]

Ok, so once we are more comfortable with the structure, the #defines and codecs
we don't need to support will go away, and we'll just have something similar to
the list in WVoE? That SGTM.

Right now, we have this divide between what we say we support (WVoE filtered
list of codecs), and what we actually do support (what's builtin to ACM. See:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....

[kwiberg-webrtc, 2016/04/28 19:35:22]

Yes, once we inject the factory from all the way out in WVoE, it would make
perfect sense to change the set of codecs supported by the factory to the set we
actually want.

+       return num_channels >= 1 ? Uniq(new AudioDecoderPcm16B(num_channels))
+                                : nullptr;
+     }},
+#ifdef WEBRTC_CODEC_G722
+    {"g722",
+     [](int clockrate_hz, int num_channels) {
+       if (clockrate_hz == 8000) {
+         if (num_channels == 1)
+           return Uniq(new AudioDecoderG722);
+         if (num_channels == 2)
+           return Uniq(new AudioDecoderG722Stereo);
+       }
+       return Uniq(nullptr);

[hlundin-webrtc, 2016/04/27 14:35:56]

Why Uniq(nullptr) here, but not in other places?

[kwiberg-webrtc, 2016/04/28 12:35:52]

Because the other places all use ?: with nullptr as one case and
std::unique_ptr<AudioDecoder> as the other case, so that the resulting
expression has type std::unique_ptr<AudioDecoder>, whereas this return
expression would have type nullptr_t without the Uniq call. And lambdas without
a declared return type are required to have the same type on all their return
expressions.

IOW, I tried that first and it didn't work. :-)

+     }},
+#endif
+#ifdef WEBRTC_CODEC_OPUS
+    {"opus",
+     [](int clockrate_hz, int num_channels) {
+       return clockrate_hz == 48000 && (num_channels == 1 || num_channels == 2)
+                  ? Uniq(new AudioDecoderOpus(num_channels))
+                  : nullptr;
+     }},
+#endif
+};
+
+class BuiltinAudioDecoderFactory : public AudioDecoderFactory {
+ public:
+  std::vector<SdpAudioFormat> GetSupportedFormats() override {
+    FATAL() << "Not implemented yet!";

[the sun, 2016/04/27 20:34:15]

We should implement this already now, as it forces us to think more carefully
about the contents SdpAudioFormat and how the factory accepts or refuses the
various combinations.

[kwiberg-webrtc, 2016/04/28 12:35:52]

Are you sure? I could implement it, but since no one is using it yet, I don't
think the "forcing us to think" part is going to happen.

[the sun, 2016/04/28 13:02:42]

Nah, you're right, leaving it out makes sense from the approach of weeding out
how to use the factory from within NetEq.

+  }
+
+  std::unique_ptr<AudioDecoder> MakeAudioDecoder(
+      const SdpAudioFormat& format) override {
+    for (const auto& dc : decoder_constructors) {
+      if (STR_CASE_CMP(format.name.c_str(), dc.name) == 0) {
+        return std::unique_ptr<AudioDecoder>(
+            dc.constructor(format.clockrate_hz, format.num_channels));
+      }
+    }
+    return nullptr;
+  }
+};
+
+}  // namespace
+
+std::unique_ptr<AudioDecoderFactory> CreateBuiltinAudioDecoderFactory() {
+  return std::unique_ptr<AudioDecoderFactory>(new BuiltinAudioDecoderFactory);
+}
+
+}  // namespace webrtc