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..ba4eafdefaeaa305bb0b9640dc776700019e1090 100644
--- a/media/audio/android/opensles_input.cc
+++ b/media/audio/android/opensles_input.cc
@@ -4,6 +4,7 @@
 
 #include "media/audio/android/opensles_input.h"
 
+#include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "media/audio/android/audio_manager_android.h"
 
@@ -24,9 +25,10 @@ OpenSLESInputStream::OpenSLESInputStream(AudioManagerAndroid* audio_manager,
       callback_(NULL),
       recorder_(NULL),
       simple_buffer_queue_(NULL),
-      active_queue_(0),
+      active_buffer_index_(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 +49,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 +59,8 @@ OpenSLESInputStream::~OpenSLESInputStream() {
 }
 
 bool OpenSLESInputStream::Open() {
+  DVLOG(2) << "OpenSLESInputStream::Open()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (engine_object_.Get())
     return false;
 
@@ -62,25 +68,42 @@ bool OpenSLESInputStream::Open() {
     return false;
 
   SetupAudioBuffer();
+  active_buffer_index_ = 0;
 
   return true;
 }
 
 void OpenSLESInputStream::Start(AudioInputCallback* callback) {
+  DVLOG(2) << "OpenSLESInputStream::Start()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(callback);
   DCHECK(recorder_);
   DCHECK(simple_buffer_queue_);
   if (started_)
     return;
 
-  // Enable the flags before streaming.
+  base::AutoLock lock(lock_);
+  DCHECK(callback_ == NULL || callback_ == callback);

[wjia(left Chromium), 2013/09/03 22:36:46]

It's more appropriate to clear |callback_| in Stop() and DCHECK(!callback_)
here.

[henrika (OOO until Aug 14), 2013/09/04 07:44:57]

Actually, No. Since the current design is that the callback is used to generate
OnClose() in Close(). It does not seem to be used on any platform but I don't
think it is OK to break this pattern only on Andrtoid. All other platforms calls
OnClose() in Close().

PS Tommi had the same comment but we landed in the existing compromise.

   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) {
+  // Check queue state and derive the number of free buffers in the queue.
+  SLAndroidSimpleBufferQueueState buffer_queue_state;
+  SLresult err = (*simple_buffer_queue_)->GetState(simple_buffer_queue_,
+                                                   &buffer_queue_state);
+  if (SL_RESULT_SUCCESS != err) {
+    HandleError(err);
+    return;
+  }
+  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) {

[wjia(left Chromium), 2013/09/03 22:36:46]

num_free_buffers is used incorrectly here, because:
1. Stop() has cleared simple_buffer_queue_. There should be no buffer held on
OpenSL side.
2. Even though some buffers are held at OpenSL side, it's possible they are
audio_data_[0 ...], not always audio_data_[num_free_buffers ...
kMaxNumOfBuffersInQueue].

It's better to just clear simple_buffer_queue_ when Stop() is called. That's
much simpler. We'd DCHECK_EQ(buffer_queue_state.count, 0).

[henrika (OOO until Aug 14), 2013/09/04 07:44:57]

I have verified this scheme on four different devices and we need it since:

Clear() does not clear simple_buffer_queue_ and buffer_queue_state.count is
*not* set to 0, hence the proposed DCHECK would always fail. My tests shows that
buffer_queue_state.count is always 2 in Stop(). i.e., 2 buffers are ready to be
filled.

num_free_buffers can only take two different values, 2 or 0 (see DCHECK above).
It will always be 2 if we have done Start/Stop and then call Start again, and
always 0 at the first Start.

Also, I don't reset active_buffer_index_ in Start, which ensures that the next
buffer which is queued is the correct one even at the second Start call.

Note that, for the output side, Clear() does clear the buffer but not for the
input side. I think that the reason is that OpenSLES expects that one should get
some extra/remaining callbacks even after Stop() is called and after that the
queue is cleared. I tried to do this but it does not seems to be possible using
a simple queue. It could work with the default Open SLES queue but I think we
are fine as is.

To summarize: the new code does not make any changes to the default
Start/Stop/Close call but it adds the possibility to do Start/Stop/Start... The
old code failed for Start/Stop/Start since we could not call Enqueue the second
time. Hence, I am solving an existing problem here.

[wjia(left Chromium), 2013/09/05 22:09:03]

The essential problem is that Clear() doesn't work as expected. But
buffer_queue_state.count is unnecessarily always be either 0 or 2, because there
is another rare case in which buffer_queue_state.count could be any value
between 0 and kMaxNumOfBuffersInQueue. Just think about a similar case to what I
mentioned in comment #20. When OpenSL delivers a buffer back, right before it
acquires the lock, it's preempted. Stop() is called. When OpenSL thread resumes,
it will return immediately without delivering buffer to client, and
OpenSLESInputStream will not send the buffer back to OpenSL either. That means
buffer_queue_state.count could be 1 now (at least, it's not 2). Then Start() is
called. 

The options are:
1. don't support start/stop/start/...
2. support start/stop/start, need a cleaner workaround. For example, keep track
of which buffers are on OpenSLESInputStream side. That will allow you to send
correct buffers to OpenSL when restarting it.

[henrika (OOO until Aug 14), 2013/09/06 08:04:53]

I understand your concern but note that the current implementation fails
directly when you do Start/Stop/Start and I have so far not been able to make
the new version fail.

I am a bit tempted to remove support for Start/Stop/Start and then try to
reinsert it once the OpenSLES bug is solved. Not sure though how to signal to
the user that the second Start failed (since it returns void).

     err = (*simple_buffer_queue_)->Enqueue(
         simple_buffer_queue_,
         audio_data_[i],
@@ -91,17 +114,27 @@ void OpenSLESInputStream::Start(AudioInputCallback* callback) {
     }
   }
 
-  // Start the recording by setting the state to |SL_RECORDSTATE_RECORDING|.
+  // Start the recording by setting the state to SL_RECORDSTATE_RECORDING.
+  // When the object is in the SL_RECORDSTATE_RECORDING state, adding buffers
+  // will implicitly start the filling process.
   err = (*recorder_)->SetRecordState(recorder_, SL_RECORDSTATE_RECORDING);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
     HandleError(err);
+    return;
+  }
+
+  started_ = true;
 }
 
 void OpenSLESInputStream::Stop() {
+  DVLOG(2) << "OpenSLESInputStream::Stop()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!started_)
     return;
 
-  // Stop recording by setting the record state to |SL_RECORDSTATE_STOPPED|.
+  base::AutoLock lock(lock_);
+
+  // Stop recording by setting the record state to SL_RECORDSTATE_STOPPED.
   LOG_ON_FAILURE_AND_RETURN(
       (*recorder_)->SetRecordState(recorder_,
                                    SL_RECORDSTATE_STOPPED));
@@ -114,17 +147,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_);
 
-  // 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;
+    if (callback_) {
+      callback_->OnClose(this);
+      callback_ = 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 +199,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[] = {
@@ -187,8 +239,8 @@ bool OpenSLESInputStream::CreateRecorder() {
 
   // Audio sink configuration.
   SLDataLocator_AndroidSimpleBufferQueue buffer_queue = {
-    SL_DATALOCATOR_ANDROIDSIMPLEBUFFERQUEUE,  // Locator type.
-    static_cast<SLuint32>(kNumOfQueuesInBuffer)  // Number of buffers.
+    SL_DATALOCATOR_ANDROIDSIMPLEBUFFERQUEUE,
+    static_cast<SLuint32>(kMaxNumOfBuffersInQueue)
   };
   SLDataSink audio_sink = { &buffer_queue, &format_ };
 
@@ -201,6 +253,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 +272,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 +319,55 @@ void OpenSLESInputStream::SimpleBufferQueueCallback(
 }
 
 void OpenSLESInputStream::ReadBufferQueue() {
-  if (!started_)
+  base::AutoLock lock(lock_);
+  if (!recorder_)

[wjia(left Chromium), 2013/09/03 22:36:46]

|recorder_| is cleared in Closed(). That means after Stop() is called, we will
still send data back to client. That's incorrect. Therefore, the desired
behavior is to clear |callback_| in Stop() and check |callback_| here.

[henrika (OOO until Aug 14), 2013/09/04 07:44:57]

Same comment as above. |callback_| must be used in Close(). The complete API
must be redesigned before we can change that. Best long-term solution is
probably to move the callback to Open() instead.

The current check was added to deal with the special case you mentioned earlier
where we could crash.

[wjia(left Chromium), 2013/09/05 22:09:03]

You can use |started_|, instead of |recorder_|, since neither |recorder_| or
|callback_| can be used as "stop" signal. Then the following check is not
needed.

[henrika (OOO until Aug 14), 2013/09/06 13:38:10]

Thanks. Will think again and probably restore the original solution.

[henrika (OOO until Aug 14), 2013/09/06 15:59:51]

Ignore my previous comment. Should be Done.

     return;
 
-  // TODO(xians): Get an accurate delay estimation.
+  TRACE_EVENT0("audio", "OpenSLESOutputStream::ReadBufferQueue");
+
+  // 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(henrika): Investigate if it is possible to get an accurate
+  // delay estimation.
   callback_->OnData(this,
-                    audio_data_[active_queue_],
+                    audio_data_[active_buffer_index_],
                     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(
       simple_buffer_queue_,
-      audio_data_[active_queue_],
+      audio_data_[active_buffer_index_],
       buffer_size_bytes_);
   if (SL_RESULT_SUCCESS != err)
     HandleError(err);
 
-  active_queue_ = (active_queue_ + 1) % kNumOfQueuesInBuffer;
+  active_buffer_index_ = (active_buffer_index_ + 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 +375,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);
 }