audioproc_f with simulated mic analog gain

webrtc/modules/audio_processing/BUILD.gn

 # Copyright (c) 2014 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.
 
 import("//build/config/arm.gni")
 import("//third_party/protobuf/proto_library.gni")
@@ skipping 482 matching lines @@
       "agc/mock_agc.h",
       "audio_buffer_unittest.cc",
       "beamformer/array_util_unittest.cc",
       "beamformer/complex_matrix_unittest.cc",
       "beamformer/covariance_matrix_generator_unittest.cc",
       "beamformer/matrix_unittest.cc",
       "beamformer/mock_nonlinear_beamformer.h",
       "config_unittest.cc",
       "echo_cancellation_impl_unittest.cc",
       "splitting_filter_unittest.cc",
+      "test/fake_recording_device_unittest.cc",
       "transient/dyadic_decimator_unittest.cc",
       "transient/file_utils.cc",
       "transient/file_utils.h",
       "transient/file_utils_unittest.cc",
       "transient/moving_moments_unittest.cc",
       "transient/transient_detector_unittest.cc",
       "transient/transient_suppressor_unittest.cc",
       "transient/wpd_node_unittest.cc",
       "transient/wpd_tree_unittest.cc",
       "utility/block_mean_calculator_unittest.cc",
       "utility/delay_estimator_unittest.cc",
       "vad/gmm_unittest.cc",
       "vad/pitch_based_vad_unittest.cc",
       "vad/pitch_internal_unittest.cc",
       "vad/pole_zero_filter_unittest.cc",
       "vad/standalone_vad_unittest.cc",
       "vad/vad_audio_proc_unittest.cc",
       "vad/vad_circular_buffer_unittest.cc",
       "vad/voice_activity_detector_unittest.cc",
     ]
 
     deps = [
       ":audio_processing",
       ":audioproc_test_utils",
+      ":fake_recording_device",
       "../..:webrtc_common",
       "../../base:gtest_prod",
       "../../base:protobuf_utils",
       "../../base:rtc_base",
       "../../base:rtc_base_approved",
       "../../common_audio:common_audio",
       "../../system_wrappers:system_wrappers",
       "../../test:test_support",
       "../audio_coding:neteq_unittest_tools",
       "test/conversational_speech:unittest",
@@ skipping 126 matching lines @@
       "//testing/gtest",
     ]
 
     if (rtc_enable_intelligibility_enhancer) {
       defines = [ "WEBRTC_INTELLIGIBILITY_ENHANCER=1" ]
     } else {
       defines = [ "WEBRTC_INTELLIGIBILITY_ENHANCER=0" ]
     }
   }
 
+  rtc_source_set("fake_recording_device") {

[peah-webrtc, 2017/05/05 20:25:21]

Is it really worthwhile having a separate build target for the
fake_recording_device? I'd suggest to put it in the audioproc_f build target as:
1) It is always build together with that.
2) The build file becomes shorter.
3) The build file becomes more easy to read.

WDYT?

[aleloi, 2017/05/08 09:46:49]

I don't agree with 3) in this case. Having a modular build shows what logical
sub-components a big target can be divided in, and shows dependencies between
sub-components. It's another clue for understanding the design of something.
There are additional benefits:

* it would get simpler to use a 'fake_recording_device' for something else
* GN can help enforce design modularity checking that header includes are stated
as dependencies 

Link to 3 slides about build design of a (public) presentation on GN:
https://docs.google.com/presentation/d/15Zwb53JcncHfEwHpnG_PoIbbzQ3GQi_cpujYw...

[aleloi, 2017/05/08 10:15:23]

Additional arguments for modular targets:
* GN will guard against including recording_device.h in places it's not needed
or can be avoided by having to explicitly state 'recording_device' as a
dependency.
* Reduced compilation time.

[peah-webrtc, 2017/05/08 11:41:33]

You definitely have a great point in that it is good in having a modular build.
But to me going for separate build targets for each class is too extreme if the
class is not used elsewhere.

*To me this class seems very tied to how audioproc_f is constructed and I don't
see how it can be used in any other of our simulators. What is your view on
where it could be used elsewhere?

*Regarding the modularity checking, there is a point, but I doubt we should do
this for individual classes. Or should we be that extreme?

*Regarding the compilation, that should only hold if fake_recording_device is an
optional part in some builds of audioproc_f, but afaics it is always build in as
a mandatory part, and will remain so. It could make sense in the linking phase,
but for one class that should not make a difference, right? 

[aleloi, 2017/05/08 12:40:49]

Ok, I'm halfway convinced by your arguments :) Last feeble protest: suppose we
develop realistic (and complex) simulations of real microphones that span over
several files. Maybe we'll want to use them in higher-level unit- and regression
tests. Then modularity would have been helpful. But maybe that's a problem for
later.

[peah-webrtc, 2017/05/08 13:06:37]

In that case definitely modelarity would make even more sense. With the current
class we have file-modularity but then it would make sense to have a
wider-modularity than that. (In that case, I guess it would make sense to group
the files into a separate subfolder, or should one have several buildtargets in
one folder?)

I think that if that case happens, we should change to have that then. I don't
think it makes sense to have that yet with the code in this CL. But if there are
concrete plans of adding that, it may for sure make sense to have that.

[AleBzk, 2017/05/16 08:53:02]

Thanks for the discussion :) Let's leave as it is, also because in the future
the FakeRecordingDevice class might be injected in a new AudioDevice class that
simulates a dummy mic (needed to mock the actual mic).

[peah-webrtc, 2017/05/16 12:19:35]

I'm not convinced with that usecase:-). 
Could you please describe this AudioDevice class? With the code that is now, I
don't see a clear need/use for such a class and I see no need for that for the
purpose of the AGC2 work.

I'd prefer to leave this in the standard build target for audioproc_f until that
change is made. The main reasons for that are:
-I strongly doubt we want to use build file targets that are specific for
classes if there is no special requirement for that.
-There is no case when we would like to build audioproc_f without this target.
-I cannot foresee any use of this class in any other build target.
-The audioproc_f target is so small anyway.

+    sources = [
+      "test/fake_recording_device.cc",
+      "test/fake_recording_device.h",
+    ]
+    deps = [
+      "../../base:rtc_base_approved",
+    ]
+  }
+
   if (rtc_enable_protobuf) {
     rtc_executable("unpack_aecdump") {
       testonly = true
       sources = [
         "test/unpack.cc",
       ]
 
       deps = [
         ":audio_processing",
         ":audioproc_debug_proto",
@@ skipping 18 matching lines @@
         "test/audioproc_float.cc",
         "test/wav_based_simulator.cc",
         "test/wav_based_simulator.h",
       ]
 
       deps = [
         ":audio_processing",
         ":audioproc_debug_proto",
         ":audioproc_protobuf_utils",
         ":audioproc_test_utils",
+        ":fake_recording_device",
         "../../base:protobuf_utils",
         "../../base:rtc_base_approved",
         "../../common_audio:common_audio",
         "../../system_wrappers",
         "../../system_wrappers:system_wrappers_default",
         "../../test:test_support",
         "//testing/gtest",
         "//third_party/gflags:gflags",
       ]
     }  # audioproc_f
@@ skipping 102 matching lines @@
 
       deps = [
         ":audioproc_debug_proto",
         "../..:webrtc_common",
         "../../base:protobuf_utils",
         "../../base:rtc_base_approved",
       ]
     }
   }
 }