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 a8d81c7e829af4ffa2697427db608c73ecfdbbdf..a9b10c63b2841791991a3333da507972fbdedf7c 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,10 +66,13 @@ bool AudioManagerAndroid::HasAudioInputDevices() {
 
 void AudioManagerAndroid::GetAudioInputDeviceNames(
     AudioDeviceNames* device_names) {
+  CreateAndInitOnAudioThread();
+
   // Always add default device parameters as first element.
   DCHECK(device_names->empty());
   AddDefaultDevice(device_names);
 
+  // Get list of available audio devices.
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobjectArray> j_device_array =
       Java_AudioManagerAndroid_getAudioInputDeviceNames(
@@ -102,10 +100,13 @@ void AudioManagerAndroid::GetAudioOutputDeviceNames(
 
 AudioParameters AudioManagerAndroid::GetInputStreamParameters(
     const std::string& device_id) {
-  JNIEnv* env = AttachCurrentThread();
+  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(),
@@ -123,28 +124,27 @@ AudioParameters AudioManagerAndroid::GetInputStreamParameters(
 AudioOutputStream* AudioManagerAndroid::MakeAudioOutputStream(
     const AudioParameters& params,
     const std::string& device_id) {
-  bool had_no_streams = HadNoAudioStreams();
+  CreateAndInitOnAudioThread();
+  bool has_no_streams = HasNoAudioStreams();
   AudioOutputStream* stream =
       AudioManagerBase::MakeAudioOutputStream(params, std::string());
 
   // The audio manager for Android creates streams intended for real-time
   // 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
+  // If a Bluetooth headset is selected, 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);
 
-  {
-    base::AutoLock lock(streams_lock_);
-    streams_.insert(static_cast<OpenSLESOutputStream*>(stream));
-  }
+  streams_.insert(static_cast<OpenSLESOutputStream*>(stream));
 
   return stream;
 }
 
 AudioInputStream* AudioManagerAndroid::MakeAudioInputStream(
     const AudioParameters& params, const std::string& device_id) {
-  bool had_no_streams = HadNoAudioStreams();
+  CreateAndInitOnAudioThread();
+  bool has_no_streams = HasNoAudioStreams();
   AudioInputStream* stream =
       AudioManagerBase::MakeAudioInputStream(params, device_id);
 
@@ -152,28 +152,31 @@ 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;
 }
 
 void AudioManagerAndroid::ReleaseOutputStream(AudioOutputStream* stream) {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
   AudioManagerBase::ReleaseOutputStream(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);
 }
 
@@ -188,6 +191,7 @@ AudioOutputStream* AudioManagerAndroid::MakeLowLatencyOutputStream(
     const std::string& device_id) {
   DLOG_IF(ERROR, !device_id.empty()) << "Not implemented!";
   DCHECK_EQ(AudioParameters::AUDIO_PCM_LOW_LATENCY, params.format());
+  CreateAndInitOnAudioThread();
   return new OpenSLESOutputStream(this, params);
 }
 
@@ -197,6 +201,7 @@ AudioInputStream* AudioManagerAndroid::MakeLinearInputStream(
   // needs it.
   DLOG_IF(ERROR, !device_id.empty()) << "Not implemented!";
   DCHECK_EQ(AudioParameters::AUDIO_PCM_LINEAR, params.format());
+  CreateAndInitOnAudioThread();
   return new OpenSLESInputStream(this, params);
 }
 
@@ -204,6 +209,8 @@ 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!";
+  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.
@@ -229,21 +236,25 @@ 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) {
+  CreateAndInitOnAudioThread();

[DaleCurtis, 2014/02/18 19:13:14]

You mention in the description that you wanted to make construction less heavy. 
Can you elaborate on that (ideally in the description of the CL or in a bug)? 
Specifically I'm wondering if your intent is to minimize the work on the UI
thread or to always defer AM construction until necessary (and if so, why?).

Now that you're using the audio thread for everything my wonder is that you
should just always post the initialization task to the audio thread during
construction like we do on other platforms.

Doing so simplifies the code here significantly by not having to run this task
in every method (as well as not running the risk that future methods forget to
do so).

[henrika (OOO until Aug 14), 2014/02/19 10:37:01]

It started out with the crash reported here:
https://codereview.chromium.org/152783005 where I initially had most parts of
this CL included in my fix. Marcus felt that it was too much and I did a more
isolated patch and kept this one as a separate "generic improvement CL".

The idea that we had was that we wanted to be able to start Chrome without
accessing any audio hardware (check if BT device exists, check if wireless
headset exists etc.) and wait with populating the list until it was needed for
the first time. On other OS:es, enumeration is done by the OS when we ask for it
and I wanted to do the same here.

When minimizing the work at startup, we realized that the threading model was
complicated and we might as well solve that as well. Goal is to clean things up
to avoid synchronization in the Java layer.

I could post the init task at construction but then I would always trigger the
enumeration (check if BT is enabled etc) and I would like to keep it as simple
as possible.

But yes, given that we are now on the audio thread, I guess that is possible. 

Do you feel that it is a requirement from your side?

tommi@: what's your opinion? 

[tommi (sloooow) - chröme, 2014/02/19 10:48:35]

Yes, I think that's (Dale's suggestion) a good idea.

[henrika (OOO until Aug 14), 2014/02/19 13:45:13]

Done.

+
   // TODO(tommi): Support |output_device_id|.
   DLOG_IF(ERROR, !output_device_id.empty()) << "Not implemented!";
   ChannelLayout channel_layout = CHANNEL_LAYOUT_STEREO;
@@ -270,13 +281,47 @@ 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() {

[DaleCurtis, 2014/02/18 19:13:14]

Regardless of the outcome from above, instead of using a message loop observer
can you just post CloseOnAudioThread() prior to calling Shutdown() in the
destructor?  That's how we handle this on other platforms.

[henrika (OOO until Aug 14), 2014/02/19 10:37:01]

Will harmonize. Thanks for pointing that out.

[henrika (OOO until Aug 14), 2014/02/19 13:45:13]

Done.

+  CloseOnAudioThread();
 }
 
-// static
-bool AudioManagerAndroid::RegisterAudioManager(JNIEnv* env) {
-  return RegisterNativesImpl(env);
+void AudioManagerAndroid::CreateAndInitOnAudioThread() {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+
+  // Ensure that we only create and initialize the Java part once.
+  if (!j_audio_manager_.is_null())
+    return;
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  CHECK(env);
+
+  // Create the Android audio manager on the audio thread.
+  DVLOG(2) << "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);
+}
+
+void AudioManagerAndroid::CloseOnAudioThread() {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  if (j_audio_manager_.is_null())
+    return;
+  Close();
+  DVLOG(2) << "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() {

[DaleCurtis, 2014/02/18 19:13:14]

These two methods are only called in one place each.  It'd be better just to
merge them into the relevant callers.

[henrika (OOO until Aug 14), 2014/02/19 10:37:01]

Will fix.

@@ -291,23 +336,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(),
@@ -315,11 +343,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 ?
@@ -346,4 +376,24 @@ 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::DoSetMuteOnAudioThread(bool muted) {
+  DCHECK(GetTaskRunner()->BelongsToCurrentThread());
+  for (OutputStreams::iterator it = streams_.begin();
+       it != streams_.end(); ++it) {
+    (*it)->SetMute(muted);
+  }
+}
+
 }  // namespace media