Initialization of audio manager for Android is now done on the audio thread

media/audio/android/audio_manager_android.cc

Index: media/audio/android/audio_manager_android.cc
diff --git a/media/audio/android/audio_manager_android.cc b/media/audio/android/audio_manager_android.cc
index 33e9e085688de87c37eebfbdbfdeddfb9949f2db..760ec8f53c725d72c521a226c2f708624e1050d3 100644
--- a/media/audio/android/audio_manager_android.cc
+++ b/media/audio/android/audio_manager_android.cc
@@ -9,6 +9,7 @@
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/logging.h"
+#include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "jni/AudioManagerAndroid_jni.h"
 #include "media/audio/android/audio_record_input.h"
@@ -47,18 +48,12 @@ AudioManager* CreateAudioManager(AudioLogFactory* audio_log_factory) {
 AudioManagerAndroid::AudioManagerAndroid(AudioLogFactory* audio_log_factory)
     : AudioManagerBase(audio_log_factory) {
   SetMaxOutputStreamsAllowed(kMaxOutputStreams);
-
-  j_audio_manager_.Reset(
-      Java_AudioManagerAndroid_createAudioManagerAndroid(
-          base::android::AttachCurrentThread(),
-          base::android::GetApplicationContext(),
-          reinterpret_cast<intptr_t>(this)));
-  Init();
 }
 
 AudioManagerAndroid::~AudioManagerAndroid() {
-  Close();
   Shutdown();
+  // Verify that WillDestroyCurrentMessageLoop() has been called.
+  DCHECK(j_audio_manager_.is_null());
 }
 
 bool AudioManagerAndroid::HasAudioOutputDevices() {
@@ -71,26 +66,24 @@ bool AudioManagerAndroid::HasAudioInputDevices() {
 
 void AudioManagerAndroid::GetAudioInputDeviceNames(
     AudioDeviceNames* device_names) {
-  // Always add default device parameters as first element.
-  DCHECK(device_names->empty());
-  AddDefaultDevice(device_names);
-
-  JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jobjectArray> j_device_array =
-      Java_AudioManagerAndroid_getAudioInputDeviceNames(
-          env, j_audio_manager_.obj());
-  jsize len = env->GetArrayLength(j_device_array.obj());
-  AudioDeviceName device;
-  for (jsize i = 0; i < len; ++i) {
-    ScopedJavaLocalRef<jobject> j_device(
-        env, env->GetObjectArrayElement(j_device_array.obj(), i));
-    ScopedJavaLocalRef<jstring> j_device_name =
-        Java_AudioDeviceName_name(env, j_device.obj());
-    ConvertJavaStringToUTF8(env, j_device_name.obj(), &device.device_name);
-    ScopedJavaLocalRef<jstring> j_device_id =
-        Java_AudioDeviceName_id(env, j_device.obj());
-    ConvertJavaStringToUTF8(env, j_device_id.obj(), &device.unique_id);
-    device_names->push_back(device);
+  CreateAndInitOnAudioThread();
+
+  // Get list of available audio devices and use the audio thread to reduce
+  // the number of calling threads to the Java layer. Also, ensure that the
+  // calling thread sees this function call as synchronous.

[bulach, 2014/02/05 19:29:39]

what is the calling thread? we obviously try to avoid blocking threads as much
as possible.. is there any chance of changing this API to be fully asynchronous,
i.e., rather than taking a AudioDeviceNames* get a callback that would be
triggered later on?
it's probably more involved than this patch, but may be worth investigating.

[henrika (OOO until Aug 14), 2014/02/05 21:47:30]

Right now the calling threads are a device manager thread or the IO thread. It
will be changed to the audio manager thread or the IO thread. This method is
sync for all other platforms and the idea here was to not change that pattern
since it causes lots of changes. And as you have noticed, this is a rather large
"fix" as is..

[DaleCurtis, 2014/02/05 21:50:13]

I suspect you can completely remove the IO thread usage from this entire system
with some clever usage of OverrideTaskRunner.  See an example here:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

[bulach, 2014/02/06 02:06:56]

yeah, let's separate the issues here.. :)
1) I'm hoping the original bug can be fixed (tentatively, at least) with just
changes in the java side. that should be far more straightforward to review,
land, cherry-pick into release branch and launch...

fairly orthogonal, and could be done in multiple smaller steps:
2) whilst I quite like the original idea of this patch, i.e., to make the
initialization less heavy, blocking the IO thread[1] is a no-no.. I'd much
rather favor a redesign of the API to make it fully asynchronous rather than
just a tweak in the implementation (trying to keep a "synchronous" API by
blocking it under the cover...)..

[1] sorry, this is fairly confusing to me.. is it BrowserThread::IO or the
"Audio IO" mentioned in audio_manager.h? I think I'm missing the thread model
diagram you mentioned on one of the TODOs.. I'd love to see that so I could get
a better understanding! also, it's not quite clear to me why do we need to
reduce the number of calling threads to the java layer... that side seems thread
safe, no?

anyways.... I don't want to be a pain, I just want to help splitting this beast
in smaller / more manageable chunks, and solve each problem separately.. how
does that sound?

[henrika (OOO until Aug 14), 2014/02/06 08:58:57]

Calls to GetAudioInputDeviceNames (7 occurrences) - excluding unit tests below
=> 2 calls:

src/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc
 (1 occurrence)
    407 AudioManager::Get()->GetAudioInputDeviceNames(&source_devices_);

Calls on the audio thread (I don't call this an IO thread). See
AudioManager::GetTaskRunner() for details.

src/content/browser/renderer_host/media/audio_input_device_manager.cc
 (1 occurrence)
    133 audio_manager_->GetAudioInputDeviceNames(&device_names);

Calls on the audio thread. Same as above.

Hence, I was wrong. It is only called on the audio thread, not the browser IO
thread. Plus unit tests.

[tommi (sloooow) - chröme, 2014/02/06 13:52:58]

I looked into this and from what I can tell there is only a single call site to
GetAudioInputDeviceNames that is not already on the audio thread and that call
site is here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
(inside AudioInputDeviceManager::EnumerateOnDeviceThread).

This can be very easily fixed by using the audio manager's task runner instead
of the device_loop_ here:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

(inside AudioInputDeviceManager::EnumerateDevices)

It's so simple and it doesn't need any clever tricks.  The OverrideTaskRunner
option is good to know about though.

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

Now only called on audio thread. Modified.

+  scoped_refptr<base::SingleThreadTaskRunner> task_runner(GetTaskRunner());
+  if (task_runner->BelongsToCurrentThread()) {
+    GetAudioInputDeviceNamesOnAudioThread(NULL, device_names);
+  } else {
+    base::WaitableEvent event(false, false);
+    task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &AudioManagerAndroid::GetAudioInputDeviceNamesOnAudioThread,
+            base::Unretained(this),
+            &event,
+            device_names));
+    event.Wait();
   }
 }
 
@@ -102,10 +95,17 @@ void AudioManagerAndroid::GetAudioOutputDeviceNames(
 
 AudioParameters AudioManagerAndroid::GetInputStreamParameters(
     const std::string& device_id) {
-  JNIEnv* env = AttachCurrentThread();
+  // TODO(henrika): improve documentation regarding the threading model.
+  // We are on the audio thread here once the MediaStreamDeviceThread is
+  // removed and replaced by the audio thread but we also call this one from
+  // RenderMessageFilter::OnGetAudioHardwareConfig() on the IO thread.
+  CreateAndInitOnAudioThread();
+
   // Use mono as preferred number of input channels on Android to save
   // resources. Using mono also avoids a driver issue seen on Samsung
   // Galaxy S3 and S4 devices. See http://crbug.com/256851 for details.
+  JNIEnv* env = AttachCurrentThread();
+  CHECK(env);
   ChannelLayout channel_layout = CHANNEL_LAYOUT_MONO;
   int buffer_size = Java_AudioManagerAndroid_getMinInputFrameSize(
       env, GetNativeOutputSampleRate(),
@@ -124,7 +124,9 @@ AudioOutputStream* AudioManagerAndroid::MakeAudioOutputStream(
     const AudioParameters& params,
     const std::string& device_id,
     const std::string& input_device_id) {
-  bool had_no_streams = HadNoAudioStreams();
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  CreateAndInitOnAudioThread();
+  bool has_no_streams = HasNoAudioStreams();
   AudioOutputStream* stream =
       AudioManagerBase::MakeAudioOutputStream(params, std::string(),
           std::string());
@@ -133,7 +135,7 @@ AudioOutputStream* AudioManagerAndroid::MakeAudioOutputStream(
   // VoIP sessions and therefore sets the audio mode to MODE_IN_COMMUNICATION.
   // If a Bluetooth headset is used, the audio stream will use the SCO
   // channel and therefore have a limited bandwidth (8-16kHz).
-  if (stream && had_no_streams)
+  if (stream && has_no_streams)
     SetCommunicationAudioModeOn(true);
 
   {
@@ -146,7 +148,9 @@ AudioOutputStream* AudioManagerAndroid::MakeAudioOutputStream(
 
 AudioInputStream* AudioManagerAndroid::MakeAudioInputStream(
     const AudioParameters& params, const std::string& device_id) {
-  bool had_no_streams = HadNoAudioStreams();
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  CreateAndInitOnAudioThread();
+  bool has_no_streams = HasNoAudioStreams();
   AudioInputStream* stream =
       AudioManagerBase::MakeAudioInputStream(params, device_id);
 
@@ -154,7 +158,7 @@ AudioInputStream* AudioManagerAndroid::MakeAudioInputStream(
   // VoIP sessions and therefore sets the audio mode to MODE_IN_COMMUNICATION.
   // If a Bluetooth headset is used, the audio stream will use the SCO
   // channel and therefore have a limited bandwidth (8kHz).
-  if (stream && had_no_streams)
+  if (stream && has_no_streams)
     SetCommunicationAudioModeOn(true);
   return stream;
 }
@@ -164,18 +168,20 @@ void AudioManagerAndroid::ReleaseOutputStream(AudioOutputStream* stream) {
 
   // Restore the audio mode which was used before the first communication-
   // mode stream was created.
-  if (HadNoAudioStreams())
+  if (HasNoAudioStreams())
     SetCommunicationAudioModeOn(false);
   base::AutoLock lock(streams_lock_);
   streams_.erase(static_cast<OpenSLESOutputStream*>(stream));
 }
 
 void AudioManagerAndroid::ReleaseInputStream(AudioInputStream* stream) {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  DCHECK(!j_audio_manager_.is_null());
   AudioManagerBase::ReleaseInputStream(stream);
 
   // Restore the audio mode which was used before the first communication-
   // mode stream was created.
-  if (HadNoAudioStreams())
+  if (HasNoAudioStreams())
     SetCommunicationAudioModeOn(false);
 }
 
@@ -191,6 +197,8 @@ AudioOutputStream* AudioManagerAndroid::MakeLowLatencyOutputStream(
     const std::string& input_device_id) {
   DLOG_IF(ERROR, !device_id.empty()) << "Not implemented!";
   DCHECK_EQ(AudioParameters::AUDIO_PCM_LOW_LATENCY, params.format());
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  CreateAndInitOnAudioThread();
   return new OpenSLESOutputStream(this, params);
 }
 
@@ -200,6 +208,8 @@ AudioInputStream* AudioManagerAndroid::MakeLinearInputStream(
   // needs it.
   DLOG_IF(ERROR, !device_id.empty()) << "Not implemented!";
   DCHECK_EQ(AudioParameters::AUDIO_PCM_LINEAR, params.format());
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  CreateAndInitOnAudioThread();
   return new OpenSLESInputStream(this, params);
 }
 
@@ -207,6 +217,9 @@ AudioInputStream* AudioManagerAndroid::MakeLowLatencyInputStream(
     const AudioParameters& params, const std::string& device_id) {
   DCHECK_EQ(AudioParameters::AUDIO_PCM_LOW_LATENCY, params.format());
   DLOG_IF(ERROR, device_id.empty()) << "Invalid device ID!";
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  CreateAndInitOnAudioThread();
+
   // Use the device ID to select the correct input device.
   // Note that the input device is always associated with a certain output
   // device, i.e., this selection does also switch the output device.
@@ -232,21 +245,29 @@ AudioInputStream* AudioManagerAndroid::MakeLowLatencyInputStream(
   return new OpenSLESInputStream(this, params);
 }
 
-int AudioManagerAndroid::GetOptimalOutputFrameSize(int sample_rate,
-                                                   int channels) {
-  if (IsAudioLowLatencySupported()) {
-    return GetAudioLowLatencyOutputFrameSize();
-  } else {
-    return std::max(kDefaultOutputBufferSize,
-                    Java_AudioManagerAndroid_getMinOutputFrameSize(
-                        base::android::AttachCurrentThread(),
-                        sample_rate, channels));
-  }
+// static
+bool AudioManagerAndroid::RegisterAudioManager(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+void AudioManagerAndroid::SetMute(JNIEnv* env, jobject obj, jboolean muted) {
+  GetTaskRunner()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &AudioManagerAndroid::DoSetMuteOnAudioThread,
+          base::Unretained(this),
+          muted));
 }
 
 AudioParameters AudioManagerAndroid::GetPreferredOutputStreamParameters(
     const std::string& output_device_id,
     const AudioParameters& input_params) {
+  // TODO(henrika): improve documentation regarding the threading model.
+  // We are on the audio thread here once the MediaStreamDeviceThread is
+  // removed and replaced by the audio thread but we also call this one from
+  // RenderMessageFilter::OnGetAudioHardwareConfig() on the IO thread.
+  CreateAndInitOnAudioThread();
+
   // TODO(tommi): Support |output_device_id|.
   DLOG_IF(ERROR, !output_device_id.empty()) << "Not implemented!";
   ChannelLayout channel_layout = CHANNEL_LAYOUT_STEREO;
@@ -273,13 +294,68 @@ AudioParameters AudioManagerAndroid::GetPreferredOutputStreamParameters(
       sample_rate, bits_per_sample, buffer_size, AudioParameters::NO_EFFECTS);
 }
 
-bool AudioManagerAndroid::HadNoAudioStreams() {
-  return output_stream_count() == 0 && input_stream_count() == 0;
+void AudioManagerAndroid::WillDestroyCurrentMessageLoop() {
+  CloseOnAudioThread();
 }
 
-// static
-bool AudioManagerAndroid::RegisterAudioManager(JNIEnv* env) {
-  return RegisterNativesImpl(env);
+void AudioManagerAndroid::CreateAndInitOnAudioThread() {
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner(GetTaskRunner());
+  if (task_runner->BelongsToCurrentThread()) {
+    DoCreateAndInitOnAudioThread(NULL);
+  } else {
+    base::WaitableEvent event(false, false);

[bulach, 2014/02/05 19:29:39]

yeah, as above, this is a really bad pattern, we should try to avoid it..

[henrika (OOO until Aug 14), 2014/02/05 21:47:30]

I think it is a topic which we perhaps could discuss some more. But I hear you
and will of course take it into consideration. Seems like we will break out a
more isolated CL from this one instead of landing as is. So, hope it is OK if we
close the topic at a later stage.

+    task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &AudioManagerAndroid::DoCreateAndInitOnAudioThread,
+            base::Unretained(this),
+            &event));
+    event.Wait();
+  }
+}
+
+void AudioManagerAndroid::DoCreateAndInitOnAudioThread(
+    base::WaitableEvent* event) {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  if (!j_audio_manager_.is_null()) {
+    if (event)
+      event->Signal();
+    return;
+  }
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  CHECK(env);
+
+  // Create the Android audio manager on the audio thread.
+  DVLOG(1) << "Creating Java part of the audio manager";
+  j_audio_manager_.Reset(
+    Java_AudioManagerAndroid_createAudioManagerAndroid(
+        env,
+        base::android::GetApplicationContext(),
+        reinterpret_cast<intptr_t>(this)));
+
+  // Prepare the list of audio devices and register receivers for device
+  // notifications.
+  Init();
+
+  // Ensure that we are notified when the audio thread dies.
+  base::MessageLoop::current()->AddDestructionObserver(this);
+
+  if (event)
+    event->Signal();
+}
+
+void AudioManagerAndroid::CloseOnAudioThread() {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  if (j_audio_manager_.is_null())
+    return;
+  Close();
+  DVLOG(1) << "Destroying Java part of the audio manager";
+  j_audio_manager_.Reset();
+}
+
+bool AudioManagerAndroid::HasNoAudioStreams() {
+  return output_stream_count() == 0 && input_stream_count() == 0;
 }
 
 void AudioManagerAndroid::Init() {
@@ -294,23 +370,6 @@ void AudioManagerAndroid::Close() {
       j_audio_manager_.obj());
 }
 
-void AudioManagerAndroid::SetMute(JNIEnv* env, jobject obj, jboolean muted) {
-  GetTaskRunner()->PostTask(
-      FROM_HERE,
-      base::Bind(
-          &AudioManagerAndroid::DoSetMuteOnAudioThread,
-          base::Unretained(this),
-          muted));
-}
-
-void AudioManagerAndroid::DoSetMuteOnAudioThread(bool muted) {
-  base::AutoLock lock(streams_lock_);
-  for (OutputStreams::iterator it = streams_.begin();
-       it != streams_.end(); ++it) {
-    (*it)->SetMute(muted);
-  }
-}
-
 void AudioManagerAndroid::SetCommunicationAudioModeOn(bool on) {
   Java_AudioManagerAndroid_setCommunicationAudioModeOn(
       base::android::AttachCurrentThread(),
@@ -318,11 +377,13 @@ void AudioManagerAndroid::SetCommunicationAudioModeOn(bool on) {
 }
 
 bool AudioManagerAndroid::SetAudioDevice(const std::string& device_id) {
-  JNIEnv* env = AttachCurrentThread();
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
 
   // Send the unique device ID to the Java audio manager and make the
   // device switch. Provide an empty string to the Java audio manager
   // if the default device is selected.
+  JNIEnv* env = AttachCurrentThread();
+  CHECK(env);
   ScopedJavaLocalRef<jstring> j_device_id = ConvertUTF8ToJavaString(
       env,
       device_id == AudioManagerBase::kDefaultDeviceId ?
@@ -349,4 +410,55 @@ int AudioManagerAndroid::GetAudioLowLatencyOutputFrameSize() {
       j_audio_manager_.obj());
 }
 
+int AudioManagerAndroid::GetOptimalOutputFrameSize(int sample_rate,
+                                                   int channels) {
+  if (IsAudioLowLatencySupported()) {
+    return GetAudioLowLatencyOutputFrameSize();
+  } else {
+    return std::max(kDefaultOutputBufferSize,
+                    Java_AudioManagerAndroid_getMinOutputFrameSize(
+                        base::android::AttachCurrentThread(),
+                        sample_rate, channels));
+  }
+}
+
+void AudioManagerAndroid::GetAudioInputDeviceNamesOnAudioThread(
+    base::WaitableEvent* event, AudioDeviceNames* device_names) {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+
+  // Always add default device parameters as first element.
+  DCHECK(device_names->empty());
+  AddDefaultDevice(device_names);
+
+  JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobjectArray> j_device_array =
+      Java_AudioManagerAndroid_getAudioInputDeviceNames(
+          env, j_audio_manager_.obj());
+  jsize len = env->GetArrayLength(j_device_array.obj());
+  AudioDeviceName device;
+  for (jsize i = 0; i < len; ++i) {
+    ScopedJavaLocalRef<jobject> j_device(
+        env, env->GetObjectArrayElement(j_device_array.obj(), i));
+    ScopedJavaLocalRef<jstring> j_device_name =
+        Java_AudioDeviceName_name(env, j_device.obj());
+    ConvertJavaStringToUTF8(env, j_device_name.obj(), &device.device_name);
+    ScopedJavaLocalRef<jstring> j_device_id =
+        Java_AudioDeviceName_id(env, j_device.obj());
+    ConvertJavaStringToUTF8(env, j_device_id.obj(), &device.unique_id);
+    device_names->push_back(device);
+  }
+
+  if (event)
+    event->Signal();
+}
+
+void AudioManagerAndroid::DoSetMuteOnAudioThread(bool muted) {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  base::AutoLock lock(streams_lock_);
+  for (OutputStreams::iterator it = streams_.begin();
+       it != streams_.end(); ++it) {
+    (*it)->SetMute(muted);
+  }
+}
+
 }  // namespace media