Added an "enable-audio-processor" flag and WebRtcAudioProcessor class

content/renderer/media/webrtc_audio_processor_unittest.cc

Index: content/renderer/media/webrtc_audio_processor_unittest.cc
diff --git a/content/renderer/media/webrtc_audio_processor_unittest.cc b/content/renderer/media/webrtc_audio_processor_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..006c3f6840efda332ff4c931feb9295da7ee46bd
--- /dev/null
+++ b/content/renderer/media/webrtc_audio_processor_unittest.cc
@@ -0,0 +1,167 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/command_line.h"
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "base/time/time.h"
+#include "content/public/common/content_switches.h"
+#include "content/renderer/media/rtc_media_constraints.h"
+#include "content/renderer/media/webrtc_audio_processor.h"
+#include "media/audio/audio_parameters.h"
+#include "media/base/audio_bus.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/libjingle/source/talk/app/webrtc/mediastreaminterface.h"
+
+using ::testing::_;
+using ::testing::AnyNumber;
+using ::testing::AtLeast;
+using ::testing::Return;
+
+namespace content {
+
+namespace {
+
+#if defined(ANDROID)
+const int kAudioProcessingSampleRate = 16000;
+#else
+const int kAudioProcessingSampleRate = 32000;
+#endif
+const int kAudioProcessingNumberOfChannel = 1;
+
+// The number of packers used for testing.
+const int kNumberOfPacketsForTest = 100;
+
+void ReadDataFromSpeechFile(char* data, int length) {
+  base::FilePath file;
+  CHECK(PathService::Get(base::DIR_SOURCE_ROOT, &file));
+  file = file.Append(FILE_PATH_LITERAL("media"))
+             .Append(FILE_PATH_LITERAL("test"))
+             .Append(FILE_PATH_LITERAL("data"))
+             .Append(FILE_PATH_LITERAL("speech_16b_stereo_48kHz.raw"));

[Henrik Grunell, 2013/11/22 09:03:36]

Is this a common way of specifying a file used in a test?

[no longer working on chromium, 2013/11/22 13:27:53]

yes.

+  DCHECK(base::PathExists(file));
+  int64 data_file_size64 = 0;
+  DCHECK(file_util::GetFileSize(file, &data_file_size64));
+  EXPECT_EQ(length, file_util::ReadFile(file, data, length));
+  DCHECK(data_file_size64 > length);

[Henrik Grunell, 2013/11/22 09:03:36]

Only do this check in debug builds? (If you change this, also change the one
above.) I think the check should be moved to before reading the file.

[no longer working on chromium, 2013/11/22 13:27:53]

that is unittest, DCHECK is enough.

+}
+
+// Constant constraint keys which enables default audio constraints on
+// mediastreams with audio.
+struct {
+  const char* key;
+  const char* value;
+} const kDefaultAudioConstraints[] = {

[tommi (sloooow) - chröme, 2013/11/21 19:52:03]

make this a static const inside ApplyFixedAudioConstraints?

[no longer working on chromium, 2013/11/22 13:27:53]

Done.

+  { webrtc::MediaConstraintsInterface::kEchoCancellation,
+    webrtc::MediaConstraintsInterface::kValueTrue },
+#if defined(OS_CHROMEOS) || defined(OS_MACOSX)

[Henrik Grunell, 2013/11/22 09:03:36]

Why do we need to set experimental AEC platform conditionally in the test? I
think you can enable or not for all.

[no longer working on chromium, 2013/11/22 13:27:53]

This is the same as the production code. Do you know if experimental AEC is
supported on those platforms? But anyhow, if they are not used in production on
those platforms, why should we test experimental AEC on those platforms?

[Henrik Grunell, 2013/11/25 10:52:02]

The reason is to not make any assumptions about the implementation of some other
class/function that is a consumer of the AudioProcessing class. If that code
changes the test will not cover that. I would prefer to test different sets of
constraints regardless of platform. See also other comment below.

[Henrik Grunell, 2013/11/26 08:08:44]

We decided to revisit further testing including this comment I have in a later
CL. Shijing to create a bug for that.

+  // Enable the extended filter mode AEC on platforms with known echo issues.
+  { webrtc::MediaConstraintsInterface::kExperimentalEchoCancellation,
+    webrtc::MediaConstraintsInterface::kValueTrue },
+#endif
+  { webrtc::MediaConstraintsInterface::kAutoGainControl,
+    webrtc::MediaConstraintsInterface::kValueTrue },
+  { webrtc::MediaConstraintsInterface::kExperimentalAutoGainControl,
+    webrtc::MediaConstraintsInterface::kValueTrue },
+  { webrtc::MediaConstraintsInterface::kNoiseSuppression,
+    webrtc::MediaConstraintsInterface::kValueTrue },
+  { webrtc::MediaConstraintsInterface::kHighpassFilter,
+    webrtc::MediaConstraintsInterface::kValueTrue },
+};
+
+void ApplyFixedAudioConstraints(RTCMediaConstraints* constraints) {
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDefaultAudioConstraints); ++i) {
+    constraints->AddMandatory(kDefaultAudioConstraints[i].key,
+                              kDefaultAudioConstraints[i].value, false);
+  }
+}
+
+}  // namespace
+
+class WebRtcAudioProcessorTest : public ::testing::Test {
+ public:
+  WebRtcAudioProcessorTest()
+      : params_(media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
+                media::CHANNEL_LAYOUT_STEREO, 48000, 16, 512) {
+    CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kEnableAudioTrackProcessing);

[tommi (sloooow) - chröme, 2013/11/21 19:52:03]

is this a common thing to do for tests?  could we somehow avoid this and still
be able to test the code?

[no longer working on chromium, 2013/11/22 13:27:53]

That is what I found from the current unittests.

Well, the falg is in the production code, and the unittest wants to test the
code behind the falg, so the unittest has to set the falg to true.

[tommi (sloooow) - chröme, 2013/11/22 14:32:42]

what is this falg you speak of? :)

I understand that the production code is currently checking for this, but can we
be a little bit more clever about that and remove the check from
InitializeAudioProcessingModule method and never construct a
WebRtcAudioProcessor object unless the flag is set?  That way we don't have to
bother about the flag in either the WebRtcAudioProcessor implementation or the
test.

[no longer working on chromium, 2013/11/25 16:36:26]

flag means switches::kEnableAudioTrackProcessing.

We had a offline discussion and agreed to keep this way. In short, the reason
for doing this is that WebRtcAudioProcessor will be used as a FIFO when the flag
is not set, that is why we put the code here.

+  }
+
+ protected:
+  // Helper method to save duplicated code.
+  void ProcessDataAndVerifyFormat(WebRtcAudioProcessor* audio_processor,
+                                  int expected_output_sample_rate,
+                                  int expected_output_channels,
+                                  int expected_output_buffer_size) {
+    // Read the audio data from a file.
+    const int packet_size =
+        params_.frames_per_buffer() * 2 * params_.channels();
+    const size_t length = packet_size * kNumberOfPacketsForTest;
+    scoped_ptr<char[]> capture_data(new char[length]);
+    ReadDataFromSpeechFile(capture_data.get(), length);
+    const int16* data_ptr = reinterpret_cast<const int16*>(capture_data.get());
+    scoped_ptr<media::AudioBus> data_bus = media::AudioBus::Create(
+        params_.channels(), params_.frames_per_buffer());
+    for (int i = 0; i < kNumberOfPacketsForTest; ++i) {
+      data_bus->FromInterleaved(data_ptr, data_bus->frames(), 2);
+      audio_processor->PushCaptureData(data_bus.get());
+
+      // Feed data as render data to the processor, this does not cost anything

[Henrik Grunell, 2013/11/22 09:03:36]

"Feed data as render data to the processor" is redundant, remove.

I think the rest is slightly off target, this small type of cost isn't really a
problem in tests. I'd suggest explain more in line with that for some cases it's
not used but we do this always anyway.

[no longer working on chromium, 2013/11/22 13:27:53]

Done.

+      // when the audio processing is off in the processor.
+      audio_processor->PushRenderData(
+          data_ptr,
+          params_.sample_rate(), params_.channels(),
+          params_.frames_per_buffer(), base::TimeDelta::FromMilliseconds(10));
+
+      // Process and consume the data in the processor.

[Henrik Grunell, 2013/11/22 09:03:36]

This comment is redundant. Remove.

[no longer working on chromium, 2013/11/22 13:27:53]

Done.

+      int16* output = NULL;
+      while(audio_processor->ProcessAndConsumeData(
+          base::TimeDelta::FromMilliseconds(10), 255, false, &output)) {
+        EXPECT_TRUE(output != NULL);
+        EXPECT_EQ(audio_processor->OutputFormat().sample_rate(),
+                  expected_output_sample_rate);
+        EXPECT_EQ(audio_processor->OutputFormat().channels(),
+                  expected_output_channels);
+        EXPECT_EQ(audio_processor->OutputFormat().frames_per_buffer(),
+                  expected_output_buffer_size);
+      }
+
+      data_ptr += params_.frames_per_buffer() * params_.channels();
+    }
+  }
+
+  media::AudioParameters params_;
+};
+
+TEST_F(WebRtcAudioProcessorTest, WithoutAudioProcessing) {

[Henrik Grunell, 2013/11/22 09:03:36]

I suggest testing function by function in separate tests if possible.
(ProcessAndConsume... obviously depends on other function calls prior.) Is it
possible to do that?

[no longer working on chromium, 2013/11/22 13:27:53]

Yes, PushCaptureData, PushRenderData and ProcessAndConsumeData are called in
order, so they have to be tested together.
Also, I don't know what to test if just calling one method.

[Henrik Grunell, 2013/11/25 10:52:02]

OK, if it doesn't make sense to test individual functions skip it.

[no longer working on chromium, 2013/11/25 16:36:26]

Done.

+  // Setup the audio processor with empty constraint.
+  RTCMediaConstraints constraints;
+  WebRtcAudioProcessor audio_processor(&constraints);
+  audio_processor.SetCaptureFormat(params_);
+  EXPECT_FALSE(audio_processor.has_audio_processing());
+
+  ProcessDataAndVerifyFormat(&audio_processor,
+                             params_.sample_rate(),
+                             params_.channels(),
+                             params_.sample_rate() / 100);
+}
+
+TEST_F(WebRtcAudioProcessorTest, WithAudioProcessing) {

[Henrik Grunell, 2013/11/22 09:03:36]

Is there any way we can test different sets of constraints? Test that the right
set of components are enabled and actually exercised?

[no longer working on chromium, 2013/11/22 13:27:53]

WebRtcAudioProcessor can take one constraint at its lifetime, the owner of
WebRtcAudioProcessor need to recreate a new object when the constraint changes.

Looks like you want tests on the owner of WebRtcAudioProcessor, this can only be
done after this CL and its follow-up are landed.

[Henrik Grunell, 2013/11/25 10:52:02]

Well, no, I mean testing this class. You set constraints in
ApplyFixedAudioConstraints. I meant set them differently. E.g. with and without
experimental AEC and other combinations that could be good to test.

[no longer working on chromium, 2013/11/25 16:36:26]

As discussed offline, I don't have time to add more tests, this will be done in
separate CLs.

+  // Setup the audio processor with default constraint.
+  RTCMediaConstraints constraints;
+  ApplyFixedAudioConstraints(&constraints);
+  WebRtcAudioProcessor audio_processor(&constraints);
+  audio_processor.SetCaptureFormat(params_);
+  EXPECT_TRUE(audio_processor.has_audio_processing());
+
+  ProcessDataAndVerifyFormat(&audio_processor,
+                             kAudioProcessingSampleRate,
+                             kAudioProcessingNumberOfChannel,
+                             kAudioProcessingSampleRate / 100);
+}
+
+}  // namespace content