Adding audio unit tests for Android

media/audio/android/opensles_input.cc

Index: media/audio/android/opensles_input.cc
diff --git a/media/audio/android/opensles_input.cc b/media/audio/android/opensles_input.cc
index 15c3eac3726389bb7f8d0a7f37ee4b8d35a82f91..115bf3340696b204b0a607e28f68f5999a4cdd2a 100644
--- a/media/audio/android/opensles_input.cc
+++ b/media/audio/android/opensles_input.cc
@@ -24,9 +24,10 @@ OpenSLESInputStream::OpenSLESInputStream(AudioManagerAndroid* audio_manager,
       callback_(NULL),
       recorder_(NULL),
       simple_buffer_queue_(NULL),
-      active_queue_(0),
+      active_buffer_(0),
       buffer_size_bytes_(0),
       started_(false) {
+  DVLOG(2) << "OpenSLESInputStream::OpenSLESInputStream()";
   format_.formatType = SL_DATAFORMAT_PCM;
   format_.numChannels = static_cast<SLuint32>(params.channels());
   // Provides sampling rate in milliHertz to OpenSLES.
@@ -47,6 +48,8 @@ OpenSLESInputStream::OpenSLESInputStream(AudioManagerAndroid* audio_manager,
 }
 
 OpenSLESInputStream::~OpenSLESInputStream() {
+  DVLOG(2) << "OpenSLESInputStream::~OpenSLESInputStream()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!recorder_object_.Get());
   DCHECK(!engine_object_.Get());
   DCHECK(!recorder_);
@@ -55,6 +58,8 @@ OpenSLESInputStream::~OpenSLESInputStream() {
 }
 
 bool OpenSLESInputStream::Open() {
+  DVLOG(2) << "OpenSLESInputStream::Open()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (engine_object_.Get())
     return false;
 
@@ -62,25 +67,43 @@ bool OpenSLESInputStream::Open() {
     return false;
 
   SetupAudioBuffer();
+  active_buffer_ = 0;
 
   return true;
 }
 
 void OpenSLESInputStream::Start(AudioInputCallback* callback) {
+  DVLOG(2) << "OpenSLESInputStream::Start()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(callback);
+  DCHECK(!callback_);

[tommi (sloooow) - chröme, 2013/08/30 14:37:13]

nit: keep the dcheck behind the lock

[henrika (OOO until Aug 14), 2013/08/30 14:44:27]

Done.

[henrika (OOO until Aug 14), 2013/09/02 09:36:15]

Actually, this will not work since we don't reset callback in Stop. The same is
true for all other platforms as well and the reason is that callback is used in
Close to signal OnClose and cant be cleared in Stop. Hope it is OK if I don't
add this check here now.

[tommi (sloooow) - chröme, 2013/09/02 09:49:41]

That's either a design flaw or we simply have bugs on all platforms.  If the
callback is set in Start, it should be cleared in Stop (assuming an
implementation bug).  Alternatively, the interface could be changed so that the
callback is set in Open() and then cleared in Close().  Given that the callback
has an OnClose method, I'm leaning towards design flaw.

As a temporary workaround, I'd like the DCHECK to then be to check: DCHECK(NULL
|| callback_ == callback);
As is, a call sequence of Open()/Start()/Stop()/Start()/Stop()/Close() would
potentially only call OnClose() on the second callback, not the first.

   DCHECK(recorder_);
   DCHECK(simple_buffer_queue_);
   if (started_)
     return;
 
-  // Enable the flags before streaming.
+  base::AutoLock lock(lock_);
   callback_ = callback;
-  active_queue_ = 0;
-  started_ = true;
 
-  SLresult err = SL_RESULT_UNKNOWN_ERROR;
-  // Enqueues |kNumOfQueuesInBuffer| zero buffers to get the ball rolling.
-  for (int i = 0; i < kNumOfQueuesInBuffer; ++i) {
+  SLAndroidSimpleBufferQueueState buffer_queue_state;
+  SLresult err = (*simple_buffer_queue_)->GetState(simple_buffer_queue_,
+                                                   &buffer_queue_state);
+  if (SL_RESULT_SUCCESS != err) {
+    HandleError(err);
+    return;
+  }
+
+  // Number of free buffers in the queue.
+  int num_free_buffers = kMaxNumOfBuffersInQueue - buffer_queue_state.count;
+  DCHECK(num_free_buffers == kMaxNumOfBuffersInQueue || num_free_buffers == 0);
+
+  // Enqueues |num_free_buffers| zero buffers to get the ball rolling.
+  // |num_free_buffers| can be zero if Stop has been called followed by a
+  // new call to Start. We do clear the queue in Stop, and the buffer state
+  // should then be cleared, but for some reason it is not. Using this
+  // approach enables call sequences like: Start, Stop, Start, even if there
+  // might be some old data remaining at the second call to Start.
+  for (int i = 0; i < num_free_buffers; ++i) {
     err = (*simple_buffer_queue_)->Enqueue(
         simple_buffer_queue_,
         audio_data_[i],
@@ -93,14 +116,21 @@ void OpenSLESInputStream::Start(AudioInputCallback* callback) {
 
   // Start the recording by setting the state to |SL_RECORDSTATE_RECORDING|.
   err = (*recorder_)->SetRecordState(recorder_, SL_RECORDSTATE_RECORDING);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
     HandleError(err);
+  }
+
+  started_ = true;

[tommi (sloooow) - chröme, 2013/08/30 14:37:13]

if we failed to set the state to 'recording' I don't think we should flip this
switch.
Stop() basically just tries to flip the switch back, which won't work if we
didn't succeed in the first place.

[henrika (OOO until Aug 14), 2013/08/30 14:44:27]

Done.

 }
 
 void OpenSLESInputStream::Stop() {
+  DVLOG(2) << "OpenSLESInputStream::Stop()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!started_)
     return;
 
+  base::AutoLock lock(lock_);
+
   // Stop recording by setting the record state to |SL_RECORDSTATE_STOPPED|.
   LOG_ON_FAILURE_AND_RETURN(
       (*recorder_)->SetRecordState(recorder_,
@@ -114,17 +144,30 @@ void OpenSLESInputStream::Stop() {
 }
 
 void OpenSLESInputStream::Close() {
+  DVLOG(2) << "OpenSLESInputStream::Close()";
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   // Stop the stream if it is still recording.
   Stop();
+  {
+    base::AutoLock lock(lock_);
+
+    if (callback_) {
+      callback_->OnClose(this);
+      callback_ = NULL;
+    }
 
-  // Explicitly free the player objects and invalidate their associated
-  // interfaces. They have to be done in the correct order.
-  recorder_object_.Reset();
-  engine_object_.Reset();
-  simple_buffer_queue_ = NULL;
-  recorder_ = NULL;
+    // Destroy the buffer queue recorder object and invalidate all associated
+    // interfaces.
+    recorder_object_.Reset();
+    simple_buffer_queue_ = NULL;
+    recorder_ = NULL;
 
-  ReleaseAudioBuffer();
+    // Destroy the engine object. We don't store any associated interface for
+    // this object.
+    engine_object_.Reset();
+    ReleaseAudioBuffer();
+  }
 
   audio_manager_->ReleaseInputStream(this);
 }
@@ -153,6 +196,12 @@ bool OpenSLESInputStream::GetAutomaticGainControl() {
 }
 
 bool OpenSLESInputStream::CreateRecorder() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!engine_object_.Get());
+  DCHECK(!recorder_object_.Get());
+  DCHECK(!recorder_);
+  DCHECK(!simple_buffer_queue_);
+
   // Initializes the engine object with specific option. After working with the
   // object, we need to free the object and its resources.
   SLEngineOption option[] = {
@@ -188,7 +237,7 @@ bool OpenSLESInputStream::CreateRecorder() {
   // Audio sink configuration.
   SLDataLocator_AndroidSimpleBufferQueue buffer_queue = {
     SL_DATALOCATOR_ANDROIDSIMPLEBUFFERQUEUE,  // Locator type.
-    static_cast<SLuint32>(kNumOfQueuesInBuffer)  // Number of buffers.
+    static_cast<SLuint32>(kMaxNumOfBuffersInQueue)  // Number of buffers.
   };
   SLDataSink audio_sink = { &buffer_queue, &format_ };
 
@@ -201,6 +250,7 @@ bool OpenSLESInputStream::CreateRecorder() {
     SL_BOOLEAN_TRUE,
     SL_BOOLEAN_TRUE
   };
+
   // Create AudioRecorder and specify SL_IID_ANDROIDCONFIGURATION.
   LOG_ON_FAILURE_AND_RETURN(
       (*engine)->CreateAudioRecorder(engine,
@@ -219,6 +269,7 @@ bool OpenSLESInputStream::CreateRecorder() {
                                      &recorder_config),
       false);
 
+  // Uses the main microphone tuned for audio communications.
   SLint32 stream_type = SL_ANDROID_RECORDING_PRESET_VOICE_COMMUNICATION;
   LOG_ON_FAILURE_AND_RETURN(
       (*recorder_config)->SetConfiguration(recorder_config,
@@ -265,37 +316,50 @@ void OpenSLESInputStream::SimpleBufferQueueCallback(
 }
 
 void OpenSLESInputStream::ReadBufferQueue() {
-  if (!started_)
+  base::AutoLock lock(lock_);
+
+  // Verify that we are in a recording state.
+  SLuint32 state;
+  SLresult err = (*recorder_)->GetRecordState(recorder_, &state);
+  if (SL_RESULT_SUCCESS != err) {
+    HandleError(err);
     return;
+  }
+  if (state != SL_RECORDSTATE_RECORDING) {
+    DLOG(WARNING) << "Received callback in non-recording state";
+    return;
+  }
 
   // TODO(xians): Get an accurate delay estimation.
   callback_->OnData(this,
-                    audio_data_[active_queue_],
+                    audio_data_[active_buffer_],
                     buffer_size_bytes_,
                     buffer_size_bytes_,
                     0.0);
 
   // Done with this buffer. Send it to device for recording.
-  SLresult err = (*simple_buffer_queue_)->Enqueue(
+  err = (*simple_buffer_queue_)->Enqueue(

[tommi (sloooow) - chröme, 2013/08/30 14:37:13]

I just noticed that a buffer_queue pointer is supplied to the callback.  Would
it make sense to dcheck that it's the same pointer as we use here or should we
use the pointer that's given to us?

[henrika (OOO until Aug 14), 2013/08/30 14:44:27]

Offline discussions => we decided that I could ignore this comment.

       simple_buffer_queue_,
-      audio_data_[active_queue_],
+      audio_data_[active_buffer_],
       buffer_size_bytes_);
   if (SL_RESULT_SUCCESS != err)
     HandleError(err);
 
-  active_queue_ = (active_queue_ + 1) % kNumOfQueuesInBuffer;
+  active_buffer_ = (active_buffer_ + 1) % kMaxNumOfBuffersInQueue;
 }
 
 void OpenSLESInputStream::SetupAudioBuffer() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!audio_data_[0]);
-  for (int i = 0; i < kNumOfQueuesInBuffer; ++i) {
+  for (int i = 0; i < kMaxNumOfBuffersInQueue; ++i) {
     audio_data_[i] = new uint8[buffer_size_bytes_];
   }
 }
 
 void OpenSLESInputStream::ReleaseAudioBuffer() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (audio_data_[0]) {
-    for (int i = 0; i < kNumOfQueuesInBuffer; ++i) {
+    for (int i = 0; i < kMaxNumOfBuffersInQueue; ++i) {
       delete [] audio_data_[i];
       audio_data_[i] = NULL;
     }
@@ -303,7 +367,7 @@ void OpenSLESInputStream::ReleaseAudioBuffer() {
 }
 
 void OpenSLESInputStream::HandleError(SLresult error) {
-  DLOG(FATAL) << "OpenSLES Input error " << error;
+  DLOG(ERROR) << "OpenSLES Input error " << error;
   if (callback_)
     callback_->OnError(this);
 }