Attempt to fix audio wedges by restarting all streams.

Attempt to fix audio wedges by restarting all streams on OSX.

Introduces two new methods to AudioOutputDispatcher:
CloseStreamsForWedgeFix() and RestartStreamsForWedgeFix().

Respectively, each method closes or restarts all active
streams owned by a given dispatcher.  The process is
completely transparent to upstream clients.

A new method on AudioManager, FixWedgedAudio() calls
CloseStreamsForWedgeFix() for all dispatchers and then
calls RestartStreamsForWedgeFix() afterward.

FixWedgedAudio() is called by each AudioOutputController
when a wedge is detected.  Multiple in flight wedge checks
are serialized by the audio thread.  The hope is that wedges
will be fixed before the next WedgeCheck() fires.

While the methods are available on all platforms, FixWedgedAudio()
is only wired up on OSX.

BUG=160920
TEST=unittest. fake wedge and observe stream recreation.
R=scherkus@chromium.org

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

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

[DaleCurtis, 2013-11-13 01:39:46]

WDYT?  My worries:
- Short fuse on WedgeCheck() may increase false positives.
- FixWedgeCheck() may not fix the problem, leading to repeated attempts as each
AudioOutputController fires.
- FixWedgeCheck() may not fix the problem fast enough before the next
WedgeCheck() fires. 
- This is really just papering over the issue, but it seems more and more likely
this is an OS issue.

Others?

[scherkus (not reviewing), 2013-11-13 18:40:56]

On 2013/11/13 01:39:46, DaleCurtis wrote:
> WDYT?  My worries:
> - Short fuse on WedgeCheck() may increase false positives.
> - FixWedgeCheck() may not fix the problem, leading to repeated attempts as
each
> AudioOutputController fires.
> - FixWedgeCheck() may not fix the problem fast enough before the next
> WedgeCheck() fires. 
> - This is really just papering over the issue, but it seems more and more
likely
> this is an OS issue.
> 
> Others?

I don't believe we have enough data and have done enough investigation to
determine exactly what the root cause is to be at the stage where we need to
land a workaround.

I'd rather see our efforts spent collecting more data and making ourselves
absolutely and positively sure it's not Chrome code.

[DaleCurtis, 2013-11-13 19:42:53]

I mostly agree a more precise root cause would be nice. However we've been
hammering on this since M23, so it's hard to say we haven't spent enough time
investigating :)

In any case, I wanted to put something together in case we determine that this
is an OS level issue; which the data is pointing more and more towards.

[scherkus (not reviewing), 2013-11-13 19:52:53]

On 2013/11/13 19:42:53, DaleCurtis wrote:
> I mostly agree a more precise root cause would be nice. However we've been
> hammering on this since M23, so it's hard to say we haven't spent enough time
> investigating :)
> 
> In any case, I wanted to put something together in case we determine that this
> is an OS level issue; which the data is pointing more and more towards.

Until we have standalone code that repros the issue we shouldn't speculate and
land workarounds in Chrome.

We have people hitting this bug daily. Heck, yesterday timsteele@ told me he
runs into it all the time and he sits right beside us! We can be doing better to
figure out what's going on.

[DaleCurtis, 2013-11-19 20:58:15]

Please review.  We're now to the point where this should be considered.

[DaleCurtis, 2013-12-02 19:13:29]

PTAL.  No word from Apple, so we need to land a workaround.

[scherkus (not reviewing), 2013-12-02 21:49:36]

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_manager_bas...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.cc:424: // Close all active streams across all
dispatchers.
these two comments aren't very helpful

it'd be more helpful to explain why we need two separate for loops instead of
calling CloseSFWF() and RestartSFWF() back-to-back in the same loop

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_cont...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_cont...
media/audio/audio_output_controller.cc:218: FROM_HERE,
TimeDelta::FromSeconds(1), this,
FYI changing this value will impact the results of
Media.AudioOutputControllerPlaybackStartupSuccess, which means it's no longer
valid to view this histogram across all versions

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_cont...
media/audio/audio_output_controller.cc:486: audio_manager_->FixWedgedAudio();
Hmm I was under the impression we'd be landing a Mac-specific workaround. Do we
really want to land this workaround for all OSes when we don't really know if a
problem exists (and for that matter one that we can reproduce?)

For example, we only know that closing all streams fixes the problem on Mac.
That may not be true on other OSes.

Options include:
  1) #ifdef'ing just this bit of code
  2) #ifdef'ing all the code
  3) renaming / shuffling things, such as:
    - Rename FixWedgedAudio() to something like DetectedWedgedAudio()
    - Make the base class implementation do nothing
    - Have the Mac backend implement the workaround

My preference is for something like (3) as it limits the workaround to the
platform that needs it w/o sprinkling #ifdefs around

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_disp...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_disp...
media/audio/audio_output_dispatcher_impl.cc:182: // This method is called by
|close_timer_|.
comment no longer valid (perhaps just remove it?)

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_disp...
media/audio/audio_output_dispatcher_impl.cc:183: void
AudioOutputDispatcherImpl::ClosePendingStreams() {
OOC any reason why this isn't called CloseIdleStreams()?

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_disp...
media/audio/audio_output_dispatcher_impl.cc:193: ClosePendingStreams();
it seems this only closes idle_streams_

do we need to worry about:
 1) the physical streams stored in proxy_to_physical_map_
 2) the physical streams in pausing_streams_ that have yet to be closed since
StopStreamTask() hasn't run yet?

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_prox...
File media/audio/audio_output_proxy_unittest.cc (right):

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_prox...
media/audio/audio_output_proxy_unittest.cc:816: 
remove extra line

[DaleCurtis, 2013-12-02 22:23:59]

(just comments)

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_cont...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_cont...
media/audio/audio_output_controller.cc:218: FROM_HERE,
TimeDelta::FromSeconds(1), this,
On 2013/12/02 21:49:37, scherkus wrote:
> FYI changing this value will impact the results of
> Media.AudioOutputControllerPlaybackStartupSuccess, which means it's no longer
> valid to view this histogram across all versions

For safety I'm going to revert this change.  Since most sounds are hung on
resume, usually due to background activation (i.e. without user interaction),
the 5 second timeout might be fine.  My hunch is users probably aren't aware
their audio is hung within that 5 second window.

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_cont...
media/audio/audio_output_controller.cc:486: audio_manager_->FixWedgedAudio();
On 2013/12/02 21:49:37, scherkus wrote:
> Hmm I was under the impression we'd be landing a Mac-specific workaround. Do
we
> really want to land this workaround for all OSes when we don't really know if
a
> problem exists (and for that matter one that we can reproduce?)
> 
> For example, we only know that closing all streams fixes the problem on Mac.
> That may not be true on other OSes.
> 
> Options include:
>   1) #ifdef'ing just this bit of code
>   2) #ifdef'ing all the code
>   3) renaming / shuffling things, such as:
>     - Rename FixWedgedAudio() to something like DetectedWedgedAudio()
>     - Make the base class implementation do nothing
>     - Have the Mac backend implement the workaround
> 
> My preference is for something like (3) as it limits the workaround to the
> platform that needs it w/o sprinkling #ifdefs around

(3) is fine, I wanted to get the idea sanitized before I made it more specific.

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_disp...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_disp...
media/audio/audio_output_dispatcher_impl.cc:182: // This method is called by
|close_timer_|.
On 2013/12/02 21:49:37, scherkus wrote:
> comment no longer valid (perhaps just remove it?)

All this is taken care of in my AOD cleanup CL, which I'll reland after this
workaround lands.  Just in case we decide to merge this to M32.

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_disp...
media/audio/audio_output_dispatcher_impl.cc:183: void
AudioOutputDispatcherImpl::ClosePendingStreams() {
On 2013/12/02 21:49:37, scherkus wrote:
> OOC any reason why this isn't called CloseIdleStreams()?

Ditto.

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_disp...
media/audio/audio_output_dispatcher_impl.cc:193: ClosePendingStreams();
On 2013/12/02 21:49:37, scherkus wrote:
> it seems this only closes idle_streams_
> 
> do we need to worry about:
>  1) the physical streams stored in proxy_to_physical_map_
>  2) the physical streams in pausing_streams_ that have yet to be closed since
> StopStreamTask() hasn't run yet?

For 1) AODI doesn't track the AudioSourceCallback information necessary to
perform the restart, only AOR does, which is why the fix is divided across both
classes.  I could move everything into AODI, but it would require adding a map
to track callbacks.

For 2) I do need to collapse pausing_streams_ into idle_streams_ here.  I'll add
that in the next update.

[DaleCurtis, 2013-12-02 22:52:07]

PTAL.

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_manager_bas...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.cc:424: // Close all active streams across all
dispatchers.
On 2013/12/02 21:49:37, scherkus wrote:
> these two comments aren't very helpful
> 
> it'd be more helpful to explain why we need two separate for loops instead of
> calling CloseSFWF() and RestartSFWF() back-to-back in the same loop

Done.

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_cont...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/61203008/diff/1/media/audio/audio_output_cont...
media/audio/audio_output_controller.cc:486: audio_manager_->FixWedgedAudio();
On 2013/12/02 22:23:59, DaleCurtis wrote:
> On 2013/12/02 21:49:37, scherkus wrote:
> > Hmm I was under the impression we'd be landing a Mac-specific workaround. Do
> we
> > really want to land this workaround for all OSes when we don't really know
if
> a
> > problem exists (and for that matter one that we can reproduce?)
> > 
> > For example, we only know that closing all streams fixes the problem on Mac.
> > That may not be true on other OSes.
> > 
> > Options include:
> >   1) #ifdef'ing just this bit of code
> >   2) #ifdef'ing all the code
> >   3) renaming / shuffling things, such as:
> >     - Rename FixWedgedAudio() to something like DetectedWedgedAudio()
> >     - Make the base class implementation do nothing
> >     - Have the Mac backend implement the workaround
> > 
> > My preference is for something like (3) as it limits the workaround to the
> > platform that needs it w/o sprinkling #ifdefs around
> 
> (3) is fine, I wanted to get the idea sanitized before I made it more
specific.

Actually 3 wasn't possible since the fix requires private details of
AudioManagerBase to implement.  It's not so bad though, only a single #ifdef is
necessary.

[scherkus (not reviewing), 2013-12-03 01:13:49]

don't forget to update CL description to say somewhere that this is a
Mac-specific fix


lgtm w/ comment

https://codereview.chromium.org/61203008/diff/150001/media/audio/audio_output...
File media/audio/audio_output_proxy_unittest.cc (right):

https://codereview.chromium.org/61203008/diff/150001/media/audio/audio_output...
media/audio/audio_output_proxy_unittest.cc:767: .Times(1);
FYI .Times(1) is implied and it seems media/audio/ code (and media/cast/ to an
extent) are the outliers in stating it explicitly
https://code.google.com/p/googlemock/wiki/V1_7_CheatSheet#Setting_Expectations

my vote would be to nuke all of the references in a separate CL and save
ourselves some lines of code

[DaleCurtis, 2013-12-03 01:26:23]

Description updated.  Thanks for review!

https://codereview.chromium.org/61203008/diff/150001/media/audio/audio_output...
File media/audio/audio_output_proxy_unittest.cc (right):

https://codereview.chromium.org/61203008/diff/150001/media/audio/audio_output...
media/audio/audio_output_proxy_unittest.cc:767: .Times(1);
On 2013/12/03 01:13:49, scherkus wrote:
> FYI .Times(1) is implied and it seems media/audio/ code (and media/cast/ to an
> extent) are the outliers in stating it explicitly
> https://code.google.com/p/googlemock/wiki/V1_7_CheatSheet#Setting_Expectations
> 
> my vote would be to nuke all of the references in a separate CL and save
> ourselves some lines of code

Sounds good. I'll nuke these now though.

[commit-bot: I haz the power, 2013-12-03 01:27:14]

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

[commit-bot: I haz the power, 2013-12-03 01:48:30]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-03 03:14:26]

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

[DaleCurtis, 2013-12-03 08:09:39]

Committed patchset #4 manually as r238325 (presubmit successful).

[DaleCurtis, 2013-12-04 00:45:55]

Committed patchset #4 manually as r238501 (presubmit successful).