Add aecdump support to audioproc_f.

webrtc/modules/audio_processing/test/audio_file_processor.h

+/*
+ *  Copyright (c) 2015 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_TEST_AUDIO_FILE_PROCESSOR_H_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_TEST_AUDIO_FILE_PROCESSOR_H_
+
+#include <vector>
+
+#include "webrtc/base/scoped_ptr.h"
+#include "webrtc/common_audio/channel_buffer.h"
+#include "webrtc/common_audio/wav_file.h"
+#include "webrtc/modules/audio_processing/include/audio_processing.h"
+#include "webrtc/modules/audio_processing/test/test_utils.h"
+#include "webrtc/system_wrappers/interface/tick_util.h"
+
+#ifdef WEBRTC_ANDROID_PLATFORM_BUILD
+#include "external/webrtc/webrtc/modules/audio_processing/debug.pb.h"
+#else
+#include "webrtc/audio_processing/debug.pb.h"
+#endif
+
+namespace webrtc {
+
+// Interface for processing an input file with an AudioProcessing instance and
+// dumping the results to an output file.
+class AudioFileProcessor {
+ public:
+  static const int kChunksPerSecond = 1000 / AudioProcessing::kChunkSizeMs;
+
+  virtual ~AudioFileProcessor() {}
+
+  // Process one AuioProcessing::kChunkSizeMs of data from the input file.

[peah-webrtc, 2015/10/20 21:17:25]

Typo: Should be AudioProcessing

[Andrew MacDonald, 2015/10/21 00:29:28]

Done.

+  virtual bool ProcessChunk() = 0;

[peah-webrtc, 2015/10/20 21:17:25]

I think another name is needed for this method. What it really does is to
produce 1 frame out processed microphone data, right? Or should it rather be to
receive 1 frame of microphone data (currently this is the same but that need not
always be the same and therefore the distinction matters for the naming).
We should at least name it so that it is distinguished from processing a chunk
of reverse data.

Furthermore, the naming should indicate that it contains all processing of
supporting APM api calls, such as set_stream_delay, that are in the aecdump
recording.

[Andrew MacDonald, 2015/10/21 00:29:28]

The name is intentionally ambiguous in order for implementations to have
different interpretations. WavFileProcessor reads one chunk from the input WAV
file, processes it and writes it to the output WAV file. AecDumpProcessor reads
events until it processes its first stream event and writes that to the output
WAV file. We might have a processor in the future that only writes reverse
chunks, or (more likely) one that writes both reverse and forward chunks.

I changed to ProcessAndWriteChunk to be a bit more self-documenting.

I've updated the implementation documentation with the above to make it clear.
As above, that won't happen for all implementations, but I added a note to
AecDumpFileProcessor.

[peah-webrtc, 2015/10/21 08:10:04]

I think ProcessChunk was better than ProcessAndWriteChunk because of the
vagueness. You have a good point that it makes sense for it to be ambiguous But
I'd actually then just call it Process because that is what it really does.
ProcessChunk typically actually involves processing several chunks
(render+capture) and the "chunk" is specified to be 10 ms of data.
I'm for sure ok with ProcessChunk though but I think it is a bit misleading.



I still think ProcessAndWriteChunk is not really correct. I'd rather call it
ProcessCaptureFrame, or in light of that its purpose is a ambiguous and it
actually can process  maybe just Process

[Andrew MacDonald, 2015/10/22 00:12:09]

Except that it might not process a capture chunk at all, or it might process
both render and capture chunks. Process feels a little too ambiguous, because it
gives no impression about how much is being processed. How about ProcessSome or
ProcessSomeData?

[peah-webrtc, 2015/10/22 04:50:40]

Then the loop does not make sense at all. As it is now, the only reason to have
the loop is to count the number of chunks, and with the other names we need to
drop the chunk counter and then I cannot see any reason at all why the loop
should be present in this file. 

I definitely see your points about ambiguousness, but I think one thing we can
rely on is that the processing will be frame-based in some manner and in that
case the loop makes sense if it is over frames. Or did you intend the loop to
possibly be over protobuf messages (i.e., one loop iteration per message)?

Another suggestion is ProcessFrame (but that is very similar to ProcessChunk).
How about that? 


Btw, regarding chunk, it is a replacement for "frame", right? Or is this a wider
term in the sense that it can have any size? I think that it is 10 ms in all
places I've seen it.

[Andrew MacDonald, 2015/10/22 05:11:55]

And update the trace time overrider. My point is that the loop-type interface is
more flexible, and introduces one extra line of code for the caller. I think the
tradeoff clearly prefers the loop-type.
Hmm. Not sure exactly what you mean. There are both frame-based (aka
chunk-based) and message-based implementations below.
Here's the nomenclature we've been trying to use:
frame: all samples recorded at a time instant (analogous to a video frame) 
chunk: arbitrary number of frames

This interpretation of frame is fairly standard. Pervasive in Chromium and
webkit/blink. Some more references:
https://developer.apple.com/library/ios/documentation/MusicAudio/Reference/Co...
https://developer.mozilla.org/en-US/docs/Web/API/Web_Audio_API/Basic_concepts...

[Andrew MacDonald, 2015/10/22 16:38:20]

I'd like to settle on a name you're happy with (or at least, least unhappy with
;)

ProcessChunk, ProcessSome, ProcessSomeData, something else?

[peah-webrtc, 2015/10/23 07:42:40]

From the link I definitely see the risk of confusion if we use frame.

The link also says 
"When an audio signal is processed, sampling means the conversion of a
continuous signal to a discrete signal; put another way, a continuous sound wave
such as a band playing live is converted to a sequence of samples (a
discrete-time signal) that allow a computer to handle the audio in distinct
blocks. A sample is a single value or set of multiple values at a specific point
in time (and space in the case of spatialisation.)"

What about ProcessBlock? Otherwise ProcessChunk is fine.

I for sure think the for loop makes sense, but what I meant was that it does not
make sense if it is used with the wrong name.
In APM, output samples are produced in blocks (frames) at requests of an
external user that operates based on a certain clock, and requests samples in
integer multiples of the APM block size based on that clock. In my mind, it
makes sense for a simulator/call recreator to operate similarly to that, in
terms of blocks.
Therefore I think the for-loop should be somehow based on blocks, regardless of
whether it is called chunk, block or frame. 
An alternative is to loop only over protobuf messages. 
The current implementation is to have two nested loops, one outer over capture
blocks, and one inner over the messages that are occurring before the next
capture block is achieved. That is fully fine.

[Andrew MacDonald, 2015/10/23 23:58:35]

We've been using block to refer to the underlying size of audio a processing
algorithm will use. Typically this refers to the FFT size. So ProcessChunk it
is!
Great, sounds good.

+
+  // Returns the total execution time of all AudioProcessing calls.
+  int64_t processing_time_ms() const { return processing_time_.Milliseconds(); }
+
+ protected:
+  // RAII class for execution time measurement. Updates the provided
+  // TickInterval with the time between object creation and leaving the
+  // enclosing scope.
+  class ScopedTimer {
+   public:
+    explicit ScopedTimer(TickInterval* interval)
+        : interval_(interval), start_time_(TickTime::Now()) {}
+
+    ~ScopedTimer() { *interval_ += TickTime::Now() - start_time_; }

[aluebs-webrtc, 2015/10/24 00:53:34]

Don't you think it is more intuitive to have a method to stop the time, instead
of letting the scope determine this?

[Andrew MacDonald, 2015/10/29 00:44:50]

I find scoped objects quite intuitive (in webrtc, consider scoped_ptr,
ScopedCriticalSection) but the main reason for doing this is to avoid making
mistakes. You can't forget to stop the timer.

[aluebs-webrtc, 2015/10/29 01:03:19]

But you can forget to scope it right. But if you find them intuitive, I don't
have a strong enough opinion to argue.

[Andrew MacDonald, 2015/10/29 01:14:33]

That's true, but I think that's harder (given the name), and the worst-case
damage is more limited (it will stop when execution leaves the enclosing
function scope).

+
+   private:
+    TickInterval* const interval_;

[aluebs-webrtc, 2015/10/24 00:53:34]

What does this const mean?

[Andrew MacDonald, 2015/10/29 00:44:50]

It means the pointer can't be reseated (its value is const). Contrast with
"const T*", which means what the pointer points at is const.  

[aluebs-webrtc, 2015/10/29 01:03:19]

Ok, thanks for clarifying.

+    TickTime start_time_;
+  };
+
+  TickInterval* processing_time() { return &processing_time_; }
+
+ private:
+  TickInterval processing_time_;
+};
+
+// Used to read from and write to WavFile objects.
+class WavFileProcessor final : public AudioFileProcessor {
+ public:
+  // Takes ownership of all parameters.
+  WavFileProcessor(rtc::scoped_ptr<AudioProcessing> ap,
+                   rtc::scoped_ptr<WavReader> in_file,
+                   rtc::scoped_ptr<WavWriter> out_file);
+  virtual ~WavFileProcessor() {}
+
+  bool ProcessChunk() override;
+
+ private:
+  rtc::scoped_ptr<AudioProcessing> ap_;
+  rtc::scoped_ptr<WavReader> in_file_;
+
+  ChannelBuffer<float> in_buf_;
+  ChannelBuffer<float> out_buf_;
+  std::vector<float> in_interleaved_;
+  const StreamConfig input_config_;
+  const StreamConfig output_config_;
+  ChannelBufferWavWriter buffer_writer_;
+};
+
+// Used to read from an aecdump file and write to a WavWriter.
+class AecDumpFileProcessor final : public AudioFileProcessor {
+ public:
+  // Takes ownership of all parameters.
+  AecDumpFileProcessor(rtc::scoped_ptr<AudioProcessing> ap,
+                       FILE* dump_file,

[aluebs-webrtc, 2015/10/24 00:53:34]

Shouldn't this be a scoped_ptr? Else, you could pass in a string and fopen it
internally.

[Andrew MacDonald, 2015/10/29 00:44:50]

It can't be a scoped_ptr unless I specialize the deleter. In its destructor,
scoped_ptr calls delete on its underlying pointer. I considered adding such a
ScopedFile, but decided it wasn't worth it. I'd be willing to add it if you
prefer :)

I used a FILE* rather than string to be consistent with WavFileProcessor (which
also takes file objects rather than file names).

[aluebs-webrtc, 2015/10/29 01:03:19]

Oh, I see. Agreed.

+                       rtc::scoped_ptr<WavWriter> out_file);
+
+  virtual ~AecDumpFileProcessor();
+
+  // Processes messages from the aecdump file until the first Stream message is
+  // completed.

[aluebs-webrtc, 2015/10/24 00:53:34]

Doesn't this mess with the chunk number in process_float?

[Andrew MacDonald, 2015/10/29 00:44:50]

No, because the chunk counter is indeed for the number of processed Stream (aka
capture) chunks.

[aluebs-webrtc, 2015/10/29 01:03:19]

Good point.

+  bool ProcessChunk() override;
+
+ private:
+  void HandleMessage(const webrtc::audioproc::Init& msg);
+  void HandleMessage(const webrtc::audioproc::Stream& msg);
+  void HandleMessage(const webrtc::audioproc::ReverseStream& msg);
+
+  rtc::scoped_ptr<AudioProcessing> ap_;
+  FILE* dump_file_;
+
+  rtc::scoped_ptr<ChannelBuffer<float>> in_buf_;
+  rtc::scoped_ptr<ChannelBuffer<float>> reverse_buf_;
+  ChannelBuffer<float> out_buf_;
+  StreamConfig input_config_;
+  StreamConfig reverse_config_;
+  const StreamConfig output_config_;
+  ChannelBufferWavWriter buffer_writer_;
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_TEST_AUDIO_FILE_PROCESSOR_H_