Adding AddTrack/RemoveTrack to native PeerConnection API.

Adding AddTrack/RemoveTrack to native PeerConnection API.

Also, now creating the RtpSender/RtpReceiver proxy objects immediately,
rather than waiting until when GetSenders/GetReceivers is called.

Committed: https://crrev.com/e1f9d837aea916e1131898ecbb6f621d91946671
Cr-Commit-Position: refs/heads/master@{#11259}

[Taylor Brandstetter, 2016-01-09 00:53:56]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2016-01-09 00:53:56]

Besides adding the AddTrack/RemoveTrack implementations, you'll also see I'm
creating the proxy objects earlier and storing them (instead of the raw objects)
in PeerConnection. This makes RemoveTrack simpler (since it can just compare
pointer values), and doesn't have any real disadvantage I can think of.

[pthatcher1, 2016-01-11 17:31:51]

https://codereview.webrtc.org/1563403002/diff/60001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1563403002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:796: std::vector<MediaStreamInterface*>
streams) {
Would it make sense to only pass in a std::vector<std::string> with just stream
IDs?  I don't see any reason to take in streams.

Also, should we just return an error/null if 2 or more are passed in?  That's
probably better than silently breaking.

https://codereview.webrtc.org/1563403002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:803: return nullptr;
Will this be supported once we switch to Unified Plan?  It's a silly thing to
do, but I don't think it's disallowed by the standard.  Thus, should we have a
TODO?

https://codereview.webrtc.org/1563403002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:812: session_.get(), stats_.get()));
So later one when we want to do something with an RtpSender within
PeerConnection, do we have to go through the RtpSenderProxy also?  Or will we
store both proxied and non-proxied?

https://codereview.webrtc.org/1563403002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:856: }
We can RemoveTrack even if IsClosed()?

https://codereview.webrtc.org/1563403002/diff/60001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1563403002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:1023: // Test for AddTrack
and RemoveTrack methods.
***

[Taylor Brandstetter, 2016-01-13 00:53:01]

https://codereview.chromium.org/1563403002/diff/60001/talk/app/webrtc/peercon...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.chromium.org/1563403002/diff/60001/talk/app/webrtc/peercon...
talk/app/webrtc/peerconnection.cc:796: std::vector<MediaStreamInterface*>
streams) {
On 2016/01/11 17:31:51, pthatcher1 wrote:
> Would it make sense to only pass in a std::vector<std::string> with just
stream
> IDs?  I don't see any reason to take in streams.
> 
> Also, should we just return an error/null if 2 or more are passed in?  That's
> probably better than silently breaking.

The reason for it to take a stream is that I want this API to match the W3C API
as closely as possible. Maybe right *now* the only thing from the stream used is
its label, but if that changes in the future, I don't want to have to refactor
things all the way up to Chromium to change the type of the argument.

I do agree about returning an error if 2 are passed in, though. That would match
what we do about other things that are in the spec but not supported by us.

https://codereview.chromium.org/1563403002/diff/60001/talk/app/webrtc/peercon...
talk/app/webrtc/peerconnection.cc:803: return nullptr;
On 2016/01/11 17:31:51, pthatcher1 wrote:
> Will this be supported once we switch to Unified Plan?  It's a silly thing to
> do, but I don't think it's disallowed by the standard.  Thus, should we have a
> TODO?

I guess I just assumed the spec didn't allow it. I'll add a TODO.

https://codereview.chromium.org/1563403002/diff/60001/talk/app/webrtc/peercon...
talk/app/webrtc/peerconnection.cc:812: session_.get(), stats_.get()));
On 2016/01/11 17:31:51, pthatcher1 wrote:
> So later one when we want to do something with an RtpSender within
> PeerConnection, do we have to go through the RtpSenderProxy also?  Or will we
> store both proxied and non-proxied?

We would use the proxy object. If a method is called while already on the
signaling thread, the proxy will just pass through to the real object, so there
isn't much disadvantage. Sure, it requires a thread check and adds one method to
the call stack, but I think going to extra effort to avoid those things would
qualify as "premature optimization".

https://codereview.chromium.org/1563403002/diff/60001/talk/app/webrtc/peercon...
talk/app/webrtc/peerconnection.cc:856: }
On 2016/01/11 17:31:51, pthatcher1 wrote:
> We can RemoveTrack even if IsClosed()?

I was just matching RemoveStream, but it looks like the spec explicitly
disallows this. I'll change it.

[pthatcher1, 2016-01-13 16:28:42]

lgtm

[Taylor Brandstetter, 2016-01-14 01:18:44]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-01-14 01:18:53]

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

[Taylor Brandstetter, 2016-01-14 01:19:22]

The CQ bit was unchecked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-01-14 18:12:45]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-01-14 18:12:45]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.chromium.org/1563403002/#ps100001
(title: "Adding default implementation for new methods.")

[commit-bot: I haz the power, 2016-01-14 18:12:49]

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

[commit-bot: I haz the power, 2016-01-14 20:12:55]

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

[commit-bot: I haz the power, 2016-01-14 20:12:55]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Taylor Brandstetter, 2016-01-14 23:31:54]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-01-14 23:32:04]

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

[commit-bot: I haz the power, 2016-01-14 23:35:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-14 23:35:49]

Description was changed from

==========
Adding AddTrack/RemoveTrack to native PeerConnection API.

Also, now creating the RtpSender/RtpReceiver proxy objects immediately,
rather than waiting until when GetSenders/GetReceivers is called.
==========

to

==========
Adding AddTrack/RemoveTrack to native PeerConnection API.

Also, now creating the RtpSender/RtpReceiver proxy objects immediately,
rather than waiting until when GetSenders/GetReceivers is called.

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

[commit-bot: I haz the power, 2016-01-14 23:35:50]

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