Use the browser UI thread for audio on OSX.

media/audio/audio_manager_base.cc

Index: media/audio/audio_manager_base.cc
diff --git a/media/audio/audio_manager_base.cc b/media/audio/audio_manager_base.cc
index 310e4be16498d5501b74f59f9a1b4d17f46ee577..84da2366ea2f191462673c8db32466a92f02ac59 100644
--- a/media/audio/audio_manager_base.cc
+++ b/media/audio/audio_manager_base.cc
@@ -6,14 +6,17 @@
 
 #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 {
 
@@ -43,16 +46,20 @@ AudioManagerBase::AudioManagerBase()
       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() {
@@ -76,6 +83,16 @@ 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
@@ -271,13 +288,18 @@ void AudioManagerBase::Shutdown() {
   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();