Injectable audio encoders: BuiltinAudioEncoderFactory

webrtc/api/audio_codecs/audio_format.h

Index: webrtc/api/audio_codecs/audio_format.h
diff --git a/webrtc/api/audio_codecs/audio_format.h b/webrtc/api/audio_codecs/audio_format.h
index db3990f1143bf5fffc5ff233abb837dda79b533c..0bff2e2cea6098b2cddc0a4d16ef63ffed8b5fc2 100644
--- a/webrtc/api/audio_codecs/audio_format.h
+++ b/webrtc/api/audio_codecs/audio_format.h
@@ -16,6 +16,8 @@
 #include <string>
 #include <utility>
 
+#include "webrtc/base/optional.h"
+
 namespace webrtc {
 
 // SDP specification for a single audio codec.
@@ -54,28 +56,68 @@ struct SdpAudioFormat {
 void swap(SdpAudioFormat& a, SdpAudioFormat& b);
 std::ostream& operator<<(std::ostream& os, const SdpAudioFormat& saf);
 
-// To avoid API breakage, and make the code clearer, AudioCodecSpec should not
+// To avoid API breakage, and make the code clearer, AudioFormatInfo should not
 // be directly initializable with any flags indicating optional support. If it
 // were, these initializers would break any time a new flag was added. It's also
 // more difficult to understand:
-//   AudioCodecSpec spec{{"format", 8000, 1}, true, false, false, true, true};
+//   AudioFormatInfo info{16000, 1, 32000, true, false, false, true, true};
 // than
-//   AudioCodecSpec spec({"format", 8000, 1});
-//   spec.allow_comfort_noise = true;
-//   spec.future_flag_b = true;
-//   spec.future_flag_c = true;
-struct AudioCodecSpec {
-  explicit AudioCodecSpec(const SdpAudioFormat& format);
-  explicit AudioCodecSpec(SdpAudioFormat&& format);
-  ~AudioCodecSpec() = default;
+//   AudioFormatInfo info(16000, 1, 32000);
+//   info.allow_comfort_noise = true;
+//   info.future_flag_b = true;
+//   info.future_flag_c = true;
+struct AudioFormatInfo {
+  AudioFormatInfo();
+  AudioFormatInfo(int sample_rate_hz, int num_channels, int bitrate_bps);
+  AudioFormatInfo(int sample_rate_hz,
+                  int num_channels,
+                  int default_bitrate_bps,
+                  int min_bitrate_bps,
+                  int max_bitrate_bps);
+  AudioFormatInfo(const AudioFormatInfo& b) = default;

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

No need for default move ctor? I forget when that is added automatically...

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

There's nothing that can be moved, so there's no need for a move constructor.
It's all just fundamental types that will be copied.

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

You get an implicitly-declared copy constructor unless you manually declare a
copy constructor. You get an implicitly-declared move constructor unless you
manually declare a copy constructor, copy assignment operator, move assignment
operator, or destructor.

For a value-like class such as this one, you should probably either declare all
five special thingys with =default, or not mention any of them. I'd go with the
latter, especially considering that the class is trivially copyable.

+  ~AudioFormatInfo() = default;
 
-  SdpAudioFormat format;
-  bool allow_comfort_noise = true;  // This codec can be used with an external
-                                    // comfort noise generator.
+  bool operator==(const AudioFormatInfo& b) const {
+    return sample_rate_hz == b.sample_rate_hz &&
+           num_channels == b.num_channels &&
+           default_bitrate_bps == b.default_bitrate_bps &&
+           min_bitrate_bps == b.min_bitrate_bps &&
+           max_bitrate_bps == b.max_bitrate_bps &&
+           allow_comfort_noise == b.allow_comfort_noise &&
+           supports_network_adaption == b.supports_network_adaption;
+  }
+
+  bool operator!=(const AudioFormatInfo& b) const {
+    return !(*this == b);
+  }
+
+  bool HasFixedBitrate() const { return min_bitrate_bps == max_bitrate_bps; }

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

In this case, should there be a DCHECK that default_bitrate_bps is the same as
well?

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

There's a check in the constructor, but we might as well have one here as well.

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

It might be a good idea to have an IsOK() function that tests all sorts of
things like this, that we can DCHECK wherever we can.

+
+  int sample_rate_hz;
+  int num_channels;
+  int default_bitrate_bps;
+  int min_bitrate_bps;
+  int max_bitrate_bps;
+
+  bool allow_comfort_noise = true;    // This codec can be used with an external

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

Since you mention "codec" here - should this struct be called "AudioCodecInfo"
instead? That makes a lot of sense to me - AudioCodecSpec ties together the
codec-specific info with the SDP format. WDYT?

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

Yeah, that's probably better. I found AudioCodecSpec and AudioCodecInfo to be a
bit too similar - what's a spec vs. the info - but maybe that's good? I'll take
another look at folding AudioCodecInfo back into AudioCodecSpec and we can see
how it looks first. I think having them separated like this might still be
beneficial.

+                                      // comfort noise generator.
   bool supports_network_adaption = false;  // This codec can adapt to varying
                                            // network conditions.
 };
 
+struct AudioCodecSpec {
+  bool operator==(const AudioCodecSpec& b) const {
+    return format == b.format && info == b.info;
+  }
+
+  bool operator!=(const AudioCodecSpec& b) const {
+    return !(*this == b);
+  }
+
+  SdpAudioFormat format;
+  AudioFormatInfo info;
+};

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

Define swap() for these two? They'll probably end up being stored in vectors
etc.

+
 }  // namespace webrtc
 
 #endif  // WEBRTC_API_AUDIO_CODECS_AUDIO_FORMAT_H_