Refactor AudioInputController and split stat by stream type.

media/audio/audio_input_controller.cc

 // Copyright (c) 2012 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 "media/audio/audio_input_controller.h"
 
 #include <inttypes.h>
 
 #include <algorithm>
 #include <limits>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
-#include "media/audio/audio_file_writer.h"
 #include "media/base/user_input_monitor.h"
 
 namespace media {
 namespace {
 
 const int kMaxInputChannels = 3;
 
 #if defined(AUDIO_POWER_MONITORING)
 // Time in seconds between two successive measurements of audio power levels.
 const int kPowerMonitorLogIntervalSeconds = 15;
@@ skipping 103 matching lines @@
   void PerformOptionalDebugRecording(const AudioBus* source) {
     // Called on the hw callback thread while recording is enabled.
     if (!controller_->debug_writer_ || !controller_->debug_writer_->WillWrite())
       return;
 
     // TODO(tommi): This is costly. AudioBus heap allocs and we create a new
     // one for every callback. We could instead have a pool of bus objects
     // that get returned to us somehow.
     // We should also avoid calling PostTask here since the implementation
     // of the debug writer will basically do a PostTask straight away anyway.
-    // Might require some modifications to AudioInputDebugWriter though since
-    // there are some threading concerns there and AudioInputDebugWriter's
+    // Might require some modifications to AudioFileWriter though since
+    // there are some threading concerns there and AudioFileWriter's
     // lifetime guarantees need to be longer than that of associated active
     // audio streams.
     std::unique_ptr<AudioBus> source_copy =
         AudioBus::Create(source->channels(), source->frames());
     source->CopyTo(source_copy.get());
     controller_->task_runner_->PostTask(
         FROM_HERE, base::Bind(&AudioInputController::WriteInputDataForDebugging,
                               weak_controller_, base::Passed(&source_copy)));
   }
 
@@ skipping 33 matching lines @@
 
 // static
 AudioInputController::Factory* AudioInputController::factory_ = nullptr;
 
 AudioInputController::AudioInputController(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
     EventHandler* handler,
     SyncWriter* sync_writer,
     std::unique_ptr<AudioFileWriter> debug_writer,
     UserInputMonitor* user_input_monitor,
-    const bool agc_is_enabled)
+    Type type)
     : creator_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       task_runner_(std::move(task_runner)),
       handler_(handler),
       stream_(nullptr),
       sync_writer_(sync_writer),
-      max_volume_(0.0),
+      type_(type),
       user_input_monitor_(user_input_monitor),
-      agc_is_enabled_(agc_is_enabled),
-#if defined(AUDIO_POWER_MONITORING)
-      power_measurement_is_enabled_(false),
-      log_silence_state_(false),
-      silence_state_(SILENCE_STATE_NO_MEASUREMENT),
-#endif
-      prev_key_down_count_(0),
       debug_writer_(std::move(debug_writer)),
       weak_ptr_factory_(this) {
   DCHECK(creator_task_runner_.get());
   DCHECK(handler_);
   DCHECK(sync_writer_);
 }
 
 AudioInputController::~AudioInputController() {
   DCHECK(!audio_callback_);
   DCHECK(!stream_);
 }
 
 // static
 scoped_refptr<AudioInputController> AudioInputController::Create(
     AudioManager* audio_manager,
     EventHandler* event_handler,
     SyncWriter* sync_writer,
+    UserInputMonitor* user_input_monitor,
+    std::unique_ptr<AudioFileWriter> debug_writer,
     const AudioParameters& params,
     const std::string& device_id,
-    UserInputMonitor* user_input_monitor) {
-  DCHECK(audio_manager);
-  DCHECK(event_handler);
-  DCHECK(sync_writer);
-
-  if (!params.IsValid() || (params.channels() > kMaxInputChannels))
-    return nullptr;
-
-  if (factory_) {
-    return factory_->Create(audio_manager->GetTaskRunner(), sync_writer,
-                            audio_manager, event_handler, params,
-                            user_input_monitor);
-  }
-
-  scoped_refptr<AudioInputController> controller(new AudioInputController(
-      audio_manager->GetTaskRunner(), event_handler, sync_writer, nullptr,
-      user_input_monitor, false));
-
-  // Create and open a new audio input stream from the existing
-  // audio-device thread.
-  if (!controller->task_runner_->PostTask(
-          FROM_HERE,
-          base::Bind(&AudioInputController::DoCreate,
-                     controller,
-                     base::Unretained(audio_manager),
-                     params,
-                     device_id))) {
-    controller = nullptr;
-  }
-
-  return controller;
-}
-
-// static
-scoped_refptr<AudioInputController> AudioInputController::CreateLowLatency(
-    AudioManager* audio_manager,
-    EventHandler* event_handler,
-    const AudioParameters& params,
-    const std::string& device_id,
-    SyncWriter* sync_writer,
-    std::unique_ptr<AudioFileWriter> debug_writer,
-    UserInputMonitor* user_input_monitor,
-    const bool agc_is_enabled) {
+    bool enable_agc) {
   DCHECK(audio_manager);
   DCHECK(sync_writer);
   DCHECK(event_handler);
 
   if (!params.IsValid() || (params.channels() > kMaxInputChannels))
     return nullptr;
 
   if (factory_) {
     return factory_->Create(audio_manager->GetTaskRunner(), sync_writer,
                             audio_manager, event_handler, params,
-                            user_input_monitor);
+                            user_input_monitor, ParamsToType(params));
   }
 
   // Create the AudioInputController object and ensure that it runs on
   // the audio-manager thread.
   scoped_refptr<AudioInputController> controller(new AudioInputController(
       audio_manager->GetTaskRunner(), event_handler, sync_writer,
-      std::move(debug_writer), user_input_monitor, agc_is_enabled));
+      std::move(debug_writer), user_input_monitor, ParamsToType(params)));
 
   // Create and open a new audio input stream from the existing
   // audio-device thread. Use the provided audio-input device.
   if (!controller->task_runner_->PostTask(
-          FROM_HERE,
-          base::Bind(&AudioInputController::DoCreateForLowLatency,
-                     controller,
-                     base::Unretained(audio_manager),
-                     params,
-                     device_id))) {
+          FROM_HERE, base::Bind(&AudioInputController::DoCreate, controller,
+                                base::Unretained(audio_manager), params,
+                                device_id, enable_agc))) {
     controller = nullptr;
   }
 
   return controller;
 }
 
 // static
 scoped_refptr<AudioInputController> AudioInputController::CreateForStream(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
     EventHandler* event_handler,
     AudioInputStream* stream,
     SyncWriter* sync_writer,
     std::unique_ptr<AudioFileWriter> debug_writer,
     UserInputMonitor* user_input_monitor) {
   DCHECK(sync_writer);
   DCHECK(stream);
   DCHECK(event_handler);
 
   if (factory_) {
-    return factory_->Create(
-        task_runner, sync_writer, AudioManager::Get(), event_handler,
-        AudioParameters::UnavailableDeviceParams(), user_input_monitor);
+    return factory_->Create(task_runner, sync_writer, AudioManager::Get(),
+                            event_handler,
+                            AudioParameters::UnavailableDeviceParams(),
+                            user_input_monitor, VIRTUAL);
   }
 
   // Create the AudioInputController object and ensure that it runs on
   // the audio-manager thread.
   scoped_refptr<AudioInputController> controller(new AudioInputController(
       task_runner, event_handler, sync_writer, std::move(debug_writer),
-      user_input_monitor, false));
+      user_input_monitor, VIRTUAL));
 
   if (!controller->task_runner_->PostTask(
           FROM_HERE, base::Bind(&AudioInputController::DoCreateForStream,
-                                controller, stream, false))) {
+                                controller, stream, /*enable_agc*/ false))) {
     controller = nullptr;
   }
 
   return controller;
 }
 
 void AudioInputController::Record() {
   DCHECK(creator_task_runner_->BelongsToCurrentThread());
   task_runner_->PostTask(FROM_HERE, base::Bind(
       &AudioInputController::DoRecord, this));
 }
 
 void AudioInputController::Close(const base::Closure& closed_task) {
   DCHECK(!closed_task.is_null());
   DCHECK(creator_task_runner_->BelongsToCurrentThread());
 
   task_runner_->PostTaskAndReply(
       FROM_HERE, base::Bind(&AudioInputController::DoClose, this), closed_task);
 }
 
 void AudioInputController::SetVolume(double volume) {
   DCHECK(creator_task_runner_->BelongsToCurrentThread());
   task_runner_->PostTask(FROM_HERE, base::Bind(
       &AudioInputController::DoSetVolume, this, volume));
 }
 
 void AudioInputController::DoCreate(AudioManager* audio_manager,
                                     const AudioParameters& params,
-                                    const std::string& device_id) {
+                                    const std::string& device_id,
+                                    bool enable_agc) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!stream_);
   SCOPED_UMA_HISTOGRAM_TIMER("Media.AudioInputController.CreateTime");
   handler_->OnLog(this, "AIC::DoCreate");
 
 #if defined(AUDIO_POWER_MONITORING)
+  // We only log silence state UMA stats for low latency mode and if we use a
+  // real device.
+  if (type_ == LOW_LATENCY)
+    log_silence_state_ = true;

[tommi (sloooow) - chröme, 2017/02/01 21:07:25]

I think we also only want to do this if the APM is in the picture (i.e. webrtc
w/echo cancellation), right?

[Max Morin, 2017/02/02 09:54:56]

Probably yes. I'll use enable_agc as an indicator of webrtc being used (since
it's the only easily available heuristic). I'll also merge this bool with
|power_measurement_is_enabled_|.

[tommi (sloooow) - chröme, 2017/02/02 14:14:21]

Acknowledged.

+
   // Disable power monitoring for streams that run without AGC enabled to
   // avoid adding logs and UMA for non-WebRTC clients.
-  power_measurement_is_enabled_ = agc_is_enabled_;
+  power_measurement_is_enabled_ = enable_agc;
   last_audio_level_log_time_ = base::TimeTicks::Now();
-  silence_state_ = SILENCE_STATE_NO_MEASUREMENT;
 #endif
 
   // MakeAudioInputStream might fail and return nullptr. If so,
   // DoCreateForStream will handle and report it.
   auto* stream = audio_manager->MakeAudioInputStream(
       params, device_id, base::Bind(&AudioInputController::LogMessage, this));
-  DoCreateForStream(stream,
-                    params.format() == AudioParameters::AUDIO_PCM_LOW_LATENCY);
-}
-
-void AudioInputController::DoCreateForLowLatency(AudioManager* audio_manager,
-                                                 const AudioParameters& params,
-                                                 const std::string& device_id) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-#if defined(AUDIO_POWER_MONITORING)
-  // We only log silence state UMA stats for low latency mode and if we use a
-  // real device.
-  if (params.format() != AudioParameters::AUDIO_FAKE)
-    log_silence_state_ = true;
-#endif
-
-  // Set the low latency timer to non-null. It'll be reset when capture starts.
-  low_latency_create_time_ = base::TimeTicks::Now();
-  DoCreate(audio_manager, params, device_id);
+  DoCreateForStream(stream, enable_agc);
 }
 
 void AudioInputController::DoCreateForStream(
     AudioInputStream* stream_to_control,
-    bool low_latency) {
+    bool enable_agc) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!stream_);
 
   if (!stream_to_control) {
-    LogCaptureStartupResult(
-        low_latency ? CAPTURE_STARTUP_CREATE_LOW_LATENCY_STREAM_FAILED
-                    : CAPTURE_STARTUP_CREATE_STREAM_FAILED);
+    LogCaptureStartupResult(CAPTURE_STARTUP_CREATE_STREAM_FAILED);
     handler_->OnError(this, STREAM_CREATE_ERROR);
     return;
   }
 
   if (!stream_to_control->Open()) {
     stream_to_control->Close();
-    LogCaptureStartupResult(low_latency
-                                ? CAPTURE_STARTUP_OPEN_LOW_LATENCY_STREAM_FAILED
-                                : CAPTURE_STARTUP_OPEN_STREAM_FAILED);
+    LogCaptureStartupResult(CAPTURE_STARTUP_OPEN_STREAM_FAILED);
     handler_->OnError(this, STREAM_OPEN_ERROR);
     return;
   }
 
-  // Set AGC state using mode in |agc_is_enabled_| which can only be enabled in
-  // CreateLowLatency().
+// Set AGC state using mode in |enable_agc_| which can only be enabled in

[tommi (sloooow) - chröme, 2017/02/01 21:07:25]

nit: |enable_agc|

+// CreateLowLatency().

[tommi (sloooow) - chröme, 2017/02/01 21:07:25]

CreateLowLatency is now gone

 #if defined(AUDIO_POWER_MONITORING)
   bool agc_is_supported =
-      stream_to_control->SetAutomaticGainControl(agc_is_enabled_);
+      stream_to_control->SetAutomaticGainControl(enable_agc);
   // Disable power measurements on platforms that does not support AGC at a
   // lower level. AGC can fail on platforms where we don't support the
   // functionality to modify the input volume slider. One such example is
   // Windows XP.
   power_measurement_is_enabled_ &= agc_is_supported;
 #else
-  stream_to_control->SetAutomaticGainControl(agc_is_enabled_);
+  stream_to_control->SetAutomaticGainControl(enable_agc);
 #endif
 
   // Finally, keep the stream pointer around, update the state and notify.
   stream_ = stream_to_control;
   handler_->OnCreated(this);
 }
 
 void AudioInputController::DoRecord() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   SCOPED_UMA_HISTOGRAM_TIMER("Media.AudioInputController.RecordTime");
 
   if (!stream_ || audio_callback_)
     return;
 
   handler_->OnLog(this, "AIC::DoRecord");
 
   if (user_input_monitor_) {
     user_input_monitor_->EnableKeyPressMonitoring();
     prev_key_down_count_ = user_input_monitor_->GetKeyPressCount();
   }
 
-  if (!low_latency_create_time_.is_null())
-    low_latency_create_time_ = base::TimeTicks::Now();
+  stream_create_time_ = base::TimeTicks::Now();
 
   audio_callback_.reset(new AudioCallback(this));
   stream_->Start(audio_callback_.get());
 }
 
 void AudioInputController::DoClose() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   SCOPED_UMA_HISTOGRAM_TIMER("Media.AudioInputController.CloseTime");
 
   if (!stream_)
     return;
 
   // Allow calling unconditionally and bail if we don't have a stream to close.
   if (audio_callback_) {
     stream_->Stop();
 
-    if (!low_latency_create_time_.is_null()) {
-      // Since the usage pattern of getUserMedia commonly is that a stream
-      // (and accompanied audio track) is created and immediately closed or
-      // discarded, we collect stats separately for short lived audio streams
-      // (500ms or less) that we did not receive a callback for, as
-      // 'stopped early' and 'never got data'.
-      const base::TimeDelta duration =
-          base::TimeTicks::Now() - low_latency_create_time_;
-      LogCaptureStartupResult(audio_callback_->received_callback()
-                                  ? CAPTURE_STARTUP_OK
-                                  : (duration.InMilliseconds() < 500
-                                         ? CAPTURE_STARTUP_STOPPED_EARLY
-                                         : CAPTURE_STARTUP_NEVER_GOT_DATA));
-      UMA_HISTOGRAM_BOOLEAN("Media.Audio.Capture.CallbackError",
-                            audio_callback_->error_during_callback());
+    // Since the usage pattern of getUserMedia commonly is that a stream

[tommi (sloooow) - chröme, 2017/02/01 21:07:25]

nit: since you've removed the if() that checks for the timestamp, the below code
is no longer specific to getUserMedia.  Hmm... not sure what's the best way to
update the comment... maybe just point it out as an example usage pattern
instead?

+    // (and accompanied audio track) is created and immediately closed or
+    // discarded, we collect stats separately for short lived audio streams
+    // (500ms or less) that we did not receive a callback for, as
+    // 'stopped early' and 'never got data'.
+    const base::TimeDelta duration =
+        base::TimeTicks::Now() - stream_create_time_;
+    CaptureStartupResult capture_startup_result =
+        audio_callback_->received_callback()
+            ? CAPTURE_STARTUP_OK
+            : (duration.InMilliseconds() < 500
+                   ? CAPTURE_STARTUP_STOPPED_EARLY
+                   : CAPTURE_STARTUP_NEVER_GOT_DATA);
+    LogCaptureStartupResult(capture_startup_result);
+    LogCallbackError();
+    if (type_ == LOW_LATENCY) {
       if (audio_callback_->received_callback()) {

[tommi (sloooow) - chröme, 2017/02/01 21:07:25]

I wonder if we should add another InputStreamDuration stat.  One that's for
streams that never received any callbacks. It could be a good indicator to see
how good the somewhat arbitrary chosen 500ms breakpoint is.  If these turn out
to be generally short lived too, it might be an indicator that slow/overloaded
machines take longer to initialize but no callbacks might not necessarily mean
failure.

[Max Morin, 2017/02/02 09:54:56]

Added Media.InputStreamWithoutCallbackDuration.

         // Log the total duration (since DoCreate) and update the histogram.
         UMA_HISTOGRAM_LONG_TIMES("Media.InputStreamDuration", duration);
         const std::string log_string = base::StringPrintf(
             "AIC::DoClose: stream duration=%" PRId64 " seconds",
             duration.InSeconds());
         handler_->OnLog(this, log_string);
       }
     }
 
     audio_callback_.reset();
@@ skipping 127 matching lines @@
 
 void AudioInputController::LogSilenceState(SilenceState value) {
   UMA_HISTOGRAM_ENUMERATION("Media.AudioInputControllerSessionSilenceReport",
                             value,
                             SILENCE_STATE_MAX + 1);
 }
 #endif
 
 void AudioInputController::LogCaptureStartupResult(
     CaptureStartupResult result) {
-  UMA_HISTOGRAM_ENUMERATION("Media.AudioInputControllerCaptureStartupSuccess",
-                            result, CAPTURE_STARTUP_RESULT_MAX + 1);
+  switch (type_) {
+    case LOW_LATENCY:
+      UMA_HISTOGRAM_ENUMERATION("Media.LowLatencyAudioCaptureStartupSuccess",
+                                result, CAPTURE_STARTUP_RESULT_MAX + 1);
+      break;
+    case HIGH_LATENCY:
+      UMA_HISTOGRAM_ENUMERATION("Media.HighLatencyAudioCaptureStartupSuccess",
+                                result, CAPTURE_STARTUP_RESULT_MAX + 1);
+      break;
+    case VIRTUAL:
+      UMA_HISTOGRAM_ENUMERATION("Media.VirtualAudioCaptureStartupSuccess",
+                                result, CAPTURE_STARTUP_RESULT_MAX + 1);
+      break;
+    default:
+      break;
+  }
+}
+
+void AudioInputController::LogCallbackError() {
+  bool error_during_callback = audio_callback_->error_during_callback();
+  switch (type_) {
+    case LOW_LATENCY:
+      UMA_HISTOGRAM_BOOLEAN("Media.Audio.Capture.LowLatencyCallbackError",
+                            error_during_callback);
+      break;
+    case HIGH_LATENCY:
+      UMA_HISTOGRAM_BOOLEAN("Media.Audio.Capture.HighLatencyCallbackError",
+                            error_during_callback);
+      break;
+    case VIRTUAL:
+      UMA_HISTOGRAM_BOOLEAN("Media.Audio.Capture.VirtualCallbackError",
+                            error_during_callback);
+      break;
+    default:
+      break;
+  }
 }
 
 void AudioInputController::DoEnableDebugRecording(
     const base::FilePath& file_name) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   if (debug_writer_)
     debug_writer_->Start(file_name);
 }
 
 void AudioInputController::DoDisableDebugRecording() {
@@ skipping 48 matching lines @@
   *mic_volume_percent = static_cast<int>(100.0 * volume);
 
   last_audio_level_log_time_ = now;
 
   return true;
 #else
   return false;
 #endif
 }
 
+// static
+AudioInputController::Type AudioInputController::ParamsToType(
+    const AudioParameters& params) {
+  switch (params.format()) {
+    case AudioParameters::Format::AUDIO_PCM_LINEAR:
+      return AudioInputController::Type::HIGH_LATENCY;
+    case AudioParameters::Format::AUDIO_PCM_LOW_LATENCY:
+      return AudioInputController::Type::LOW_LATENCY;
+    default:
+      // Currently, the remaining supported type is fake. Reconsider if other
+      // formats become supported.
+      return AudioInputController::Type::FAKE;
+  }
+  return AudioInputController::Type::FAKE;
+}
+
 }  // namespace media