Add AudioInputControllerCaptureStartupSuccess UMA

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 "base/bind.h"
 #include "base/threading/thread_restrictions.h"
 #include "media/base/limits.h"
 #include "media/base/scoped_histogram_timer.h"
@@ skipping 191 matching lines @@
     return;
   }
 
   DCHECK(!no_data_timer_.get());
 
   // The timer is enabled for logging purposes. The NO_DATA_ERROR triggered
   // from the timer must be ignored by the EventHandler.
   // TODO(henrika): remove usage of timer when it has been verified on Canary
   // that we are safe doing so. Goal is to get rid of |no_data_timer_| and
   // everything that is tied to it. crbug.com/357569.
   enable_nodata_timer = true;

[henrika (OOO until Aug 14), 2014/05/09 13:15:07]

When was this guy enabled again? I had disabled it to avoid issues on Mac and
now it is on again. What has happened?

[tommi (sloooow) - chröme, 2014/05/09 13:40:43]

This was changed about a month ago:
https://codereview.chromium.org/229573003

 
   if (enable_nodata_timer) {
-    // Create the data timer which will call DoCheckForNoData(). The timer
+    // Create the data timer which will call FirstCheckForNoData(). The timer
     // is started in DoRecord() and restarted in each DoCheckForNoData()
     // callback.
     no_data_timer_.reset(new base::Timer(
         FROM_HERE, base::TimeDelta::FromSeconds(kTimerInitialIntervalSeconds),
-        base::Bind(&AudioInputController::DoCheckForNoData,
+        base::Bind(&AudioInputController::FirstCheckForNoData,
                    base::Unretained(this)), false));
   } else {
     DVLOG(1) << "Disabled: timer check for no data.";
   }
 
   state_ = CREATED;
   if (handler_)
     handler_->OnCreated(this);
 
   if (user_input_monitor_) {
     user_input_monitor_->EnableKeyPressMonitoring();
     prev_key_down_count_ = user_input_monitor_->GetKeyPressCount();
   }
 }
 
 void AudioInputController::DoRecord() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   SCOPED_UMA_HISTOGRAM_TIMER("Media.AudioInputController.RecordTime");
 
   if (state_ != CREATED)
     return;
 
   {
     base::AutoLock auto_lock(lock_);
     state_ = RECORDING;
   }
 
   if (no_data_timer_) {
     // Start the data timer. Once |kTimerResetIntervalSeconds| have passed,
-    // a callback to DoCheckForNoData() is made.
+    // a callback to FirstCheckForNoData() is made.
     no_data_timer_->Reset();
   }
 
   stream_->Start(this);
   if (handler_)
     handler_->OnRecording(this);
 }
 
 void AudioInputController::DoClose() {
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ skipping 50 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_NE(state_, RECORDING);
 
   // Ensure that the AGC state only can be modified before streaming starts.
   if (state_ != CREATED)
     return;
 
   stream_->SetAutomaticGainControl(enabled);
 }
 
+void AudioInputController::FirstCheckForNoData() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  UMA_HISTOGRAM_BOOLEAN("Media.AudioInputControllerCaptureStartupSuccess",
+                        GetDataIsActive());

[tommi (sloooow) - chröme, 2014/05/09 13:40:43]

How do you know that SetDataIsActive has been called at this point?

[no longer working on chromium, 2014/05/09 13:48:22]

OnData() calls SetDataIsActive(true) when getting recording data. So if OnData()
is called before the check, GetDataIsActive() return a true, which means the
capture starts successfully.
If GetDataIsActive() is a false, that means we do not get any OnData() after the
stream has started.

Let me know if I misunderstand things here.

[tommi (sloooow) - chröme, 2014/05/09 13:56:21]

Yeah I know how it's supposed to work but I'm wondering how we make sure we
don't report false positives.
If we get this callback before OnData is called - but OnData gets called
immediately after, then we will have incorrectly recorded that we capture is not
working.  I'm basically asking, is that not possible? and if so, how do we
guarantee that it is not possible.

[tommi (sloooow) - chröme, 2014/05/09 14:46:15]

Actually, I think this is fine.  The 5 second period should be more than enough
to check this, so lgtm.

One more thing to think about down the line.  If we start seeing that
GetDataIsActive returns 0 in our logs, what would be the next steps to look at? 
Also, see one request below.

+  DoCheckForNoData();
+}
+
 void AudioInputController::DoCheckForNoData() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (!GetDataIsActive()) {
     // The data-is-active marker will be false only if it has been more than
     // one second since a data packet was recorded. This can happen if a
     // capture device has been removed or disabled.
     if (handler_)
       handler_->OnError(this, NO_DATA_ERROR);
   }
@@ skipping 25 matching lines @@
   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.";
   }
 
   // Mark data as active to ensure that the periodic calls to
   // DoCheckForNoData() does not report an error to the event handler.
   SetDataIsActive(true);

[tommi (sloooow) - chröme, 2014/05/09 14:46:15]

Can you move this flag to the top of the function, above where we set the state?

[no longer working on chromium, 2014/05/09 15:35:07]

Done.

 
   // Use SharedMemory and SyncSocket if the client has created a SyncWriter.
   // Used by all low-latency clients except WebSpeech.
   if (SharedMemoryAndSyncSocketMode()) {
     sync_writer_->Write(data, size, volume, key_pressed);
     sync_writer_->UpdateRecordedBytes(hardware_delay_bytes);
     return;
   }
 
   // TODO(henrika): Investigate if we can avoid the extra copy here.
@@ skipping 36 matching lines @@
 
 void AudioInputController::SetDataIsActive(bool enabled) {
   base::subtle::Release_Store(&data_is_active_, enabled);
 }
 
 bool AudioInputController::GetDataIsActive() {
   return (base::subtle::Acquire_Load(&data_is_active_) != false);
 }
 
 }  // namespace media