Split libjingle's signaling thread from the UI thread

Split libjingle's signaling thread from the UI thread.
This addresses the issue of getStats blocking the UI thread for excessive amounts of time and also opens the door for us to do other heavy-lifters such as setRemoteDescription and setLocalDescription asynchronously.

This CL builds on several others that have landed this week (see bug for details), but here are the key changes:

- PeerConnectionDependencyFactory has a separate signaling thread instead of using the UI thread.
- Classes that join WebRTC and Blink objects need to be aware of the signaling thread being different.  This means that callbacks will come in on the signaling thread from webrtc and need to be sent to the UI thread for javascript callbacks.

BUG=369796
Committed: https://crrev.com/ab51debd424645f6ab4a3281f14ebc403377464a
Cr-Commit-Position: refs/heads/master@{#302395}

[tommi (sloooow) - chröme, 2014-10-29 19:55:55]

Fix mac build error

[tommi (sloooow) - chröme, 2014-10-29 21:27:59]

Update dcheck for getdefaultcapturer

[tommi (sloooow) - chröme, 2014-10-29 21:48:14]

Rebase

[tommi (sloooow) - chröme, 2014-10-29 22:28:13]

more dcheck polish

[tommi (sloooow) - chröme, 2014-10-29 22:55:15]

Revert change to disable test now that libjingle has been rolled

[tommi (sloooow) - chröme, 2014-10-30 09:52:41]

Rebase after landing data channel change

[tommi (sloooow) - chröme, 2014-10-30 10:01:57]

tommi@chromium.org changed reviewers:
+ perkj@chromium.org, xians@chromium.org

[tommi (sloooow) - chröme, 2014-10-30 10:01:58]

Hey guys - here's the CL that introduces the signaling thread.  Here's a
proposal for how to split the review:

xians: *audio*
perkj: !*audio* :)

[no longer working on chromium, 2014-10-30 11:55:14]

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:328: if
(!MediaStreamAudioProcessor::IsAudioTrackProcessingEnabled()) {
just remove line 328-335
:) or you probably should consider letting me land the clean-up cl first, lots
of code here is removed
https://codereview.chromium.org/671793004/

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:28:
signaling_thread = pc_factory->GetWebRtcSignalingThread();
WebRtcLocalAudioTrackAdapter is created by PeerConnectionDependencyFactory
today, can you move the code to PeerConnectionDependencyFactory and inject the
signal thread to the adapter?

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:89: return
ret;
how about:
if (signaling_thread_.get() && !signaling_thread_->BelongsToCurrentThread()) {
    signaling_thread_->PostTask(FROM_HERE, ..
    return true;
}

return
webrtc::MediaStreamTrack<webrtc::AudioTrackInterface>::set_enabled(enable);

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.cc:148:
DCHECK(main_render_thread_checker_.CalledOnValidThread());
shouldn't RemoveSink be changed in the same way as AddSink?

[perkj_chrome, 2014-10-30 12:42:36]

Fist pass.

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:43: class
RemoteMediaStreamTrack : public MediaStreamTrack {
This is only used by Remote Audio tracks it looks like. Can you please rename it
as such since there is a LocalAudioTrack and put in a separate file. 

If the implementation had been complete, we should not have needed this special
case at all...

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:169: RemoteVideoTrackAdapter(
Add comment here to about  // Called on the signaling thread

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:188:
scoped_ptr<MediaStreamRemoteVideoSource> video_source(
thread check please. 
This is done on the main render thread right?

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:300:
CreateAdaptersForTracks(webrtc_stream_->GetAudioTracks(),
I would much prefer if this and the trackadapters was also created on the main
render thread. I  don't think there is anything taking time so it shouldn't be a
problem in that sence.

OnChanged would of course still be called by webrtc on the signaling thread. But
if you create two vectors with all tracks and post them to the main thread, that
can be used for comparison of what tracks have been created and which are
removed. 
This would be similar to what you did in
MediaStreamRemoveVideoSource::OnChanged.


...
With a bit of luck, the spec is changed so that tracks are added and removed
from pcs instead of complete MediaStreams.

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.h:47: void
InitializeOnWebKitThread(const std::string& label);
I am a bit confused by the naming here. 
I thought WebKitThread = main thread = render thread. You use |main_thread|
above.

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:222: if
(!main_thread_->BelongsToCurrentThread()) {
Why is this? To avoid having a separate method OnSuccessOnMainThread with a
thread check? 
Shouldn't this always be called from libjingles signaling thread?

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:267: if
(!main_thread_->BelongsToCurrentThread()) {
Same question as 222.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:308:
reports_.reserve(reports.size());
reports_ are used to two threads.
Please add thread check that this is called on the signaling thread.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:321: // TODO: response_
doesn't need to be a member.
please fix.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:458: class
RTCPeerConnectionHandler::Observer
Please add a comment about the purpose of this class.
Ie, move PC callbacks from the sigaling thread to the main render thread.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:470: // TODO(perkj):
Implement.
Or rater remove from the pc interface.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:488: new
RemoteMediaStreamImpl(main_thread_, stream));
So this is instantiating |remote_stream| on the signaling thread? 
Wouldnt it be much simpler to instantiate it on the main render thread instead.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:823: // TODO(tommi): This
cannot be posted to the signaling thread unfortunately
Can we talk about this?  Is this only due to our tests are wrong or that the
spec actually say otherwise? This is async in the spec so it sounds like this
should also be async.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:824: // because the caller
depends on the state being changed synchronously.
We should at least but a similar explanation with setLocalDescription.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:837:
native_peer_connection_->local_description();
Are these methods the problem?

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1006: // TODO:
native_peer_connection_ will introduce a thread hop. Do this
What is the status of this?

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:131:
MediaStreamRemoteVideoSource::Observer::Observer(
I think we should try to create this object on the main render thread and only
have Observer::OnChanged() called on the signaling thread.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:172:
DLOG_IF(ERROR, !source_) << "Dropping a state change event. " << state;
Can this happen before |source_| have been set?

[tommi (sloooow) - chröme, 2014-10-30 17:25:30]

Address comments

[tommi (sloooow) - chröme, 2014-10-30 20:33:49]

Fix missing reference and fix incorrect documentation

[tommi (sloooow) - chröme, 2014-10-30 20:37:37]

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:43: class
RemoteMediaStreamTrack : public MediaStreamTrack {
On 2014/10/30 12:42:35, perkj wrote:
> This is only used by Remote Audio tracks it looks like. Can you please rename
it
> as such since there is a LocalAudioTrack and put in a separate file. 
> 
> If the implementation had been complete, we should not have needed this
special
> case at all...

Renamed to RemoteMediaStreamAudioTrack.
Are you OK with a TODO on adding a new file for this class?

Here's what I did for MediaStreamTrack - piranha plant worthy changes I think:
- made SetEnabled(), Stop() and GetAudioAdapter pure virtual
- removed the track_ variable
- changed the ctor and call sites to remove the track param.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:169: RemoteVideoTrackAdapter(
On 2014/10/30 12:42:35, perkj wrote:
> Add comment here to about  // Called on the signaling thread

Done.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:188:
scoped_ptr<MediaStreamRemoteVideoSource> video_source(
On 2014/10/30 12:42:35, perkj wrote:
> thread check please. 
> This is done on the main render thread right?

Done. Yes, this is on the main render thread (we call InitializeWebkitTrack()
here which has this dcheck already).

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:300:
CreateAdaptersForTracks(webrtc_stream_->GetAudioTracks(),
On 2014/10/30 12:42:35, perkj wrote:
> I would much prefer if this and the trackadapters was also created on the main
> render thread. I  don't think there is anything taking time so it shouldn't be
a
> problem in that sence.
> 
> OnChanged would of course still be called by webrtc on the signaling thread.
But
> if you create two vectors with all tracks and post them to the main thread,
that
> can be used for comparison of what tracks have been created and which are
> removed. 
> This would be similar to what you did in
> MediaStreamRemoveVideoSource::OnChanged.
> 
> 
> ...
> With a bit of luck, the spec is changed so that tracks are added and removed
> from pcs instead of complete MediaStreams.

I think we discussed this offline and agreed to stick with this approach so that
the event handlers/observers for webrtc objects are created on the signaling
thread, which is the same thread that they will receive callbacks on.  Let me
know if you think there's an action I should take.

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.h:47: void
InitializeOnWebKitThread(const std::string& label);
On 2014/10/30 12:42:35, perkj wrote:
> I am a bit confused by the naming here. 
> I thought WebKitThread = main thread = render thread. You use |main_thread|
> above.

Changed to InitializeOnMainThread()

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:222: if
(!main_thread_->BelongsToCurrentThread()) {
On 2014/10/30 12:42:35, perkj wrote:
> Why is this? To avoid having a separate method OnSuccessOnMainThread with a
> thread check? 
> Shouldn't this always be called from libjingles signaling thread?

Yes, this is first always called on the signaling thread. It's just for
convenience to keep the implementation in one place.  In places where we fetch
webrtc state on the signaling thread for optimizations sake, that's not
possible.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:267: if
(!main_thread_->BelongsToCurrentThread()) {
On 2014/10/30 12:42:35, perkj wrote:
> Same question as 222.

Let me know if you prefer to split these up into separate methods

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:308:
reports_.reserve(reports.size());
On 2014/10/30 12:42:35, perkj wrote:
> reports_ are used to two threads.
ah, right, there's no need for that (meant to do this actually so thanks for the
reminder). fixed.

> Please add thread check that this is called on the signaling thread.

done.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:321: // TODO: response_
doesn't need to be a member.
On 2014/10/30 12:42:35, perkj wrote:
> please fix.

Done.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:458: class
RTCPeerConnectionHandler::Observer
On 2014/10/30 12:42:35, perkj wrote:
> Please add a comment about the purpose of this class.
> Ie, move PC callbacks from the sigaling thread to the main render thread.

Done.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:470: // TODO(perkj):
Implement.
On 2014/10/30 12:42:35, perkj wrote:
> Or rater remove from the pc interface.

Done.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:488: new
RemoteMediaStreamImpl(main_thread_, stream));
On 2014/10/30 12:42:35, perkj wrote:
> So this is instantiating |remote_stream| on the signaling thread? 
> Wouldnt it be much simpler to instantiate it on the main render thread
instead. 

Discussed offline

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:823: // TODO(tommi): This
cannot be posted to the signaling thread unfortunately
On 2014/10/30 12:42:35, perkj wrote:
> Can we talk about this?  Is this only due to our tests are wrong or that the
> spec actually say otherwise? This is async in the spec so it sounds like this
> should also be async.

Added a TODO to do this in a following cl as discussed.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:824: // because the caller
depends on the state being changed synchronously.
On 2014/10/30 12:42:35, perkj wrote:
> We should at least but a similar explanation with setLocalDescription.

Done.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:837:
native_peer_connection_->local_description();
On 2014/10/30 12:42:35, perkj wrote:
> Are these methods the problem? 

Yes - or rather the html test page had a bug which I've now fixed.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1006: // TODO:
native_peer_connection_ will introduce a thread hop. Do this
On 2014/10/30 12:42:35, perkj wrote:
> What is the status of this?

Move all of this over to the signaling thread as discussed.
Also added TODOs for other methods such as setRemote and Local description to
move them to the signaling thread.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:131:
MediaStreamRemoteVideoSource::Observer::Observer(
On 2014/10/30 12:42:36, perkj wrote:
> I think we should try to create this object on the main render thread and only
> have Observer::OnChanged() called on the signaling thread.
>  

Discussed offline

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:172:
DLOG_IF(ERROR, !source_) << "Dropping a state change event. " << state;
On 2014/10/30 12:42:35, perkj wrote:
> Can this happen before |source_| have been set? 

Only if a unittest is doing something funky.  Changed it to be a DCHECK.

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:328: if
(!MediaStreamAudioProcessor::IsAudioTrackProcessingEnabled()) {
On 2014/10/30 11:55:14, xians1 wrote:
> just remove line 328-335
> :) or you probably should consider letting me land the clean-up cl first, lots
> of code here is removed
> https://codereview.chromium.org/671793004/

Good point.  Looked at your cl and lgtm-ed it.

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:28:
signaling_thread = pc_factory->GetWebRtcSignalingThread();
On 2014/10/30 11:55:14, xians1 wrote:
> WebRtcLocalAudioTrackAdapter is created by PeerConnectionDependencyFactory
> today, can you move the code to PeerConnectionDependencyFactory and inject the
> signal thread to the adapter?

Yes, I plan to do that and added a TODO to do that.  There currently are so many
unrelated call sites to this method though that I wanted to do that in a
separate cl.

See:
https://code.google.com/p/chromium/codesearch#search/&q=WebRtcLocalAudioTrack...

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:89: return
ret;
On 2014/10/30 11:55:14, xians1 wrote:
> how about:
> if (signaling_thread_.get() && !signaling_thread_->BelongsToCurrentThread()) {
>     signaling_thread_->PostTask(FROM_HERE, ..
>     return true;
> }
> 
> return
> webrtc::MediaStreamTrack<webrtc::AudioTrackInterface>::set_enabled(enable);

Done.

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_track.cc:148:
DCHECK(main_render_thread_checker_.CalledOnValidThread());
On 2014/10/30 11:55:14, xians1 wrote:
> shouldn't RemoveSink be changed in the same way as AddSink?

Absolutely

[tommi (sloooow) - chröme, 2014-10-30 20:45:18]

Fix android build error

[tommi (sloooow) - chröme, 2014-10-30 21:09:52]

Fix another build error (why don't I get this on windows!)

[tommi (sloooow) - chröme, 2014-10-30 22:01:08]

rebase

[tommi (sloooow) - chröme, 2014-10-30 23:10:03]

Make setLocalDescription and setRemoteDescription run on the signaling thread

[tommi (sloooow) - chröme, 2014-10-30 23:23:25]

Update test

[tommi (sloooow) - chröme, 2014-10-31 06:34:16]

Hey guys - please ignore patch sets 13 and 14. I was playing around with making
setLocalDescription and setRemoteDescription async as well but more work needs
to be done there, so I'm going to move that work to a separate cl.

[perkj_chrome, 2014-10-31 08:37:25]

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

https://codereview.chromium.org/675013005/diff/120001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:267: if
(!main_thread_->BelongsToCurrentThread()) {
On 2014/10/30 20:37:36, tommi wrote:
> On 2014/10/30 12:42:35, perkj wrote:
> > Same question as 222.
> 
> Let me know if you prefer to split these up into separate methods
I personally think this is fine at least on this type of wrappers. But I got
review feedback earlier that preferered separate methods. If you split this you
should split all and that means a lot of new methods....

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/media_stream_track.cc (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/media_stream_track.cc:28: webrtc::AudioTrackInterface*
MediaStreamTrack::GetAudioAdapter() {
Can this be pure virtual? I saw you todo in .h so just ignore if you can't.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/media_stream_track.h:36: // TODO(xians): Make this pure
virtual when Stop[Track] has been
nit: remove this todo or move it to the new implementation.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/peer_connection_tracker.cc (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/peer_connection_tracker.cc:314: it->first->GetStats(
oh, this is the reason there is a getStats and GetStats in PCHandler....
ok then....

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:47: const
scoped_refptr<webrtc::MediaStreamTrackInterface>& track,
This can be webrtc::AudioTrackInterface instead of
webrtc::MediaStreamTrackInterface.
You can then skip the static_cast.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:62: // we get a direct
pointer to the object.
I don't think this is true for remote audio tracks: Can you please verify again.


https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

It uses the same templates as for remote video. 
(Not that video uses it any longer since we see a remote video track as a
source)

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:76:
webrtc::AudioTrackInterface* GetAudioAdapter() override {
This method is really weird. 
I see its being used only when creating a local audio track and when a
DTMFSender is being created. But I expect DTMFSender only works for local audio
tracks anyway? Shijing, do you know? 

Is there a way to control the volume of individual remote tracks? What interface
does that use?

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:96: class
RemoteMediaStreamAudioTrackAdapter
This looks like a search and replace error. You did not meant to rename this one
right?

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:169: : public
RemoteMediaStreamAudioTrackAdapter<webrtc::VideoTrackInterface> {
Weird name of baseclass...

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:349: // TODO(tommi):
Unretained safe?
We must figure that out.
OnChanged here is called as a result of setRemoteDescription.
Changes to a remote mediastream happen before pc observer :: OnRemoveStream
which is what normally cause ~RemoteMediaStreamImpl.

The other reason ~RemoteMediaStreamImpl is when the PeerConnectionHandler is
destroyed. I (think/hope) that can not happen during processing of a remote
description. If it does, we are screwed anyway....

So yes, I think this is safe. But maybe not for a "code readable" reason. 
Is it hard to reuse the observer pattern you have used elsewhere here to? ie,
let the observer be implemented as a refcounted delegate.

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

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:308:
scoped_ptr<std::vector<StatsReportCopyable>> report_copies(
This could also easily be avoided if this was rtc::scoped_ptr<const StatsReport>
reports)
It looks like a substantial amount of string copies.

Add a todo and maybe use the same bug as you filed for datachannels?


https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1037: void
RTCPeerConnectionHandler::GetStats(
nit: Can this method be merged into getStats instead for readability? getStats
and GetStats...

[tommi (sloooow) - chröme, 2014-10-31 10:06:33]

Changes on the way

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/media_stream_track.cc (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/media_stream_track.cc:28: webrtc::AudioTrackInterface*
MediaStreamTrack::GetAudioAdapter() {
On 2014/10/31 08:37:24, perkj wrote:
> Can this be pure virtual? I saw you todo in .h so just ignore if you can't.

Shijing has pending cleanup work for this method so I think he'll take care of
it.  There are other implementations that will need to implement this if we make
it pure virtual. As is the default implementation should never be used.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/media_stream_track.h:36: // TODO(xians): Make this pure
virtual when Stop[Track] has been
On 2014/10/31 08:37:24, perkj wrote:
> nit: remove this todo or move it to the new implementation.

Done.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/peer_connection_tracker.cc (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/peer_connection_tracker.cc:314: it->first->GetStats(
On 2014/10/31 08:37:24, perkj wrote:
> oh, this is the reason there is a getStats and GetStats in PCHandler....
> ok then....

Yup, I tried to remove it but yeah

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:47: const
scoped_refptr<webrtc::MediaStreamTrackInterface>& track,
On 2014/10/31 08:37:25, perkj wrote:
> This can be webrtc::AudioTrackInterface instead of
> webrtc::MediaStreamTrackInterface.
> You can then skip the static_cast.

Done.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:62: // we get a direct
pointer to the object.
On 2014/10/31 08:37:24, perkj wrote:
> I don't think this is true for remote audio tracks: Can you please verify
again.
> 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
> It uses the same templates as for remote video. 
> (Not that video uses it any longer since we see a remote video track as a
> source)

Yes, you're right. The code is bit by bit becoming less crazy!

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:76:
webrtc::AudioTrackInterface* GetAudioAdapter() override {
On 2014/10/31 08:37:24, perkj wrote:
> This method is really weird. 
> I see its being used only when creating a local audio track and when a
> DTMFSender is being created. But I expect DTMFSender only works for local
audio
> tracks anyway? Shijing, do you know? 

Yes, DTMFSender only works for local audio tracks.
> 
> Is there a way to control the volume of individual remote tracks? What
interface
> does that use? 

Yes there is and it's an interesting hack :)  Shijing can explain.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:96: class
RemoteMediaStreamAudioTrackAdapter
On 2014/10/31 08:37:25, perkj wrote:
> This looks like a search and replace error. You did not meant to rename this
one
> right?

Doh, thanks.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:169: : public
RemoteMediaStreamAudioTrackAdapter<webrtc::VideoTrackInterface> {
On 2014/10/31 08:37:24, perkj wrote:
> Weird name of baseclass... 

Acknowledged.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:349: // TODO(tommi):
Unretained safe?
On 2014/10/31 08:37:24, perkj wrote:
> We must figure that out.
> OnChanged here is called as a result of setRemoteDescription.
> Changes to a remote mediastream happen before pc observer :: OnRemoveStream
> which is what normally cause ~RemoteMediaStreamImpl.
> 
> The other reason ~RemoteMediaStreamImpl is when the PeerConnectionHandler is
> destroyed. I (think/hope) that can not happen during processing of a remote
> description. If it does, we are screwed anyway....
> 
> So yes, I think this is safe. But maybe not for a "code readable" reason. 
> Is it hard to reuse the observer pattern you have used elsewhere here to? ie,
> let the observer be implemented as a refcounted delegate.

I'll implement an observer pattern like I did elsewhere. This todo was meant to
be a reminder to myself so thanks for catching it.

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

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:308:
scoped_ptr<std::vector<StatsReportCopyable>> report_copies(
On 2014/10/31 08:37:25, perkj wrote:
> This could also easily be avoided if this was rtc::scoped_ptr<const
StatsReport>
> reports)
> It looks like a substantial amount of string copies.
> 
> Add a todo and maybe use the same bug as you filed for datachannels?
> 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

It is, but it's considerably less than what we have been doing! :) (I've
actually reduced thousands of string copies on the libjingle side)

[tommi (sloooow) - chröme, 2014-10-31 10:08:59]

Broken changes for review

[perkj_chrome, 2014-10-31 10:24:26]

LGTM with the comments below considered.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:76:
webrtc::AudioTrackInterface* GetAudioAdapter() override {
On 2014/10/31 10:06:33, tommi wrote:
> On 2014/10/31 08:37:24, perkj wrote:
> > This method is really weird. 
> > I see its being used only when creating a local audio track and when a
> > DTMFSender is being created. But I expect DTMFSender only works for local
> audio
> > tracks anyway? Shijing, do you know? 
> 
> Yes, DTMFSender only works for local audio tracks.
> > 
> > Is there a way to control the volume of individual remote tracks? What
> interface
> > does that use? 
> 
> Yes there is and it's an interesting hack :)  Shijing can explain.
My point is, if this should never be used on remote tracks- shouldn't it be
removed? And this checked if a user try to create a DTFMSender on a remote
track?

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

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:308:
scoped_ptr<std::vector<StatsReportCopyable>> report_copies(
On 2014/10/31 10:06:33, tommi wrote:
> On 2014/10/31 08:37:25, perkj wrote:
> > This could also easily be avoided if this was rtc::scoped_ptr<const
> StatsReport>
> > reports)
> > It looks like a substantial amount of string copies.
> > 
> > Add a todo and maybe use the same bug as you filed for datachannels?
> > 
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
> It is, but it's considerably less than what we have been doing! :) (I've
> actually reduced thousands of string copies on the libjingle side)

Put a scoped_ptr still make sense right if you look at how |reports| are
generated currently? Please add a todo.

https://codereview.chromium.org/675013005/diff/270001/content/renderer/media/...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/675013005/diff/270001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:298:
webrtc_stream_->UnregisterObserver(this);
DCHECK thread ?

https://codereview.chromium.org/675013005/diff/270001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:318: if (media_stream_)
thread check?

[tommi (sloooow) - chröme, 2014-10-31 12:30:04]

Finish observer impl

[no longer working on chromium, 2014-10-31 13:02:11]

On 2014/10/31 12:30:04, tommi wrote:
> Finish observer impl

rubber stamp lgtm

[tommi (sloooow) - chröme, 2014-10-31 13:20:42]

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:76:
webrtc::AudioTrackInterface* GetAudioAdapter() override {
On 2014/10/31 10:24:26, perkj wrote:
> On 2014/10/31 10:06:33, tommi wrote:
> > On 2014/10/31 08:37:24, perkj wrote:
> > > This method is really weird. 
> > > I see its being used only when creating a local audio track and when a
> > > DTMFSender is being created. But I expect DTMFSender only works for local
> > audio
> > > tracks anyway? Shijing, do you know? 
> > 
> > Yes, DTMFSender only works for local audio tracks.
> > > 
> > > Is there a way to control the volume of individual remote tracks? What
> > interface
> > > does that use? 
> > 
> > Yes there is and it's an interesting hack :)  Shijing can explain.
> My point is, if this should never be used on remote tracks- shouldn't it be
> removed? And this checked if a user try to create a DTFMSender on a remote
> track?

Yes, you're right (and reading my comment I guess I knew about this).  Method
removed.

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

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:308:
scoped_ptr<std::vector<StatsReportCopyable>> report_copies(
On 2014/10/31 10:24:26, perkj wrote:
> On 2014/10/31 10:06:33, tommi wrote:
> > On 2014/10/31 08:37:25, perkj wrote:
> > > This could also easily be avoided if this was rtc::scoped_ptr<const
> > StatsReport>
> > > reports)
> > > It looks like a substantial amount of string copies.
> > > 
> > > Add a todo and maybe use the same bug as you filed for datachannels?
> > > 
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> > 
> > It is, but it's considerably less than what we have been doing! :) (I've
> > actually reduced thousands of string copies on the libjingle side)
> 
> Put a scoped_ptr still make sense right if you look at how |reports| are
> generated currently? Please add a todo.

Done.

https://codereview.chromium.org/675013005/diff/180002/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1037: void
RTCPeerConnectionHandler::GetStats(
On 2014/10/31 08:37:25, perkj wrote:
> nit: Can this method be merged into getStats instead for readability? getStats
> and GetStats... 

It can but currently the pc tracker depends on it. I'll add a todo.

https://codereview.chromium.org/675013005/diff/270001/content/renderer/media/...
File content/renderer/media/remote_media_stream_impl.cc (right):

https://codereview.chromium.org/675013005/diff/270001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:298:
webrtc_stream_->UnregisterObserver(this);
On 2014/10/31 10:24:26, perkj wrote:
> DCHECK thread ? 

It can be either the signaling thread or main thread.  I'll add a comment.

https://codereview.chromium.org/675013005/diff/270001/content/renderer/media/...
content/renderer/media/remote_media_stream_impl.cc:318: if (media_stream_)
On 2014/10/31 10:24:26, perkj wrote:
> thread check?

Done.

[tommi (sloooow) - chröme, 2014-10-31 13:20:49]

Address comments

[tommi (sloooow) - chröme, 2014-10-31 13:28:43]

Remove async set{Local|Remote}Description

[tommi (sloooow) - chröme, 2014-10-31 13:40:30]

Fix build

[tommi (sloooow) - chröme, 2014-10-31 14:31:18]

Restore original order of operations

[tommi (sloooow) - chröme, 2014-10-31 15:04:50]

Revert GetPcFactory call

[tommi (sloooow) - chröme, 2014-10-31 16:02:26]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-10-31 16:03:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/675013005/390001

[commit-bot: I haz the power, 2014-10-31 17:27:48]

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

[commit-bot: I haz the power, 2014-10-31 17:27:50]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommi (sloooow) - chröme, 2014-11-01 19:52:03]

Update tests, remove circular dependency

[tommi (sloooow) - chröme, 2014-11-01 20:52:07]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-11-01 20:52:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/675013005/400001

[commit-bot: I haz the power, 2014-11-01 21:34:09]

Committed patchset #22 (id:400001)

[commit-bot: I haz the power, 2014-11-01 21:34:54]

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