Use the browser UI thread for audio on OSX.

media/audio/audio_manager_base.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/audio_manager_base.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/command_line.h"
 #include "base/message_loop_proxy.h"
 #include "base/threading/thread.h"
+#include "build/build_config.h"
 #include "media/audio/audio_output_dispatcher_impl.h"
 #include "media/audio/audio_output_proxy.h"
 #include "media/audio/audio_output_resampler.h"
 #include "media/audio/audio_util.h"
 #include "media/audio/fake_audio_input_stream.h"
 #include "media/audio/fake_audio_output_stream.h"
+#include "media/base/media_switches.h"
 
 namespace media {
 
 static const int kStreamCloseDelaySeconds = 5;
 
 // Default maximum number of output streams that can be open simultaneously
 // for all platforms.
 static const int kDefaultMaxOutputStreams = 16;
 
 // Default maximum number of input streams that can be open simultaneously
 // for all platforms.
 static const int kDefaultMaxInputStreams = 16;
 
 static const int kMaxInputChannels = 2;
 
 const char AudioManagerBase::kDefaultDeviceName[] = "Default";
 const char AudioManagerBase::kDefaultDeviceId[] = "default";
 
 AudioManagerBase::AudioManagerBase()
     : num_active_input_streams_(0),
       max_num_output_streams_(kDefaultMaxOutputStreams),
       max_num_input_streams_(kDefaultMaxInputStreams),
       num_output_streams_(0),
       num_input_streams_(0),
       output_listeners_(
           ObserverList<AudioDeviceListener>::NOTIFY_EXISTING_ONLY),
       audio_thread_(new base::Thread("AudioThread")) {
 #if defined(OS_WIN)
   audio_thread_->init_com_with_mta(true);
+#elif defined(OS_MACOSX)
+  // CoreAudio calls must occur on the main thread of the process, which in our
+  // case is sadly the browser UI thread.  Failure to execute calls on the right
+  // thread leads to crashes and odd behavior.  See http://crbug.com/158170.
+  const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
+  if (!cmd_line->HasSwitch(switches::kDisableMainThreadAudio) &&
+      base::MessageLoopProxy::current()) {

[scherkus (not reviewing), 2013/05/31 19:30:35]

when would this be NULL?

[DaleCurtis, 2013/06/03 19:51:10]

Many unittests create an AudioManager without owning a message loop.

[scherkus (not reviewing), 2013/06/03 22:04:28]

Hmm... it's unfortunate that production code is altered for tests. How big of a
change would it be to fix the tests instead of having this check?

Also ... does this mean the new production code path isn't being covered w/
tests?

[DaleCurtis, 2013/06/03 22:26:35]

"Fixing" means giving those tests a MessageLoop and possibly a thread to resolve
a hidden implicit dependency.  Not good.  Ideally everything should just work or
the dependency should be explicit.

Making it explicit means having BrowserMainLoop check the
--disable-main-thread-audio flag and choose to inject its UI message loop or not
and allows all existing tests to continue to function. The downside is we'll
only have test coverage via browser tests then; the upside is the dependency is
a lot clearer.

WDYT?

[DaleCurtis, 2013/06/03 22:59:28]

Actually just changing BrowserMainLoop would means tests will be using the wrong
thread... Which could be a source of occasional flakiness.

[scherkus (not reviewing), 2013/06/03 23:22:18]

I agree that implicit dependencies kinda suck, hence the reason why our code is
pretty explicit about passing in message loops.

While I would say IWBN to change AudioManager::Create() to let clients pass in
an option thread-to-run-on, this case is a bit trickier because it's only used
on one platform and is still controlled via runtime flag. I'd hate to make a big
change if we end up reverting / changing bits of this CL.

I'm fine leaving as is. As you note we should be getting test coverage via
content_browsertests

[DaleCurtis, 2013/06/04 00:15:17]

Lets leave it as is until it's sticky then I'll submit a follow up CL with an
explicit dependency.

[scherkus (not reviewing), 2013/06/04 00:21:30]

SGTM

+    message_loop_ = base::MessageLoopProxy::current();
+    return;
+  }
 #endif
-#if defined(OS_MACOSX)
-  // On Mac, use a UI loop to get native message pump so that CoreAudio property
-  // listener callbacks fire.
-  CHECK(audio_thread_->StartWithOptions(
-      base::Thread::Options(base::MessageLoop::TYPE_UI, 0)));
-#else
+
   CHECK(audio_thread_->Start());

[scherkus (not reviewing), 2013/05/31 19:30:35]

does it make sense to lazily start this thread?

[DaleCurtis, 2013/06/03 19:51:10]

It is lazily started? See if (!worker_loop_) on line 86.

[scherkus (not reviewing), 2013/06/03 22:04:28]

I was referring to the Linux/Win/CrOS/disable-main-thread-audio case where this
thread gets launched immediately when an AudioManager is created.

You don't have to change this in this CL -- mostly curious.

[DaleCurtis, 2013/06/03 22:26:35]

Probably? I don't see any reason why not. We would need to be careful with the
GetMessageLoop() semantics and avoid using message_loop_ directly.

[scherkus (not reviewing), 2013/06/03 23:22:18]

Yeah I'm wonder if there would be any races as AFAIK AM is accessed on the IO
thread.

Reason why I'm wondering is that this code is in the critical path for browser
launch. Deferring a thread launch if it's easy to do so would be nice.

[DaleCurtis, 2013/06/04 00:15:17]

Shouldn't be too hard, we'd need to make generic InitializeOnAudioThread()
methods though that could be called lazily instead of automatically in each
platform specific constructor.  Listed in the AudioManager cleanup bug.

-#endif
-  message_loop_ = audio_thread_->message_loop_proxy();
+  worker_loop_ = message_loop_ = audio_thread_->message_loop_proxy();

[scherkus (not reviewing), 2013/05/31 19:30:35]

nit: can you split the assignments?

multiple assignments are pretty subtle

alternatively ... do we need worker_loop_? couldn't we inspect audio_thread_ as
to whether it's been started and just return audio_thread_->m_l_p()?

[DaleCurtis, 2013/06/03 19:51:10]

Done.  It shouldn't be possible to call GetWorkerLoop() when audio_thread is
NULL.

 }
 
 AudioManagerBase::~AudioManagerBase() {
   // The platform specific AudioManager implementation must have already
   // stopped the audio thread. Otherwise, we may destroy audio streams before
   // stopping the thread, resulting an unexpected behavior.
   // This way we make sure activities of the audio streams are all stopped
   // before we destroy them.
   CHECK(!audio_thread_.get());
   // All the output streams should have been deleted.
   DCHECK_EQ(0, num_output_streams_);
   // All the input streams should have been deleted.
   DCHECK_EQ(0, num_input_streams_);
 }
 
 string16 AudioManagerBase::GetAudioInputDeviceModel() {
   return string16();
 }
 
 scoped_refptr<base::MessageLoopProxy> AudioManagerBase::GetMessageLoop() {
   return message_loop_;
 }
 
+scoped_refptr<base::MessageLoopProxy> AudioManagerBase::GetWorkerLoop() {
+  // Start the worker loop on demand.
+  if (!worker_loop_) {
+    CHECK(audio_thread_->Start());
+    worker_loop_ = audio_thread_->message_loop_proxy();
+  }
+
+  return worker_loop_;
+}
+
 AudioOutputStream* AudioManagerBase::MakeAudioOutputStream(
     const AudioParameters& params) {
   // TODO(miu): Fix ~50 call points across several unit test modules to call
   // this method on the audio thread, then uncomment the following:
   // DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (!params.IsValid()) {
     DLOG(ERROR) << "Audio parameters are invalid";
     return NULL;
   }
@@ skipping 175 matching lines @@
   // via a local variable while not holding the audio thread lock.
   scoped_ptr<base::Thread> audio_thread;
   {
     base::AutoLock lock(audio_thread_lock_);
     audio_thread_.swap(audio_thread);
   }
 
   if (!audio_thread)
     return;
 
-  CHECK_NE(base::MessageLoop::current(), audio_thread->message_loop());
+#if defined(OS_MACOSX)
+  // Only true when we're sharing the UI message loop with the browser.
+  if (message_loop_->BelongsToCurrentThread()) {
+    // After ShutdownOnAudioThread() there should be no tasks running on the
+    // |worker_loop_|.
+    ShutdownOnAudioThread();
+    return audio_thread->Stop();

[scherkus (not reviewing), 2013/05/31 19:30:35]

split return to next line -- it's too subtle and makes it appear like
Thread::Stop() returns a value (it doesn't)

[DaleCurtis, 2013/06/03 19:51:10]

Done.

+  }
+#endif
 
-  // We must use base::Unretained since Shutdown might have been called from
-  // the destructor and we can't alter the refcount of the object at that point.
-  audio_thread->message_loop()->PostTask(FROM_HERE, base::Bind(
-      &AudioManagerBase::ShutdownOnAudioThread,
-      base::Unretained(this)));
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &AudioManagerBase::ShutdownOnAudioThread, base::Unretained(this)));
 
   // Stop() will wait for any posted messages to be processed first.
   audio_thread->Stop();
 }
 
 void AudioManagerBase::ShutdownOnAudioThread() {
 // IOS implements audio input only.
 #if defined(OS_IOS)
   return;
 #else
@@ skipping 42 matching lines @@
   return GetPreferredOutputStreamParameters(AudioParameters());
 }
 
 AudioParameters AudioManagerBase::GetInputStreamParameters(
     const std::string& device_id) {
   NOTREACHED();
   return AudioParameters();
 }
 
 }  // namespace media