Exposing RtpSenders and RtpReceivers from PeerConnection.

Exposing RtpSenders and RtpReceivers from PeerConnection.

This CL essentially converts [Local|Remote]TrackHandler to
Rtp[Sender|Receiver], and adds a "SetTrack" method for RtpSender.

It also gets rid of MediaStreamHandler and MediaStreamHandlerContainer,
since these classes weren't really anything more than containers.
PeerConnection now manages the RtpSenders and RtpReceivers directly.

Committed: https://crrev.com/70ab1a1ca89d280a7d51e3fadc51d4be9df209ca
Cr-Commit-Position: refs/heads/master@{#10100}

[Taylor Brandstetter, 2015-09-17 01:42:35]

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

[Taylor Brandstetter, 2015-09-17 01:42:35]

Hey Peter; I haven't cleaned this up AT ALL yet, but if you wanted more of a
sneak peek, here you go.

Also, ignore the files it thinks are renamed. I guess it's doing that because
the copyright notice takes up 50% of the file; pretty comical.

[pthatcher1, 2015-09-17 04:25:47]

This is looking really good already.

Mostly, I think we just need to remove mid() and the necessary stuff to pass
that around and add an id() instead that could be MID (for UnifiedPlan) or MSID
(for Plan B) or neither (for ORTC).

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/mediastreamsi...
File talk/app/webrtc/mediastreamsignaling.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/mediastreamsi...
talk/app/webrtc/mediastreamsignaling.h:70: const std::string& mid) = 0;
I don't see how MID is even useful here.  I think we should just remove it from
RtpSenders and RtpReceivers for now.  

Here's an idea:  instead of getting the (stream, track, ssrc, mid) from the SDP,
why not instead get (sender_id, ssrc, track), where the sender_id is MSID for
"Plan B" SDP and MID for Unified Plan later?   

We should also add a TODO to change all calls through BaseChannel/MediaChannel
to use "sender_id" instead of SSRC.   Then it would just be (sender_id, track).

We should also add a TODO to change addTrack to put the track on the RtpSender
before the SDP comes in.  Then this callback could just be (sender_id).

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:850: rtp_receivers_.erase(it);
Long-term, I think we want to keep senders and receivers in here (perhaps as
Transceivers), because we'll want to use them track m-lines, and m-lines can't
go away

Just having a TODO might be sufficient for now.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:969: -> bool { return sender->track() ==
track; });
Is the "-> bool" necessary?  I thought it could infer that.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectioninterface.h:327: typedef
std::vector<rtc::scoped_refptr<RtpSenderInterface>> RtpSenders;
I think we should call it something like RefcountedRtpSenders to make it clear
they are refcounted. 

Also, we can pull it out of into the main namespace, I think.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.cc
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.c...
talk/app/webrtc/rtpreceiver.cc:88: provider_->SetVideoPlayout(ssrc_, true,
track_->GetSource()->FrameInput());
Can you put a TODO for unifying SetVideoPlayout and SetVideoSend?  It doesn't
make sense to me that we have two different methods to call.  And, why do we
assume track_->enabled() here?

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.c...
talk/app/webrtc/rtpreceiver.cc:110: if (track_->enabled() && source) {
Wait... why do we track the previous track_->enabled() state for audio but not
for video?  Can you put a TODO for making it consistent between the two?

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h
File talk/app/webrtc/rtpreceiver.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:64: std::string mid() const override { return
mid_; }
I think we should either remove this or just call it "id" so that it can work
well between Plan B (MSID), Unified Plan (MID), and ORTC (an ID unto itself).

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:67: void Stop() override;
Does this also shutdown the media, or does setLocal/setRemoteDescription do
that?  We should probably make it clear in the comment one way or the other.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:73: uint32 ssrc_;
Can you put a TODO to change the key we use for passing into
BaseChannel/MediaChannel to a "sender_id" string?

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:75: AudioProviderInterface* provider_;
Can you comment that this is just a crazy way to get to the BaseChannel, and
leave a TODO for refactoring that?

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:76: bool enabled_;
Can you rename this to track_enabled_?  That would make it much more clear,
especially if you add a comment like "stored separately from track_ because we
need to know whether it changes or not."

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:79: class VideoRtpReceiver : public
RtpReceiverInterface {
All the comment I left for audio apply to video (except the track_enabled_).

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpsender.h
File talk/app/webrtc/rtpsender.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpsender.h#n...
talk/app/webrtc/rtpsender.h:111: VideoProviderInterface* provider);
Same here with the order like with the receivers.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpsenderinte...
File talk/app/webrtc/rtpsenderinterface.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpsenderinte...
talk/app/webrtc/rtpsenderinterface.h:46: virtual std::string mid() const = 0;
I think there is a whole lot of code added to pass around the MID, but I don't
think we actually need it on the object.  I'd say we just remove it all if we
can.

[Taylor Brandstetter, 2015-09-23 00:10:45]

Note that this isn't quite complete; I still need to have SetTrack return true
or false to indicate success.

And... We probably want to have someone else review the Java side. It ended up
being somewhat funky. Since it's a C++ PeerConnectionObserver subclass that
creates the Java MediaStreamTracks, the Java PeerConnection doesn't have access
to them to attach to RtpReceivers. So I just have RtpSender/Receivers creating
their own Java MediaStreamTracks, and in the end they get disposed
independently. It's ugly, but at least in the end there aren't any dangling
references to tracks.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/mediastreamsi...
File talk/app/webrtc/mediastreamsignaling.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/mediastreamsi...
talk/app/webrtc/mediastreamsignaling.h:70: const std::string& mid) = 0;
On 2015/09/17 04:25:46, pthatcher1 wrote:
> I don't see how MID is even useful here.  I think we should just remove it
from
> RtpSenders and RtpReceivers for now.  
> 
> Here's an idea:  instead of getting the (stream, track, ssrc, mid) from the
SDP,
> why not instead get (sender_id, ssrc, track), where the sender_id is MSID for
> "Plan B" SDP and MID for Unified Plan later?   
> 
> We should also add a TODO to change all calls through BaseChannel/MediaChannel
> to use "sender_id" instead of SSRC.   Then it would just be (sender_id,
track).
> 
> We should also add a TODO to change addTrack to put the track on the RtpSender
> before the SDP comes in.  Then this callback could just be (sender_id).

It's not useful right now. It's just something I knew we'd need eventually, so I
thought I'd go ahead and add it. But if it's just going to be clutter, I'll go
ahead and remove it.

Also, I'll do the "sender_id" change in the next CL, since it's more relevant
there.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:850: rtp_receivers_.erase(it);
On 2015/09/17 04:25:46, pthatcher1 wrote:
> Long-term, I think we want to keep senders and receivers in here (perhaps as
> Transceivers), because we'll want to use them track m-lines, and m-lines can't
> go away
> 
> Just having a TODO might be sufficient for now.

Can't an m-line go away if someone calls "stop" on the Transceiver? Or if it's
send-only and the local track is removed? Anyway, for now I'll add a TODO.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:969: -> bool { return sender->track() ==
track; });
On 2015/09/17 04:25:46, pthatcher1 wrote:
> Is the "-> bool" necessary?  I thought it could infer that.

Oh, I guess so. Removed.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectioninterface.h:327: typedef
std::vector<rtc::scoped_refptr<RtpSenderInterface>> RtpSenders;
On 2015/09/17 04:25:46, pthatcher1 wrote:
> I think we should call it something like RefcountedRtpSenders to make it clear
> they are refcounted. 
> 
> Also, we can pull it out of into the main namespace, I think.

I assume you mean "out of PeerConnectionInterface"? Also, how about
"RtpSenderRefptrs" for extra clarity? "RefcountedRtpSenders" seems a little
redundant since RtpSenderInterface is inherently ref-counted.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.cc
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.c...
talk/app/webrtc/rtpreceiver.cc:88: provider_->SetVideoPlayout(ssrc_, true,
track_->GetSource()->FrameInput());
On 2015/09/17 04:25:46, pthatcher1 wrote:
> Can you put a TODO for unifying SetVideoPlayout and SetVideoSend?  It doesn't
> make sense to me that we have two different methods to call.  And, why do we
> assume track_->enabled() here?

Calling SetVideoSend here was a copy/paste mistake (understand, I just barely
got this code compiling...)

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.c...
talk/app/webrtc/rtpreceiver.cc:110: if (track_->enabled() && source) {
On 2015/09/17 04:25:46, pthatcher1 wrote:
> Wait... why do we track the previous track_->enabled() state for audio but not
> for video?  Can you put a TODO for making it consistent between the two?

See above comment.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h
File talk/app/webrtc/rtpreceiver.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:64: std::string mid() const override { return
mid_; }
On 2015/09/17 04:25:46, pthatcher1 wrote:
> I think we should either remove this or just call it "id" so that it can work
> well between Plan B (MSID), Unified Plan (MID), and ORTC (an ID unto itself).

Removed.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:67: void Stop() override;
On 2015/09/17 04:25:46, pthatcher1 wrote:
> Does this also shutdown the media, or does setLocal/setRemoteDescription do
> that?  We should probably make it clear in the comment one way or the other.

I'm not completely sure what "shutdown the media" entails, but currently this
just detaches the track from the AudioProviderInterface (the media channel).

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:73: uint32 ssrc_;
On 2015/09/17 04:25:46, pthatcher1 wrote:
> Can you put a TODO to change the key we use for passing into
> BaseChannel/MediaChannel to a "sender_id" string?

Done.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:75: AudioProviderInterface* provider_;
On 2015/09/17 04:25:46, pthatcher1 wrote:
> Can you comment that this is just a crazy way to get to the BaseChannel, and
> leave a TODO for refactoring that?

Done.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpreceiver.h...
talk/app/webrtc/rtpreceiver.h:76: bool enabled_;
On 2015/09/17 04:25:46, pthatcher1 wrote:
> Can you rename this to track_enabled_?  That would make it much more clear,
> especially if you add a comment like "stored separately from track_ because we
> need to know whether it changes or not."

Changed to cached_track_enabled_.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpsenderinte...
File talk/app/webrtc/rtpsenderinterface.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/rtpsenderinte...
talk/app/webrtc/rtpsenderinterface.h:46: virtual std::string mid() const = 0;
On 2015/09/17 04:25:46, pthatcher1 wrote:
> I think there is a whole lot of code added to pass around the MID, but I don't
> think we actually need it on the object.  I'd say we just remove it all if we
> can.

Done.

[pthatcher1, 2015-09-23 14:47:38]

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

[pthatcher1, 2015-09-23 14:47:40]

I agree that we should have another pair of eyes on the Java parts.  

Alex, would you please take a look at the Java parts?

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.cc:850: rtp_receivers_.erase(it);
On 2015/09/23 00:10:45, Taylor Brandstetter wrote:
> On 2015/09/17 04:25:46, pthatcher1 wrote:
> > Long-term, I think we want to keep senders and receivers in here (perhaps as
> > Transceivers), because we'll want to use them track m-lines, and m-lines
can't
> > go away
> > 
> > Just having a TODO might be sufficient for now.
> 
> Can't an m-line go away if someone calls "stop" on the Transceiver? Or if it's
> send-only and the local track is removed? Anyway, for now I'll add a TODO.

No, an m-line can never go away.

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1351803002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectioninterface.h:327: typedef
std::vector<rtc::scoped_refptr<RtpSenderInterface>> RtpSenders;
On 2015/09/23 00:10:45, Taylor Brandstetter wrote:
> On 2015/09/17 04:25:46, pthatcher1 wrote:
> > I think we should call it something like RefcountedRtpSenders to make it
clear
> > they are refcounted. 
> > 
> > Also, we can pull it out of into the main namespace, I think.
> 
> I assume you mean "out of PeerConnectionInterface"? Also, how about
> "RtpSenderRefptrs" for extra clarity? "RefcountedRtpSenders" seems a little
> redundant since RtpSenderInterface is inherently ref-counted.

Yes, out of PeerConnectionInterface.

Actually, the more I think about it, the more I think we should just use
std::vector<rtc::scoped_refptr<RtpReceiverInterface>> instead of
RtpSenderRefptrs.  It's longer, but it's only in a few places and it makes
everything more clear.

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:165: private final
List<RtpReceiver> rtpReceivers;
I'd just shorten these to senders and receivers.

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:345: }
It seems like a helper method called hasReceiver could clean this up a bit:

bool native hasReceiver(Iterable<RtpReceiver> receivers, RtpReceiver receiver) {
  for (RtpReceiver receiver2: receivers) {
    if (receiver.nativeRtpReceiver == receiver2.nativeRtpReceiver) {
      return true;
    }
  }
  return false;
}
for (RtpReceiver oldReceiver: receivers) {
  if (!hasReceiver(newReceivers, oldReceiver)) {
    toRemove.add(oldReceiver);
  }
}

for (RtpReceiver removed : toRemove) {
  rtpReceivers.remove(removed);
  removed.dispose();
}

for (RtpReceiver newReceiver : newReceivers) {
  if (hasReceiver(receivers, newReceiver)) {
    newReceiver.dispose();
  } else {
    receivers.add(newReceiver);
  }
}


And if you just copy the old receivers, it's even more readable:

List<RtpReceiver> oldReceivers = getReceivers();
for (RtpReceiver oldReceiver: oldReceivers) {
  if (!hasReceiver(newReceivers, oldReceiver)) {
    receivers.remove(oldReceiver);
  }
}

for (RtpReceiver newReceiver : newReceivers) {
  if (hasReceiver(oldReceivers, newReceiver)) {
    newReceiver.dispose();
  } else {
    receivers.add(newReceiver);
  }
}

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java (right):

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java:34: private
MediaStreamTrack cachedTrack;
Why not just call it track?

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpReceiver.java:39: cachedTrack = new
MediaStreamTrack(track);
Please use the "this." prefix in both place or neither.

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/RtpSender.java (right):

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/RtpSender.java:39: cachedTrack = (track ==
0) ? null : new MediaStreamTrack(track);
Same here as with receivers.

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

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:433: // description hasn't yet been set.
More like "Replace with addTrack, which may create RtpSenders and RtpReceivers
immediately.".

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:852: // description.
Well, in unified plan m-lines can never go away.  So this TODO is really
"Implement Unified Plan, in which case RtpReceivers will never be removed."

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

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:179: rtc::scoped_refptr<RtpReceiverInterface>>
RtpReceiverPair;
This seems a bit too complicated.  Can we just move Stop/Detach into the
RtpReceiverInterface and then not have to have a
rtc::scoped_refptr<BaseRtpReceiver> and then just have a
rtc::scoped_refptr<RtpReceiverInterface>?  That seems like it would be much more
simple.

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:215: RtpReceiverPairList rtp_receiver_pairs_;
***

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.cc:70: provider_->SetAudioPlayout(ssrc_, false,
nullptr);
If you put 

if (!provider_) {
  return;
}

At the beginning, it will be more safe and you won't have to check it everywhere
else.  You can just call Stop().

And the same idea for all of (Audio, Video)(Sender, Receiver).

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.cc:74: void AudioRtpReceiver::UpdateProvider() {
I think that it would make sense to call this something like Reconfigure(). 
Right now that means going through the provider.  In the future, it will do
something else.  Might as well not have "provider" in the name.

And the same idea for all of (Audio, Video)(Sender, Receiver).

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
File talk/app/webrtc/rtpreceiver.h (right):

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.h:45: virtual void DetachFromProvider() = 0;
I think it would make sense to call this method Stop(), because it's exactly
like the RtpReceiver::Stop method in ORTC.  In fact, I think we should put the
method on RtpReceiverInterface.  Then you might not need the base class.

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.h:80: bool cached_track_enabled_;
Why not just track_enabled_, with a comment explaining that we keep it separate
to know when track_->enabled() changed.

https://codereview.webrtc.org/1351803002/diff/60001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1351803002/diff/60001/talk/libjingle.gyp#newcod...
talk/libjingle.gyp:764: 'app/webrtc/rtpsenderinterface.h',
Don't forget about BUILD.gn and the FYI bots :).

[pthatcher1, 2015-09-23 14:59:09]

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

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:215: RtpReceiverPairList rtp_receiver_pairs_;
On 2015/09/23 14:47:39, pthatcher1 wrote:
> ***

This was a not to myself to come back and say "Why can't this just be
std::vector<rtc::scoped_refptr<RtpReceiver>> receivers_"?   Why can't we return
the same RtpReceiver objects every time?

[AlexG, 2015-09-23 17:03:02]

I think it may be worth to split this CL into two parts - submit C++ changes
first and then create a separate CL for Java and JNI support.

As a first comment - please fix Java style - use 2 spaces instead of 4.

[Taylor Brandstetter, 2015-09-23 17:06:03]

On 2015/09/23 17:03:02, AlexG wrote:
> I think it may be worth to split this CL into two parts - submit C++ changes
> first and then create a separate CL for Java and JNI support.
> 
> As a first comment - please fix Java style - use 2 spaces instead of 4.

That's a good point. I'll split it.

Also, the last patch ("Fixing indentation") switches to 2 spaces. "git cl
format" automatically made it 4 spaces, because apparently that's what Chromium
uses for Java code.

[AlexG, 2015-09-23 17:10:26]

On 2015/09/23 17:06:03, Taylor Brandstetter wrote:
> On 2015/09/23 17:03:02, AlexG wrote:
> > I think it may be worth to split this CL into two parts - submit C++ changes
> > first and then create a separate CL for Java and JNI support.
> > 
> > As a first comment - please fix Java style - use 2 spaces instead of 4.
> 
> That's a good point. I'll split it.
> 
> Also, the last patch ("Fixing indentation") switches to 2 spaces. "git cl
> format" automatically made it 4 spaces, because apparently that's what
Chromium
> uses for Java code.

Yes, but WebRTC Java uses 2 spaces. Let's keep it this way for now to be
consistent with other Java files.

[Taylor Brandstetter, 2015-09-23 20:50:37]

deadbeef@webrtc.org changed reviewers:
- glaznev@webrtc.org

[Taylor Brandstetter, 2015-09-23 20:50:37]

As discussed, I added an id method (currently just using id of track passed in),
and got rid of the mess with proxies since the Java PC can now identify
senders/receivers by ID.

[Taylor Brandstetter, 2015-09-23 20:50:39]

As discussed, I added an id method (currently just using id of track passed in),
and got rid of the mess with proxies since the Java PC can now identify
senders/receivers by ID.

[pthatcher1, 2015-09-24 06:32:30]

Mostly names and some style things.  Otherwise, it's looking great.

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

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:433: // description hasn't yet been set.
On 2015/09/23 14:47:39, pthatcher1 wrote:
> More like "Replace with addTrack, which may create RtpSenders and RtpReceivers
> immediately.".

Can you update this comment?

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.cc:70: provider_->SetAudioPlayout(ssrc_, false,
nullptr);
On 2015/09/23 14:47:39, pthatcher1 wrote:
> If you put 
> 
> if (!provider_) {
>   return;
> }
> 
> At the beginning, it will be more safe and you won't have to check it
everywhere
> else.  You can just call Stop().
> 
> And the same idea for all of (Audio, Video)(Sender, Receiver).

I still think this is a good idea.

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.cc:74: void AudioRtpReceiver::UpdateProvider() {
On 2015/09/23 14:47:39, pthatcher1 wrote:
> I think that it would make sense to call this something like Reconfigure(). 
> Right now that means going through the provider.  In the future, it will do
> something else.  Might as well not have "provider" in the name.
> 
> And the same idea for all of (Audio, Video)(Sender, Receiver).

I still think this is a good idea.

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
File talk/app/webrtc/rtpreceiver.h (right):

https://codereview.webrtc.org/1351803002/diff/60001/talk/app/webrtc/rtpreceiv...
talk/app/webrtc/rtpreceiver.h:45: virtual void DetachFromProvider() = 0;
On 2015/09/23 14:47:40, pthatcher1 wrote:
> I think it would make sense to call this method Stop(), because it's exactly
> like the RtpReceiver::Stop method in ORTC.  In fact, I think we should put the
> method on RtpReceiverInterface.  Then you might not need the base class.

I still think we should call this method Stop.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:482: }
Why do we still have proxies here?
 

Can't we just "return senders_"?  If so, why not return a const ref?

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:864: (*it)->DetachFromProvider();
Nit: our usual style is it.second->Foo() rather than (*it)->Foo()

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:177: BaseRtpReceiverRefptrs;
The more I think about this, the more I think we shouldn't typedef these, and we
should just use the full name.  It's more in line with Google C++ style, and it
will only be an issue in a few places.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:205: BaseRtpReceiverRefptrs rtp_receivers_;
I think we can just call them senders_ and receivers_.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:153: RtpReceiverRefptrs;
As mentioned elsewhere, I think we should not have typedefs like these after
all.  They don't really buy us that much now that we have C++11.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/rtprecei...
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/rtprecei...
talk/app/webrtc/rtpreceiver.cc:44: UpdateProvider();
I still think this should be called Reconfigure();

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/rtprecei...
File talk/app/webrtc/rtpreceiver.h (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/rtprecei...
talk/app/webrtc/rtpreceiver.h:48: std::string id() const override { return id_;
}
If you move DetachFromProvider (hopefully named Stop) into RtpReceiverInterface,
could you make PeerConnection have a vector of RtpReceiverIntefaces instead of
BaseRtpReceivers?

[Taylor Brandstetter, 2015-09-24 20:54:20]

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:482: }
On 2015/09/24 06:32:29, pthatcher1 wrote:
> Why do we still have proxies here?
>  
> 
> Can't we just "return senders_"?  If so, why not return a const ref?

We still need to return an object that does thread hopping, since the RtpSender
expects everything to be called on the signaling thread. Just like we do for the
DataChannel, and the PeerConnection itself.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:864: (*it)->DetachFromProvider();
On 2015/09/24 06:32:29, pthatcher1 wrote:
> Nit: our usual style is it.second->Foo() rather than (*it)->Foo()

But this isn't an iterator to a pair of pointers, it's just an iterator to a
pointer.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:177: BaseRtpReceiverRefptrs;
On 2015/09/24 06:32:29, pthatcher1 wrote:
> The more I think about this, the more I think we shouldn't typedef these, and
we
> should just use the full name.  It's more in line with Google C++ style, and
it
> will only be an issue in a few places.

Done.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:205: BaseRtpReceiverRefptrs rtp_receivers_;
On 2015/09/24 06:32:29, pthatcher1 wrote:
> I think we can just call them senders_ and receivers_.

Done.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:153: RtpReceiverRefptrs;
On 2015/09/24 06:32:29, pthatcher1 wrote:
> As mentioned elsewhere, I think we should not have typedefs like these after
> all.  They don't really buy us that much now that we have C++11.

Done.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/rtprecei...
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/rtprecei...
talk/app/webrtc/rtpreceiver.cc:44: UpdateProvider();
On 2015/09/24 06:32:29, pthatcher1 wrote:
> I still think this should be called Reconfigure();

Done.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/rtprecei...
File talk/app/webrtc/rtpreceiver.h (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/rtprecei...
talk/app/webrtc/rtpreceiver.h:48: std::string id() const override { return id_;
}
On 2015/09/24 06:32:29, pthatcher1 wrote:
> If you move DetachFromProvider (hopefully named Stop) into
RtpReceiverInterface,
> could you make PeerConnection have a vector of RtpReceiverIntefaces instead of
> BaseRtpReceivers?

That's the way it was before, but I purposefully made this change because
DetachFromProvider really isn't a "Stop" as in the ORTC spec, and it should only
be called internally by PeerConnection. It means more boilerplate code, but I
think it better separates the interface from implementation.

If DetachFromProvider is really SUPPOSED to be "Stop" (and tear down everything
down to the MediaChannel), I'd be fine with putting things back and leaving a
"TODO" to implement it properly. But with the "RtpTransceiver" PR, are
individual senders/receivers really going to have a Stop method?

[pthatcher1, 2015-09-24 21:18:34]

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:482: }
On 2015/09/24 20:54:20, Taylor Brandstetter wrote:
> On 2015/09/24 06:32:29, pthatcher1 wrote:
> > Why do we still have proxies here?
> >  
> > 
> > Can't we just "return senders_"?  If so, why not return a const ref?
> 
> We still need to return an object that does thread hopping, since the
RtpSender
> expects everything to be called on the signaling thread. Just like we do for
the
> DataChannel, and the PeerConnection itself.

OK, I get it now.  Thanks.

https://codereview.webrtc.org/1351803002/diff/120001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:864: (*it)->DetachFromProvider();
On 2015/09/24 20:54:20, Taylor Brandstetter wrote:
> On 2015/09/24 06:32:29, pthatcher1 wrote:
> > Nit: our usual style is it.second->Foo() rather than (*it)->Foo()
> 
> But this isn't an iterator to a pair of pointers, it's just an iterator to a
> pointer.

Oh, duh :).

[pthatcher1, 2015-09-24 21:35:22]

lgtm

Great work.

https://codereview.webrtc.org/1351803002/diff/160001/talk/app/webrtc/rtprecei...
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1351803002/diff/160001/talk/app/webrtc/rtprecei...
talk/app/webrtc/rtpreceiver.cc:98: void VideoRtpReceiver::Stop() {
We should probably put 

if (provider_) {
}

In the Stop method rather than in the caller of the Stop method (above in the
desctructor).

[Taylor Brandstetter, 2015-09-24 22:00:53]

https://codereview.webrtc.org/1351803002/diff/160001/talk/app/webrtc/rtprecei...
File talk/app/webrtc/rtpreceiver.cc (right):

https://codereview.webrtc.org/1351803002/diff/160001/talk/app/webrtc/rtprecei...
talk/app/webrtc/rtpreceiver.cc:98: void VideoRtpReceiver::Stop() {
On 2015/09/24 21:35:22, pthatcher1 wrote:
> We should probably put 
> 
> if (provider_) {
> }
> 
> In the Stop method rather than in the caller of the Stop method (above in the
> desctructor).

Done.

[Taylor Brandstetter, 2015-09-28 18:07:15]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-09-28 18:07:16]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1351803002/#ps200001 (title:
"Merge with master (stubbed files becoming real files)")

[commit-bot: I haz the power, 2015-09-28 18:07:48]

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

[Taylor Brandstetter, 2015-09-28 18:09:25]

The CQ bit was unchecked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-09-28 19:58:37]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-09-28 19:58:38]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1351803002/#ps220001 (title:
"Adding some stubs so that Chromium build won't break.")

[commit-bot: I haz the power, 2015-09-28 19:59:07]

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

[commit-bot: I haz the power, 2015-09-28 20:42:46]

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

[commit-bot: I haz the power, 2015-09-28 20:42:47]

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

[Taylor Brandstetter, 2015-09-28 23:52:30]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2015-09-28 23:52:55]

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

[commit-bot: I haz the power, 2015-09-28 23:54:01]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-09-28 23:54:13]

Patchset 12 (id:??) landed as
https://crrev.com/70ab1a1ca89d280a7d51e3fadc51d4be9df209ca
Cr-Commit-Position: refs/heads/master@{#10100}