Adding audio unit tests for Android

media/audio/android/opensles_input.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/android/opensles_input.h"
 
 #include "base/logging.h"
 #include "media/audio/android/audio_manager_android.h"
 
 #define LOG_ON_FAILURE_AND_RETURN(op, ...) \
   do { \
     SLresult err = (op);            \
     if (err != SL_RESULT_SUCCESS) { \
       DLOG(ERROR) << #op << " failed: " << err; \
       return __VA_ARGS__; \
     } \
   } while (0)
 
 namespace media {
 
 OpenSLESInputStream::OpenSLESInputStream(AudioManagerAndroid* audio_manager,
                                          const AudioParameters& params)
     : audio_manager_(audio_manager),
       callback_(NULL),
       recorder_(NULL),
       simple_buffer_queue_(NULL),
       active_queue_(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.
   format_.samplesPerSec = static_cast<SLuint32>(params.sample_rate() * 1000);
   format_.bitsPerSample = params.bits_per_sample();
   format_.containerSize = params.bits_per_sample();
   format_.endianness = SL_BYTEORDER_LITTLEENDIAN;
   if (format_.numChannels == 1)
     format_.channelMask = SL_SPEAKER_FRONT_CENTER;
   else if (format_.numChannels == 2)
     format_.channelMask = SL_SPEAKER_FRONT_LEFT | SL_SPEAKER_FRONT_RIGHT;
   else
     NOTREACHED() << "Unsupported number of channels: " << format_.numChannels;
 
   buffer_size_bytes_ = params.GetBytesPerBuffer();
 
   memset(&audio_data_, 0, sizeof(audio_data_));
 }
 
 OpenSLESInputStream::~OpenSLESInputStream() {
+  DVLOG(2) << "OpenSLESInputStream::~OpenSLESInputStream()";
+  DCHECK(thread_checker_.CalledOnValidThread());

[tommi (sloooow) - chröme, 2013/08/29 09:56:27]

nice to have the thread checker in here.

[henrika (OOO until Aug 14), 2013/08/29 11:59:07]

Thanks. It was a really smart guy who suggested it ;-)

   DCHECK(!recorder_object_.Get());
   DCHECK(!engine_object_.Get());
   DCHECK(!recorder_);
   DCHECK(!simple_buffer_queue_);
   DCHECK(!audio_data_[0]);
 }
 
 bool OpenSLESInputStream::Open() {
+  DVLOG(2) << "OpenSLESInputStream::Open()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (engine_object_.Get())
     return false;
 
   if (!CreateRecorder())
     return false;
 
   SetupAudioBuffer();
 
   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_)

[tommi (sloooow) - chröme, 2013/08/29 09:56:27]

this needs to be checked under the lock

[henrika (OOO until Aug 14), 2013/08/29 11:59:07]

Done.

     return;
-
-  // Enable the flags before streaming.
-  callback_ = callback;
-  active_queue_ = 0;
-  started_ = true;
+  {
+    base::AutoLock lock(lock_);
+    // Enable the flags before streaming.
+    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) {
     err = (*simple_buffer_queue_)->Enqueue(

[tommi (sloooow) - chröme, 2013/08/29 09:56:27]

As is, it looks to me that you need to hold the lock whenever you use the
simple_buffer_queue_ variable.  It's being used from the worker thread as well
as here and it's cleared in Close().
If however, there is a way to guarantee that the worker thread is never running
when the value of simple_buffer_queue_ changes, then the lock is not required. 
If that's true though, I would like to see that documented at least to prevent
regressions.

[henrika (OOO until Aug 14), 2013/08/29 11:59:07]

I find it difficult to guarantee that the worker thread is dead at all changes
and therefore extend the scope of the lock somewhat. Can't see that it wort hurt
us.

         simple_buffer_queue_,
         audio_data_[i],
         buffer_size_bytes_);
     if (SL_RESULT_SUCCESS != err) {
       HandleError(err);
       return;
     }
   }
 
   // Start the recording by setting the state to |SL_RECORDSTATE_RECORDING|.
   err = (*recorder_)->SetRecordState(recorder_, SL_RECORDSTATE_RECORDING);
   if (SL_RESULT_SUCCESS != err)
     HandleError(err);
 }
 
 void OpenSLESInputStream::Stop() {
+  DVLOG(2) << "OpenSLESInputStream::Stop()";
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!started_)

[tommi (sloooow) - chröme, 2013/08/29 09:56:27]

need to hold lock.

[henrika (OOO until Aug 14), 2013/08/29 11:59:07]

Done.

     return;
 
   // Stop recording by setting the record state to |SL_RECORDSTATE_STOPPED|.
   LOG_ON_FAILURE_AND_RETURN(
       (*recorder_)->SetRecordState(recorder_,
                                    SL_RECORDSTATE_STOPPED));
+  {
+    base::AutoLock lock(lock_);
 
-  // Clear the buffer queue to get rid of old data when resuming recording.
-  LOG_ON_FAILURE_AND_RETURN(
-      (*simple_buffer_queue_)->Clear(simple_buffer_queue_));
+    // Clear the buffer queue to get rid of old data when resuming recording.
+    LOG_ON_FAILURE_AND_RETURN(
+        (*simple_buffer_queue_)->Clear(simple_buffer_queue_));
 
-  started_ = false;
+    started_ = false;
+  }
 }
 
 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();

[tommi (sloooow) - chröme, 2013/08/29 09:56:27]

If I understand things correctly, resetting the engine should be done last since
it actually deletes the object that implements the interfaces.  As is, you've
here deleted the engine object but simple_buffer_queue_ is still non-NULL but
points to a deleted object (i.e. use after free problem).

[henrika (OOO until Aug 14), 2013/08/29 11:59:07]

Good point, thanks. Analyzed how things are created and rewrote.

-  simple_buffer_queue_ = NULL;
+  {
+    base::AutoLock lock(lock_);
+    simple_buffer_queue_ = NULL;

[tommi (sloooow) - chröme, 2013/08/29 09:56:27]

here you actually do grab the lock before changing the value... this doesn't
mean anything though unless you do the same in all other places that use the
pointer.

[henrika (OOO until Aug 14), 2013/08/29 11:59:07]

Done.

+  }
   recorder_ = NULL;
 
   ReleaseAudioBuffer();
 
   audio_manager_->ReleaseInputStream(this);
 }
 
 double OpenSLESInputStream::GetMaxVolume() {
   NOTIMPLEMENTED();
   return 0.0;
@@ skipping 11 matching lines @@
 void OpenSLESInputStream::SetAutomaticGainControl(bool enabled) {
   NOTIMPLEMENTED();
 }
 
 bool OpenSLESInputStream::GetAutomaticGainControl() {
   NOTIMPLEMENTED();
   return false;
 }
 
 bool OpenSLESInputStream::CreateRecorder() {
+  DCHECK(thread_checker_.CalledOnValidThread());

[tommi (sloooow) - chröme, 2013/08/29 09:56:27]

We should also dcheck that all interface and object pointers are NULL when this
is called.

If this is considered part of the object's construction sequence, it is OK to
not grab the lock (since no other threads should have access to them) but DCHECK
that all member variables are in their newly-constructed state.

[henrika (OOO until Aug 14), 2013/08/29 11:59:07]

Done.

   // Initializes the engine object with specific option. After working with the
   // object, we need to free the object and its resources.
   SLEngineOption option[] = {
     { SL_ENGINEOPTION_THREADSAFE, static_cast<SLuint32>(SL_BOOLEAN_TRUE) }
   };
   LOG_ON_FAILURE_AND_RETURN(slCreateEngine(engine_object_.Receive(),
                                            1,
                                            option,
                                            0,
                                            NULL,
@@ skipping 92 matching lines @@
 }
 
 void OpenSLESInputStream::SimpleBufferQueueCallback(
     SLAndroidSimpleBufferQueueItf buffer_queue, void* instance) {
   OpenSLESInputStream* stream =
       reinterpret_cast<OpenSLESInputStream*>(instance);
   stream->ReadBufferQueue();
 }
 
 void OpenSLESInputStream::ReadBufferQueue() {
+  base::AutoLock lock(lock_);
   if (!started_)
     return;
 
   // TODO(xians): Get an accurate delay estimation.
   callback_->OnData(this,
                     audio_data_[active_queue_],
                     buffer_size_bytes_,
                     buffer_size_bytes_,
                     0.0);
 
   // Done with this buffer. Send it to device for recording.
   SLresult err = (*simple_buffer_queue_)->Enqueue(
       simple_buffer_queue_,
       audio_data_[active_queue_],
       buffer_size_bytes_);
   if (SL_RESULT_SUCCESS != err)
     HandleError(err);
 
   active_queue_ = (active_queue_ + 1) % kNumOfQueuesInBuffer;
 }
 
 void OpenSLESInputStream::SetupAudioBuffer() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!audio_data_[0]);
   for (int i = 0; i < kNumOfQueuesInBuffer; ++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) {
       delete [] audio_data_[i];
       audio_data_[i] = NULL;
     }
   }
 }
 
 void OpenSLESInputStream::HandleError(SLresult error) {
   DLOG(FATAL) << "OpenSLES Input error " << error;
   if (callback_)
     callback_->OnError(this);
 }
 
 }  // namespace media