Integrate Intelligibility with APM

webrtc/modules/audio_processing/include/audio_processing.h

Index: webrtc/modules/audio_processing/include/audio_processing.h
diff --git a/webrtc/modules/audio_processing/include/audio_processing.h b/webrtc/modules/audio_processing/include/audio_processing.h
index 085484466c3e3d1e1730b7d9c678224410ca44c5..74a9ff18853bc4ffb79952ab07e28816071663a9 100644
--- a/webrtc/modules/audio_processing/include/audio_processing.h
+++ b/webrtc/modules/audio_processing/include/audio_processing.h
@@ -116,6 +116,18 @@ struct Beamforming {
   const std::vector<Point> array_geometry;
 };
 
+// Use to enable intelligibility enhancer in audio processing. Must be provided
+// though the constructor. It will have no impact if used with
+// AudioProcessing::SetExtraOptions().
+//
+// Note: If enabled and the reverse stream has more than one output channel,
+// the reverse stream will become an upmixed mono signal.
+struct Intelligibility {
+  Intelligibility() : enabled(false) {}
+  explicit Intelligibility(bool enabled) : enabled(enabled) {}
+  bool enabled;
+};
+
 static const int kAudioProcMaxNativeSampleRateHz = 32000;
 
 // The Audio Processing Module (APM) provides a collection of voice processing
@@ -333,21 +345,28 @@ class AudioProcessing {
   // |input_sample_rate_hz()|
   //
   // TODO(ajm): add const to input; requires an implementation fix.
+  // DEPRECATED: Use |ProcessReverseStream| instead.
+  // TODO(ekm): Remove once all users have updated to |ProcessReverseStream|.
   virtual int AnalyzeReverseStream(AudioFrame* frame) = 0;
 
+  // Same as |AnalyzeReverseStream|, but may modify |data| if intelligibility

[Andrew MacDonald, 2015/07/30 18:48:53]

modify |frame|

[ekm, 2015/07/30 22:38:44]

Done.

+  // is enabled.
+  virtual int ProcessReverseStream(AudioFrame* frame) = 0;
+
   // Accepts deinterleaved float audio with the range [-1, 1]. Each element
   // of |data| points to a channel buffer, arranged according to |layout|.
-  //
   // TODO(mgraczyk): Remove once clients are updated to use the new interface.
   virtual int AnalyzeReverseStream(const float* const* data,
                                    int samples_per_channel,
-                                   int sample_rate_hz,
+                                   int rev_sample_rate_hz,
                                    ChannelLayout layout) = 0;
 
   // Accepts deinterleaved float audio with the range [-1, 1]. Each element of
   // |data| points to a channel buffer, arranged according to |reverse_config|.
-  virtual int AnalyzeReverseStream(const float* const* data,
-                                   const StreamConfig& reverse_config) = 0;
+  virtual int ProcessReverseStream(const float* const* src,
+                                   const StreamConfig& reverse_input_config,
+                                   const StreamConfig& reverse_output_config,

[Andrew MacDonald, 2015/07/30 18:48:53]

I'm not sure we want to do this. The way you have it now makes the interface
inconsistent: the user can specify an output config, but they'll only get it if
intelligibility is enabled.

We either need to always support converting the stream, or remove the output
config. I'm a bit divided on this. Alex what do you think?

[aluebs-webrtc, 2015/07/30 18:56:34]

You bring a good point here. I think it makes sense to always convert (except
when they are the same of course), so that the reverse stream is similar to the
capture stream. Then enabling IE (or any other future component) doesn't change
the behavior of the APM.

[ekm, 2015/07/30 21:23:50]

Are we talking about converting reverse to capture, or reverse output to reverse
input? Since before IE apm left reverse input as is, it makes sense to me to
convert reverse out to reverse in. In that case, I can revert the expansion of
ProcessingConfig, etc.

What about adding back the AnalyzeReverseStream that only takes the input
stream+config? Is that too much interface?

[aluebs-webrtc, 2015/07/30 23:09:51]

I would prefer to avoid adding yet another interface. So the input would be 
converted to the output format, as it happens in the capture stream. Andrew?

[ekm, 2015/07/30 23:20:17]

Ok. Why are the two configs in the ProcessStream interface necessary?

[aluebs-webrtc, 2015/07/30 23:23:38]

So that the APM knows what the user expects as output and does the right thing.

+                                   float* const* dest) = 0;
 
   // This must be called if and only if echo processing is enabled.
   //
@@ -469,8 +488,8 @@ class StreamConfig {
   // has_keyboard: True if the stream has a keyboard channel. When has_keyboard
   //               is true, the last channel in any corresponding list of
   //               channels is the keyboard channel.
-  StreamConfig(int sample_rate_hz = 0,
-               int num_channels = 0,
+  StreamConfig(int sample_rate_hz = 16000,
+               int num_channels = 1,

[aluebs-webrtc, 2015/07/30 15:28:07]

I think this zero-initialization here is on purpose to force the user to set
reasonable values. Actually not setting them is a way of signalling that that
stream is not necessary, for example when a reverse stream is not available. Why
do you need to have these default values?

[Andrew MacDonald, 2015/07/30 18:48:53]

Agreed.

[ekm, 2015/07/30 22:38:44]

That makes sense.

I thought it'd be a nice way to get around the asserts in audio_buffer, but I
see it defeats the purpose. I re-zeroed these and added a new conditional at the
beginning of InitializeLocked() instead.

                bool has_keyboard = false)
       : sample_rate_hz_(sample_rate_hz),
         num_channels_(num_channels),
@@ -517,7 +536,8 @@ class ProcessingConfig {
   enum StreamName {
     kInputStream,
     kOutputStream,
-    kReverseStream,
+    kReverseInputStream,
+    kReverseOutputStream,
     kNumStreamNames,
   };
 
@@ -527,13 +547,21 @@ class ProcessingConfig {
   const StreamConfig& output_stream() const {
     return streams[StreamName::kOutputStream];
   }
-  const StreamConfig& reverse_stream() const {
-    return streams[StreamName::kReverseStream];
+  const StreamConfig& reverse_input_stream() const {
+    return streams[StreamName::kReverseInputStream];
+  }
+  const StreamConfig& reverse_output_stream() const {
+    return streams[StreamName::kReverseOutputStream];
   }
 
   StreamConfig& input_stream() { return streams[StreamName::kInputStream]; }
   StreamConfig& output_stream() { return streams[StreamName::kOutputStream]; }
-  StreamConfig& reverse_stream() { return streams[StreamName::kReverseStream]; }
+  StreamConfig& reverse_input_stream() {
+    return streams[StreamName::kReverseInputStream];
+  }
+  StreamConfig& reverse_output_stream() {
+    return streams[StreamName::kReverseOutputStream];
+  }
 
   bool operator==(const ProcessingConfig& other) const {
     for (int i = 0; i < StreamName::kNumStreamNames; ++i) {