Refactor AudioInputController and related interfaces.

media/audio/audio_input_controller.cc

Index: media/audio/audio_input_controller.cc
diff --git a/media/audio/audio_input_controller.cc b/media/audio/audio_input_controller.cc
index 888df2c410d56a6ba421fb0fdc8b3b4f912e0300..f5e42d024b556308416fb27f9f771fb1f6fffed7 100644
--- a/media/audio/audio_input_controller.cc
+++ b/media/audio/audio_input_controller.cc
@@ -20,6 +20,7 @@
 #include "media/audio/audio_input_writer.h"
 #include "media/base/user_input_monitor.h"
 
+namespace media {
 namespace {
 
 const int kMaxInputChannels = 3;
@@ -49,7 +50,7 @@ void LogMicrophoneMuteResult(MicrophoneMuteResult result) {
 
 // Helper method which calculates the average power of an audio bus. Unit is in
 // dBFS, where 0 dBFS corresponds to all channels and samples equal to 1.0.
-float AveragePower(const media::AudioBus& buffer) {
+float AveragePower(const AudioBus& buffer) {
   const int frames = buffer.frames();
   const int channels = buffer.channels();
   if (frames <= 0 || channels <= 0)
@@ -81,22 +82,85 @@ float AveragePower(const media::AudioBus& buffer) {
 
 }  // namespace
 
-namespace media {
+class AudioInputController::AudioCallback
+    : public AudioInputStream::AudioInputCallback {
+ public:
+  explicit AudioCallback(AudioInputController* controller)
+      : controller_(controller) {}
+  ~AudioCallback() override {}
+
+  bool received_callback() const { return received_callback_; }
+  bool error_during_callback() const { return error_during_callback_; }

[o1ka, 2017/01/12 12:07:38]

Similar data race (see AudioInputController comment) for these two members: they
are updated on callback thread; but we read them on audio thread in
AudioInputController::DoClose() right after we called Stop() on the stream (i.e.
while there may be in progress/pending callbacks).

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

Yes, that pattern is safe. It's just that the documentation for Stop() was
incorrect.

[o1ka, 2017/01/12 17:27:15]

Acknowledged.

+
+ private:
+  void OnData(AudioInputStream* stream,
+              const AudioBus* source,
+              uint32_t hardware_delay_bytes,
+              double volume) override {
+    TRACE_EVENT0("audio", "AC::OnData");
+
+    received_callback_ = true;
+
+    DeliverDataToSyncWriter(source, hardware_delay_bytes, volume);
+    PerformOptionalDebugRecording(source);
+  }
+
+  void OnError(AudioInputStream* stream) override {
+    error_during_callback_ = true;
+    controller_->task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&AudioInputController::DoReportError, controller_));
+  }
+
+  void PerformOptionalDebugRecording(const AudioBus* source) {
+    // Called on the audio callback thread while recording is enabled.

[o1ka, 2017/01/12 12:07:38]

In other places it's called "hw callback thread" (I like "audio" version more).

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

hmm... changed to "audio/hw callback thread" :-|

+    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.

[o1ka, 2017/01/12 12:07:38]

"Requires some refactoring since both
AudioInputController::WriteInputDataForDebugging() and
AudioInputDebugWriter::Write() are not thread-safe" - it's not immediately
obvious, and having a comment may prevent a quick dangerous change in the
future.

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

Comment updated. WriteInputDataForDebugging itself is safe as is, but with this
refactoring, it would be going away anyway.

+    // Btw, AudioInputDebugWriter is the only implementation of
+    // AudioInputWriter...  remove the interface?

[o1ka, 2017/01/12 12:07:38]

The interface is in media together with controller, and the impl is in content -
this seems to be the reason for having it.

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

ah, makes sense.  removed this part of the comment.

+    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,
+                              controller_, base::Passed(&source_copy)));
+  }
+
+  void DeliverDataToSyncWriter(const AudioBus* source,
+                               uint32_t hardware_delay_bytes,
+                               double volume) {
+    bool key_pressed = controller_->CheckForKeyboardInput();
+    controller_->sync_writer_->Write(source, volume, key_pressed,
+                                     hardware_delay_bytes);
+    controller_->CheckAudioPower(source, volume);
+  }
+
+  AudioInputController* const controller_;
+  bool received_callback_ = false;
+  bool error_during_callback_ = false;
+};
 
 // static
 AudioInputController::Factory* AudioInputController::factory_ = nullptr;
 
 AudioInputController::AudioInputController(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
     EventHandler* handler,
     SyncWriter* sync_writer,
     std::unique_ptr<AudioInputWriter> debug_writer,
     UserInputMonitor* user_input_monitor,
     const bool agc_is_enabled)
     : creator_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      task_runner_(std::move(task_runner)),
       handler_(handler),
       stream_(nullptr),
-      should_report_stats(0),
-      state_(CLOSED),
       sync_writer_(sync_writer),
       max_volume_(0.0),
       user_input_monitor_(user_input_monitor),
@@ -109,34 +173,39 @@ AudioInputController::AudioInputController(
       prev_key_down_count_(0),
       debug_writer_(std::move(debug_writer)) {
   DCHECK(creator_task_runner_.get());
+  DCHECK(handler_);
+  DCHECK(sync_writer_);
 }
 
 AudioInputController::~AudioInputController() {
-  DCHECK_EQ(state_, CLOSED);
+  DCHECK(!audio_callback_);
+  DCHECK(!stream_);
 }
 
 // static
 scoped_refptr<AudioInputController> AudioInputController::Create(
     AudioManager* audio_manager,
     EventHandler* event_handler,
+    SyncWriter* sync_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*/ nullptr, audio_manager,
-                            event_handler, params, user_input_monitor);
+    return factory_->Create(audio_manager->GetTaskRunner(), sync_writer,
+                            audio_manager, event_handler, params,
+                            user_input_monitor);
   }
 
   scoped_refptr<AudioInputController> controller(new AudioInputController(
-      event_handler, nullptr, nullptr, user_input_monitor, false));
-
-  controller->task_runner_ = audio_manager->GetTaskRunner();
+      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.
@@ -165,6 +234,7 @@ scoped_refptr<AudioInputController> AudioInputController::CreateLowLatency(
     const bool agc_is_enabled) {
   DCHECK(audio_manager);
   DCHECK(sync_writer);
+  DCHECK(event_handler);
 
   if (!params.IsValid() || (params.channels() > kMaxInputChannels))
     return nullptr;
@@ -178,9 +248,8 @@ scoped_refptr<AudioInputController> AudioInputController::CreateLowLatency(
   // Create the AudioInputController object and ensure that it runs on
   // the audio-manager thread.
   scoped_refptr<AudioInputController> controller(new AudioInputController(
-      event_handler, sync_writer, std::move(debug_writer), user_input_monitor,
-      agc_is_enabled));
-  controller->task_runner_ = audio_manager->GetTaskRunner();
+      audio_manager->GetTaskRunner(), event_handler, sync_writer,
+      std::move(debug_writer), user_input_monitor, agc_is_enabled));
 
   // Create and open a new audio input stream from the existing
   // audio-device thread. Use the provided audio-input device.
@@ -207,19 +276,19 @@ scoped_refptr<AudioInputController> AudioInputController::CreateForStream(
     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,
-        media::AudioParameters::UnavailableDeviceParams(), user_input_monitor);
+        AudioParameters::UnavailableDeviceParams(), user_input_monitor);
   }
 
   // Create the AudioInputController object and ensure that it runs on
   // the audio-manager thread.
   scoped_refptr<AudioInputController> controller(new AudioInputController(
-      event_handler, sync_writer, std::move(debug_writer), user_input_monitor,
-      false));
-  controller->task_runner_ = task_runner;
+      task_runner, event_handler, sync_writer, std::move(debug_writer),
+      user_input_monitor, false));
 
   if (!controller->task_runner_->PostTask(
           FROM_HERE,
@@ -233,6 +302,7 @@ scoped_refptr<AudioInputController> AudioInputController::CreateForStream(
 }
 
 void AudioInputController::Record() {
+  DCHECK(creator_task_runner_->BelongsToCurrentThread());
   task_runner_->PostTask(FROM_HERE, base::Bind(
       &AudioInputController::DoRecord, this));
 }
@@ -246,6 +316,7 @@ void AudioInputController::Close(const base::Closure& closed_task) {
 }
 
 void AudioInputController::SetVolume(double volume) {
+  DCHECK(creator_task_runner_->BelongsToCurrentThread());
   task_runner_->PostTask(FROM_HERE, base::Bind(
       &AudioInputController::DoSetVolume, this, volume));
 }
@@ -254,9 +325,9 @@ void AudioInputController::DoCreate(AudioManager* audio_manager,
                                     const AudioParameters& params,
                                     const std::string& device_id) {
   DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(!stream_);
   SCOPED_UMA_HISTOGRAM_TIMER("Media.AudioInputController.CreateTime");
-  if (handler_)
-    handler_->OnLog(this, "AIC::DoCreate");
+  handler_->OnLog(this, "AIC::DoCreate");
 
 #if defined(AUDIO_POWER_MONITORING)
   // Disable power monitoring for streams that run without AGC enabled to
@@ -266,6 +337,8 @@ void AudioInputController::DoCreate(AudioManager* audio_manager,
   silence_state_ = SILENCE_STATE_NO_MEASUREMENT;
 #endif
 
+  // MakeAudioInputStream might fail and return nullptr. If so,
+  // DoCreateForStream will handle and report it.
   DoCreateForStream(audio_manager->MakeAudioInputStream(
       params, device_id, base::Bind(&AudioInputController::LogMessage, this)));
 }
@@ -289,99 +362,96 @@ void AudioInputController::DoCreateForLowLatency(AudioManager* audio_manager,
 void AudioInputController::DoCreateForStream(
     AudioInputStream* stream_to_control) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-
   DCHECK(!stream_);
-  stream_ = stream_to_control;
-  should_report_stats = 1;
 
-  if (!stream_) {
-    if (handler_)
-      handler_->OnError(this, STREAM_CREATE_ERROR);
+  if (!stream_to_control) {
     LogCaptureStartupResult(CAPTURE_STARTUP_CREATE_STREAM_FAILED);
+    handler_->OnError(this, STREAM_CREATE_ERROR);
     return;
   }
 
-  if (stream_ && !stream_->Open()) {
-    stream_->Close();
-    stream_ = nullptr;
-    if (handler_)
-      handler_->OnError(this, STREAM_OPEN_ERROR);
+  if (!stream_to_control->Open()) {
+    stream_to_control->Close();
     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().
 #if defined(AUDIO_POWER_MONITORING)
-  bool agc_is_supported = false;
-  agc_is_supported = stream_->SetAutomaticGainControl(agc_is_enabled_);
+  bool agc_is_supported =
+      stream_to_control->SetAutomaticGainControl(agc_is_enabled_);
   // 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_->SetAutomaticGainControl(agc_is_enabled_);
+  stream_to_control->SetAutomaticGainControl(agc_is_enabled_);
 #endif
 
-
-  state_ = CREATED;
-  if (handler_)
-    handler_->OnCreated(this);
-
   if (user_input_monitor_) {
     user_input_monitor_->EnableKeyPressMonitoring();
     prev_key_down_count_ = user_input_monitor_->GetKeyPressCount();
   }
+
+  // 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 (state_ != CREATED)
+  if (!stream_)
     return;
 
-  {
-    base::AutoLock auto_lock(lock_);
-    state_ = RECORDING;
-  }
+  DCHECK(!audio_callback_);

[Max Morin, 2017/01/12 09:31:10]

DoRecord might be called multiple times, so it should probably just return if
audio_callback_.

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

Thanks. Done. Do you know in what situation that happens?

[Max Morin, 2017/01/12 15:17:12]

It would only happen with a buggy/misbehaving renderer.

+  audio_callback_.reset(new AudioCallback(this));
 
-  if (handler_)
-    handler_->OnLog(this, "AIC::DoRecord");
+  handler_->OnLog(this, "AIC::DoRecord");
 
-  stream_->Start(this);
+  stream_->Start(audio_callback_.get());
 }
 
 void AudioInputController::DoClose() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   SCOPED_UMA_HISTOGRAM_TIMER("Media.AudioInputController.CloseTime");
-  // If we have already logged something, this does nothing.
-  // Otherwise, we haven't recieved data.
-  LogCaptureStartupResult(CAPTURE_STARTUP_NEVER_GOT_DATA);
 
-  if (state_ == CLOSED)
+  if (!stream_)
     return;
 
-  // If this is a low-latency stream, log the total duration (since DoCreate)
-  // and add it to a UMA histogram.
-  if (!low_latency_create_time_.is_null()) {
-    base::TimeDelta duration =
-        base::TimeTicks::Now() - low_latency_create_time_;
-    UMA_HISTOGRAM_LONG_TIMES("Media.InputStreamDuration", duration);
-    if (handler_) {
-      std::string log_string =
-          base::StringPrintf("AIC::DoClose: stream duration=");
-      log_string += base::Int64ToString(duration.InSeconds());
-      log_string += " seconds";
-      handler_->OnLog(this, log_string);
+  // 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()) {
+      LogCaptureStartupResult(audio_callback_->received_callback()
+                                  ? CAPTURE_STARTUP_OK
+                                  : CAPTURE_STARTUP_NEVER_GOT_DATA);
+      UMA_HISTOGRAM_BOOLEAN("Media.Audio.Capture.CallbackError",
+                            audio_callback_->error_during_callback());
+      if (audio_callback_->received_callback()) {
+        // Log the total duration (since DoCreate) and update the histogram.
+        base::TimeDelta duration =

[o1ka, 2017/01/12 12:07:38]

const?

[tommi (sloooow) - chröme, 2017/01/12 18:58:19]

Done. (forgot to ack in the patch set that I made it const)

+            base::TimeTicks::Now() - low_latency_create_time_;
+        UMA_HISTOGRAM_LONG_TIMES("Media.InputStreamDuration", duration);
+        std::string log_string =
+            base::StringPrintf("AIC::DoClose: stream duration=");

[Max Morin, 2017/01/12 09:31:10]

Unnecessary StringPrintf (or crate the entire log_string here).

[o1ka, 2017/01/12 12:07:38]

- then can be const as well.

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

ah, sorry I missed this somehow (this is moved code).  Made const and reduced
the string copying+conversion inefficiencies.

+        log_string += base::Int64ToString(duration.InSeconds()) + " seconds";
+        handler_->OnLog(this, log_string);
+      }
     }
+
+    audio_callback_.reset();
   }
 
-  DoStopCloseAndClearStream();
+  stream_->Close();
+  stream_ = nullptr;
 
-  if (SharedMemoryAndSyncSocketMode())
-    sync_writer_->Close();
+  sync_writer_->Close();
 
   if (user_input_monitor_)
     user_input_monitor_->DisableKeyPressMonitoring();
@@ -396,13 +466,13 @@ void AudioInputController::DoClose() {
   if (debug_writer_)
     debug_writer_->Stop();
 
-  state_ = CLOSED;
+  max_volume_ = 0.0;
+  low_latency_create_time_ = base::TimeTicks();  // Reset to null.
 }
 
 void AudioInputController::DoReportError() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  if (handler_)
-    handler_->OnError(this, STREAM_ERROR);
+  handler_->OnError(this, STREAM_ERROR);
 }
 
 void AudioInputController::DoSetVolume(double volume) {
@@ -410,7 +480,7 @@ void AudioInputController::DoSetVolume(double volume) {
   DCHECK_GE(volume, 0);
   DCHECK_LE(volume, 1.0);
 
-  if (state_ != CREATED && state_ != RECORDING)
+  if (!stream_)
     return;
 
   // Only ask for the maximum volume at first call and use cached value
@@ -428,100 +498,11 @@ void AudioInputController::DoSetVolume(double volume) {
   stream_->SetVolume(max_volume_ * volume);
 }
 
-void AudioInputController::OnData(AudioInputStream* stream,
-                                  const AudioBus* source,
-                                  uint32_t hardware_delay_bytes,
-                                  double volume) {
-  TRACE_EVENT0("audio", "AudioInputController::OnData");
-  if (debug_writer_ && debug_writer_->WillWrite()) {
-    std::unique_ptr<AudioBus> source_copy =
-        AudioBus::Create(source->channels(), source->frames());
-    source->CopyTo(source_copy.get());
-    task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(
-            &AudioInputController::WriteInputDataForDebugging,
-            this,
-            base::Passed(&source_copy)));
-  }
-  // Now we have data, so we know for sure that startup was ok.
-  LogCaptureStartupResult(CAPTURE_STARTUP_OK);
-
-  {
-    base::AutoLock auto_lock(lock_);
-    if (state_ != RECORDING)
-      return;
-  }
-
-  bool key_pressed = false;
-  if (user_input_monitor_) {
-    size_t current_count = user_input_monitor_->GetKeyPressCount();
-    key_pressed = current_count != prev_key_down_count_;
-    prev_key_down_count_ = current_count;
-    DVLOG_IF(6, key_pressed) << "Detected keypress.";
-  }
-
-  // Use SharedMemory and SyncSocket if the client has created a SyncWriter.
-  // Used by all low-latency clients except WebSpeech.
-  if (SharedMemoryAndSyncSocketMode()) {
-    sync_writer_->Write(source, volume, key_pressed, hardware_delay_bytes);
-
-#if defined(AUDIO_POWER_MONITORING)
-    // Only do power-level measurements if DoCreate() has been called. It will
-    // ensure that logging will mainly be done for WebRTC and WebSpeech
-    // clients.
-    if (!power_measurement_is_enabled_)
-      return;
-
-    // Perform periodic audio (power) level measurements.
-    if ((base::TimeTicks::Now() - last_audio_level_log_time_).InSeconds() >
-        kPowerMonitorLogIntervalSeconds) {
-      // Calculate the average power of the signal, or the energy per sample.
-      const float average_power_dbfs = AveragePower(*source);
-
-      // Add current microphone volume to log and UMA histogram.
-      const int mic_volume_percent = static_cast<int>(100.0 * volume);
-
-      // Use event handler on the audio thread to relay a message to the ARIH
-      // in content which does the actual logging on the IO thread.
-      task_runner_->PostTask(FROM_HERE,
-                             base::Bind(&AudioInputController::DoLogAudioLevels,
-                                        this,
-                                        average_power_dbfs,
-                                        mic_volume_percent));
-
-      last_audio_level_log_time_ = base::TimeTicks::Now();
-    }
-#endif
-    return;
-  }
-
-  // TODO(henrika): Investigate if we can avoid the extra copy here.
-  // (see http://crbug.com/249316 for details). AFAIK, this scope is only
-  // active for WebSpeech clients.
-  std::unique_ptr<AudioBus> audio_data =
-      AudioBus::Create(source->channels(), source->frames());
-  source->CopyTo(audio_data.get());
-
-  // Ownership of the audio buffer will be with the callback until it is run,
-  // when ownership is passed to the callback function.
-  task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(
-          &AudioInputController::DoOnData, this, base::Passed(&audio_data)));
-}
-
-void AudioInputController::DoOnData(std::unique_ptr<AudioBus> data) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  if (handler_)
-    handler_->OnData(this, data.get());
-}
-
 void AudioInputController::DoLogAudioLevels(float level_dbfs,
                                             int microphone_volume_percent) {
 #if defined(AUDIO_POWER_MONITORING)
   DCHECK(task_runner_->BelongsToCurrentThread());
-  if (!handler_)
+  if (!stream_)
     return;
 
   // Detect if the user has enabled hardware mute by pressing the mute
@@ -555,14 +536,9 @@ void AudioInputController::DoLogAudioLevels(float level_dbfs,
 #endif
 }
 
-void AudioInputController::OnError(AudioInputStream* stream) {
-  // Handle error on the audio-manager thread.
-  task_runner_->PostTask(FROM_HERE, base::Bind(
-      &AudioInputController::DoReportError, this));
-}
-
 void AudioInputController::EnableDebugRecording(
     const base::FilePath& file_name) {
+  DCHECK(creator_task_runner_->BelongsToCurrentThread());
   task_runner_->PostTask(
       FROM_HERE, base::Bind(&AudioInputController::DoEnableDebugRecording, this,
                             file_name));
@@ -575,20 +551,6 @@ void AudioInputController::DisableDebugRecording() {
       base::Bind(&AudioInputController::DoDisableDebugRecording, this));
 }
 
-void AudioInputController::DoStopCloseAndClearStream() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  // Allow calling unconditionally and bail if we don't have a stream to close.
-  if (stream_ != nullptr) {
-    stream_->Stop();
-    stream_->Close();
-    stream_ = nullptr;
-  }
-
-  // The event handler should not be touched after the stream has been closed.
-  handler_ = nullptr;
-}
-
 #if defined(AUDIO_POWER_MONITORING)
 void AudioInputController::UpdateSilenceState(bool silence) {
   if (silence) {
@@ -621,15 +583,8 @@ void AudioInputController::LogSilenceState(SilenceState value) {
 
 void AudioInputController::LogCaptureStartupResult(
     CaptureStartupResult result) {
-  // Decrement shall_report_stats and check if it was 1 before decrement,
-  // which would imply that this is the first time this method is called
-  // after initialization. To avoid underflow, we
-  // also check if should_report_stats is one before decrementing.
-  if (base::AtomicRefCountIsOne(&should_report_stats) &&
-      !base::AtomicRefCountDec(&should_report_stats)) {
-    UMA_HISTOGRAM_ENUMERATION("Media.AudioInputControllerCaptureStartupSuccess",
-                              result, CAPTURE_STARTUP_RESULT_MAX + 1);
-  }
+  UMA_HISTOGRAM_ENUMERATION("Media.AudioInputControllerCaptureStartupSuccess",
+                            result, CAPTURE_STARTUP_RESULT_MAX + 1);
 }
 
 void AudioInputController::DoEnableDebugRecording(
@@ -657,4 +612,45 @@ void AudioInputController::LogMessage(const std::string& message) {
   handler_->OnLog(this, message);
 }
 
+bool AudioInputController::CheckForKeyboardInput() {
+  if (!user_input_monitor_)
+    return false;
+
+  size_t current_count = user_input_monitor_->GetKeyPressCount();
+  bool key_pressed = current_count != prev_key_down_count_;

[o1ka, 2017/01/12 12:07:38]

both const?

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

Done.

+  prev_key_down_count_ = current_count;
+  DVLOG_IF(6, key_pressed) << "Detected keypress.";
+
+  return key_pressed;
+}
+
+void AudioInputController::CheckAudioPower(const AudioBus* source,
+                                           double volume) {
+#if defined(AUDIO_POWER_MONITORING)
+  // Only do power-level measurements if DoCreate() has been called. It will
+  // ensure that logging will mainly be done for WebRTC and WebSpeech
+  // clients.
+  if (!power_measurement_is_enabled_)
+    return;
+
+  // Perform periodic audio (power) level measurements.
+  if ((base::TimeTicks::Now() - last_audio_level_log_time_).InSeconds() >
+      kPowerMonitorLogIntervalSeconds) {

[o1ka, 2017/01/12 12:07:38]

Can be early return instead.

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

Done.

+    // Calculate the average power of the signal, or the energy per sample.
+    const float average_power_dbfs = AveragePower(*source);
+
+    // Add current microphone volume to log and UMA histogram.
+    const int mic_volume_percent = static_cast<int>(100.0 * volume);

[o1ka, 2017/01/12 12:07:38]

Is "floor" operation intentional? We won't distinguish between 0.9% and 1% - not
sure if it matters though.

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

I think it's better that henrika@ explains the thinking. I'll add him to the
review.

[henrika (OOO until Aug 14), 2017/01/12 15:16:14]

Good comment. But it does not matter. The point of the stat was to be able to
look for "very low values" to see if the volume control level could be causing a
claimed low volume. 

Guess we could to static_cast<int>(100 * volume + 0.5) to be "more correct". 

[o1ka, 2017/01/12 17:27:15]

Acknowledged.

+
+    // Use event handler on the audio thread to relay a message to the ARIH
+    // in content which does the actual logging on the IO thread.
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&AudioInputController::DoLogAudioLevels, this,
+                              average_power_dbfs, mic_volume_percent));
+
+    last_audio_level_log_time_ = base::TimeTicks::Now();

[o1ka, 2017/01/12 12:07:39]

This is a rather costly call, so I would make it once before l.637.

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

Agreed and likely more accurate too.

+  }
+#endif
+}
+
 }  // namespace media