MediaStream audio: Refactor 3 separate "glue" implementations into one.

MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

Diagram of post-refactoring class relationships: https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-zwo/edit?usp=sharing

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881, 577874
Committed: https://crrev.com/e640bad76cad1155335d4068ecd46d250327bb9e
Cr-Commit-Position: refs/heads/master@{#392845}

[miu, 2016-03-28 22:02:54]

Description was changed from

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing pipeline.  Also: a)
Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation; now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881,577874
==========

to

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881,577874
==========

[miu, 2016-03-28 22:02:54]

miu@chromium.org changed reviewers:
+ olka@chromium.org

[miu, 2016-03-28 23:26:23]

o1ka: PTAL.  The LOC changed looks daunting, but most of this is file renames
and moving existing code to new locations.  Let's just start with
media_stream_audio_source.* and media_stream_audio_track.* files, since the
whole refactoring is based on those.  If those look good, then feel free to
proceed on with the rest.  Otherwise, we'll iterate.

Also, I'll include a the updated unit tests in a later patchset once you agree
the MSAudioSource and MSAudioTrack changes are good.

[o1ka, 2016-03-30 15:00:58]

Great stuff, I like the idea of change a lot: would be wonderful to have those
code paths unified!

Looked at MediaStreamSource hierarchy so far. I think there are some problems
there which could be solved by a slightly better separation of concerns. 3
layers of implementation inheritance seems to be a bit too complicated (I added
a couple of comments to illustrate the complexity from unprepared
reader/maintainer/bugfixer point of view).

Probably it's possible to have MediaStreamAudioSource as the final
implementation/framework, and make it own some "delegate" interface instance to
deal with all the specifics - instead of the third layer of inheritance. WDYT?

I'm just learning the code myself, so I'll go on with reading the CL.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.cc:27: DCHECK(is_stopped_) <<
"BUG: Subclass must ensure StopSource() is called.";
Since this flag is supposed to be set by a child class, the fact that it's On
does not guarantee that StopSource() was called.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.h:94: protected:
Readability side note: We've got here overrides, virtuals to override and
helpers to be called. It would be nice to sort out the list accordingly. Even
better if we can distinguish them by names... And probably even better if we do
not have to deal with such a variety :)

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.h:123: bool is_stopped_;
I think this needs to be private. The reason: now it's accessed by both
MadiaStreamAudioSource and its children, which makes it very difficult to ensure
that the access is thread-safe.
(It's actually required by the style guide as well
https://google.github.io/styleguide/cppguide.html#Access_Control)

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.h:127: base::ThreadChecker
thread_checker_;
I think each class in the hierarchy should have its own thread checker: thread
checking is a predicate which should remain the same no matter who inherited the
class => it should no be altered by children => should not be accessed by them.
And it's against the style guide anyways to have it protected.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_track.cc:124: base::AutoLock
auto_lock(lock_);
According to the header comment, lock_ is not supposed to protect params_

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_source.h:75: SourceStoppedCallback
stop_callback_;
Looks like we need to add thread checker to this class to make sure all the
access to this data is thread safe. We should not rely on child classes for
this: it's a source of future bugs and also makes it difficult to read the code.
For example, to ensure thread safety of  calls like
[media_stream_audio_source.cc:155], I need to look up all the access to
stop_callback_ in MediaStreamAudioSource - which is not a straightforward things
since it's hidden in MediaStreamSource methods.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/processed_local_audio_source.cc:100: StopSource();
As reader I find the call stack here a bit unparsable:
ProcessedLocalAudioSource is a grandchild of MediaStreamSource.
So, what we have here:
Grandchild implements a method of parent EnsureSourceIsStarted() in which it
calls a method of grandparent StopSource() which will call a method of its child
(grandchild's parent) DoStopSource()... or maybe grandchild method
DoStopSource() if there is implementation which I missed.

[miu, 2016-03-31 04:46:26]

Patchset #2 (id:20001) has been deleted

[miu, 2016-03-31 04:57:59]

On 2016/03/30 15:00:58, o1ka wrote:
> Looked at MediaStreamSource hierarchy so far. I think there are some problems
> there which could be solved by a slightly better separation of concerns. 3
> layers of implementation inheritance seems to be a bit too complicated (I
added
> a couple of comments to illustrate the complexity from unprepared
> reader/maintainer/bugfixer point of view).

I resolved this issue by making sure each subclass was only responsible for
maintaining itself (i.e., no more complex call-back-into-parent-class foo, and
no more shared protected data members).

> Probably it's possible to have MediaStreamAudioSource as the final
> implementation/framework, and make it own some "delegate" interface instance
to
> deal with all the specifics - instead of the third layer of inheritance. WDYT?

IMO, it wasn't the 3 layers of inheritance that was at issue.  Instead, there
were requirements that the subclass's extension of the implementation was being
forced to deal with, but these should have been dealt fully within the parent
class.  Now that I've cleaned those up, I think we're good.

PTAL at PS2.  I've addressed your comments:

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.cc:27: DCHECK(is_stopped_) <<
"BUG: Subclass must ensure StopSource() is called.";
On 2016/03/30 15:00:57, o1ka wrote:
> Since this flag is supposed to be set by a child class, the fact that it's On
> does not guarantee that StopSource() was called.

Fixed.  As described in a comment below, I reworked things so that subclasses
are only responsible for their own stuff.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.h:94: protected:
On 2016/03/30 15:00:57, o1ka wrote:
> Readability side note: We've got here overrides, virtuals to override and
> helpers to be called. It would be nice to sort out the list accordingly. Even
> better if we can distinguish them by names... And probably even better if we
do
> not have to deal with such a variety :)

I've tried, but I simply don't understand any part of your comment here.  Each
method is clearly documented in terms of what the subclass should do with it. 
What needs to be sorted differently?  What do you mean by "variety" and why is
it hurting readability?  Perhaps you could provide a code sample to help
explain?

Maybe now that I've untangled the StopSource()/DoStopSource()/is_stopped_
hairball, everything looks good to you now?

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.h:123: bool is_stopped_;
On 2016/03/30 15:00:57, o1ka wrote:
> I think this needs to be private. The reason: now it's accessed by both
> MadiaStreamAudioSource and its children, which makes it very difficult to
ensure
> that the access is thread-safe.
> (It's actually required by the style guide as well
> https://google.github.io/styleguide/cppguide.html#Access_Control)

Done.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.h:127: base::ThreadChecker
thread_checker_;
On 2016/03/30 15:00:57, o1ka wrote:
> I think each class in the hierarchy should have its own thread checker: thread
> checking is a predicate which should remain the same no matter who inherited
the
> class => it should no be altered by children => should not be accessed by
them.
> And it's against the style guide anyways to have it protected.

Done.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_track.cc:124: base::AutoLock
auto_lock(lock_);
On 2016/03/30 15:00:58, o1ka wrote:
> According to the header comment, lock_ is not supposed to protect params_

Fixed header comment.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_source.h:75: SourceStoppedCallback
stop_callback_;
On 2016/03/30 15:00:58, o1ka wrote:
> Looks like we need to add thread checker to this class to make sure all the
> access to this data is thread safe. We should not rely on child classes for
> this: it's a source of future bugs and also makes it difficult to read the
code.
> For example, to ensure thread safety of  calls like
> [media_stream_audio_source.cc:155], I need to look up all the access to
> stop_callback_ in MediaStreamAudioSource - which is not a straightforward
things
> since it's hidden in MediaStreamSource methods.

Done.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/processed_local_audio_source.cc:100: StopSource();
On 2016/03/30 15:00:58, o1ka wrote:
> As reader I find the call stack here a bit unparsable:
> ProcessedLocalAudioSource is a grandchild of MediaStreamSource.
> So, what we have here:
> Grandchild implements a method of parent EnsureSourceIsStarted() in which it
> ...

Agree, it's weird.  This is all "fall-out" from this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

...and this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

I thought about this a bit more, and I've cleaned-up this complex "shutdown"
path.  Now, each subclass is only responsible for ensuring that its own part of
implementation is in a good state, and is no longer responsible for calling any
parent class methods ahead of destruction.

[o1ka, 2016-03-31 16:35:35]

Still getting through the graph to get the full picture of the change, enjoying
it a lot (minus 1500+ lines - so cool!).
I like the new version!

Got a couple (literally) comments so far, will need to take a closer look into
details.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.h:94: protected:
On 2016/03/31 04:57:59, miu wrote:
> On 2016/03/30 15:00:57, o1ka wrote:
> > Readability side note: We've got here overrides, virtuals to override and
> > helpers to be called. It would be nice to sort out the list accordingly.
Even
> > better if we can distinguish them by names... And probably even better if we
> do
> > not have to deal with such a variety :)
> 
> I've tried, but I simply don't understand any part of your comment here.  Each
> method is clearly documented in terms of what the subclass should do with it. 
> What needs to be sorted differently?  What do you mean by "variety" and why is
> it hurting readability?  Perhaps you could provide a code sample to help
> explain?
> 
> Maybe now that I've untangled the StopSource()/DoStopSource()/is_stopped_
> hairball, everything looks good to you now?
Sorry, I meant it would be nice to have the protected methods ordered here, like
CreateMediaStreamAudioTrack and EnsureSourceStarted together, etc.

Now it's just perfect.

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/processed_local_audio_source.cc:100: StopSource();
On 2016/03/31 04:57:59, miu wrote:
> On 2016/03/30 15:00:58, o1ka wrote:
> > As reader I find the call stack here a bit unparsable:
> > ProcessedLocalAudioSource is a grandchild of MediaStreamSource.
> > So, what we have here:
> > Grandchild implements a method of parent EnsureSourceIsStarted() in which it
> > ...
> 
> Agree, it's weird.  This is all "fall-out" from this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> ...and this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> I thought about this a bit more, and I've cleaned-up this complex "shutdown"
> path.  Now, each subclass is only responsible for ensuring that its own part
of
> implementation is in a good state, and is no longer responsible for calling
any
> parent class methods ahead of destruction.

Now it is great: very clean and easy to follow. Thanks!

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_source.h:119:
scoped_refptr<media::AudioInputDevice> source_;
media::AudioCapturerSource is the only external interface of AID, so I think
it's enough to store it as media::AudioCapturerSource (and AudioDeviceFactory
will return ACS instead of AID soon in my CL)

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_media_stream_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_media_stream_adapter.cc:107:
webrtc_media_stream_->AddTrack(track_interface.get());
If track_interface was created at l.101, doesn't it mean that you are passing
here a pointer to an object which is about to be destroyed?

[miu, 2016-03-31 22:35:31]

On 2016/03/31 16:35:35, o1ka wrote:
> Still getting through the graph to get the full picture of the change,
enjoying
> it a lot (minus 1500+ lines - so cool!).
> I like the new version!

Thanks!  :)

PTAL at PS3 (AudioInputDevice --> AudioCapturerSource, and addressed the refptr
weirdness in webrtc_media_stream_adapter.cc).

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_source.h:119:
scoped_refptr<media::AudioInputDevice> source_;
On 2016/03/31 16:35:35, o1ka wrote:
> media::AudioCapturerSource is the only external interface of AID, so I think
> it's enough to store it as media::AudioCapturerSource (and AudioDeviceFactory
> will return ACS instead of AID soon in my CL)

Done.  Actually, I meant to change this after seeing your CL, but it slipped my
mind.  :)

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_media_stream_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_media_stream_adapter.cc:107:
webrtc_media_stream_->AddTrack(track_interface.get());
On 2016/03/31 16:35:35, o1ka wrote:
> If track_interface was created at l.101, doesn't it mean that you are passing
> here a pointer to an object which is about to be destroyed?

webrtc::AudioTrackInterface implements libjingle's ref-counting scheme
(rtc::RefCountInterface), not Chromium's.  However, while Chromium's
scoped_refptr<> class can work with libjingle's ref-counting methods (AddRef(),
etc.), scoped_refptr<> cannot be directly converted to rtc::scoped_refptr<>.

Unfortunately, you'll see this stuff throughout the content/renderer/media code.
 Another clean-up TODO, I guess.  ;-)

For this code change, I've explicitly wrapped the raw pointer in a
rtc::scoped_refptr<> to make this ref-counting container transfer clearer in the
code.

[o1ka, 2016-04-01 15:11:41]

I do not feel I got it all yet. Some questions for today :)

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
File content/renderer/media/webaudio_media_stream_source.cc (right):

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
content/renderer/media/webaudio_media_stream_source.cc:92:
wrapper_bus_->set_frames(number_of_frames);
Don't you need to protect |wrapper_bus_| from the concurrent access here and in
setFormat()? If not, than it would be nice to have a comment/DCHECK explaining
why.

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_media_stream_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_media_stream_adapter.cc:107:
webrtc_media_stream_->AddTrack(track_interface.get());
On 2016/03/31 22:35:30, miu wrote:
> On 2016/03/31 16:35:35, o1ka wrote:
> > If track_interface was created at l.101, doesn't it mean that you are
passing
> > here a pointer to an object which is about to be destroyed?
> 
> webrtc::AudioTrackInterface implements libjingle's ref-counting scheme
> (rtc::RefCountInterface), not Chromium's.  However, while Chromium's
> scoped_refptr<> class can work with libjingle's ref-counting methods
(AddRef(),
> etc.), scoped_refptr<> cannot be directly converted to rtc::scoped_refptr<>.
> 
> Unfortunately, you'll see this stuff throughout the content/renderer/media
code.
>  Another clean-up TODO, I guess.  ;-)
> 
> For this code change, I've explicitly wrapped the raw pointer in a
> rtc::scoped_refptr<> to make this ref-counting container transfer clearer in
the
> code.

Ok, this is someting new for me, thanks!
But does webrtc_media_stream_->AddTrack() takes an ownership? It takes a pointer
to AudioTrackInterface, how can I assume that WebRtcLocalAudioTrackAdapter will
be owned by it?
Should't newly-created WebRtcLocalAudioTrackAdapter be owned by
WebRtcMediaStreamAdapter? (something like in CreateVideoTrack)

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:132:
track->SetFormat(params_);
I would probably use a separate lock for |params_|.
And then I would use getter to access them here, there seem to be no need to
call SetFormat() under the lock.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:153: sink->OnData(audio_bus,
reference_time);
I feel somewhat uneasy about this call to an unknown implementation under the
lock which can be acquired by the public/protected interface of the class as
well (MediaStreamAudioTrack::Stop(), MediaStreamAudioTrack::SetFormat(), etc).
Does not seem to be a problem right now, but does not look right since it may
potentially cause deadlocks.
Maybe it's an irrational fear though.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.cc
(right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_remote_audio_source.cc:122:
base::TimeTicks playout_time(base::TimeTicks::Now());
Probably have paranoid thread checks in all those callbacks to make sure they
are coming from the same thread?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_source.h:62:
blink::WebMediaConstraints source_constraints() const { return constraints_; }
I just don't know if it's ok from WebRTC point of view to be able to do it only
ones. Is there a possibility that they are changed dynamically during the call?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_audio_sink_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_audio_sink_adapter.cc:59:
fifo_.Reset(params_.frames_per_buffer());
What is the guarantee that there is no race between OnSetFormat() and OnData()?
It would be nice to have a comment or thread check

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:65: track_ =
track;
This part I haven't processed entirely yet. What if SetMediaStreamAudioTrack()
is called twice? And what if AddSink() is called before it?

[o1ka, 2016-04-06 18:39:09]

olka@chromium.org changed reviewers:
+ tommi@chromium.org

[o1ka, 2016-04-06 18:39:11]

I feel bad about reviewing it that long, but the reality is it's a huge change,
and also my knowledge of "before" picture is limited.

I think the new structure is way much better - amazing job!
It's still rather complicated and probably(?) there is some place for
improvement, but I don't have any ideas for now.

(BTW I partially sketched the new picture here
https://docs.google.com/a/google.com/drawings/d/1qoh9CnrEOkMuuNyCYx-BkaGLFLL0...;
and if you have any pictures too, it would be nice point to them here - it's
hard to embrace such a change without visualizing)

I'd like to spend some more time on (1) following capture-render pipelines and
checking how locks affect it and (2) checking mutlithreading parts for races.

Adding tommi@, since he has much more knowledge of prior situation, and can
assess if any changes (like setting source constraints only ones before the
call) affect functionality.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:65: track_ =
track;
The interface does set any expectations on SetMediaStreamAudioTrack: when it is
called, or how many times. But from implementation it looks like it's expected
that it's called only once and before any AddSink calls. If it's called after
AddSink(), then the previously added sink won't be added to the track, but on
RemoveSink() there will be an attempt to remove it from the track. Also, AddSink
can race with SetMediaStreamAudioTrack.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:99: // null for
single-threaded unit tests.
Are you planing to have some multi-threaded unit tests as well? (for example,for
checking done_event trick is not broken)

[o1ka, 2016-04-08 12:21:25]

olka@chromium.org changed reviewers:
+ perkj@chromium.org

[o1ka, 2016-04-08 12:21:26]

Adding Per - he knows stream/thread stuff!

[perkj_chrome, 2016-04-08 14:05:42]

An impressive cleanup!

And a massive cl. I took a first pass. I don't know the audio specifics but I
know the video parts pretty well.

https://codereview.chromium.org/1834323002/diff/60001/content/content_rendere...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/1834323002/diff/60001/content/content_rendere...
content/content_renderer.gypi:322:
'renderer/media/media_stream_audio_source.cc',
put all mediastream audio files where the video counter part is. (I don't
understand why its called private_renderer_webrtc_sources since it is not webrtc
specific)

https://codereview.chromium.org/1834323002/diff/60001/content/content_rendere...
content/content_renderer.gypi:720:
'renderer/media/webrtc/peer_connection_remote_audio_source.cc',
For video this is called media_stream_remote_video_source. - can audio use the
same naming?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/external_media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/external_media_stream_audio_source.cc:70:
audio_delay_milliseconds));
nit: indentation looks weird. git cl format?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:17: :
is_local_source_(is_local_source),
remove?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:52: if (is_stopped_)
Is it necessary to check for if the source is stopped?
I think that a track that is added a to a stopped source should produce a track
where the state is ended. 
That can happen if you clone a track that has already been stopped in JS.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:110: void
MediaStreamAudioSource::DoStopSource() {
why this extra indirection? Just for naming?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:154: // explicitly closed,
rather than auto-stop on the removal of the last track?
This is according to spec. 

If you have two tracks for the same source- stopping the first track only stops
that track. Stopping the second track actually stops the source.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:31: // implement the
DoStopSource() and EnsureSourceIsStarted() methods, and call
How does DoStopSource and EnsureSourceStoped relate? Looks like this should be
EnsureSourceIsStopped()

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:49: //          
->ConnectToTrack(blink_track)) {
For video- the tracks register and deregister with the source instead using a
ConnectToTrack. Would it make sence for audio to work the same way for
consistency?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:101: // successful.  While a
the source is running, it may provide audio on any
nit: While a the /s  While the...

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:165: // Provides weak
pointers so that MediaStreamAudioTracks won't call
Good but I think its safe regardless- a blink track keeps the blink source
alive.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:138: if (!is_enabled_)
MediaStreamTrack.enabled = false means that audio packets should still be
delivered but with energy 0.
https://www.w3.org/TR/mediacapture-streams/#life-cycle-and-media-flow
This is a bit weird though. 

In video- we currently produce a memory backed black frame.
media_stream_video_track.cc

Does this cl change the current behavior?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_center.cc:44:
PeerConnectionDependencyFactory* factory) {
please remove |factory| I think this might be the last dependency in this class.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_center.cc:110: : rtc_factory_(factory) {}
please remove

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_renderer_factory_impl.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_renderer_factory_impl.cc:124: if
(!PeerConnectionRemoteAudioTrack::From(audio_track)) {
PeerConnectionRemoteAudioTrack is probably not needed at all soon. The problem
as I can see is MediaStreamTrack.enabled=false on remote audio tracks since I
guess the remote audio renderer will not know if the
webrtc::MediaStreamAudioTrackInteface.set_enabled(false) has not been called. 
Nice solution with making the CreateAudioTrack a virtual to make sure cloning a
track always create the right track type!

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_source.h:48: // |stop_callback_| (if set),
and then sets the
nit: formatting.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:610: MediaStreamAudioSource*
audio_source;
Prefer to a virtual method CreateAudioSource that the test can override. See
CreateVideoSource.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.h
(right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_remote_audio_source.h:52: class
PeerConnectionRemoteAudioSource final
Suggest name MediaStreamRemoteAudioSource to match with
MediaStreamRemoteVideoSource .

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_source.cc:255: if (!source_)
can source_ really be null? looks like that can only happen if
EnsureSourceIsStarted has not been called or EnsureSourceIsStopped() is called
twice. 
Can you add a comment if EnsureSourceIsStopped() can be called twice?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_source.h:62:
blink::WebMediaConstraints source_constraints() const { return constraints_; }
On 2016/04/01 15:11:41, o1ka wrote:
> I just don't know if it's ok from WebRTC point of view to be able to do it
only
> ones. Is there a possibility that they are changed dynamically during the
call?

Constraints are supposed to be able to change from js
MediaStreamTrack.appy_constraints or something like that. But implementing that
will probably require tons of other changes and not in the scope of this cl.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_track.h:35: const
scoped_refptr<WebRtcLocalAudioTrackAdapter>& adapter() const {
Why isn't WebRtcLocalAudioTrackAdapter just a sink to a MediaStreamAudioTrack
that is created when needed in rtc_peerconnection_handler? Too big of a change?

[miu, 2016-04-08 18:36:47]

Status update: Olga's comments from April 1 led to an insight that allowed me to
detangle a few other things, resulting in less code, and resolving a number of
other comments automatically.  However, I've been distracted with other things,
so I don't have a new patch set ready yet (prob EOD today/Monday).

Also, yes, I understand the change is huge.  I was struggling to find ways to
make it smaller; but every time I moved one thing, 2+ other things had to be
reworked.  And, as you've noticed, this is an iterative process: As I get input
from you guys and make one thing better, a few other things have to be reworked
to make things fit again.  I'm trying to minimize complexity and churn in this
code review process, but I also want to iterate until everything is the way we
all want it to be.

Keep the great comments coming!  :)

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
File content/renderer/media/webaudio_media_stream_source.cc (right):

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
content/renderer/media/webaudio_media_stream_source.cc:92:
wrapper_bus_->set_frames(number_of_frames);
On 2016/04/01 15:11:41, o1ka wrote:
> Don't you need to protect |wrapper_bus_| from the concurrent access here and
in
> setFormat()? If not, than it would be nice to have a comment/DCHECK explaining
> why.

The header file includes a comment for these methods: It explains that blink
will not call into them concurrently.

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_media_stream_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_media_stream_adapter.cc:107:
webrtc_media_stream_->AddTrack(track_interface.get());
On 2016/04/01 15:11:41, o1ka wrote:
> But does webrtc_media_stream_->AddTrack() takes an ownership? It takes a
pointer
> to AudioTrackInterface, how can I assume that WebRtcLocalAudioTrackAdapter
will
> be owned by it?

Regarding the code in Patch Set 3: Yes, webrtc_media_stream_->AddTrack() takes
ownership because it will increment the object's ref count.

> Should't newly-created WebRtcLocalAudioTrackAdapter be owned by
> WebRtcMediaStreamAdapter? (something like in CreateVideoTrack)

This is a very interesting and a very good question.  As I looked into this
further, it seems that the original code was "hacky" in that it just took a
MediaStreamAudioTrack and treated it like it was a MediaStreamAudioSink. 
Furthermore, WebRtcLocalAudioTrackAdapter had this complex issue where it had
methods that could be run on both the main or "signaling" threads.

As I started to fix this, to make it more like in CreateVideoTrack() as you
suggested, I realized I could merge WebRtcLocalAudioTrackAdapter and
WebRtcAudioSinkAdapter together, and then split the code into an outer "shell"
class whose methods run on the main thread, and a private inner class whose
methods run on the libjingle signaling thread.  This allowed me to get rid of
all the thread-hopping code and passing around extra weak references (e.g., via
WebRtcLocalAudioTrackAdapter::SetMediaStreamAudioTrack).

So, to summarize what's changed in Patch Set 4:

1. WebRtcLocalAudioTrackAdapter and WebRtcAudioSinkAdapter have been refactored
into a single new class called WebRtcAudioSink, which is a subclass of
MediaStreamAudioSink.  WebRtcAudioSink runs exclusively on the main thread, and
there is a separate private inner class for the code that runs on the "libjingle
signaling thread."

2. The code here, in CreateAudioTrack(), has become just like that in
CreateVideoTrack().  I also did a little minor clean-up in
WebRtcMediaStreamAdapter, mainly because the naming of things was inaccurate.

3. After everything settled, I was also able to simply delete the
ProcessedLocalAudioTrack subclass, and just use plain 'ol MediaStreamAudioTrack.

Net result: Another ~200 LOC deleted, and the left-over code is better organized
(IMHO).  :)

[miu, 2016-04-08 18:39:31]

Oops!  By sending my last message, I published my comments prematurely. 
However, they do give a preview of what's coming in Patch Set 4 (not uploaded
yet).

[miu, 2016-04-19 00:40:23]

o1ka/perkj_chrome: PTAL.  To summarize what's changed between PS3 and PS4:

0. Rebased (mostly picked up the scoped_ptr-->std::unique_ptr migration).

1. Fixed the impl according to Per's comments about behaviors required by the
spec: a) silent audio flows through disabled tracks; b) tracks created on
stopped sources should be created anyway, and in the "ended" state.

2. WebRtcLocalAudioTrackAdapter and WebRtcAudioSinkAdapter have been refactored
into a single new class called WebRtcAudioSink, which is a subclass of
MediaStreamAudioSink.  WebRtcAudioSink runs exclusively on the main thread, and
there is a separate private inner class for the code that runs on the "libjingle
signaling thread."  No more WaitableEvent or SetMSAudioTrack() or thread-hopping
"weirdness" now!  :)

3. Common functionality that was pretty much duplicated in MSAudioSource and
MSAudioTrack has been factored out into a MediaStreamAudioDeliverer utility
class.

Responses to comments in-line:

On 2016/04/06 18:39:11, o1ka wrote:
> I feel bad about reviewing it that long, but the reality is it's a huge
change,
> and also my knowledge of "before" picture is limited.

No problem.  As you said, it's a huge change, and I expect it'll take some time
to go through it all.  The diagram you set up was great, and I've tried to
clarify things further with my modifications:
https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...

On 2016/04/06 18:39:11, o1ka wrote:
> I think the new structure is way much better - amazing job!
> It's still rather complicated and probably(?) there is some place for
> improvement, but I don't have any ideas for now.

Thanks!  :)  The clean-up is an iterative process.  What I was intending to do
here is get the "foundation" in-place.  Hopefully, any clean-up from here can be
done in smaller patches.

https://codereview.chromium.org/1834323002/diff/60001/content/content_rendere...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/1834323002/diff/60001/content/content_rendere...
content/content_renderer.gypi:322:
'renderer/media/media_stream_audio_source.cc',
On 2016/04/08 14:05:41, perkj_chrome wrote:
> put all mediastream audio files where the video counter part is. (I don't
> understand why its called private_renderer_webrtc_sources since it is not
webrtc
> specific)

IIUC, the build files are organized around code that can only be built when
USE_WEBRTC is defined.  I moved these files here because they no longer have any
dependencies on src/third_party/webrtc/... and should build/link/run regardless
of whether USE_WEBRTC is defined.

https://codereview.chromium.org/1834323002/diff/60001/content/content_rendere...
content/content_renderer.gypi:720:
'renderer/media/webrtc/peer_connection_remote_audio_source.cc',
On 2016/04/08 14:05:41, perkj_chrome wrote:
> For video this is called media_stream_remote_video_source. - can audio use the
> same naming?

Discussed in reply to your comment in the .h file.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/external_media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/external_media_stream_audio_source.cc:70:
audio_delay_milliseconds));
On 2016/04/08 14:05:41, perkj_chrome wrote:
> nit: indentation looks weird. git cl format? 

Done.  Yeah, `git cl format` did that.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:17: :
is_local_source_(is_local_source),
On 2016/04/08 14:05:41, perkj_chrome wrote:
> remove?

Looked into this.  To make a long-story short, this is plumbed through to the
track and the MediaStreamAudioRenderer's.  However, it's not used for anything
anywhere except the UMA stats in TrackAudioRenderer:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

That said, this is plumbed-in across almost a dozen files.  It would make this
huge CL even larger.  So, let's fix this in a follow-up CL.  Sound good?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:52: if (is_stopped_)
On 2016/04/08 14:05:41, perkj_chrome wrote:
> Is it necessary to check for if the source is stopped?
> I think that a track that is added a to a stopped source should produce a
track
> where the state is ended. 
> That can happen if you clone a track that has already been stopped in JS.

Good point.  Done.  I also just moved the code from
ConnectStartedSourceToTrack() into this method since nothing else calls it.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:110: void
MediaStreamAudioSource::DoStopSource() {
On 2016/04/08 14:05:41, perkj_chrome wrote:
> why this extra indirection? Just for naming? 

Are you asking why I have this call EnsureSourceIsStopped(), instead of just
requiring subclasses override DoStopSource() directly?  The main reason is that
there is some state changed in MSAudioSource (the is_stopped_ bool is set to
true), and some separate unknown state managed by subclasses.  Olga and I talked
about this in prior comments, and decided we didn't want to place requirements
on the code in subclasses to have to be sure to also change state in their
parent class.  This keeps the functionality specific to each class in the
inheritance hierarchy better encapsulated.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:132:
track->SetFormat(params_);
On 2016/04/01 15:11:41, o1ka wrote:
> I would probably use a separate lock for |params_|.
> And then I would use getter to access them here, there seem to be no need to
> call SetFormat() under the lock.

Done.  BTW--I realized MediaStreamAudioSource and MediaStreamAudioTrack both do
the same thing w.r.t. Add/Remove and audio format/deliver; so I factored those
things out into a separate MediaStreamAudioDeliverer "utility" class.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:154: // explicitly closed,
rather than auto-stop on the removal of the last track?
On 2016/04/08 14:05:41, perkj_chrome wrote:
> This is according to spec. 
> 
> If you have two tracks for the same source- stopping the first track only
stops
> that track. Stopping the second track actually stops the source.

Thanks for the clarification.  I've replaced this TODO comment with a comment
that states the spec requires this.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:31: // implement the
DoStopSource() and EnsureSourceIsStarted() methods, and call
On 2016/04/08 14:05:41, perkj_chrome wrote:
> How does DoStopSource and EnsureSourceStoped relate? Looks like this should be
> EnsureSourceIsStopped()

Whoops.  I needed to update this comment.  The intention is for subclasses to
only worry about implementing EnsureSourceIsStarted() and
EnsureSourceIsStopped().  I also declared DoStopSource() as "final" to make this
clearer.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:49: //          
->ConnectToTrack(blink_track)) {
On 2016/04/08 14:05:41, perkj_chrome wrote:
> For video- the tracks register and deregister with the source instead using a
> ConnectToTrack. Would it make sence for audio to work the same way for
> consistency?

I looked into this a bit.  IMO, it would be better for MediaStreamVideoSource to
instead change to the scheme used here since it makes the client code much
simpler.  Some fine points:

1. MSAudioSources sometimes need to provide extended MSAudioTrack
implementations.  It's more appropriate for the source to create the
MSAudioTrack rather than the client code (e.g., media_stream_center.cc) since
the client code does not (and probably should not) know the implementation
details of the source.

2. Client code must be sure to call MSVideoSource::RemoveTrack() at the right
time and get it right for all possible tear-down sequences and code paths.  With
this scheme, MSAudioSources and MSAudioTracks are guaranteed to be safely
shutdown and destroyed whenever their blink owners are destroyed, with no
additional client code written to ensure this.

3. Safety/Robustness: There are comments in MSVideoSource about it's blink owner
having to outlive all tracks.  To me, that feels a bit brittle, as future
changes to blink code could break this assumption.  If the blink code happens to
be changed in some way that causes a MSAudioSource to tear-down before its
tracks, the weak references will make sure nothing bad happens.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:101: // successful.  While a
the source is running, it may provide audio on any
On 2016/04/08 14:05:41, perkj_chrome wrote:
> nit: While a the /s  While the... 

Good catch.  Done.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:165: // Provides weak
pointers so that MediaStreamAudioTracks won't call
On 2016/04/08 14:05:41, perkj_chrome wrote:
> Good but I think its safe regardless- a blink track keeps the blink source
> alive.

Ack.  Discussed in reply to your comment above.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:138: if (!is_enabled_)
On 2016/04/08 14:05:41, perkj_chrome wrote:
> MediaStreamTrack.enabled = false means that audio packets should still be
> delivered but with energy 0.
> https://www.w3.org/TR/mediacapture-streams/#life-cycle-and-media-flow
> This is a bit weird though. 
> 
> In video- we currently produce a memory backed black frame.
> media_stream_video_track.cc
> 
> Does this cl change the current behavior?

Ah, good point.  My understanding of "enabled" was wrong.  Actually, if you look
at the different implementations of audio flow tracks-->sink (i.e., prior to
this refactoring), some halt the audio as well.  So, I assumed incorrectly from
that.

I've fixed this so that zero'ed-out data will continue to flow in all cases, as
the spec requires.  Glad this refactoring is helping to resolve these kinds of
inconsistencies/bugs!  :)

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:153: sink->OnData(audio_bus,
reference_time);
On 2016/04/01 15:11:41, o1ka wrote:
> I feel somewhat uneasy about this call to an unknown implementation under the
> lock which can be acquired by the public/protected interface of the class as
> well (MediaStreamAudioTrack::Stop(), MediaStreamAudioTrack::SetFormat(), etc).
> Does not seem to be a problem right now, but does not look right since it may
> potentially cause deadlocks.
> Maybe it's an irrational fear though.

I think what you're worried about is re-entrancy.  No need to worry about this
since, in debug builds, base::Lock DCHECKs that the same lock is not acquired a
second time on the same thread.

Holding the lock here is legitimate and required.  It's also a statement that
the object graph cannot be modified during the transfer of a chunk of audio
data.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_center.cc:44:
PeerConnectionDependencyFactory* factory) {
On 2016/04/08 14:05:42, perkj_chrome wrote:
> please remove |factory| I think this might be the last dependency in this
class.

Done.  Yes it is!  :)

I've removed |rtc_factory_| from MediaStreamCenter, but I added a TODO to remove
the unnecessary ctor args in a follow-up change since I didn't want this change
to spread-out into even more files.  ;)

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_center.cc:110: : rtc_factory_(factory) {}
On 2016/04/08 14:05:42, perkj_chrome wrote:
> please remove

Done.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_renderer_factory_impl.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_renderer_factory_impl.cc:124: if
(!PeerConnectionRemoteAudioTrack::From(audio_track)) {
On 2016/04/08 14:05:42, perkj_chrome wrote:
> PeerConnectionRemoteAudioTrack is probably not needed at all soon. The problem
> as I can see is MediaStreamTrack.enabled=false on remote audio tracks since I
> guess the remote audio renderer will not know if the
> webrtc::MediaStreamAudioTrackInteface.set_enabled(false) has not been called. 
> Nice solution with making the CreateAudioTrack a virtual to make sure cloning
a
> track always create the right track type!

Acknowledged.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_source.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_source.h:48: // |stop_callback_| (if set),
and then sets the
On 2016/04/08 14:05:42, perkj_chrome wrote:
> nit: formatting.

Done.  Looks like `git cl format` doesn't fix comment text.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:610: MediaStreamAudioSource*
audio_source;
On 2016/04/08 14:05:42, perkj_chrome wrote:
> Prefer to a virtual method CreateAudioSource that the test can override. See
> CreateVideoSource.

Good idea.  Done.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.cc
(right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_remote_audio_source.cc:122:
base::TimeTicks playout_time(base::TimeTicks::Now());
On 2016/04/01 15:11:41, o1ka wrote:
> Probably have paranoid thread checks in all those callbacks to make sure they
> are coming from the same thread?

Done.  But, it's not that they should be coming from the same thread, it's that
we want to make sure OnData() is not being executed on more than one thread at
any time.

I added a check here, but I did not use ThreadChecker because that would assume
this Chromium code knows something about the threading model of an external
library.  For example, it'd be legitimate for libjingle to use a different
thread to invoke this method (for some reason specific to its internal workings;
perhaps whenever the audio format changes).

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.h
(right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_remote_audio_source.h:52: class
PeerConnectionRemoteAudioSource final
On 2016/04/08 14:05:42, perkj_chrome wrote:
> Suggest name MediaStreamRemoteAudioSource to match with
> MediaStreamRemoteVideoSource .

I renamed it from that because this source is PeerConnection/WebRTC-specific. 
There are other non-WebRTC remote audio sources in Chromium nowadays (e.g., the
Cast Streaming Receiver), so I wanted to make the distinction to avoid
confusion.  Sound good?

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_source.cc:255: if (!source_)
On 2016/04/08 14:05:42, perkj_chrome wrote:
> can source_ really be null? looks like that can only happen if
> EnsureSourceIsStarted has not been called or EnsureSourceIsStopped() is called
> twice. 
> Can you add a comment if EnsureSourceIsStopped() can be called twice? 

Ack.  (EnsureSourceIsStopped() shouldn't be called twice.)

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_track.h:35: const
scoped_refptr<WebRtcLocalAudioTrackAdapter>& adapter() const {
On 2016/04/08 14:05:42, perkj_chrome wrote:
> Why isn't WebRtcLocalAudioTrackAdapter just a sink to a MediaStreamAudioTrack
> that is created when needed in rtc_peerconnection_handler? Too big of a
change? 
> 

It is now!  :)  As of Patch Set 4, WebRtcAudioSink was introduced to get rid of
this weirdness.  In fact, I was also able to delete ProcessedLocalAudioTrack as
well (it's no longer needed).

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_audio_sink_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_audio_sink_adapter.cc:59:
fifo_.Reset(params_.frames_per_buffer());
On 2016/04/01 15:11:41, o1ka wrote:
> What is the guarantee that there is no race between OnSetFormat() and
OnData()?
> It would be nice to have a comment or thread check

Done.  (This method moved to the new WebRtcAudioSink class in Patch Set 4.)

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:65: track_ =
track;
On 2016/04/01 15:11:41, o1ka wrote:
> This part I haven't processed entirely yet. What if SetMediaStreamAudioTrack()
> is called twice? And what if AddSink() is called before it?

No longer applicable (Patch Set 4).  See my prior e-mail about refactoring this
class so weird things like this are no longer needed.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:65: track_ =
track;
On 2016/04/06 18:39:10, o1ka wrote:
> The interface does set any expectations on SetMediaStreamAudioTrack: when it
is
> called, or how many times. But from implementation it looks like it's expected
> that it's called only once and before any AddSink calls. If it's called after
> AddSink(), then the previously added sink won't be added to the track, but on
> RemoveSink() there will be an attempt to remove it from the track. Also,
AddSink
> can race with SetMediaStreamAudioTrack.

No longer applicable (Patch Set 4).

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:99: // null for
single-threaded unit tests.
On 2016/04/06 18:39:10, o1ka wrote:
> Are you planing to have some multi-threaded unit tests as well? (for
example,for
> checking done_event trick is not broken)

Possibly.  But, with Patch Set 4 I was able to get rid of the "done_event trick"
altogether!  :)

To answer your question more broadly: I haven't modified the tests yet because
we are still working out major code structure.  Once it feels like the code
structure has stabilized, I'll modify existing tests and add whatever extra
tests are needed.

[perkj_chrome, 2016-04-20 13:34:54]

Looks very good to me.

Tommi- can you please try to take a look as well? I don't know the audio
specific parts and I think olga wants help with another pair of eyes.

https://codereview.chromium.org/1834323002/diff/60001/content/content_rendere...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/1834323002/diff/60001/content/content_rendere...
content/content_renderer.gypi:322:
'renderer/media/media_stream_audio_source.cc',
On 2016/04/19 00:40:21, miu wrote:
> On 2016/04/08 14:05:41, perkj_chrome wrote:
> > put all mediastream audio files where the video counter part is. (I don't
> > understand why its called private_renderer_webrtc_sources since it is not
> webrtc
> > specific)
> 
> IIUC, the build files are organized around code that can only be built when
> USE_WEBRTC is defined.  I moved these files here because they no longer have
any
> dependencies on src/third_party/webrtc/... and should build/link/run
regardless
> of whether USE_WEBRTC is defined.

ok- I wonder why the video counter parts are under webrtc in that case. I will
try to get time to fix that.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:17: :
is_local_source_(is_local_source),
On 2016/04/19 00:40:22, miu wrote:
> On 2016/04/08 14:05:41, perkj_chrome wrote:
> > remove?
> 
> Looked into this.  To make a long-story short, this is plumbed through to the
> track and the MediaStreamAudioRenderer's.  However, it's not used for anything
> anywhere except the UMA stats in TrackAudioRenderer:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> That said, this is plumbed-in across almost a dozen files.  It would make this
> huge CL even larger.  So, let's fix this in a follow-up CL.  Sound good?

sure

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:110: void
MediaStreamAudioSource::DoStopSource() {
On 2016/04/19 00:40:22, miu wrote:
> On 2016/04/08 14:05:41, perkj_chrome wrote:
> > why this extra indirection? Just for naming? 
> 
> Are you asking why I have this call EnsureSourceIsStopped(), instead of just
> requiring subclasses override DoStopSource() directly?  The main reason is
that
> there is some state changed in MSAudioSource (the is_stopped_ bool is set to
> true), and some separate unknown state managed by subclasses.  Olga and I
talked
> about this in prior comments, and decided we didn't want to place requirements
> on the code in subclasses to have to be sure to also change state in their
> parent class.  This keeps the functionality specific to each class in the
> inheritance hierarchy better encapsulated.

Acknowledged.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:49: //          
->ConnectToTrack(blink_track)) {
On 2016/04/19 00:40:22, miu wrote:
> On 2016/04/08 14:05:41, perkj_chrome wrote:
> > For video- the tracks register and deregister with the source instead using
a
> > ConnectToTrack. Would it make sence for audio to work the same way for
> > consistency?
> 
> I looked into this a bit.  IMO, it would be better for MediaStreamVideoSource
to
> instead change to the scheme used here since it makes the client code much
> simpler.  Some fine points:
> 
> 1. MSAudioSources sometimes need to provide extended MSAudioTrack
> implementations.  It's more appropriate for the source to create the
> MSAudioTrack rather than the client code (e.g., media_stream_center.cc) since
> the client code does not (and probably should not) know the implementation
> details of the source.
> 
> 2. Client code must be sure to call MSVideoSource::RemoveTrack() at the right
> time and get it right for all possible tear-down sequences and code paths. 
With
> this scheme, MSAudioSources and MSAudioTracks are guaranteed to be safely
> shutdown and destroyed whenever their blink owners are destroyed, with no
> additional client code written to ensure this.
> 
> 3. Safety/Robustness: There are comments in MSVideoSource about it's blink
owner
> having to outlive all tracks.  To me, that feels a bit brittle, as future
> changes to blink code could break this assumption.  If the blink code happens
to
> be changed in some way that causes a MSAudioSource to tear-down before its
> tracks, the weak references will make sure nothing bad happens.

Ok- we should try to get video to behave the same once this is landed. Feel free
to file a bug and assign to me.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:165: // Provides weak
pointers so that MediaStreamAudioTracks won't call
On 2016/04/19 00:40:22, miu wrote:
> On 2016/04/08 14:05:41, perkj_chrome wrote:
> > Good but I think its safe regardless- a blink track keeps the blink source
> > alive.
> 
> Ack.  Discussed in reply to your comment above.

Acknowledged.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.h
(right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_remote_audio_source.h:52: class
PeerConnectionRemoteAudioSource final
On 2016/04/19 00:40:22, miu wrote:
> On 2016/04/08 14:05:42, perkj_chrome wrote:
> > Suggest name MediaStreamRemoteAudioSource to match with
> > MediaStreamRemoteVideoSource .
> 
> I renamed it from that because this source is PeerConnection/WebRTC-specific. 
> There are other non-WebRTC remote audio sources in Chromium nowadays (e.g.,
the
> Cast Streaming Receiver), so I wanted to make the distinction to avoid
> confusion.  Sound good?

Acknowledged.

https://codereview.chromium.org/1834323002/diff/80001/content/public/renderer...
File content/public/renderer/media_stream_utils.cc (right):

https://codereview.chromium.org/1834323002/diff/80001/content/public/renderer...
content/public/renderer/media_stream_utils.cc:86: new
ExternalMediaStreamAudioSource(std::move(audio_source), sample_rate,
nit: You probably now better than me. audio_source is a scoped_refptr -
should the argument then be const scoped_refptr<media::AudioCapturerSource>&
audio_source then instead of using std::move on the scoped_refptr ?

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/external_media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/external_media_stream_audio_source.h:14: // Represents an
externally-provided local or remote source of audio data.  This
nit- remove extra space

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_deliverer.h (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_deliverer.h:40: // Begin delivering
audio to |consumer|.  The caller must guarantee |consumer|
nit: Remove extra space

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_deliverer.h:134: // changed audio
format, are placed in this list.  This includes consumers
nit: remove extra space

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_deliverer.h:136: // before audio data
is first delivered.  Consumers are moved from this list
dito

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:54: return false;
is it safe to return here? ie- that means that the blink track will not have a
MediaStreamAudioTrack in the extra data field. ie- what if SetEnabled or Stop
etc is called on the blink track?

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:25: // the source
implementation and one or more MediaStreamAudioTracks.  This is
I am starting to think you want to have to spaces after each "." ?

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.h:85: void Start(const
base::Closure& stop_callback);
No risk that MediaStreamTrack::Stop() is called before Start is called by the
source?

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_center.cc:132: MediaStreamTrack*
native_track = MediaStreamTrack::GetTrack(track);
This will crash if  MediaStreamTrack::GetTrack(track) does not return a native
track. We need to decide if there should always exist a native track for all
created blink tracks. I think it should. See my comment in ConnectToTrack..

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1509: const
std::unique_ptr<WebRtcAudioSink> dummy_sink(new WebRtcAudioSink(
Can you instead find the correct webrtc audio track from 
the local mediastreams added to the pc in local_streams_
(WebRtcMediaStreamAdapter) .

Something like:


webrtc::AudioTrackInterface* webrtc_track = nullptr;
for (auto adapter_it : local_streams_) {
webrtc_track = 
adapter_it->webrtc_media_stream()->FindAudioTrack(track.id().utf8()));
if (webrtc_track)
  break;
}

[miu, 2016-04-20 21:58:58]

Description was changed from

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881,577874
==========

to

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

Diagram, for reference:
https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...

BUG=577881,577874
==========

[miu, 2016-04-20 22:04:53]

perkj: Addressed your comments.  PTAL at Patch Set 5.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:49: //          
->ConnectToTrack(blink_track)) {
On 2016/04/20 13:34:53, perkj_chrome wrote:
> On 2016/04/19 00:40:22, miu wrote:
> > On 2016/04/08 14:05:41, perkj_chrome wrote:
> > > ConnectToTrack. Would it make sence for audio to work the same way for
> > > consistency?
> 
> Ok- we should try to get video to behave the same once this is landed. Feel
free
> to file a bug and assign to me.

Done.  https://bugs.chromium.org/p/chromium/issues/detail?id=605250

https://codereview.chromium.org/1834323002/diff/80001/content/public/renderer...
File content/public/renderer/media_stream_utils.cc (right):

https://codereview.chromium.org/1834323002/diff/80001/content/public/renderer...
content/public/renderer/media_stream_utils.cc:86: new
ExternalMediaStreamAudioSource(std::move(audio_source), sample_rate,
On 2016/04/20 13:34:53, perkj_chrome wrote:
> nit: You probably now better than me. audio_source is a scoped_refptr -
> should the argument then be const scoped_refptr<media::AudioCapturerSource>&
> audio_source then instead of using std::move on the scoped_refptr ?

The guidance on this, for the Chromium project as-a-whole, was recently made
clearer.  When a method (or ctor) is going to add a reference on a ref-counted
object, the argument is passed by value rather than by const-ref; and then
std::move() is used.

See the section on "calling conventions" here:
https://www.chromium.org/developers/smart-pointer-guidelines  While they only
show examples for scoped_ptr, the e-mail discussion thread agreed that the same
should apply to scoped_refptr's also for consistency.

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/external_media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/external_media_stream_audio_source.h:14: // Represents an
externally-provided local or remote source of audio data.  This
On 2016/04/20 13:34:53, perkj_chrome wrote:
> nit- remove extra space

Done.

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_deliverer.h (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_deliverer.h:40: // Begin delivering
audio to |consumer|.  The caller must guarantee |consumer|
On 2016/04/20 13:34:54, perkj_chrome wrote:
> nit: Remove extra space

Done.

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_deliverer.h:134: // changed audio
format, are placed in this list.  This includes consumers
On 2016/04/20 13:34:53, perkj_chrome wrote:
> nit: remove extra space

Done.

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_deliverer.h:136: // before audio data
is first delivered.  Consumers are moved from this list
On 2016/04/20 13:34:54, perkj_chrome wrote:
> dito

Done.

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:54: return false;
On 2016/04/20 13:34:54, perkj_chrome wrote:
> is it safe to return here? ie- that means that the blink track will not have a
> MediaStreamAudioTrack in the extra data field. ie- what if SetEnabled or Stop
> etc is called on the blink track? 

Done.  I see your point.  Agree that it's safer to always make sure there's
always something to find in the extra data field.

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:25: // the source
implementation and one or more MediaStreamAudioTracks.  This is
On 2016/04/20 13:34:54, perkj_chrome wrote:
> I am starting to think you want to have to spaces after each "." ? 

Done.  Sorry, I'm old-school (it's a habit).  I've noticed there's a mix of both
1 or 2 spaces after a period.  I did some searching around on the web and it
seems I should just start adopting the 1-space rule.  :)

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.h:85: void Start(const
base::Closure& stop_callback);
On 2016/04/20 13:34:54, perkj_chrome wrote:
> No risk that MediaStreamTrack::Stop() is called before Start is called by the
> source?

No risk.  MediaStreamAudioSource::ConnectToTrack() creates the MSAudioTrack and
calls Start() on it before any external code could call Stop().

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_center.cc:132: MediaStreamTrack*
native_track = MediaStreamTrack::GetTrack(track);
On 2016/04/20 13:34:54, perkj_chrome wrote:
> This will crash if  MediaStreamTrack::GetTrack(track) does not return a native
> track. We need to decide if there should always exist a native track for all
> created blink tracks. I think it should. See my comment in ConnectToTrack..

Acknowledged. Agree that ConnectToTrack() should always create a native track
object, and I made that code change. So, it would be a bug to ever encounter a
null pointer here.

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1834323002/diff/80001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1509: const
std::unique_ptr<WebRtcAudioSink> dummy_sink(new WebRtcAudioSink(
On 2016/04/20 13:34:54, perkj_chrome wrote:
> Can you instead find the correct webrtc audio track from 
> the local mediastreams added to the pc in local_streams_
> (WebRtcMediaStreamAdapter) .

Done.  Yes!  This is what I was looking for.  :)

BTW--Tested successfully using this demo:
https://webrtc.github.io/samples/src/content/peerconnection/dtmf/

[o1ka, 2016-04-21 18:51:23]

I can't believe it's becoming THAT clear comparing to what it used to be :)

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:132:
track->SetFormat(params_);
On 2016/04/19 00:40:22, miu wrote:
> On 2016/04/01 15:11:41, o1ka wrote:
> > I would probably use a separate lock for |params_|.
> > And then I would use getter to access them here, there seem to be no need to
> > call SetFormat() under the lock.
> 
> Done.  BTW--I realized MediaStreamAudioSource and MediaStreamAudioTrack both
do
> the same thing w.r.t. Add/Remove and audio format/deliver; so I factored those
> things out into a separate MediaStreamAudioDeliverer "utility" class.

Really like it!

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_audio_track.cc:153: sink->OnData(audio_bus,
reference_time);
On 2016/04/19 00:40:22, miu wrote:
> On 2016/04/01 15:11:41, o1ka wrote:
> > I feel somewhat uneasy about this call to an unknown implementation under
the
> > lock which can be acquired by the public/protected interface of the class as
> > well (MediaStreamAudioTrack::Stop(), MediaStreamAudioTrack::SetFormat(),
etc).
> > Does not seem to be a problem right now, but does not look right since it
may
> > potentially cause deadlocks.
> > Maybe it's an irrational fear though.
> 
> I think what you're worried about is re-entrancy.  No need to worry about this
> since, in debug builds, base::Lock DCHECKs that the same lock is not acquired
a
> second time on the same thread.
> 
> Holding the lock here is legitimate and required.  It's also a statement that
> the object graph cannot be modified during the transfer of a chunk of audio
> data.

Acknowledged.

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.cc
(right):

https://codereview.chromium.org/1834323002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_remote_audio_source.cc:122:
base::TimeTicks playout_time(base::TimeTicks::Now());
On 2016/04/19 00:40:22, miu wrote:
> On 2016/04/01 15:11:41, o1ka wrote:
> > Probably have paranoid thread checks in all those callbacks to make sure
they
> > are coming from the same thread?
> 
> Done.  But, it's not that they should be coming from the same thread, it's
that
> we want to make sure OnData() is not being executed on more than one thread at
> any time.
> 
> I added a check here, but I did not use ThreadChecker because that would
assume
> this Chromium code knows something about the threading model of an external
> library.  For example, it'd be legitimate for libjingle to use a different
> thread to invoke this method (for some reason specific to its internal
workings;
> perhaps whenever the audio format changes).

Acknowledged.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_audio_deliverer.h (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_deliverer.h:92: {
nit: it would probably be more consistent to have a setter to pair with getter,
so that params_lock usage is visibly localized.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:72: // stop the track to set
its ready state to "ended."
"ended."->"ended".
It's not quite clear what "ready state" is in relation to MediaStreamAudioTrack.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:74: track->Stop();
Consumer is added at l.69 and it is removed here when track->Stop() calls
StopAudioDeliveryTo() as a callback. Is it intentional? Can it be done in a more
obvious way? What is the "state" of the track if we have not called Start()?
Shouldn't it be "ended"?

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.cc
(right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/peer_connection_remote_audio_source.cc:125: //
about the private threading model of libjingle.
I think "for example.." part of your review comment would be useful here.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source.cc:387: return
RenderThreadImpl::current()->GetAudioHardwareConfig()
I'm not sure if it makes sense to return output buffer size for input. And let's
avoid using HardwareConfig, because it provides information on default device,
and we want to be able to switch devices, so the gaol is to hopefully deprecate
HardwareConfig at some point.
Let's leave it 10 ms as it was before?

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source.h:60: // before the
first call to ConnectToTrack().
Is it the case now? (Sorry, it's a bit difficult to navigate through all this
code to check)

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/webrtc_audio_sink.cc (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/webrtc_audio_sink.cc:68:
DCHECK(audio_thread_checker_.CalledOnValidThread());
I think I'm missing something: what is the guarantee that OnSetFormat does not
race with OnData? Is it possible that the last OnData/DeliverRebufferedAudio is
still progress when we reattach to another thread?

[miu, 2016-04-21 20:42:30]

o1ka: Thanks again for reviewing.  Addressed all your comments.  PTAL at Patch
Set 6.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_audio_deliverer.h (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_deliverer.h:92: {
On 2016/04/21 18:51:22, o1ka wrote:
> nit: it would probably be more consistent to have a setter to pair with
getter,
> so that params_lock usage is visibly localized.

This method *is* the setter.  ;)  But, I see that you're talking about this
inner block of code: I'd prefer not to split this out since |consumers_lock_|
should also be held while changing the audio params (to synchronize when we
notify consumers that the audio format has changed).  IMHO, it makes things more
readable to see that happening alongside with the params change.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:72: // stop the track to set
its ready state to "ended."
On 2016/04/21 18:51:22, o1ka wrote:
> "ended."->"ended".

Done.

> It's not quite clear what "ready state" is in relation to
MediaStreamAudioTrack.

I had assumed "ready state" is a property of the blink::WebMediaStreamTrack,
which is changed by MediaStreamAudioTrack.  However, looking at this more
in-depth, I see that this is only used as a way to notify sinks when a source
has ended.  See reply to comment below...

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:74: track->Stop();
On 2016/04/21 18:51:22, o1ka wrote:
> Consumer is added at l.69 and it is removed here when track->Stop() calls
> StopAudioDeliveryTo() as a callback. Is it intentional?

It was.  Per suggested (previous round of comments) that we always create an
"ended" track when the source is stopped.  

> Can it be done in a more
> obvious way? What is the "state" of the track if we have not called Start()?
> Shouldn't it be "ended"?

Yep.  See my reply to your comment above (L72).  Since it's impossible for any
sinks to have been connected to this track, there's no need to start/stop it. 
So, I've changed this code to just return early without the extra overhead.  The
MediaStreamAudioTrack will be in the correct state.

Also, I've added some code to MediaStreamAudioTrack::AddSink(): If client code
still tries to add sinks to a stopped track, we'll just notify the sink that the
track has ended and NOT add the sink to the track.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.cc
(right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/peer_connection_remote_audio_source.cc:125: //
about the private threading model of libjingle.
On 2016/04/21 18:51:22, o1ka wrote:
> I think "for example.." part of your review comment would be useful here.

Done.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source.cc:387: return
RenderThreadImpl::current()->GetAudioHardwareConfig()
On 2016/04/21 18:51:22, o1ka wrote:
> I'm not sure if it makes sense to return output buffer size for input. And
let's
> avoid using HardwareConfig, because it provides information on default device,
> and we want to be able to switch devices, so the gaol is to hopefully
deprecate
> HardwareConfig at some point.
> Let's leave it 10 ms as it was before?

Done.  There's a side-issue with audio buffering that'll have to be addressed in
a separate change: Basically, customers contacted me--and IIRC, you're cc'ed on
the e-mail from a few weeks back--and they said the small buffer sizes were
eating up too much CPU and power for their use case.  I think, but I'm not
entirely sure, that this is tracking resolution of this issue:
https://crbug.com/602587

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source.h:60: // before the
first call to ConnectToTrack().
On 2016/04/21 18:51:22, o1ka wrote:
> Is it the case now? (Sorry, it's a bit difficult to navigate through all this
> code to check)

Yes.  This is called from user_media_client_impl.cc.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/webrtc_audio_sink.cc (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/webrtc_audio_sink.cc:68:
DCHECK(audio_thread_checker_.CalledOnValidThread());
On 2016/04/21 18:51:22, o1ka wrote:
> I think I'm missing something: what is the guarantee that OnSetFormat does not
> race with OnData? Is it possible that the last OnData/DeliverRebufferedAudio
is
> still progress when we reattach to another thread?

This is a sink, so only a MediaStreamAudioTrack should call
OnSetFormat()/OnData(). MediaStreamAudioTrack, in turn, delegates this
functionality to the MediaStreamAudioDeliverer, which always calls OnSetFormat()
on the same thread as OnData().

[miu, 2016-04-21 20:43:54]

Description was changed from

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

Diagram, for reference:
https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...

BUG=577881,577874
==========

to

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

Diagram of post-refactoring class relationships:
https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881,577874
==========

[o1ka, 2016-04-22 11:29:26]

I think this CL is in a very good shape.
tommi@ - we need your input, since you are the only one but  miu@ who is
familiar with the "before" picture.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:72: // stop the track to set
its ready state to "ended."
On 2016/04/21 20:42:30, miu wrote:
> On 2016/04/21 18:51:22, o1ka wrote:
> > "ended."->"ended".
> 
> Done.
> 
> > It's not quite clear what "ready state" is in relation to
> MediaStreamAudioTrack.
> 
> I had assumed "ready state" is a property of the blink::WebMediaStreamTrack,
> which is changed by MediaStreamAudioTrack.  However, looking at this more
> in-depth, I see that this is only used as a way to notify sinks when a source
> has ended.  See reply to comment below...

Yes, I realized it, the problem about the comment was that it had not enough of
context since it referred to a state of a blink class but mentioned media class
only (but looks like my review comment did not have enough of the context either
:) )

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:74: track->Stop();
On 2016/04/21 20:42:30, miu wrote:
> On 2016/04/21 18:51:22, o1ka wrote:
> > Consumer is added at l.69 and it is removed here when track->Stop() calls
> > StopAudioDeliveryTo() as a callback. Is it intentional?
> 
> It was.  Per suggested (previous round of comments) that we always create an
> "ended" track when the source is stopped.  
> 
> > Can it be done in a more
> > obvious way? What is the "state" of the track if we have not called Start()?
> > Shouldn't it be "ended"?
> 
> Yep.  See my reply to your comment above (L72).  Since it's impossible for any
> sinks to have been connected to this track, there's no need to start/stop it. 
> So, I've changed this code to just return early without the extra overhead. 
The
> MediaStreamAudioTrack will be in the correct state.
> 
> Also, I've added some code to MediaStreamAudioTrack::AddSink(): If client code
> still tries to add sinks to a stopped track, we'll just notify the sink that
the
> track has ended and NOT add the sink to the track.

Yes, I understand the reason of a changed, just was confused with the execution
flow. Great that it can be done in a more straightforward way.

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/peer_connection_remote_audio_source.cc
(right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/peer_connection_remote_audio_source.cc:125: //
about the private threading model of libjingle.
On 2016/04/21 20:42:30, miu wrote:
> On 2016/04/21 18:51:22, o1ka wrote:
> > I think "for example.." part of your review comment would be useful here.
> 
> Done.

Thanks!

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/1834323002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source.cc:387: return
RenderThreadImpl::current()->GetAudioHardwareConfig()
On 2016/04/21 20:42:30, miu wrote:
> On 2016/04/21 18:51:22, o1ka wrote:
> > I'm not sure if it makes sense to return output buffer size for input. And
> let's
> > avoid using HardwareConfig, because it provides information on default
device,
> > and we want to be able to switch devices, so the gaol is to hopefully
> deprecate
> > HardwareConfig at some point.
> > Let's leave it 10 ms as it was before?
> 
> Done.  There's a side-issue with audio buffering that'll have to be addressed
in
> a separate change: Basically, customers contacted me--and IIRC, you're cc'ed
on
> the e-mail from a few weeks back--and they said the small buffer sizes were
> eating up too much CPU and power for their use case.  I think, but I'm not
> entirely sure, that this is tracking resolution of this issue:
> https://crbug.com/602587

Yes, I keep that discussion about high latency input in mind
(https://crbug.com/602587 is a different bug though, it's related to Mac
specifics of handling several open devices, it's a browser-side logic, and is
affecting output buffer size mostly, since input buffer size is initially pretty
small)
Let's just keep any rebuffering/resamling changes outside of this CL.

[perkj_chrome, 2016-04-25 15:18:17]

lgtm
Lets get it in and see if it works :->

[o1ka, 2016-04-25 15:21:30]

On 2016/04/25 15:18:17, perkj_chrome wrote:
> lgtm
> Lets get it in and see if it works :->

Let's have a couple of unit tests first ;)

[miu, 2016-04-25 20:55:53]

On 2016/04/25 15:21:30, o1ka wrote:
> On 2016/04/25 15:18:17, perkj_chrome wrote:
> > lgtm
> > Lets get it in and see if it works :->
> 
> Let's have a couple of unit tests first ;)

Yes, absolutely!  As I mentioned a while back, I wanted things to "finalize" a
bit before I started on those.  WIP...

[tommi (sloooow) - chröme, 2016-04-25 21:07:16]

currently at processed_audio_source.  Not many comments, overall looks great :)

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:72: is_enabled_ = enabled;
nit: would it make sense for this to be an atomic?

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:122: base::AutoLock
auto_lock(enabled_lock_);
using an atomic might actually make a difference here.  On Mac, mutexes are 100x
slower than on Linux.  Atomics are much more efficient, so it looks like you're
using is_enabled_ essentially as if it were atomic, so we might as well do that.

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_audio_track.h:72: virtual void*
GetClassIdentifier() const;
what is this used for now?  Should the return value be an enum instead of a
pointer?

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_center.cc:51: // TODO(miu): This needs to be
moved to an appropriate location. A WebAudio
Can you test your cl with go/wa-rtc and see if getting remote audio from webrtc
into webaudio, works? You should get bars for both the local and remote end.
(just ping me if you have questions)

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/webaudio_media_stream_source.cc (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/webaudio_media_stream_source.cc:72: if
(is_registered_consumer_) {
nit: can do early return instead

if (!is_registered_consumer_ || blink_source_.isNull())
  return;

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source.h:61:
blink::WebMediaConstraints source_constraints() const { return constraints_; }
const &?

[miu, 2016-04-28 20:18:50]

Thanks, tommi.  Addressed your comments on PS6 (please see diff between PS6 and
PS7 for changes made).

Still working on migrating the unit tests to this new factoring...

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:72: is_enabled_ = enabled;
On 2016/04/25 21:07:16, tommi-chrömium wrote:
> nit: would it make sense for this to be an atomic?

Done.

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:122: base::AutoLock
auto_lock(enabled_lock_);
On 2016/04/25 21:07:16, tommi-chrömium wrote:
> using an atomic might actually make a difference here.  On Mac, mutexes are
100x
> slower than on Linux.  Atomics are much more efficient, so it looks like
you're
> using is_enabled_ essentially as if it were atomic, so we might as well do
that.

Done. My understanding is that the timing of when the audio thread sees the new
"is_enabled_" value can be relaxed, and there are no other memory dependencies
between these threads. Thus, I used NoBarrier_Load() here.

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_audio_track.h:72: virtual void*
GetClassIdentifier() const;
On 2016/04/25 21:07:16, tommi-chrömium wrote:
> what is this used for now?  Should the return value be an enum instead of a
> pointer?

This is used for identifying a specific implementation of MSAudioTrack. It's
being used in MediaStreamRendererFactoryImpl and WebRtcAudioRenderer to identify
PeerConnectionRemoteAudioTrack for special handling.

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_center.cc (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_center.cc:51: // TODO(miu): This needs to be
moved to an appropriate location. A WebAudio
On 2016/04/25 21:07:16, tommi-chrömium wrote:
> Can you test your cl with go/wa-rtc and see if getting remote audio from
webrtc
> into webaudio, works? You should get bars for both the local and remote end.
> (just ping me if you have questions)

Done. Yep, works perfectly!  :)

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/webaudio_media_stream_source.cc (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/webaudio_media_stream_source.cc:72: if
(is_registered_consumer_) {
On 2016/04/25 21:07:16, tommi-chrömium wrote:
> nit: can do early return instead
> 
> if (!is_registered_consumer_ || blink_source_.isNull())
>   return;

Done.

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source.h (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source.h:61:
blink::WebMediaConstraints source_constraints() const { return constraints_; }
On 2016/04/25 21:07:16, tommi-chrömium wrote:
> const &?

Done.

[o1ka, 2016-04-29 09:53:46]

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:122: base::AutoLock
auto_lock(enabled_lock_);
On 2016/04/28 20:18:49, miu wrote:
> On 2016/04/25 21:07:16, tommi-chrömium wrote:
> > using an atomic might actually make a difference here.  On Mac, mutexes are
> 100x
> > slower than on Linux.  Atomics are much more efficient, so it looks like
> you're
> > using is_enabled_ essentially as if it were atomic, so we might as well do
> that.
> 
> Done. My understanding is that the timing of when the audio thread sees the
new
> "is_enabled_" value can be relaxed, and there are no other memory dependencies
> between these threads. Thus, I used NoBarrier_Load() here.

Could you put it into a comment?

https://codereview.chromium.org/1834323002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_track.h:111: base::subtle::AtomicWord
is_enabled_;
Atomic32 maybe? (AtomicWord size varies depending on the platform; it's
recommended to be used for pointers)

[miu, 2016-05-04 02:47:53]

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/120001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:122: base::AutoLock
auto_lock(enabled_lock_);
On 2016/04/29 09:53:46, o1ka wrote:
> On 2016/04/28 20:18:49, miu wrote:
> > On 2016/04/25 21:07:16, tommi-chrömium wrote:
> > > using an atomic might actually make a difference here.  On Mac, mutexes
are
> > 100x
> > > slower than on Linux.  Atomics are much more efficient, so it looks like
> > you're
> > > using is_enabled_ essentially as if it were atomic, so we might as well do
> > that.
> > 
> > Done. My understanding is that the timing of when the audio thread sees the
> new
> > "is_enabled_" value can be relaxed, and there are no other memory
dependencies
> > between these threads. Thus, I used NoBarrier_Load() here.
> 
> Could you put it into a comment?

Done.

https://codereview.chromium.org/1834323002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_track.h:111: base::subtle::AtomicWord
is_enabled_;
On 2016/04/29 09:53:46, o1ka wrote:
> Atomic32 maybe? (AtomicWord size varies depending on the platform; it's
> recommended to be used for pointers)

Done.

[miu, 2016-05-04 02:49:15]

Thanks for your patience. Tests are now ready! :)

Recommendation: Just review diff between PS7 and PS8. This Chrome extension can
help, if you don't already have it:
https://chrome.google.com/webstore/detail/rietveld-usability-toolki/nmljjlfbn...

[o1ka, 2016-05-04 08:49:24]

Looks really nice, a couple of comments (some bots do not look happy thought).

Side note: all the reviewers are from STO, and we've got a half-day today and a
holiday tomorrow (and some people will be away on Friday as well); so,
unfortunately, please expect a bit of a delay with this review round.

https://codereview.chromium.org/1834323002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.h (right):

https://codereview.chromium.org/1834323002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_audio_track.h:77: virtual void OnStop();
Nice that we don't have it any more!

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:24: Stop();
Is there a test to check correctness of non-stopped track destruction?

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/media_stream_audio_unittest.cc (right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_unittest.cc:189:
ASSERT_EQ(expected_sample_count_, data[i]);
So, from source to sink, you converted int64->float->int64 here. I'm not sure if
the check will pass for big int64 values, because float precision should drop
after max_int32.

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_unittest.cc:230: class
MediaStreamAudioTest : public ::testing::Test {
Very nice testing!

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_unittest.cc:361:
EXPECT_TRUE(expected_params.Equals(sink.params()));
What are track()->GetOutputFormat() and sink.params() initially? Shouldn't we
check in the beginning that they are no equal to expected_params, to make this
unit test more robust?

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/mock_audio_device_factory.cc (right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/mock_audio_device_factory.cc:27: NOTIMPLEMENTED();
You can just mock them.

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc
(right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:29:
constexpr int kBufferSize = 960;  // Android works with a 20ms buffer size.
The original comment is "bigger than 20ms" (and the truth seems to be "not less
than"). 
Nit: the comment is confusing since there is not sample rate to be seen around,
so maybe make sample rate a constant here as well?

[miu, 2016-05-04 22:10:10]

Thanks again! Addressed comments on PS8 and fixed trybot failures (pepper code
was doing a weird thing with multiple inheritance).

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:24: Stop();
On 2016/05/04 08:49:23, o1ka wrote:
> Is there a test to check correctness of non-stopped track destruction?

Yes. Many of the tests in media_stream_audio_unittest.cc do not explicitly stop
the tracks, and so the track auto-stops when the blink WebMediaStreamTrack is
reset (in TearDown()).

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/media_stream_audio_unittest.cc (right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_unittest.cc:189:
ASSERT_EQ(expected_sample_count_, data[i]);
On 2016/05/04 08:49:24, o1ka wrote:
> So, from source to sink, you converted int64->float->int64 here. I'm not sure
if
> the check will pass for big int64 values, because float precision should drop
> after max_int32.

Good point. Actually, 2^24 is the largest exactly representable integer when
using 32-bit floats.

I added a check in the code for safety. Also, I lowered the kSampleRate for
these tests, and now it would take 2,097 seconds--well beyond the watchdog
timeout--to reach the danger point.

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_unittest.cc:230: class
MediaStreamAudioTest : public ::testing::Test {
On 2016/05/04 08:49:23, o1ka wrote:
> Very nice testing!

Thanks! :)

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_unittest.cc:361:
EXPECT_TRUE(expected_params.Equals(sink.params()));
On 2016/05/04 08:49:24, o1ka wrote:
> What are track()->GetOutputFormat() and sink.params() initially? Shouldn't we
> check in the beginning that they are no equal to expected_params, to make this
> unit test more robust?

That is done on line 350, where the sink's params are checked to make sure they
start out invalid.

We cannot do this for the track because the call (on line 347) to
ConnectToTrack() starts the source, which can set the track's audio format
immediately before the test code can examine the track. IMO, the fact that the
test later checks for an audio format change propagating to both track and sink
should be sufficient to confirm the plumbing here.

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/mock_audio_device_factory.cc (right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/mock_audio_device_factory.cc:27: NOTIMPLEMENTED();
On 2016/05/04 08:49:24, o1ka wrote:
> You can just mock them.

Done.

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc
(right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:29:
constexpr int kBufferSize = 960;  // Android works with a 20ms buffer size.
On 2016/05/04 08:49:24, o1ka wrote:
> The original comment is "bigger than 20ms" (and the truth seems to be "not
less
> than"). 
> Nit: the comment is confusing since there is not sample rate to be seen
around,
> so maybe make sample rate a constant here as well?

Done. Made the audio parameters more explicit with comments to help explain
requested vs. expected source format vs. expected output format. Also made this
more clear in other parts of this test code below (as I was resolving the
Anrdoid trybot failure).

[o1ka, 2016-05-06 16:53:57]

At this point I've done everything I could to review it - let's see what tommi@
has to say.

LGTM, looking forward to see how it works in real life :)

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/media_stream_audio_track.cc (right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_track.cc:24: Stop();
On 2016/05/04 22:10:09, miu wrote:
> On 2016/05/04 08:49:23, o1ka wrote:
> > Is there a test to check correctness of non-stopped track destruction?
> 
> Yes. Many of the tests in media_stream_audio_unittest.cc do not explicitly
stop
> the tracks, and so the track auto-stops when the blink WebMediaStreamTrack is
> reset (in TearDown()).

Acknowledged.

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/media_stream_audio_unittest.cc (right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_unittest.cc:189:
ASSERT_EQ(expected_sample_count_, data[i]);
On 2016/05/04 22:10:09, miu wrote:
> On 2016/05/04 08:49:24, o1ka wrote:
> > So, from source to sink, you converted int64->float->int64 here. I'm not
sure
> if
> > the check will pass for big int64 values, because float precision should
drop
> > after max_int32.
> 
> Good point. Actually, 2^24 is the largest exactly representable integer when
> using 32-bit floats.
> 
> I added a check in the code for safety. Also, I lowered the kSampleRate for
> these tests, and now it would take 2,097 seconds--well beyond the watchdog
> timeout--to reach the danger point.

Please put it into the comment for kSampleRate, and let's have a
comment/diagnostic output for those watchdog checks as well, so that it would be
easier to identify the problem if the watchdog starts firing.

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/media_stream_audio_unittest.cc:361:
EXPECT_TRUE(expected_params.Equals(sink.params()));
On 2016/05/04 22:10:09, miu wrote:
> On 2016/05/04 08:49:24, o1ka wrote:
> > What are track()->GetOutputFormat() and sink.params() initially? Shouldn't
we
> > check in the beginning that they are no equal to expected_params, to make
this
> > unit test more robust?
> 
> That is done on line 350, where the sink's params are checked to make sure
they
> start out invalid.
> 
> We cannot do this for the track because the call (on line 347) to
> ConnectToTrack() starts the source, which can set the track's audio format
> immediately before the test code can examine the track. IMO, the fact that the
> test later checks for an audio format change propagating to both track and
sink
> should be sufficient to confirm the plumbing here.

Acknowledged.

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc
(right):

https://codereview.chromium.org/1834323002/diff/160001/content/renderer/media...
content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:29:
constexpr int kBufferSize = 960;  // Android works with a 20ms buffer size.
On 2016/05/04 22:10:09, miu wrote:
> On 2016/05/04 08:49:24, o1ka wrote:
> > The original comment is "bigger than 20ms" (and the truth seems to be "not
> less
> > than"). 
> > Nit: the comment is confusing since there is not sample rate to be seen
> around,
> > so maybe make sample rate a constant here as well?
> 
> Done. Made the audio parameters more explicit with comments to help explain
> requested vs. expected source format vs. expected output format. Also made
this
> more clear in other parts of this test code below (as I was resolving the
> Anrdoid trybot failure).

Thanks!

[miu, 2016-05-09 20:27:12]

miu@chromium.org changed reviewers:
+ nick@chromium.org

[miu, 2016-05-09 20:27:14]

nick: PTAL at content/public/renderer/media_stream_utils.cc.

tommi: Just waiting on your LGTM. I hate to rush things, but two people (working
on different product features) have told me they are blocked waiting on this
change to land.

[ncarter (slow), 2016-05-09 20:29:36]

On 2016/05/09 20:27:14, miu wrote:
> nick: PTAL at content/public/renderer/media_stream_utils.cc.
> 
> tommi: Just waiting on your LGTM. I hate to rush things, but two people
(working
> on different product features) have told me they are blocked waiting on this
> change to land.

media_stream_utils.cc rubber stamp lgtm

[tommi (sloooow) - chröme, 2016-05-10 08:07:43]

lgtm

[tommi (sloooow) - chröme, 2016-05-10 08:08:53]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2016-05-10 08:08:53]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org
Link to the patchset: https://codereview.chromium.org/1834323002/#ps180001
(title: "Addressed o1ka's comments on PS8 and fixed trybot failures.")

[commit-bot: I haz the power, 2016-05-10 08:09:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1834323002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1834323002/180001

[commit-bot: I haz the power, 2016-05-10 08:11:53]

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

[commit-bot: I haz the power, 2016-05-10 08:11:55]

Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[miu, 2016-05-11 02:48:58]

Thanks everyone for this VERY COMPLEX review! :)

FYI--After tommi's lgtm, I rebased and during re-testing I found a minor race
condition where MediaStreamAudioProcessor could, in very rare cases, be
destroyed on a thread other than the main thread. I put a workaround in place
for now, and will address this in a follow-up change.

[miu, 2016-05-11 02:49:09]

The CQ bit was checked by miu@chromium.org

[miu, 2016-05-11 02:49:10]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org,
nick@chromium.org, tommi@chromium.org, olka@chromium.org
Link to the patchset: https://codereview.chromium.org/1834323002/#ps200001
(title: "REBASE + Workaround to ensure MediaStreamAudioProcessor is destroyed on
the main thread.")

[commit-bot: I haz the power, 2016-05-11 02:49:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1834323002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1834323002/200001

[commit-bot: I haz the power, 2016-05-11 04:04:27]

Description was changed from

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

Diagram of post-refactoring class relationships:
https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881,577874
==========

to

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

Diagram of post-refactoring class relationships:
https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881,577874
==========

[commit-bot: I haz the power, 2016-05-11 04:04:29]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-05-11 04:06:10]

Description was changed from

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

Diagram of post-refactoring class relationships:
https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881,577874
==========

to

==========
MediaStream audio: Refactor 3 separate "glue" implementations into one.

This is a refactoring of the MediaStreamAudioSource/Track object graph
such that life-cycle and audio flow control are unified into a single
architecture.  Previously, each of three implementations solved this
same problem in three different ways; and this has made it difficult to
maintain the code in src/content/renderer/media across all product
features, WebRTC or otherwise.

Diagram of post-refactoring class relationships:
https://docs.google.com/drawings/d/1yTsXvRMIyMlXjIEeQOVqXPxMPWgekB5ePWUVSI5-z...

The new architeture is as follows:

1. MediaStreamAudioSource becomes a base class implementation for
creating MediaStreamAudioTracks and delivering audio to them from the
source (i.e., an AudioInputDevice, a PeerConnection remote source, or a
WebAudio source).  All of its methods are now assumed to be run on the
main thread, while audio flow may optionally occur on a separate
thread.

2. MediaStreamAudioTrack becomes a base class implementation for
connecting/disconnecting MediaStreamAudioSinks, and delivering audio to
them from the source.

3. Both MediaStreamAudioSource and MediaStreamAudioTrack are owned by
their blink counterparts (WebMediaStreamSource and WebMediaStreamTrack),
and their destruction may safely occur any time the blink implementation
requires it.

Following the new architeture, the refactoring is:

1. WebRtcAudioCapturer becomes a sub-class of MediaStreamAudioSource and
is renamed to ProcessedLocalAudioSource.  This new class owns and
manages the WebRTC-specific audio processing on the source data.  Also:
a) Significant implementation from PeerConnectionDependencyFactory was
consolidated/moved to this class.  b) The "EnablePeerConnectionMode"
functionality, which re-started the audio input with a different buffer
size, has been removed to simplify the implementation.  Now, buffer size
is determined BEFORE the first time the audio input device is started.
c) Currently, all local audio sources (i.e., via AudioInputDevice) are
routed through this pipeline, but a soon-upcoming change will split the
WebRTC-specific cases from the ones that should not go through audio
processing.

2. MediaStreamRemoteAudioTrack becomes a sub-class of
MediaStreamAudioSource and is renamed to
PeerConnectionRemoteAudioSource.  This new class owns and manages the
flow of audio data from a PeerConnection into the MediaStream
framework.

3. WebAudioCapturer becomes a sub-class of MediaStreamAudioSource and is
renamed to WebAudioMediaStreamSource.  As a
blink::WebAudioDestinationConsumer, it manages the flow of audio from a
WebAudio destination node into the MediaStream framework.

BUG=577881,577874

Committed: https://crrev.com/e640bad76cad1155335d4068ecd46d250327bb9e
Cr-Commit-Position: refs/heads/master@{#392845}
==========

[commit-bot: I haz the power, 2016-05-11 04:06:12]

Patchset 10 (id:??) landed as
https://crrev.com/e640bad76cad1155335d4068ecd46d250327bb9e
Cr-Commit-Position: refs/heads/master@{#392845}

[tommycli, 2016-05-11 17:00:04]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/1966043006/ by tommycli@chromium.org.

The reason for reverting is: Unfortunate to revert such a mega patch, but I
think this broke:
https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Test...

First breaking build:
https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Test....

[miu, 2016-05-12 22:14:39]

FYI--This change was re-landed (with a fix for the locking behavior):
https://codereview.chromium.org/1977553002/

Confirmed that the TSAN bots that had failed on the original attempt are green
with the re-landed code. (And I had confirmed this on a local TSAN build/run
before commit.)