Adds Media.Audio.InputStartupSuccessMac UMA stat for Mac OS X

media/audio/mac/audio_low_latency_input_mac.cc

Index: media/audio/mac/audio_low_latency_input_mac.cc
diff --git a/media/audio/mac/audio_low_latency_input_mac.cc b/media/audio/mac/audio_low_latency_input_mac.cc
index e5e51bb3404538184952b17fb1d04a905322fb10..341d1586614b1c097ae61c03874a1ed68e9984df 100644
--- a/media/audio/mac/audio_low_latency_input_mac.cc
+++ b/media/audio/mac/audio_low_latency_input_mac.cc
@@ -9,7 +9,9 @@
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/mac/mac_logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
+#include "base/time/time.h"
 #include "media/audio/mac/audio_manager_mac.h"
 #include "media/base/audio_bus.h"
 #include "media/base/data_buffer.h"
@@ -23,6 +25,12 @@ const int kNumberOfBlocksBufferInFifo = 2;
 // The stream will be stopped as soon as this time limit is passed.
 const int kMaxErrorTimeoutInSeconds = 1;
 
+// A one-shot timer is created and started in Start() and it triggers
+// CheckInputStartupSuccess() after this amount of time. UMA stats marked
+// Media.Audio.InputStartupSuccessMac is then updated where true is added
+// if input callbacks have started, and false otherwise.
+const int kInputCallbackStartTimeoutInSeconds = 5;
+
 static std::ostream& operator<<(std::ostream& os,
                                 const AudioStreamBasicDescription& format) {
   os << "sample rate       : " << format.mSampleRate << std::endl
@@ -53,7 +61,8 @@ AUAudioInputStream::AUAudioInputStream(AudioManagerMac* manager,
       number_of_channels_in_frame_(0),
       fifo_(input_params.channels(),
             number_of_frames_,
-            kNumberOfBlocksBufferInFifo) {
+            kNumberOfBlocksBufferInFifo),
+      input_callback_is_active_(false) {
   DCHECK(manager_);
 
   // Set up the desired (output) format specified by the client.
@@ -91,6 +100,7 @@ AUAudioInputStream::~AUAudioInputStream() {}
 
 // Obtain and open the AUHAL AudioOutputUnit for recording.
 bool AUAudioInputStream::Open() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   // Verify that we are not already opened.
   if (audio_unit_)
     return false;
@@ -225,6 +235,7 @@ bool AUAudioInputStream::Open() {
 }
 
 void AUAudioInputStream::Start(AudioInputCallback* callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(callback);
   DLOG_IF(ERROR, !audio_unit_) << "Open() has not been called successfully";
   if (started_ || !audio_unit_)
@@ -250,26 +261,39 @@ void AUAudioInputStream::Start(AudioInputCallback* callback) {
   OSStatus result = AudioOutputUnitStart(audio_unit_);
   if (result == noErr) {
     started_ = true;
+    // For UMA stat purposes, start a one-shot timer which detects when input
+    // callbacks starts indicating if input audio recording works as intended.
+    // CheckInputStartupSuccess() will check if |input_callback_is_active_| is
+    // true when the timer expires. This timer delay is currently set to
+    // 5 seconds to avoid false alarms.
+    input_callback_timer_.reset(new base::OneShotTimer());
+    input_callback_timer_->Start(
+        FROM_HERE,
+        base::TimeDelta::FromSeconds(kInputCallbackStartTimeoutInSeconds), this,

[tommi (sloooow) - chröme, 2015/10/30 15:08:44]

I think this uses base::Unretained() and due to that there may be a race since
the timer is created on the audio thread, destroyed there as well, but executes
on the browser thread (right?) and there it uses an unretained pointer to the
AUAudioInputStream object.  So if the timer is executing at the time the stream
is being deleted, we would have a use after free problem.

[henrika (OOO until Aug 14), 2015/10/30 15:56:35]

Discussed off line. Special case for Mac which does not create a unique
AudioThread but uses the main browser thread instead.

+        &AUAudioInputStream::CheckInputStartupSuccess);
   }
   OSSTATUS_DLOG_IF(ERROR, result != noErr, result)
       << "Failed to start acquiring data";
 }
 
 void AUAudioInputStream::Stop() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!started_)
     return;
   StopAgc();
+  input_callback_timer_.reset();
   OSStatus result = AudioOutputUnitStop(audio_unit_);
   DCHECK_EQ(result, noErr);
+  SetInputCallbackIsActive(false);
   started_ = false;
   sink_ = NULL;
   fifo_.Clear();
-
   OSSTATUS_DLOG_IF(ERROR, result != noErr, result)
       << "Failed to stop acquiring data";
 }
 
 void AUAudioInputStream::Close() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   // It is valid to call Close() before calling open or Start().
   // It is also valid to call Close() after Start() has been called.
   if (started_) {
@@ -372,7 +396,7 @@ void AUAudioInputStream::SetVolume(double volume) {
 
 double AUAudioInputStream::GetVolume() {
   // Verify that we have a valid device.
-  if (input_device_id_ == kAudioObjectUnknown){
+  if (input_device_id_ == kAudioObjectUnknown) {
     NOTREACHED() << "Device ID is unknown";
     return 0.0;
   }
@@ -466,6 +490,15 @@ OSStatus AUAudioInputStream::InputProc(void* user_data,
   if (!audio_input)
     return kAudioUnitErr_InvalidElement;
 
+  // Indicate that input callbacks have started on the internal AUHAL IO
+  // thread. The |input_callback_is_active_| member is read from the creating
+  // thread when a timer fires once and set to false in Stop() on the same
+  // thread. It means that this thread is the only writer of
+  // |input_callback_is_active_| once the tread starts and it should therefore
+  // be safe to compare and then modify.
+  if (!audio_input->GetInputCallbackIsActive())

[tommi (sloooow) - chröme, 2015/10/30 15:08:44]

is this check necessary?

[henrika (OOO until Aug 14), 2015/10/30 15:56:35]

Not really; removed. Now only calls Set.

+    audio_input->SetInputCallbackIsActive(true);
+
   // Update the |mDataByteSize| value in the audio_buffer_list() since
   // |number_of_frames| can be changed on the fly.
   // |mDataByteSize| needs to be exactly mapping to |number_of_frames|,
@@ -717,4 +750,30 @@ bool AUAudioInputStream::IsVolumeSettableOnChannel(int channel) {
   return (result == noErr) ? is_settable : false;
 }
 
+void AUAudioInputStream::SetInputCallbackIsActive(bool enabled) {
+  base::subtle::Release_Store(&input_callback_is_active_, enabled);
+}
+
+bool AUAudioInputStream::GetInputCallbackIsActive() {
+  return (base::subtle::Acquire_Load(&input_callback_is_active_) != false);
+}
+
+void AUAudioInputStream::CheckInputStartupSuccess() {
+  DCHECK(thread_checker_.CalledOnValidThread());

[tommi (sloooow) - chröme, 2015/10/30 15:08:44]

hmm... this suggests that we're actually not on the main thread but on the audio
thread, right?

[henrika (OOO until Aug 14), 2015/10/30 15:56:35]

For the record, we are on the main thread. Special case for Mac.

+  if (started_) {
+    // Check if we have called Start() and input callbacks have actually
+    // started in time as they should. If that is not the case, we have a
+    // problem and the stream is considered dead.
+    const bool input_callback_is_active = GetInputCallbackIsActive();
+    UMA_HISTOGRAM_BOOLEAN("Media.Audio.InputStartupSuccessMac",
+                          input_callback_is_active);
+    DVLOG(1) << "input_callback_is_active: " << input_callback_is_active;
+
+    if (!input_callback_is_active) {
+      // TODO(henrika): perhaps we should close the stream here and trigger
+      // HandleError with as suitable error code.
+    }
+  }
+}
+
 }  // namespace media