Injectable audio encoders: BuiltinAudioEncoderFactory

webrtc/modules/audio_coding/codecs/audio_encoder_factory.h

+/*
+ *  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.
+ */
+
+#ifndef WEBRTC_MODULES_AUDIO_CODING_CODECS_AUDIO_ENCODER_FACTORY_H_
+#define WEBRTC_MODULES_AUDIO_CODING_CODECS_AUDIO_ENCODER_FACTORY_H_
+
+#include <memory>
+#include <vector>
+
+#include "webrtc/api/audio_codecs/audio_format.h"
+#include "webrtc/base/refcount.h"
+#include "webrtc/modules/audio_coding/codecs/audio_encoder.h"
+
+namespace webrtc {
+
+// A factory that creates AudioEncoders.
+// NOTE: This class is still under development and may change without notice.
+class AudioEncoderFactory : public rtc::RefCountInterface {
+ public:
+  virtual std::vector<AudioCodecSpec> GetSupportedEncoders() = 0;
+
+  virtual bool IsSupportedEncoder(const SdpAudioFormat& format) = 0;
+
+  virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat(

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

May be good to add "Encoder" to this method name - decoder/encoder factory will
sometimes be implemented on the same object.

I guess a way to get by with just "IsSupportedEncoder()" would be to let that
return an rtc::Optional<AudioCodecInfo>?

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

The decoder has no QueryAudioFormat method as it stands today, but maybe it
should have in future. I think QueryAudioEncoder and renaming to AudioCodecInfo
makes a lot of sense.

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

FWIW, I think I'd prefer to drop IsSupported[Encoder|Decoder]() in favor of
Query[Encoder|Decoder]().

Perhaps IsSupported... could live on as a non-virtual helper method, with
implementation right here in the class decl?

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

Hmm. Having one object implement multiple interfaces isn't usually a good idea,
I think. Shared ownership makes it technically possible, but still... code is
easier to read if objects are small and do just one job.

+1 for having unique method names, though. Makes them easier to search for.
+1 for combining those two methods, since this isn't performance critical---we
can live with creating a few AudioFormatInfos when all the caller wanted was a
bool.

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

The helper is unnecessary---rtc::Optional converts to bool:

  if (foo.QueryAudioFormat(...)) {

[the sun, 2017/03/15 13:37:44]

Yeah, but it may make the code more readable...

[the sun, 2017/03/15 13:37:44]

Agree, but in certain cases there may be shared resources backing the codec
objects (e.g. hardware enc/dec).

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

In this case not much, I'd argue. It's pretty obvious what the boolean version
of the return value of "QueryAudioFormat" could be. But I agree in the general
case.

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

In which case the encoder and decoder objects should share owership of the
object that manages the resource.

I'm not saying that not implementing more than one interface is an end in
itself, just that it's a common consequence of not making objects that are too
large and/or do too many things.

+      const SdpAudioFormat& format) = 0;
+
+  virtual std::unique_ptr<AudioEncoder> MakeAudioEncoder(
+      int payload_type,
+      const SdpAudioFormat& format) = 0;
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_CODING_CODECS_AUDIO_ENCODER_FACTORY_H_