Handle audio device changes on Mac (Take 2).

Handle audio device changes on Mac (Take 2).

Takeover from sail's CL.  I've uploaded his patch set #9 as
the base ("Original") for this CL for comparison;
https://codereview.chromium.org/11344005/

This is the Mac version of this Windows CL:
https://codereview.chromium.org/11233023/

Essentially we now listen for default device changes (including
things like modifying the sample rate) and then forward those
calls through the AudioManager::AudioDeviceListener framework.

BUG=156788
TEST=Keep audio stream playing while changing devices through
Audio MIDI Setup.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165597

[DaleCurtis, 2012-10-30 01:34:45]

scherkus, crogers: Please review.
sail, shess: FYI.

Per discussion with scherkus there's no point in creating tons of code for an
unlikely hypothetical situation.  In this vein I've changed the CL such that the
callback is passed directly to the AudioObjectPropertyListenerProc and stack
allocated within AudioManagerMac.  Even in the worst hypothetical case where the
callback is being called after AudioObjectRemovePropertyListener() completes
(which I find highly unlikely), we should still be in good shape.

If we find crashes in the field we can switch to the uglier singleton model or
some other fancy hotness.  I doubt even if this is racy we'd ever see a problem
though since the likelihood of a device change occurring precisely during
shutdown seems infinitesimally small.

[scherkus (not reviewing), 2012-10-31 23:17:22]

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
File media/audio/mac/audio_device_listener_mac.cc (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
media/audio/mac/audio_device_listener_mac.cc:54: if (!listener_cb_)
you can use is_null() instead since you DCHECK it can't be null when created

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
File media/audio/mac/audio_device_listener_mac.h (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
media/audio/mac/audio_device_listener_mac.h:29: base::Closure* listener_cb_;
what the!? just make a copy

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_low_...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_low_...
media/audio/mac/audio_low_latency_output_mac.cc:120: AudioComponent comp;
what's up w/ the change here?

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_mana...
File media/audio/mac/audio_manager_mac.h (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.h:44: virtual void InitializeOnAudioThread()
OVERRIDE;
silliness alert!

Initialize() is an empty virtual provided by AMB + called by
AudioManager::Create()

Destruct is handled by the OS-specific AM impl

Seeing as GetMessageLoop() is guaranteed to be valid inside of an AMImpl how
about we move Initialize to be OS-specific AM impl and remove the PostTask()
from AM::Create?

[Scott Hess - ex-Googler, 2012-10-31 23:39:11]

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
File media/audio/mac/audio_device_listener_mac.cc (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
media/audio/mac/audio_device_listener_mac.cc:28:
static_cast<base::Closure*>(context)->Run();
If you've arranged for the callback to live forever, then you don't need
AudioDeviceListenerMac class and manual scoped_ptr junk at all, it can just be
static methods to register/unregister.

If you _haven't_ arranged for the callback to live forever, then I don't see how
you're providing thread-safety.

AFAICT, you've only arranged for the context-callback to outlive
AudioDeviceListenerMac, and for AudioDeviceListenerMac to be created and deleted
on the "audio" thread (which AFAICT has no relationship to the thread that this
callback function is called on).  So this callback function could be executing
when ~AudioDeviceListenerMac() has completed.

[scherkus (not reviewing), 2012-11-01 00:15:40]

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
File media/audio/mac/audio_device_listener_mac.cc (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
media/audio/mac/audio_device_listener_mac.cc:28:
static_cast<base::Closure*>(context)->Run();
On 2012/10/31 23:39:11, shess wrote:
> If you've arranged for the callback to live forever, then you don't need
> AudioDeviceListenerMac class and manual scoped_ptr junk at all, it can just be
> static methods to register/unregister.

I do like having register/unregister auto-scoped, but I will concede we're not
taking advantage of that today :)

> If you _haven't_ arranged for the callback to live forever, then I don't see
how
> you're providing thread-safety.
> 
> AFAICT, you've only arranged for the context-callback to outlive
> AudioDeviceListenerMac, and for AudioDeviceListenerMac to be created and
deleted
> on the "audio" thread (which AFAICT has no relationship to the thread that
this
> callback function is called on).  So this callback function could be executing
> when ~AudioDeviceListenerMac() has completed.

While it's certainly possible to have the callback execute _during_ ~ADLM(), I
haven't seen any documentation/evidence that states whether it's possible for
the callback to execute after removing the listener.

I certainly hope AudioObjectRemovePropertyListener() guarantees that any
existing callback would complete and that no further callbacks are executed --
otherwise it forces everyone to handle a spurious callback firing w/ potentially
invalid userdata pointer!

[DaleCurtis, 2012-11-01 00:16:33]

<just comments>

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
File media/audio/mac/audio_device_listener_mac.cc (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
media/audio/mac/audio_device_listener_mac.cc:28:
static_cast<base::Closure*>(context)->Run();
On 2012/10/31 23:39:11, shess wrote:
> If you've arranged for the callback to live forever, then you don't need
> AudioDeviceListenerMac class and manual scoped_ptr junk at all, it can just be
> static methods to register/unregister.
> 
> If you _haven't_ arranged for the callback to live forever, then I don't see
how
> you're providing thread-safety.
> 
> AFAICT, you've only arranged for the context-callback to outlive
> AudioDeviceListenerMac, and for AudioDeviceListenerMac to be created and
deleted
> on the "audio" thread (which AFAICT has no relationship to the thread that
this
> callback function is called on).  So this callback function could be executing
> when ~AudioDeviceListenerMac() has completed.

Indeed it could, which is exactly what the method documentation says.

The callback now lives nearly "forever" since AudioManager won't be destroyed
until BrowserMainLoop dies which is near enough to process shutdown that I don't
think we need to worry about it any further.  If we see crashes in the field we
can consider a singleton type pattern.

static register/unregister methods aren't a bad idea and less crazy than keeping
a local pointer copy. I'll switch to that in the next patch set.

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
media/audio/mac/audio_device_listener_mac.cc:54: if (!listener_cb_)
On 2012/10/31 23:17:22, scherkus wrote:
> you can use is_null() instead since you DCHECK it can't be null when created

No, it's set to NULL if AddPropertyListener fails.

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
File media/audio/mac/audio_device_listener_mac.h (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
media/audio/mac/audio_device_listener_mac.h:29: base::Closure* listener_cb_;
On 2012/10/31 23:17:22, scherkus wrote:
> what the!? just make a copy

Nope, we want the callback to outlive AudioDeviceListenerMac, if we reference a
copy and a callback is in progress during destruction it'll use-after-free.

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_low_...
File media/audio/mac/audio_low_latency_output_mac.cc (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_low_...
media/audio/mac/audio_low_latency_output_mac.cc:120: AudioComponent comp;
On 2012/10/31 23:17:22, scherkus wrote:
> what's up w/ the change here?

Taken from sail's original patch set, I was hoping he or crogers could provide
some additional context.

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_mana...
File media/audio/mac/audio_manager_mac.h (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.h:44: virtual void InitializeOnAudioThread()
OVERRIDE;
On 2012/10/31 23:17:22, scherkus wrote:
> silliness alert!
> 
> Initialize() is an empty virtual provided by AMB + called by
> AudioManager::Create()
> 
> Destruct is handled by the OS-specific AM impl
> 
> Seeing as GetMessageLoop() is guaranteed to be valid inside of an AMImpl how
> about we move Initialize to be OS-specific AM impl and remove the PostTask()
> from AM::Create?

I don't know how that would work? You can't post a task to the message loop on
another thread using "this" during construction. That was the original design
and that was why we had to move InitializeOnAudioThread() to AM::Create().

[scherkus (not reviewing), 2012-11-01 00:47:48]

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
File media/audio/mac/audio_device_listener_mac.h (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_devi...
media/audio/mac/audio_device_listener_mac.h:29: base::Closure* listener_cb_;
On 2012/11/01 00:16:33, DaleCurtis wrote:
> On 2012/10/31 23:17:22, scherkus wrote:
> > what the!? just make a copy
> 
> Nope, we want the callback to outlive AudioDeviceListenerMac, if we reference
a
> copy and a callback is in progress during destruction it'll use-after-free.

This concern still feels hinged on not knowing if it's possible to have the
callback execute post-removing-the-listener.

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_mana...
File media/audio/mac/audio_manager_mac.h (right):

http://codereview.chromium.org/11338024/diff/11001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.h:44: virtual void InitializeOnAudioThread()
OVERRIDE;
On 2012/11/01 00:16:33, DaleCurtis wrote:
> On 2012/10/31 23:17:22, scherkus wrote:
> > silliness alert!
> > 
> > Initialize() is an empty virtual provided by AMB + called by
> > AudioManager::Create()
> > 
> > Destruct is handled by the OS-specific AM impl
> > 
> > Seeing as GetMessageLoop() is guaranteed to be valid inside of an AMImpl how
> > about we move Initialize to be OS-specific AM impl and remove the PostTask()
> > from AM::Create?
> 
> I don't know how that would work? You can't post a task to the message loop on
> another thread using "this" during construction. That was the original design
> and that was why we had to move InitializeOnAudioThread() to AM::Create().

I believe you're referring to https://chromiumcodereview.appspot.com/11260002/

In that case you were posting a task from inside the non-concrete
AudioManagerBase ctor but as a result introduced a race as the subclass' ctor
hadn't executed yet.

If you post a task using |this| inside of a concrete ctor (e.g., AMLinux, AMWin)
it's functionally the same as executing it post-concrete-ctor as
AudioManager::Create().

[Scott Hess - ex-Googler, 2012-11-01 00:50:25]

Hmm.

On 2012/11/01 00:15:40, scherkus wrote:
> I certainly hope AudioObjectRemovePropertyListener() guarantees that any
> existing callback would complete and that no further callbacks are executed --
> otherwise it forces everyone to handle a spurious callback firing w/
potentially
> invalid userdata pointer!

I'm willing to grant that a well-written API could have made sure that the
callbacks have completed before the remove function returns.  OTOH, there's no
way for the API to prevent deadlock in that case if the current thread holds a
lock which the callback requires.  That's why the earlier CL ended up with us
making sure the callback and registration were mutually synchronized.

I don't have any evidence either way on this.  I think hitting the race is super
unlikely, so I can see ignoring it if it's annoying to code around.

On 2012/11/01 00:16:33, DaleCurtis wrote:
> The callback now lives nearly "forever" since AudioManager won't be destroyed
> until BrowserMainLoop dies which is near enough to process shutdown that I
don't
> think we need to worry about it any further.  If we see crashes in the field
we
> can consider a singleton type pattern.

I don't quite follow.  The registration scoper is being torn down in the
AudioManager destructor, so the callback is guaranteed to not live much longer
than the scoper.  What really saves you is that notifications of changes just
won't happen all that often.

> static register/unregister methods aren't a bad idea and less crazy than
keeping
> a local pointer copy. I'll switch to that in the next patch set.

I guess my concern with using a scoping register/unregister is that the calling
code then takes care to destruct only on the audio thread, which to me argues
that you might as well not use the indirection in the first place.  If you're
doing it all manually, might as well just dispense with the additional class and
pull things right into audio_manager_mac.cc.

[DaleCurtis, 2012-11-01 01:08:29]

On 2012/11/01 00:50:25, shess wrote:
> Hmm.
> 
> On 2012/11/01 00:15:40, scherkus wrote:
> > I certainly hope AudioObjectRemovePropertyListener() guarantees that any
> > existing callback would complete and that no further callbacks are executed
--
> > otherwise it forces everyone to handle a spurious callback firing w/
> potentially
> > invalid userdata pointer!
> 
> I'm willing to grant that a well-written API could have made sure that the
> callbacks have completed before the remove function returns.  OTOH, there's no
> way for the API to prevent deadlock in that case if the current thread holds a
> lock which the callback requires.  That's why the earlier CL ended up with us
> making sure the callback and registration were mutually synchronized.
> 
> I don't have any evidence either way on this.  I think hitting the race is
super
> unlikely, so I can see ignoring it if it's annoying to code around.
> 
> On 2012/11/01 00:16:33, DaleCurtis wrote:
> > The callback now lives nearly "forever" since AudioManager won't be
destroyed
> > until BrowserMainLoop dies which is near enough to process shutdown that I
> don't
> > think we need to worry about it any further.  If we see crashes in the field
> we
> > can consider a singleton type pattern.
> 
> I don't quite follow.  The registration scoper is being torn down in the
> AudioManager destructor, so the callback is guaranteed to not live much longer
> than the scoper.  What really saves you is that notifications of changes just
> won't happen all that often.
> 
> > static register/unregister methods aren't a bad idea and less crazy than
> keeping
> > a local pointer copy. I'll switch to that in the next patch set.
> 
> I guess my concern with using a scoping register/unregister is that the
calling
> code then takes care to destruct only on the audio thread, which to me argues
> that you might as well not use the indirection in the first place.  If you're
> doing it all manually, might as well just dispense with the additional class
and
> pull things right into audio_manager_mac.cc.

Given the uncertainty around lifetimes, I think that's a prudent choice. I've
collapsed everything into the AudioManagerMac class which removes a lot of
boiler plate code. PTAL.

[scherkus (not reviewing), 2012-11-01 01:20:14]

LGTM w/ nits

lol'ing at how small, then large, then small, then exceedingly small this CL got

http://codereview.chromium.org/11338024/diff/17001/media/audio/mac/audio_mana...
File media/audio/mac/audio_manager_mac.cc (right):

http://codereview.chromium.org/11338024/diff/17001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.cc:235: // Property address to monitor for
device changes.
how about replacing this comment by changing kPA to kDeviceChangePropertyAddress
(you would do this anyway if you had multiple property addresses)

http://codereview.chromium.org/11338024/diff/17001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.cc:268: // Clear |listener_cb_| so we don't
try to remove the listener later.
comment isn't adding value given proximity to dtor

http://codereview.chromium.org/11338024/diff/17001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.cc:276: // Tear down our listener.
comment isn't adding value

[DaleCurtis, 2012-11-01 01:28:17]

Yeah, this has been a rather round-a-bout CL unfortunately. Almost there though
:)

http://codereview.chromium.org/11338024/diff/17001/media/audio/mac/audio_mana...
File media/audio/mac/audio_manager_mac.cc (right):

http://codereview.chromium.org/11338024/diff/17001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.cc:235: // Property address to monitor for
device changes.
On 2012/11/01 01:20:14, scherkus wrote:
> how about replacing this comment by changing kPA to
kDeviceChangePropertyAddress
> (you would do this anyway if you had multiple property addresses)

Whoops, yeah that old name is a bad choice now. Done.

http://codereview.chromium.org/11338024/diff/17001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.cc:268: // Clear |listener_cb_| so we don't
try to remove the listener later.
On 2012/11/01 01:20:14, scherkus wrote:
> comment isn't adding value given proximity to dtor

Done.

http://codereview.chromium.org/11338024/diff/17001/media/audio/mac/audio_mana...
media/audio/mac/audio_manager_mac.cc:276: // Tear down our listener.
On 2012/11/01 01:20:14, scherkus wrote:
> comment isn't adding value

Done.

[Scott Hess - ex-Googler, 2012-11-01 03:21:18]

lgtm - I like this better than anything prior.  This code already is making all
those crazy OSX AudioObject calls anyhow ...

I will admit that I still have no idea what the s/Component/AudioComponent/
change is doing.  I assume it's not something sneaking in from some other CL?

[DaleCurtis, 2012-11-01 04:32:42]

sail: can you elaborate on the AudioComponent changes? I assume they are for a
related crash you mentioned you found.

[sail, 2012-11-01 22:32:41]

On 2012/11/01 04:32:42, DaleCurtis wrote:
> sail: can you elaborate on the AudioComponent changes? I assume they are for a
> related crash you mentioned you found.

With out the AudioComment change I got a crash when changing the default output
device. On my machine the crash would usually happen after changing the default
setting two or three times.

It's not clear if the AudioComment change is an actual fix or just covering up a
real problem. Note that the old API is deprecated so this might still be a good
change to make.

[Chris Rogers, 2012-11-01 23:20:45]

lgtm

[commit-bot: I haz the power, 2012-11-01 23:21:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11338024/20001

[commit-bot: I haz the power, 2012-11-02 02:45:19]

Change committed as 165597