Tab Audio Mirroring: WebContentsAudioInputStream is a new implementation which represents the lifet…

Tab Audio Mirroring: WebContentsAudioInputStream is a new implementation which represents and manages the lifetime of a tab audio mirroring session.  WCAIS utilizes the audio mixing implementation provided by media::VirtualAudio*putStream.  The result is a single audio input data stream consisting of all of one tab's audio output.

Updated AudioInputRendererHost to use WCAIS for tab-level audio mirroring, rather than the browser-wide audio mirroring provided via AudioManager.

Changed lifecycle semantics of media::VirtualAudio*putStream to allow the use of these classes outside of AudioManager's ownership.  Cleaned up some suspicious multiple-message-loop variable passing, and a bunch of little things.

BUG=153392
TEST=Run content_unittests and media_unittests.


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

[miu, 2013-01-12 01:40:51]

Hi all,

This is the final piece to complete tab-level audio mirroring, based on the work
in: https://codereview.chromium.org/11413078/ (committed recently).

Thanks in advance for reviewing,
Yuri

[tommi (sloooow) - chröme, 2013-01-14 14:06:48]

looks great overall.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:257: //
TODO(henrika): replace CreateLowLatency() with Create() as soon
should this TODO be moved into the else block below?

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:57:
kConstructed,
CONSTRUCTED, OPENED, etc.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:131: if
(state_ != kConstructed)
should we, in order to keep the implementation simple and the contract strict,
replace these "calling this method at an incorrect point in time won't work"
checks with DCHECKs instead of returning false here and assume that the caller
will handle the error?

I don't have a clear enough picture in my head about how this will all hang
together, but if it is a programmer error to call Open() when the state is not
kConstructed, then I would prefer to handle that with a DCHECK() or even a
CHECK() instead of an if()+return which can go unnoticed.

If however if it is a legitimate part of the contract that Open can be called at
any time and the caller should know what to do if it fails in cases like this
(i.e. handle mixer_stream_->Open() failures in the same way as state_ !=
kConstructed), then this is fine.  It just feels like the first check (state_ !=
kConstructed) would be the result of the programmer having done something wrong
and the second more likely to be related to some runtime error.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:150: if
(state_ != kOpened)
same question here

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:259:
return;  // No change.
{}

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:281:
//static
// static

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:291: }
else {
no need for else?

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.h
(right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.h:42:
virtual ~WebContentsAudioInputStream();
since the object deletes itself (as per the comment for Create()), should the
dtor be private?

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.h:75:
scoped_refptr<Impl> impl_;
add a comment about why we're using a pimpl.
(I'm assuming it's because of the Impl being ref counted?)

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File
content/browser/renderer_host/media/web_contents_audio_input_stream_unittest.cc
(right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream_unittest.cc:267:
ASSERT_TRUE(!!out);
does this not work?
ASSERT_TRUE(out)

having to not not something can sometimes not be unconfusing to readers.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream_unittest.cc:277:
ASSERT_TRUE(!streams_.empty());
ASSERT_FALSE(streams_.empty()) ?

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream_unittest.cc:345:
for (int i = 0; i < kNumIterations; ++i) {
nit: no need for {}

[miu, 2013-01-14 21:53:03]

Thanks for taking a look, Tommi.  Responses...

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:257: //
TODO(henrika): replace CreateLowLatency() with Create() as soon
On 2013/01/14 14:06:48, tommi wrote:
> should this TODO be moved into the else block below?

Done.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:57:
kConstructed,
On 2013/01/14 14:06:48, tommi wrote:
> CONSTRUCTED, OPENED, etc.

Done.  Yeah, I keep forgetting the new style on these.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:131: if
(state_ != kConstructed)
On 2013/01/14 14:06:48, tommi wrote:
> should we, in order to keep the implementation simple and the contract strict,
> replace these "calling this method at an incorrect point in time won't work"
> checks with DCHECKs instead of returning false here and assume that the caller
> will handle the error?

I totally agree with you on principle.  I'm mainly worried about out-of-ordering
issues across threads (IO vs audio thread), and across render-->browser process.
 I took a look at the linux, mac, and win AudioInputStream implementations, and
all do similar "if state == X" guards in their Open, Start, Stop, and Close
methods.  So, there might be something that requires the guards be in-place for
stability?

For safety, I'm going to keep the if-guards in Start, Stop, and Close; but will
put a DCHECK in Open() and at the end of Close().  It's pretty clear that
Open()/Close() must only be called once, and must be called before/after any of
the others.

BTW--Another good reason to keep the if-guards: On the output side of things,
I've noticed AudioOutputController actually calls stream_->Stop() multiple
times.  I have a change I'm working on to fix this, and in the process of doing
so, I've already unveiled a bug in PPAPI!  So, it seems the code base isn't in
the pristine state we need it to be in to replace with DCHECKs everywhere.  ;-)

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:259:
return;  // No change.
On 2013/01/14 14:06:48, tommi wrote:
> {}

I thought we don't put braces around one-line then-clauses?

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:281:
//static
On 2013/01/14 14:06:48, tommi wrote:
> // static

Done.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:291: }
else {
On 2013/01/14 14:06:48, tommi wrote:
> no need for else?

Done.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.h
(right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.h:42:
virtual ~WebContentsAudioInputStream();
On 2013/01/14 14:06:48, tommi wrote:
> since the object deletes itself (as per the comment for Create()), should the
> dtor be private?

Done.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.h:75:
scoped_refptr<Impl> impl_;
On 2013/01/14 14:06:48, tommi wrote:
> add a comment about why we're using a pimpl.
> (I'm assuming it's because of the Impl being ref counted?)

Done.  (Comment added for the forward decl a few lines up.)

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File
content/browser/renderer_host/media/web_contents_audio_input_stream_unittest.cc
(right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream_unittest.cc:267:
ASSERT_TRUE(!!out);
On 2013/01/14 14:06:48, tommi wrote:
> does this not work?
> ASSERT_TRUE(out)
> 
> having to not not something can sometimes not be unconfusing to readers.

Done.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream_unittest.cc:277:
ASSERT_TRUE(!streams_.empty());
On 2013/01/14 14:06:48, tommi wrote:
> ASSERT_FALSE(streams_.empty()) ?

Done.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream_unittest.cc:345:
for (int i = 0; i < kNumIterations; ++i) {
On 2013/01/14 14:06:48, tommi wrote:
> nit: no need for {}

Done.

[tommi (sloooow) - chröme, 2013-01-15 10:58:42]

lgtm

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:131: if
(state_ != kConstructed)
On 2013/01/14 21:53:03, Yuri wrote:
> On 2013/01/14 14:06:48, tommi wrote:
> > should we, in order to keep the implementation simple and the contract
strict,
> > replace these "calling this method at an incorrect point in time won't work"
> > checks with DCHECKs instead of returning false here and assume that the
caller
> > will handle the error?
> 
> I totally agree with you on principle.  I'm mainly worried about
out-of-ordering
> issues across threads (IO vs audio thread), and across render-->browser
process.
>  I took a look at the linux, mac, and win AudioInputStream implementations,
and
> all do similar "if state == X" guards in their Open, Start, Stop, and Close
> methods.  So, there might be something that requires the guards be in-place
for
> stability?
> 
> For safety, I'm going to keep the if-guards in Start, Stop, and Close; but
will
> put a DCHECK in Open() and at the end of Close().  It's pretty clear that
> Open()/Close() must only be called once, and must be called before/after any
of
> the others.
> 
> BTW--Another good reason to keep the if-guards: On the output side of things,
> I've noticed AudioOutputController actually calls stream_->Stop() multiple
> times.  I have a change I'm working on to fix this, and in the process of
doing
> so, I've already unveiled a bug in PPAPI!  So, it seems the code base isn't in
> the pristine state we need it to be in to replace with DCHECKs everywhere. 
;-)

Great. Thanks for adding the checks and thanks for digging into the call
sequences.  Now that you mention it I think I've seen that multiple Stop() thing
too.

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:259:
return;  // No change.
On 2013/01/14 21:53:03, Yuri wrote:
> On 2013/01/14 14:06:48, tommi wrote:
> > {}
> 
> I thought we don't put braces around one-line then-clauses?

Yeah, so this one I've learned through reviews and on the chromium-dev list (see
e.g. somewhat recent thread about lint).  The thing is that a one-line if()
means 1 line for the condition and 1 line for the body.  I.e. 2 lines total.  As
soon as either the body or condition needs to be wrapped, the convention is to
use {}.  I believe this is an old google style convention.

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:149: if
(state_ != OPENED)
DCHECK_EQ(state_, OPENED)?

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.h
(right):

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.h:70: //
thread, and the VirtualAudioOuputStreams on the audio thread.
nice!

[DaleCurtis, 2013-01-15 22:02:18]

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_capture_util.h (right):

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_capture_util.h:46: class
CONTENT_EXPORT WebContentsTracker
Why put this here vs separate file vs private class in
WebContentsAudioInputStream? There's no coupling between WebContentsCaptureUtil
and this class.

https://codereview.chromium.org/11416350/diff/32003/media/audio/audio_input_c...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/11416350/diff/32003/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:120: controller->stream_ = stream;
Seems like it might be clearer to make a DoCreate() or DoFinishCreate() take a
stream input to ensure assumptions about what DoCreate() is doing don't change.

I'm not really crazy about the static setting message_loop_ either.
AudioInputController has access to the AudioManager.

https://codereview.chromium.org/11416350/diff/32003/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/11416350/diff/32003/media/audio/audio_manager...
media/audio/audio_manager_base.cc:122: vaos->RunOnceClosed(base::Bind(
Why not make this callback required for construction? We do this frequently in
the media pipeline. Something like close_cb_, etc.

https://codereview.chromium.org/11416350/diff/32003/media/audio/virtual_audio...
File media/audio/virtual_audio_input_stream.cc (right):

https://codereview.chromium.org/11416350/diff/32003/media/audio/virtual_audio...
media/audio/virtual_audio_input_stream.cc:147: // will cause cut-outs!
Justin has a fix out for this?

https://codereview.chromium.org/11416350/diff/32003/media/audio/virtual_audio...
File media/audio/virtual_audio_output_stream.h (right):

https://codereview.chromium.org/11416350/diff/32003/media/audio/virtual_audio...
media/audio/virtual_audio_output_stream.h:39: void RunOnceClosed(const
base::Closure& on_close_cb);
Ditto.

[miu, 2013-01-16 03:22:18]

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/19001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:259:
return;  // No change.
On 2013/01/15 10:58:42, tommi wrote:
> On 2013/01/14 21:53:03, Yuri wrote:
> > I thought we don't put braces around one-line then-clauses?
> 
> Yeah, so this one I've learned through reviews and on the chromium-dev list
(see
> e.g. somewhat recent thread about lint).  The thing is that a one-line if()

Done.  Okay, I understand it now.  I really wish the style decision had been to
always use braces.  :-/

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_capture_util.h (right):

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_capture_util.h:46: class
CONTENT_EXPORT WebContentsTracker
On 2013/01/15 22:02:18, DaleCurtis wrote:
> Why put this here vs separate file vs private class in
> WebContentsAudioInputStream? There's no coupling between
WebContentsCaptureUtil
> and this class.

This class is a utility meant to be used by only WebContentsAudioInputStream and
WebContentsVideoCaptureDevice.  I was thinking it'd be good to keep such code
within the same module.  They're both related in that they're both used to find
and follow a RenderView by its IDs.

One option I had considered was to make it an inner class of
WebContentsCaptureUtil.  What do you think?

In a later CL, I'll change WebContentsVideoCaptureDevice to use this.  That way,
both audio and video capture will be tracking tab navigation exactly the same
way.

https://codereview.chromium.org/11416350/diff/32003/media/audio/audio_input_c...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/11416350/diff/32003/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:120: controller->stream_ = stream;
On 2013/01/15 22:02:18, DaleCurtis wrote:
> Seems like it might be clearer to make a DoCreate() or DoFinishCreate() take a
> stream input to ensure assumptions about what DoCreate() is doing don't
change.
> 
> I'm not really crazy about the static setting message_loop_ either.
> AudioInputController has access to the AudioManager.

Done.  I renamed DoFinishCreate() to DoCreateForStream(), and I pass in the
stream as as argument.

As for the message_loop_ thing...Yeah it's ugly.  I noticed that, and several
other things.  I'd like to tackle cleaning up AudioInputController (it doesn't
even have any unit tests).  I'd prefer to do this in a future change, though. 
SGTY?

https://codereview.chromium.org/11416350/diff/32003/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/11416350/diff/32003/media/audio/audio_manager...
media/audio/audio_manager_base.cc:122: vaos->RunOnceClosed(base::Bind(
On 2013/01/15 22:02:18, DaleCurtis wrote:
> Why not make this callback required for construction? We do this frequently in
> the media pipeline. Something like close_cb_, etc.

Done.

https://codereview.chromium.org/11416350/diff/32003/media/audio/virtual_audio...
File media/audio/virtual_audio_input_stream.cc (right):

https://codereview.chromium.org/11416350/diff/32003/media/audio/virtual_audio...
media/audio/virtual_audio_input_stream.cc:147: // will cause cut-outs!
On 2013/01/15 22:02:18, DaleCurtis wrote:
> Justin has a fix out for this?

Yep (https://codereview.chromium.org/11889041/).  The TODO comment was written
before that work was started.  I'll take it out and rebase once his change is
committed.

https://codereview.chromium.org/11416350/diff/32003/media/audio/virtual_audio...
File media/audio/virtual_audio_output_stream.h (right):

https://codereview.chromium.org/11416350/diff/32003/media/audio/virtual_audio...
media/audio/virtual_audio_output_stream.h:39: void RunOnceClosed(const
base::Closure& on_close_cb);
On 2013/01/15 22:02:18, DaleCurtis wrote:
> Ditto.

Done.

[DaleCurtis, 2013-01-17 01:15:51]

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_capture_util.h (right):

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_capture_util.h:46: class
CONTENT_EXPORT WebContentsTracker
On 2013/01/16 03:22:18, Yuri wrote:
> On 2013/01/15 22:02:18, DaleCurtis wrote:
> > Why put this here vs separate file vs private class in
> > WebContentsAudioInputStream? There's no coupling between
> WebContentsCaptureUtil
> > and this class.
> 
> This class is a utility meant to be used by only WebContentsAudioInputStream
and
> WebContentsVideoCaptureDevice.  I was thinking it'd be good to keep such code
> within the same module.  They're both related in that they're both used to
find
> and follow a RenderView by its IDs.
> 
> One option I had considered was to make it an inner class of
> WebContentsCaptureUtil.  What do you think?
> 
> In a later CL, I'll change WebContentsVideoCaptureDevice to use this.  That
way,
> both audio and video capture will be tracking tab navigation exactly the same
> way.

Hmm, it depends on the usage pattern. In general, utility classes indicate
things could be better organized. If both classes are going to use this, I'd
just as soon see it as a standalone class.

https://codereview.chromium.org/11416350/diff/42001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/42001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:290:
struct Helper {
bind_helpers exposes a DoNothing you can use instead.

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:126: struct Adapter {
Why is this necessary? What was wrong with your original bind?

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:326: --num_input_streams_;
Funnel to ReleaseInputStream() ?

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
File media/audio/virtual_audio_input_stream.cc (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
media/audio/virtual_audio_input_stream.cc:153: const AfterCloseCallback cb =
after_close_cb_;
Are you doing this to prevent Close() from being called twice or something?
Seems you can just call after_close_cb_ and be done with it?

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
File media/audio/virtual_audio_input_stream.h (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
media/audio/virtual_audio_input_stream.h:34: typedef
base::Callback<void(VirtualAudioInputStream* vais)>
I think "It is safe for the callback to delete this." needs elaboration. Given
the current usage pattern, it seems required to delete this object?

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
File media/audio/virtual_audio_output_stream.cc (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
media/audio/virtual_audio_output_stream.cc:50: if (!after_close_cb_.is_null()) {
Ditto.

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
File media/audio/virtual_audio_output_stream.h (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
media/audio/virtual_audio_output_stream.h:31: // Callback invoked just after
VirtualAudioOutputStream is closed.  It is safe
Ditto.

[miu, 2013-01-17 05:33:55]

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_capture_util.h (right):

https://codereview.chromium.org/11416350/diff/32003/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_capture_util.h:46: class
CONTENT_EXPORT WebContentsTracker
On 2013/01/17 01:15:52, DaleCurtis wrote:
> Hmm, it depends on the usage pattern. In general, utility classes indicate
> things could be better organized. If both classes are going to use this, I'd
> just as soon see it as a standalone class.

Agreed.  This stuff with the device_id parsing is an interim solution until the
MediaStream messaging/factoring is worked out.  xians@ is in the middle of some
of this work, IIRC.

So, I took your comment to mean that you'd prefer this class to be in its own
.h/.cc files.  Done.

https://codereview.chromium.org/11416350/diff/42001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/42001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:290:
struct Helper {
On 2013/01/17 01:15:52, DaleCurtis wrote:
> bind_helpers exposes a DoNothing you can use instead.

Fixed.  bind_helpers doesn't provide a DoNothing() with the function signature I
need.  Fortunately, I just realized you can instantiate a "null callback" by
using the zero-arg constructor.

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:126: struct Adapter {
On 2013/01/17 01:15:52, DaleCurtis wrote:
> Why is this necessary? What was wrong with your original bind?

Two things going on here:

1. Bind() needs to return a Callback1<void(VAOS*)> for the VAOS ctor.  It was
providing a Closure before.
2. AudioManagerBase::ReleaseOutputStream() takes an AudioOutputStream* argument
and not a VirtualAudioOutputStream* argument.

Fixed.  Just added a simple AudioManagerBase::ReleaseVirtualAudioOutputStream()
method, and cleaned up the code here.

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:326: --num_input_streams_;
On 2013/01/17 01:15:52, DaleCurtis wrote:
> Funnel to ReleaseInputStream() ?

Done.  Good idea.

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
File media/audio/virtual_audio_input_stream.cc (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
media/audio/virtual_audio_input_stream.cc:153: const AfterCloseCallback cb =
after_close_cb_;
On 2013/01/17 01:15:52, DaleCurtis wrote:
> Are you doing this to prevent Close() from being called twice or something?
> Seems you can just call after_close_cb_ and be done with it?

Two things going on here:

1. The callback can delete |this| and so we can't invoke
this->after_close_cb_.Run() directly since it might be destroyed during the call
to Run().

2. If the ctor was provided a "null callback," we don't want to Run() it.

I added comments to the code explaining this.

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
File media/audio/virtual_audio_input_stream.h (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
media/audio/virtual_audio_input_stream.h:34: typedef
base::Callback<void(VirtualAudioInputStream* vais)>
On 2013/01/17 01:15:52, DaleCurtis wrote:
> I think "It is safe for the callback to delete this." needs elaboration. Given
> the current usage pattern, it seems required to delete this object?

I took out the "safe to delete this" text.  It's actually irrelevant since it's
up to the owner of the object whether it wants to delete it upon close or at
some later time:

AudioManagerBase: The callback runs a method that forces all outstanding VAOS
objects to be closed (via the device change mechanism) and then deletes VAIS.

WebContentsAudioInputStream: The callback is unused.  VAIS is deleted via
scoped_ptr in the WCAIS::Impl class.  The reason for this is that there's no way
to synchronously force-close all associated VAOS objects: The path between WCAIS
and AudioOutputController must go through AudioMirroringManager and
AudioRendererHost, which live on the IO thread.  Each VAOS object created by
WCAIS::Impl holds a ref on WCAIS::Impl to make sure VAIS outlives all the VAOS
objects.

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
File media/audio/virtual_audio_output_stream.cc (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
media/audio/virtual_audio_output_stream.cc:50: if (!after_close_cb_.is_null()) {
On 2013/01/17 01:15:52, DaleCurtis wrote:
> Ditto.

Added the comments here as well.

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
File media/audio/virtual_audio_output_stream.h (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/virtual_audio...
media/audio/virtual_audio_output_stream.h:31: // Callback invoked just after
VirtualAudioOutputStream is closed.  It is safe
On 2013/01/17 01:15:52, DaleCurtis wrote:
> Ditto.

Done.

[DaleCurtis, 2013-01-17 21:11:11]

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:126: struct Adapter {
On 2013/01/17 05:33:55, Yuri wrote:
> On 2013/01/17 01:15:52, DaleCurtis wrote:
> > Why is this necessary? What was wrong with your original bind?
> 
> Two things going on here:
> 
> 1. Bind() needs to return a Callback1<void(VAOS*)> for the VAOS ctor.  It was
> providing a Closure before.
> 2. AudioManagerBase::ReleaseOutputStream() takes an AudioOutputStream*
argument
> and not a VirtualAudioOutputStream* argument.
> 
> Fixed.  Just added a simple
AudioManagerBase::ReleaseVirtualAudioOutputStream()
> method, and cleaned up the code here.

I still don't follow why the VirtualAudioOutputStream can't just do what every
other stream does and call AudioManager::ReleaseOutputStream() directly w/o a
callback?

https://codereview.chromium.org/11416350/diff/54002/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/54002/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:297:
media::VirtualAudioInputStream::AfterCloseCallback()));
This is a bit weird, you're binding in a callback to the class you're creating. 
Why not just do this internally?

[miu, 2013-01-17 22:44:11]

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/11416350/diff/42001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:126: struct Adapter {
On 2013/01/17 21:11:11, DaleCurtis wrote:
> I still don't follow why the VirtualAudioOutputStream can't just do what every
> other stream does and call AudioManager::ReleaseOutputStream() directly w/o a
> callback?

Because VAOS is not always created/owned by AudioManager.  Please take a look at
how VAOS and VAIS are used in web_contents_audio_input_stream.cc.  AudioManager
is not involved in anything there, remember?  ;-)

https://codereview.chromium.org/11416350/diff/54002/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_audio_input_stream.cc
(right):

https://codereview.chromium.org/11416350/diff/54002/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_audio_input_stream.cc:297:
media::VirtualAudioInputStream::AfterCloseCallback()));
On 2013/01/17 21:11:12, DaleCurtis wrote:
> This is a bit weird, you're binding in a callback to the class you're
creating. 
> Why not just do this internally?

No, that's not what's going on here at all.  AfterCloseCallback is declared as:

  typedef base::Callback1<void(VirtualAudioInputStream*)> AfterCloseCallback;

All I'm doing here is calling the zero-arg Callback1 constructor, which
equivalent to binding a "do nothing" function.

This is why VAIS and VAOS originally had the separate RunOnceClosed() method. 
There are times (e.g., here and in unit tests) where we don't want VAIS to run
anything on Close().

[DaleCurtis, 2013-01-17 23:00:04]

lgtm

[Charlie Reis, 2013-01-18 00:29:26]

Rubber stamp owners LGTM for content/content_browser.gypi and
content/content_tests.gypi.

[commit-bot: I haz the power, 2013-01-18 03:06:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/11416350/60002

[commit-bot: I haz the power, 2013-01-18 05:32:19]

Change committed as 177605