Integrate Intelligibility with APM

webrtc/modules/audio_processing/include/audio_processing.h

 /*
  *  Copyright (c) 2012 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.
  */
 
@@ skipping 98 matching lines @@
   Beamforming()
       : enabled(false),
         array_geometry() {}
   Beamforming(bool enabled, const std::vector<Point>& array_geometry)
       : enabled(enabled),
         array_geometry(array_geometry) {}
   const bool enabled;
   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
 // components designed for real-time communications software.
 //
 // APM operates on two audio streams on a frame-by-frame basis. Frames of the
 // primary stream, on which all processing is applied, are passed to
 // |ProcessStream()|. Frames of the reverse direction stream, which are used for
 // analysis by some components, are passed to |AnalyzeReverseStream()|. On the
 // client-side, this will typically be the near-end (capture) and far-end
@@ skipping 197 matching lines @@
   // reverse stream forms the echo reference signal. It is recommended, but not
   // necessary, to provide if gain control is enabled. On the server-side this
   // typically will not be used. If you're not sure what to pass in here,
   // chances are you don't need to use it.
   //
   // The |sample_rate_hz_|, |num_channels_|, and |samples_per_channel_|
   // members of |frame| must be valid. |sample_rate_hz_| must correspond to
   // |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.
   //
   // Sets the |delay| in ms between AnalyzeReverseStream() receiving a far-end
   // frame and ProcessStream() receiving a near-end frame containing the
   // corresponding echo. On the client-side this can be expressed as
   //   delay = (t_render - t_analyze) + (t_process - t_capture)
   // where,
   //   - t_analyze is the time a frame is passed to AnalyzeReverseStream() and
   //     t_render is the time the first sample of the same frame is rendered by
@@ skipping 101 matching lines @@
   // num_channels: The number of audio channels in the stream, excluding the
   //               keyboard channel if it is present. When passing a
   //               StreamConfig with an array of arrays T*[N],
   //
   //                N == {num_channels + 1  if  has_keyboard
   //                     {num_channels      if  !has_keyboard
   //
   // 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),
         has_keyboard_(has_keyboard),
         num_frames_(calculate_frames(sample_rate_hz)) {}
 
   void set_sample_rate_hz(int value) {
     sample_rate_hz_ = value;
     num_frames_ = calculate_frames(value);
   }
@@ skipping 26 matching lines @@
   int num_channels_;
   bool has_keyboard_;
   int num_frames_;
 };
 
 class ProcessingConfig {
  public:
   enum StreamName {
     kInputStream,
     kOutputStream,
-    kReverseStream,
+    kReverseInputStream,
+    kReverseOutputStream,
     kNumStreamNames,
   };
 
   const StreamConfig& input_stream() const {
     return streams[StreamName::kInputStream];
   }
   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) {
       if (this->streams[i] != other.streams[i]) {
         return false;
       }
     }
     return true;
   }
 
@@ skipping 359 matching lines @@
   // This does not impact the size of frames passed to |ProcessStream()|.
   virtual int set_frame_size_ms(int size) = 0;
   virtual int frame_size_ms() const = 0;
 
  protected:
   virtual ~VoiceDetection() {}
 };
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_PROCESSING_INCLUDE_AUDIO_PROCESSING_H_