Always PostTask device change notifications.

Always PostTask device change notifications.

Some CoreAudio drivers may fire device change notifications when
creating a stream; resulting in re-entrancy issues.

BUG=349604
TEST=unittests still pass.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255496

[DaleCurtis, 2014-03-06 01:17:02]

-ooo scherkus, +wolenetz to mix it up :)

[wolenetz, 2014-03-06 16:01:42]

This is looking pretty good. Please consider adding a unit test for the scenario
in the bug: is such a test feasible?

https://codereview.chromium.org/187693006/diff/20001/media/audio/mac/audio_ma...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/187693006/diff/20001/media/audio/mac/audio_ma...
media/audio/mac/audio_manager_mac.cc:569: output_device_listener_.reset(
nit: add comment about why BTCL is required here vs conditional post-task.

https://codereview.chromium.org/187693006/diff/20001/media/audio/mac/audio_ma...
media/audio/mac/audio_manager_mac.cc:570: new
AudioDeviceListenerMac(BindToCurrentLoop(base::Bind(
I'm not too familiar with audio pipeline. Is it possible for the callback task
posted by AudioDeviceListenerMac to outlive this manager (in the path where the
listener previously called HandeDeviceChanges() vs the new path where it posts a
task to the method but possibly this manager is deleted before the task
execution is scheduled)? I suspect not, given the predominance of manager code
conditionally post-tasking within the manager to itself, but if so, use weak
pointer?

[DaleCurtis, 2014-03-06 19:27:36]

Funnily enough the unittest for the device listener uses BindToCurrentLoop and
says:

// Force a post task using BindToCurrentLoop() to ensure device listener
// internals are working correctly.

I can't think of a clean way to test this since the callbacks are coming from
the OS.  Simulating this would require some convoluted test code since you need
a real OSX AudioManager, but have MakeAudioOutputStreamProxy() mock'd out.  The
test would also require access to the AM internals.  It could be done, but seems
like overkill.

https://codereview.chromium.org/187693006/diff/20001/media/audio/mac/audio_ma...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/187693006/diff/20001/media/audio/mac/audio_ma...
media/audio/mac/audio_manager_mac.cc:569: output_device_listener_.reset(
On 2014/03/06 16:01:42, wolenetz wrote:
> nit: add comment about why BTCL is required here vs conditional post-task.

Done.

https://codereview.chromium.org/187693006/diff/20001/media/audio/mac/audio_ma...
media/audio/mac/audio_manager_mac.cc:570: new
AudioDeviceListenerMac(BindToCurrentLoop(base::Bind(
On 2014/03/06 16:01:42, wolenetz wrote:
> I'm not too familiar with audio pipeline. Is it possible for the callback task
> posted by AudioDeviceListenerMac to outlive this manager (in the path where
the
> listener previously called HandeDeviceChanges() vs the new path where it posts
a
> task to the method but possibly this manager is deleted before the task
> execution is scheduled)? I suspect not, given the predominance of manager code
> conditionally post-tasking within the manager to itself, but if so, use weak
> pointer?

No, in most cases AM shutdown blocks on the audio thread shutdown. However on
OSX, tasks are just dropped since the UI loop has already stopped running by the
time this class is destructed.  See the comment in AudioManagerBase::Shutdown()

[wolenetz, 2014-03-06 20:18:15]

lgtm

[DaleCurtis, 2014-03-06 20:26:04]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-06 21:06:47]

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

[commit-bot: I haz the power, 2014-03-06 22:01:21]

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

[commit-bot: I haz the power, 2014-03-07 02:25:06]

Change committed as 255496