Tracks all open input streams in AudioManagerBase.

Tracks all open input streams in AudioManagerBase.

Instead of just tracking the open stream count, this patch tracks stream handles so that they can be explicitly closed on shutdown.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/4b2ff4ac1890f5f48eb144198796b3853ed1ac44
Cr-Commit-Position: refs/heads/master@{#432786}

[alokp, 2016-11-15 00:09:33]

Description was changed from

==========
Counts fake input/output audio streams to track open streams.

This is temporary code to diagnose if CHECKs about input/output streams
not getting closed before AudioManager destruction can be attributed to
fake streams.

BUG=608049
==========

to

==========
Counts fake input/output audio streams to track open streams.

This is temporary code to diagnose if CHECKs about input/output streams
not getting closed before AudioManager destruction can be attributed to
fake streams.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[alokp, 2016-11-15 00:09:40]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[alokp, 2016-11-15 00:11:03]

alokp@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-11-15 00:11:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2016-11-15 00:33:13]

I don't think this is worth landing. Just move the stream tracking code out of
the OS specific audio managers and introduce an
AudioManagerBase::CloseInputStreams(), CloseOutputStreams() that must be
explicitly called by AudioManagerMac.

That'll save a step of dealing with the inevitable crashes from this :)

[alokp, 2016-11-15 01:03:31]

On 2016/11/15 00:33:13, DaleCurtis wrote:
> I don't think this is worth landing. Just move the stream tracking code out of
> the OS specific audio managers and introduce an
> AudioManagerBase::CloseInputStreams(), CloseOutputStreams() that must be
> explicitly called by AudioManagerMac.
> 
> That'll save a step of dealing with the inevitable crashes from this :)

Do we want to track input/output streams long term? Today we only track on mac
(all kinds of streams) and android (only output). There are TODO comments on mac
implementation to get rid of tracking. If we move the tracking to
AudioManagerBase, we would also need to expose a way for derived classes to
iterate through the streams. Derived classes would also need to downcast
streams, which is probably frowned upon.

I was hoping to track down and fix at least fake streams if indeed those are
causing the crashes. Sounds like you are convinced that these crashes are
happening due to fake streams?

[DaleCurtis, 2016-11-15 01:11:31]

At this point there's not much else it could be; and until we move the
AudioManager into another process there's nothing to be done about removing
stream close functionality.

To keep all the tracking in AMBase you'd need to track each list of streams
separately and provide protected const accessors; which is probably also okay.
Alternatively, you can just expose CloseStreams() to derived AMs, since they
generally static_cast to various internal implementations for their own lists.

[commit-bot: I haz the power, 2016-11-15 02:14:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 02:14:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alokp, 2016-11-15 14:29:38]

To avoid one more round of fixing crashes, I am now explicitly closing
outstanding fake streams on mac. Since fake streams are local to
AudioManagerBase, I do not see much value in exposing the tracking to derived
classes. The derived classes can choose to track (or not) whatever streams they
choose to.

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

https://codereview.chromium.org/2503693002/diff/20001/media/audio/mac/audio_m...
media/audio/mac/audio_manager_mac.cc:387: Shutdown();
Moved here so that proxy streams are closed before the real ones. The proxy ones
may be holding references to real streams.

[DaleCurtis, 2016-11-15 19:20:20]

Hmm why not move all stream tracking into this method? I.e. just have a
output_streams_ and input_streams_ entry in AMB; remove num_xxx variables and
replace them with .size() variants. Now we have tracking split in many places
for little reason.

[alokp, 2016-11-15 20:54:55]

On 2016/11/15 19:20:20, DaleCurtis wrote:
> Hmm why not move all stream tracking into this method? I.e. just have a
> output_streams_ and input_streams_ entry in AMB; remove num_xxx variables and
> replace them with .size() variants. Now we have tracking split in many places
> for little reason.

Sure, we could do that. But as I mentioned previously, it is only useful on mac.
Also, we will cannot track all kinds of streams in a single input_streams_
container. We will need three containers - fake, basic, low-latency - so that
AudioManagerMac can static-cast them to the correct type. The same needs to
happen for output streams. Is that fine with you?

[DaleCurtis, 2016-11-15 22:37:04]

On 2016/11/15 at 20:54:55, alokp wrote:
> On 2016/11/15 19:20:20, DaleCurtis wrote:
> > Hmm why not move all stream tracking into this method? I.e. just have a
> > output_streams_ and input_streams_ entry in AMB; remove num_xxx variables
and
> > replace them with .size() variants. Now we have tracking split in many
places
> > for little reason.
> 
> Sure, we could do that. But as I mentioned previously, it is only useful on
mac. Also, we will cannot track all kinds of streams in a single input_streams_
container. We will need three containers - fake, basic, low-latency - so that
AudioManagerMac can static-cast them to the correct type. The same needs to
happen for output streams. Is that fine with you?

Sorry I was unclear. I wouldn't create all those, just a single input stream
list which can be handled during Shutdown(). Output streams are already force
closed by the dispatchers during Shutdown(). As far as exposing things to AMMac,
I think for now we should just delete the closure code in ~AMMac in favor of
having it in AMB::Shutdown() just like we have for output streams. AMMac can
deal with its own accounting for low latency input / output since it needs the
static_cast. If you want the basic input streams one in AMMac, can be removed in
favor of a AMB::num_input_streams() != low_latency_input_streams_.size() check.

[alokp, 2016-11-16 00:45:41]

On 2016/11/15 22:37:04, DaleCurtis wrote:
> On 2016/11/15 at 20:54:55, alokp wrote:
> > On 2016/11/15 19:20:20, DaleCurtis wrote:
> > > Hmm why not move all stream tracking into this method? I.e. just have a
> > > output_streams_ and input_streams_ entry in AMB; remove num_xxx variables
> and
> > > replace them with .size() variants. Now we have tracking split in many
> places
> > > for little reason.
> > 
> > Sure, we could do that. But as I mentioned previously, it is only useful on
> mac. Also, we will cannot track all kinds of streams in a single
input_streams_
> container. We will need three containers - fake, basic, low-latency - so that
> AudioManagerMac can static-cast them to the correct type. The same needs to
> happen for output streams. Is that fine with you?
> 
> Sorry I was unclear. I wouldn't create all those, just a single input stream
> list which can be handled during Shutdown(). Output streams are already force
> closed by the dispatchers during Shutdown(). As far as exposing things to
AMMac,
> I think for now we should just delete the closure code in ~AMMac in favor of
> having it in AMB::Shutdown() just like we have for output streams. AMMac can
> deal with its own accounting for low latency input / output since it needs the
> static_cast. If you want the basic input streams one in AMMac, can be removed
in
> favor of a AMB::num_input_streams() != low_latency_input_streams_.size()
check.

I see - thanks for clarifying. This should work but would duplicate some
tracking in AMB and derived classes. Also, IIUC output dispatchers only close
physical streams used by AudioOutputProxy streams. The non-proxy (physical)
streams created using AM::MakeAudioInput[Output]Stream will not be closed by
dispatchers during shutdown.

Another option is to make FakeAudioInput[Output]Stream similar to
AudioOutputProxy, which is not tracked by AMB. FakeAudioStream::Close can
"delete this" instead of calling AM::ReleaseStream. We can also eliminate the
raw reference to AudioManager in fake streams, which should remove the concern
for use-after-free. WDYT?

[DaleCurtis, 2016-11-16 00:51:58]

On 2016/11/16 at 00:45:41, alokp wrote:
> On 2016/11/15 22:37:04, DaleCurtis wrote:
> > On 2016/11/15 at 20:54:55, alokp wrote:
> > > On 2016/11/15 19:20:20, DaleCurtis wrote:
> > > > Hmm why not move all stream tracking into this method? I.e. just have a
> > > > output_streams_ and input_streams_ entry in AMB; remove num_xxx
variables
> > and
> > > > replace them with .size() variants. Now we have tracking split in many
> > places
> > > > for little reason.
> > > 
> > > Sure, we could do that. But as I mentioned previously, it is only useful
on
> > mac. Also, we will cannot track all kinds of streams in a single
input_streams_
> > container. We will need three containers - fake, basic, low-latency - so
that
> > AudioManagerMac can static-cast them to the correct type. The same needs to
> > happen for output streams. Is that fine with you?
> > 
> > Sorry I was unclear. I wouldn't create all those, just a single input stream
> > list which can be handled during Shutdown(). Output streams are already
force
> > closed by the dispatchers during Shutdown(). As far as exposing things to
AMMac,
> > I think for now we should just delete the closure code in ~AMMac in favor of
> > having it in AMB::Shutdown() just like we have for output streams. AMMac can
> > deal with its own accounting for low latency input / output since it needs
the
> > static_cast. If you want the basic input streams one in AMMac, can be
removed in
> > favor of a AMB::num_input_streams() != low_latency_input_streams_.size()
check.
> 
> I see - thanks for clarifying. This should work but would duplicate some
tracking in AMB and derived classes. Also, IIUC output dispatchers only close
physical streams used by AudioOutputProxy streams. The non-proxy (physical)
streams created using AM::MakeAudioInput[Output]Stream will not be closed by
dispatchers during shutdown.

Ah, I thought those methods were private. We don't have any callers to
MakeAudioOutputStream that are not dispatchers though, so the point stands,
though it's opaque. There's no proxy version for input which is what we're
discussing adding here. I think the tracking duplication is okay given the use
case that AMMac wants.

> 
> Another option is to make FakeAudioInput[Output]Stream similar to
AudioOutputProxy, which is not tracked by AMB. FakeAudioStream::Close can
"delete this" instead of calling AM::ReleaseStream. We can also eliminate the
raw reference to AudioManager in fake streams, which should remove the concern
for use-after-free. WDYT?

I don't think this is worth the extra effort.

[alokp, 2016-11-16 06:34:49]

Replaced input-stream-count with a set of input streams that get explicitly
closed during shutdown.

I also took a crack at removing tracking for fake streams. It is not too bad
either and also eliminates the need to tracking streams in AMB:
https://codereview.chromium.org/2507643002/

[alokp, 2016-11-16 06:35:15]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 06:35:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 07:08:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 07:08:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[alokp, 2016-11-16 14:33:30]

Description was changed from

==========
Counts fake input/output audio streams to track open streams.

This is temporary code to diagnose if CHECKs about input/output streams
not getting closed before AudioManager destruction can be attributed to
fake streams.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Tracks all open input streams in AudioManagerBase.

Instead of just tracking the open stream count, this patch tracks stream handles
so that they can be explicitly closed on shutdown.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[alokp, 2016-11-16 14:36:46]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 14:37:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 14:59:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 14:59:59]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[DaleCurtis, 2016-11-16 21:56:53]

lgtm

https://codereview.chromium.org/2503693002/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/2503693002/diff/80001/media/audio/audio_manag...
media/audio/audio_manager_base.cc:295: input_streams_.erase(stream);
Add the same dcheck you did for output streams?

[alokp, 2016-11-17 00:55:49]

https://codereview.chromium.org/2503693002/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/2503693002/diff/80001/media/audio/audio_manag...
media/audio/audio_manager_base.cc:295: input_streams_.erase(stream);
On 2016/11/16 21:56:53, DaleCurtis wrote:
> Add the same dcheck you did for output streams?

Output steam is CHECK. I added the same here.

[alokp, 2016-11-17 01:03:27]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-11-17 01:03:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2503693002/#ps120001
(title: "fixes android compile error")

[commit-bot: I haz the power, 2016-11-17 01:04:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 02:30:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 02:30:05]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alokp, 2016-11-17 05:21:12]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-11-17 05:21:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2503693002/#ps140001
(title: "fixes compile error on windows")

[commit-bot: I haz the power, 2016-11-17 05:21:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 06:43:28]

Description was changed from

==========
Tracks all open input streams in AudioManagerBase.

Instead of just tracking the open stream count, this patch tracks stream handles
so that they can be explicitly closed on shutdown.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Tracks all open input streams in AudioManagerBase.

Instead of just tracking the open stream count, this patch tracks stream handles
so that they can be explicitly closed on shutdown.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-11-17 06:43:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-17 06:47:24]

Description was changed from

==========
Tracks all open input streams in AudioManagerBase.

Instead of just tracking the open stream count, this patch tracks stream handles
so that they can be explicitly closed on shutdown.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Tracks all open input streams in AudioManagerBase.

Instead of just tracking the open stream count, this patch tracks stream handles
so that they can be explicitly closed on shutdown.

BUG=608049
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/4b2ff4ac1890f5f48eb144198796b3853ed1ac44
Cr-Commit-Position: refs/heads/master@{#432786}
==========

[commit-bot: I haz the power, 2016-11-17 06:47:25]

Patchset 8 (id:??) landed as
https://crrev.com/4b2ff4ac1890f5f48eb144198796b3853ed1ac44
Cr-Commit-Position: refs/heads/master@{#432786}

[henrika (OOO until Aug 14), 2016-11-17 08:42:06]

Thanks both! Looking forward to track the progress of this change.