Add empty residual echo detector.

webrtc/modules/audio_processing/residual_echo_detector.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_PROCESSING_RESIDUAL_ECHO_DETECTOR_H_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_RESIDUAL_ECHO_DETECTOR_H_
+
+#include <memory>
+#include <vector>
+
+#include "webrtc/base/constructormagic.h"
+#include "webrtc/base/criticalsection.h"

[hlundin-webrtc, 2016/10/14 06:59:18]

You are still using critsects below. The way the code is written now, you could
get away with forward declaring rtc::CriticalSection, but if you go for local
locks, I think they should be plain member variables, not pointers.

+#include "webrtc/base/optional.h"
+#include "webrtc/base/swap_queue.h"
+#include "webrtc/modules/audio_processing/include/audio_processing.h"
+#include "webrtc/modules/audio_processing/render_queue_item_verifier.h"
+
+namespace webrtc {
+
+class AudioBuffer;
+class EchoDetector;
+
+class ResidualEchoDetector {
+ public:
+  ResidualEchoDetector(rtc::CriticalSection* crit_render,

[hlundin-webrtc, 2016/10/13 11:45:20]

I don't think you need the shared locks with the rest of APM. Create local
locks, or even better, use thread checkers instead.

[ivoc, 2016/10/13 13:46:16]

I'm not that familiar with the locking scheme in the APM, so I copied this
approach from the AEC/AECM. I'm not sure if thread checkers are a good idea
here. For example ReadQueuedRenderData() needs to hold both the render and
capture locks and is called from a place which only holds the render lock. I'm
not sure how to get the same behavior with thread checkers.

I can add local locks in this module if you think that's better, but it is a
different approach than the other modules are currently taking. WDYT?

[hlundin-webrtc, 2016/10/14 06:59:18]

I think local locks is preferred, since they only protect local assets. But peah
would know the APM locking structure much better than I do. He will have to make
the decision.

[peah-webrtc, 2016/10/14 07:12:29]

I agree with hlundin@ that no locks should be required. That was something I
also should have pointed out in my review.
It is better to rewrite it without locks from the beginning.

The locks in the AEC/AECM are only needed because of their APIs being part of
the public AudioProcessing API which means that the locks are needed to ensure
proper behavior.

Since you are not adding this submodule to the public API, you can instead
explicitly ensure that the submodule is called properly from APM and does not
need to use any locks to ensure that.

Afaics, the threadcheckers cannot really be used to solve all the issues with
threading here, since some the ReadQueuedRenderData may be called from different
threads. One way could, however, be to have two variants of
ReadQueuedRenderData, one for each thread.

[ivoc, 2016/10/14 09:53:18]

I have now removed all locks from this class and moved the responsibility for
locking to the APM.

+                       rtc::CriticalSection* crit_capture);
+  ~ResidualEchoDetector();
+
+  void AnalyzeRenderAudio(const AudioBuffer* audio);
+  void AnalyzeCaptureAudio(AudioBuffer* audio);

[hlundin-webrtc, 2016/10/13 11:45:20]

Not const?

[ivoc, 2016/10/13 13:46:16]

Done.

+
+  void Initialize(int sample_rate_hz);
+
+  // Reads render side data that has been queued on the render call.
+  // Called holding the capture lock.

[hlundin-webrtc, 2016/10/13 11:45:20]

Document this locking assumption with EXCLUSIVE_LOCKS_REQUIRED. But, as
mentioned above, rather get rid of the locks if possible.

[ivoc, 2016/10/13 13:46:16]

Adding EXCLUSIVE_LOCKS_REQUIRED here does not work because this function is
called directly from APM in a few places. Since the lock is a private member of
ResidualEchoDetector, it cannot be taken from inside APM.

[hlundin-webrtc, 2016/10/14 06:59:18]

Acknowledged.

+  void ReadQueuedRenderData();
+
+  float get_echo_likelihood();

[hlundin-webrtc, 2016/10/13 11:45:20]

const method.

[ivoc, 2016/10/13 13:46:16]

Done.

+
+ private:
+  rtc::CriticalSection* const crit_render_ ACQUIRED_BEFORE(crit_capture_);
+  rtc::CriticalSection* const crit_capture_;
+
+  std::vector<float> render_queue_buffer_ GUARDED_BY(crit_render_);
+  std::vector<float> capture_queue_buffer_ GUARDED_BY(crit_capture_);
+
+  // Lock protection not needed.
+  std::unique_ptr<SwapQueue<std::vector<float>, RenderQueueItemVerifier<float>>>
+      render_signal_queue_;
+
+  std::unique_ptr<EchoDetector> detector_;
+
+  RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(ResidualEchoDetector);
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_RESIDUAL_ECHO_DETECTOR_H_