Wire up transport sequence number / send time callbacks to webrtc via libjingle.

Wire up packet_id / send time callbacks to webrtc via libjingle.

BUG=webrtc:4173
Committed: https://crrev.com/c1aeaf0dc37d96f31c92f893f4e30e7a5f7cc2b7
Cr-Commit-Position: refs/heads/master@{#10289}

[stefan-webrtc, 2015-09-25 14:45:01]

Patchset #1 (id:1) has been deleted

[stefan-webrtc, 2015-09-25 14:47:50]

Patchset #1 (id:20001) has been deleted

[stefan-webrtc, 2015-09-25 14:52:12]

stefan@webrtc.org changed reviewers:
+ pbos@webrtc.org, pthatcher@webrtc.org

[stefan-webrtc, 2015-09-25 14:52:13]

Hi, it would be great if you guys could take a look at this CL.

Besides regular review, I'd also be interested in suggestions on how to test
this in a good way. In particular, I find it important and rather difficult to
test the changes in BaseChannel. I have considered using a
FakeTransportController and use that in some way to monitor the packets going
out from BaseChannel. Then I could build fake RTP packets with the header
extension and see if they get parsed correctly. However, this also requires me
to build and set content on BaseChannel.

Is this the way to go? I don't want to spend much more time on it before I know
it's the best way to test this... :)

[pthatcher1, 2015-09-25 23:24:58]

I think the way to test it is:

1.  Test the network code can do send/sent with a given packet ID separately.

2.  Test that when a packet is given to BaseChannel to send, the call gets a
"sent" callback.  I think you could do that with a fake transport channel that
fires "sent" and having BaseChannel have a direct connection to call_.

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/constants.cc
File talk/media/base/constants.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/constants...
talk/media/base/constants.cc:93: const char kRtcpFbParamTransport[] =
"goog-transport-fb";
Are we avoiding "goog" prefixes?  

And can we come up with a more descriptive name?  Perhaps "transport-seq-num"?

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/constants.h
File talk/media/base/constants.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/constants...
talk/media/base/constants.h:110: // and according to
This won't make sense if we remove the comments above :).   Maybe just keep it
as

// rtcp-fb message according to

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/fakemedia...
File talk/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/fakemedia...
talk/media/base/fakemediaengine.h:167: rtc::SentPacket last_packet_sent() const
{ return last_packet_sent_; }
last_sent_packet_ would fit a little better with the type name, and is perfectly
good English.

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/fakemedia...
talk/media/base/fakemediaengine.h:208: virtual void OnPacketSent(const
rtc::SentPacket& packet_sent) {
Similarly, sent_packet would be a slightly better variable name.

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:552: virtual void OnPacketSent(const
rtc::SentPacket& packet_sent) = 0;
Can you just make this default to {}, so we don't have to define a method in all
these data channels where it doesn't make sense?

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1485:
call_->OnSentPacket(webrtc_packet_sent);
Why not just propogate from the TransportChannel to Call_ directly (maybe going
through BaseChannel)?  Why go through the MediaChannel?

https://codereview.webrtc.org/1363573002/diff/40001/talk/session/media/channe...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/session/media/channe...
talk/session/media/channel.cc:486: media_channel_->OnPacketSent(packet_sent);
This means that all 3 channels (audio, video, and data) will get called back for
all of the packets for all three.  That seems a bit overkill.  And in the future
when we have N channels, it will be worse.  Can we just call directly into the
Call object somehow?

https://codereview.webrtc.org/1363573002/diff/40001/talk/session/media/channe...
talk/session/media/channel.cc:575:
ParseTransportSequenceNumberHeaderExtension(data, len);
I really think is the wrong place for this.  It would be much better to change
BaseChannel::SendPacket to take a packet_id as parameter, perhaps by replacing
dscp with packet_options like it is in the lower layers.  Then the layer
creating the packet can just put the ID on the packet rather than having to do
this crazy parsing thing.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
File webrtc/base/asyncpacketsocket.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
webrtc/base/asyncpacketsocket.h:42: int32 transport_sequence_number;  // 16
bits, -1 represents "not set".
I'm not to excited about something so RTP-specific being jammed into the lowest
level of our network code.  But I think we could make this more generic. 
Basically all you want is a way to know which packet was sent which means you
want a value in the PacketOptions that matches the value in the SentPacket.  In
that case, let's try this:

1.  Call this "packet_id".
2.  Put the SentPacket struct right below this one, also with "packet_id".

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
webrtc/base/asyncpacketsocket.h:125: sigslot::signal2<AsyncPacketSocket*, const
SentPacket&> SignalPacketSent;
I don't actually see this implemented anywhere.  Does that come later?

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1352: if (!FindConnection(connection))
I don't think this check is necessary.  And, do we really want to iterate
through all of the connections for every packet?

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:470: sigslot::signal2<Connection*, const
rtc::SentPacket&> SignalSentPacket;
I don't see this actually wired up to anything.  Does that come later?

[stefan-webrtc, 2015-09-28 12:10:50]

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:552: virtual void OnPacketSent(const
rtc::SentPacket& packet_sent) = 0;
On 2015/09/25 23:24:57, pthatcher1 wrote:
> Can you just make this default to {}, so we don't have to define a method in
all
> these data channels where it doesn't make sense?

Great suggestion. :)

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1485:
call_->OnSentPacket(webrtc_packet_sent);
On 2015/09/25 23:24:57, pthatcher1 wrote:
> Why not just propogate from the TransportChannel to Call_ directly (maybe
going
> through BaseChannel)?  Why go through the MediaChannel?

Hm, this is probably an artifact of my lack of overview of libjingle code. I
basically followed what was being done with OnReadyToSend().

https://codereview.webrtc.org/1363573002/diff/40001/talk/session/media/channe...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/session/media/channe...
talk/session/media/channel.cc:486: media_channel_->OnPacketSent(packet_sent);
On 2015/09/25 23:24:57, pthatcher1 wrote:
> This means that all 3 channels (audio, video, and data) will get called back
for
> all of the packets for all three.  That seems a bit overkill.  And in the
future
> when we have N channels, it will be worse.  Can we just call directly into the
> Call object somehow?

That would be nicer. I'll look into it.

https://codereview.webrtc.org/1363573002/diff/40001/talk/session/media/channe...
talk/session/media/channel.cc:575:
ParseTransportSequenceNumberHeaderExtension(data, len);
On 2015/09/25 23:24:57, pthatcher1 wrote:
> I really think is the wrong place for this.  It would be much better to change
> BaseChannel::SendPacket to take a packet_id as parameter, perhaps by replacing
> dscp with packet_options like it is in the lower layers.  Then the layer
> creating the packet can just put the ID on the packet rather than having to do
> this crazy parsing thing.

Yes, I definitely agree that it would make more sense to pass the sequence
number with the packet all the way from where it is created. I'll discuss with
others to see if we can change the WebRTC transport interface to allow this.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
File webrtc/base/asyncpacketsocket.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
webrtc/base/asyncpacketsocket.h:42: int32 transport_sequence_number;  // 16
bits, -1 represents "not set".
On 2015/09/25 23:24:58, pthatcher1 wrote:
> I'm not to excited about something so RTP-specific being jammed into the
lowest
> level of our network code.  But I think we could make this more generic. 
> Basically all you want is a way to know which packet was sent which means you
> want a value in the PacketOptions that matches the value in the SentPacket. 
In
> that case, let's try this:
> 
> 1.  Call this "packet_id".
> 2.  Put the SentPacket struct right below this one, also with "packet_id".

I could call it packet_id. I'm not sure that's a lot more generic than
packet_sequence_number though? And a sequence number implies that it is also
incremented by one for every packet, which might be good to know.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
webrtc/base/asyncpacketsocket.h:125: sigslot::signal2<AsyncPacketSocket*, const
SentPacket&> SignalPacketSent;
On 2015/09/25 23:24:57, pthatcher1 wrote:
> I don't actually see this implemented anywhere.  Does that come later?

It's implemented in Chromium here: https://codereview.chromium.org/1345583004/

Other platforms don't really need this since a send timestamp set at the webrtc
layer roughly corresponds to when the send socket gets the packet.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1352: if (!FindConnection(connection))
On 2015/09/25 23:24:58, pthatcher1 wrote:
> I don't think this check is necessary.  And, do we really want to iterate
> through all of the connections for every packet?

I mostly did this to follow the convention in OnReadPacket(). Should I instead
check for connection == best_connection_ as in OnReadyToSend()? I assume there
are multiple connections for one P2PTransportChannel, but only one active?

[pthatcher1, 2015-09-28 23:58:40]

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1485:
call_->OnSentPacket(webrtc_packet_sent);
On 2015/09/28 12:10:49, stefan-webrtc (holmer) wrote:
> On 2015/09/25 23:24:57, pthatcher1 wrote:
> > Why not just propogate from the TransportChannel to Call_ directly (maybe
> going
> > through BaseChannel)?  Why go through the MediaChannel?
> 
> Hm, this is probably an artifact of my lack of overview of libjingle code. I
> basically followed what was being done with OnReadyToSend().

Not your fault, but OnReadyToSend needs to be cleaned up.  It's not a very good
model for copying.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
File webrtc/base/asyncpacketsocket.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
webrtc/base/asyncpacketsocket.h:42: int32 transport_sequence_number;  // 16
bits, -1 represents "not set".
On 2015/09/28 12:10:50, stefan-webrtc (holmer) wrote:
> On 2015/09/25 23:24:58, pthatcher1 wrote:
> > I'm not to excited about something so RTP-specific being jammed into the
> lowest
> > level of our network code.  But I think we could make this more generic. 
> > Basically all you want is a way to know which packet was sent which means
you
> > want a value in the PacketOptions that matches the value in the SentPacket. 
> In
> > that case, let's try this:
> > 
> > 1.  Call this "packet_id".
> > 2.  Put the SentPacket struct right below this one, also with "packet_id".
> 
> I could call it packet_id. I'm not sure that's a lot more generic than
> packet_sequence_number though?

It's more generic because the layer that's doing the delivery has no notion of
"sequence" and once you put "sequence" in the name, you have to define what a
"sequence" is at this layer of the code.  So I think we should remove the word
"sequence".  

Similarly, if we put "transport" in the name, we have to define what a
"transport" is.  So I think we should remove "transport" from the name also.

This layer just needs a way to say "packet X was delivered", with some day to
identify "X".  If we remove "sequence" and "transport", we are left with just
"number", which doesn't say what the number is for.  What is it for?  It
identifies the packet.  So, perhaps "packet_number".  But I think "packet_id"
sounds slightly better.


> And a sequence number implies that it is also
> incremented by one for every packet, which might be good to know.

If we ever come to a point where we wanted more meaning in value, then we could
rename it to match the new meaning.  But for now, it's just an ID.


However, if you feel like we will have multiple sequences going to the same
socket, then it would make sense to have a transport_id and a
transport_sequence_number, because then we could define "transport" and
"sequence" with something meaningful.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1352: if (!FindConnection(connection))
On 2015/09/28 12:10:50, stefan-webrtc (holmer) wrote:
> On 2015/09/25 23:24:58, pthatcher1 wrote:
> > I don't think this check is necessary.  And, do we really want to iterate
> > through all of the connections for every packet?
> 
> I mostly did this to follow the convention in OnReadPacket(). Should I instead
> check for connection == best_connection_ as in OnReadyToSend()? I assume there
> are multiple connections for one P2PTransportChannel, but only one active?


I don't really think the check in OnReadPacket is necessary either. But at least
in theory it's saying "if I remove a connection, stop receiving on it".  This
would be saying "if I remove a connection, stop telling me when things that I
asked to send are not sent", which doesn't sound as useful.

[stefan-webrtc, 2015-10-02 13:29:12]

I have made an attempt to address your comments. 

I'm still not sure about the testing though. Ideally I'd like to test this
end-to-end, but I made an attempt and it was really hard to set something up, so
I gave up after a days work. If you have more detailed suggestions, I could make
another try. Otherwise we may have to live with this being tested piece by
piece.

I also wired this up for the async udp/tcp sockets in this CL, so that it's not
only wired up in Chromium.

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/constants.cc
File talk/media/base/constants.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/constants...
talk/media/base/constants.cc:93: const char kRtcpFbParamTransport[] =
"goog-transport-fb";
On 2015/09/25 23:24:57, pthatcher1 wrote:
> Are we avoiding "goog" prefixes?  
> 
> And can we come up with a more descriptive name?  Perhaps "transport-seq-num"?

This is actually an rtcp message. Renaming it transport-cc-fb.

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/constants.h
File talk/media/base/constants.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/constants...
talk/media/base/constants.h:110: // and according to
On 2015/09/25 23:24:57, pthatcher1 wrote:
> This won't make sense if we remove the comments above :).   Maybe just keep it
> as
> 
> // rtcp-fb message according to

Done.

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/fakemedia...
File talk/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/fakemedia...
talk/media/base/fakemediaengine.h:167: rtc::SentPacket last_packet_sent() const
{ return last_packet_sent_; }
On 2015/09/25 23:24:57, pthatcher1 wrote:
> last_sent_packet_ would fit a little better with the type name, and is
perfectly
> good English.

Done.

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/base/fakemedia...
talk/media/base/fakemediaengine.h:208: virtual void OnPacketSent(const
rtc::SentPacket& packet_sent) {
On 2015/09/25 23:24:57, pthatcher1 wrote:
> Similarly, sent_packet would be a slightly better variable name.

Done.

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1485:
call_->OnSentPacket(webrtc_packet_sent);
On 2015/09/25 23:24:57, pthatcher1 wrote:
> Why not just propogate from the TransportChannel to Call_ directly (maybe
going
> through BaseChannel)?  Why go through the MediaChannel?

Done.

https://codereview.webrtc.org/1363573002/diff/40001/talk/session/media/channe...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/talk/session/media/channe...
talk/session/media/channel.cc:575:
ParseTransportSequenceNumberHeaderExtension(data, len);
On 2015/09/28 12:10:50, stefan-webrtc (holmer) wrote:
> On 2015/09/25 23:24:57, pthatcher1 wrote:
> > I really think is the wrong place for this.  It would be much better to
change
> > BaseChannel::SendPacket to take a packet_id as parameter, perhaps by
replacing
> > dscp with packet_options like it is in the lower layers.  Then the layer
> > creating the packet can just put the ID on the packet rather than having to
do
> > this crazy parsing thing.
> 
> Yes, I definitely agree that it would make more sense to pass the sequence
> number with the packet all the way from where it is created. I'll discuss with
> others to see if we can change the WebRTC transport interface to allow this.

Done.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
File webrtc/base/asyncpacketsocket.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/base/asyncpacketso...
webrtc/base/asyncpacketsocket.h:42: int32 transport_sequence_number;  // 16
bits, -1 represents "not set".
On 2015/09/28 23:58:40, pthatcher1 wrote:
> On 2015/09/28 12:10:50, stefan-webrtc (holmer) wrote:
> > On 2015/09/25 23:24:58, pthatcher1 wrote:
> > > I'm not to excited about something so RTP-specific being jammed into the
> > lowest
> > > level of our network code.  But I think we could make this more generic. 
> > > Basically all you want is a way to know which packet was sent which means
> you
> > > want a value in the PacketOptions that matches the value in the
SentPacket. 
> > In
> > > that case, let's try this:
> > > 
> > > 1.  Call this "packet_id".
> > > 2.  Put the SentPacket struct right below this one, also with "packet_id".
> > 
> > I could call it packet_id. I'm not sure that's a lot more generic than
> > packet_sequence_number though?
> 
> It's more generic because the layer that's doing the delivery has no notion of
> "sequence" and once you put "sequence" in the name, you have to define what a
> "sequence" is at this layer of the code.  So I think we should remove the word
> "sequence".  
> 
> Similarly, if we put "transport" in the name, we have to define what a
> "transport" is.  So I think we should remove "transport" from the name also.
> 
> This layer just needs a way to say "packet X was delivered", with some day to
> identify "X".  If we remove "sequence" and "transport", we are left with just
> "number", which doesn't say what the number is for.  What is it for?  It
> identifies the packet.  So, perhaps "packet_number".  But I think "packet_id"
> sounds slightly better.
> 
> 
> > And a sequence number implies that it is also
> > incremented by one for every packet, which might be good to know.
> 
> If we ever come to a point where we wanted more meaning in value, then we
could
> rename it to match the new meaning.  But for now, it's just an ID.
> 
> 
> However, if you feel like we will have multiple sequences going to the same
> socket, then it would make sense to have a transport_id and a
> transport_sequence_number, because then we could define "transport" and
> "sequence" with something meaningful.

I'm convinced. Fixed.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:1352: if (!FindConnection(connection))
On 2015/09/28 23:58:40, pthatcher1 wrote:
> On 2015/09/28 12:10:50, stefan-webrtc (holmer) wrote:
> > On 2015/09/25 23:24:58, pthatcher1 wrote:
> > > I don't think this check is necessary.  And, do we really want to iterate
> > > through all of the connections for every packet?
> > 
> > I mostly did this to follow the convention in OnReadPacket(). Should I
instead
> > check for connection == best_connection_ as in OnReadyToSend()? I assume
there
> > are multiple connections for one P2PTransportChannel, but only one active?
> 
> 
> I don't really think the check in OnReadPacket is necessary either. But at
least
> in theory it's saying "if I remove a connection, stop receiving on it".  This
> would be saying "if I remove a connection, stop telling me when things that I
> asked to send are not sent", which doesn't sound as useful.

No, that doesn't make a lot of sense. Fixing.

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1363573002/diff/40001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:470: sigslot::signal2<Connection*, const
rtc::SentPacket&> SignalSentPacket;
On 2015/09/25 23:24:58, pthatcher1 wrote:
> I don't see this actually wired up to anything.  Does that come later?

I dropped the ball here. I think it should be properly wired up now.

[stefan-webrtc, 2015-10-02 13:32:23]

Remove byteorder changes

[stefan-webrtc, 2015-10-02 13:46:06]

Note that there was a rebase in patch-set 2. Sorry about that. The best way to
review is to look at the full patch.

[stefan-webrtc, 2015-10-02 14:17:26]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-02 14:17:53]

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

[commit-bot: I haz the power, 2015-10-02 14:37:52]

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

[commit-bot: I haz the power, 2015-10-02 14:37:53]

Dry run: Try jobs failed on following builders:
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/9756)

[stefan-webrtc, 2015-10-05 13:43:19]

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asyncpackets...
File webrtc/base/asyncpacketsocket.h (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asyncpackets...
webrtc/base/asyncpacketsocket.h:124: SignalPacketSent;
I'm not sure if I actually need SocketAddress here, I suspect not? It doesn't
matter to me if the address changes, the packet ids are still the same. Or can
multiple peer connections share the same socket?

[pthatcher1, 2015-10-05 18:30:14]

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:48: virtual void OnPacketSent(const
webrtc::SentPacket& sent_packet) = 0;
Why would a caller call media_controller_->OnPacketSent(sp) instead of
media_controller_->call_w()->OnPacketSent(sp)?

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:48: virtual void OnPacketSent(const
webrtc::SentPacket& sent_packet) = 0;
Are we sure that packet_ids will not collide when we bundle audio and video over
the same transport?  If they do, this will cause problems.  If they don't, then
we're fine.

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:1491: false);
Can you make all the "nullptr, NULL" instances be "nullptr, nullptr"?  It's just
weird to see both right next to each other.

https://codereview.webrtc.org/1363573002/diff/120001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:1666: const webrtc::PacketOptions&
options) {
Would it be possible to have one PacketOptions struct instead of two?  If not,
can we enumerate the reasons why not in the definition of the second one and
then have a TODO saying when we should unify them?

https://codereview.webrtc.org/1363573002/diff/120001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/session/media/chann...
talk/session/media/channel.cc:488: return;
{}s please

https://codereview.webrtc.org/1363573002/diff/120001/talk/session/media/chann...
talk/session/media/channel.cc:491: sent_packet.send_time_ms);
Similarly, can we have just one SentPacket struct?  If not, can we enumerate why
not with a TODO saying when we can?

https://codereview.webrtc.org/1363573002/diff/120001/talk/session/media/chann...
talk/session/media/channel.cc:492:
media_controller_->OnPacketSent(webrtc_sent_packet);
Would it make sense for this to be
media_controller_->call_w()->OnPacketSent(webrtc_sent_packet) ?

https://codereview.webrtc.org/1363573002/diff/120001/talk/session/media/chann...
File talk/session/media/channel_unittest.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/session/media/chann...
talk/session/media/channel_unittest.cc:2545:
EXPECT_TRUE(call_.on_packet_sent_called());
Can we change this test to send a particular packet_id and then make sure that
particular packet_id gets the "sent" signal?  Just testing that it ever gets
fired with anything seems a little bit to easy too pass.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asyncpackets...
File webrtc/base/asyncpacketsocket.h (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asyncpackets...
webrtc/base/asyncpacketsocket.h:124: SignalPacketSent;
On 2015/10/05 13:43:19, stefan-webrtc (holmer) wrote:
> I'm not sure if I actually need SocketAddress here, I suspect not? It doesn't
> matter to me if the address changes, the packet ids are still the same. Or can
> multiple peer connections share the same socket?

You shouldn't care what socket it is, as long as the packet IDs don't overlap
between different transports (different audio and video streams if they are not
bundled).  If the IDs don't overlap, then you're fine.  However, the typical
style is to include the object as the first parameter.  So even though you don't
need it, it's consistent to include it.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asynctcpsock...
File webrtc/base/asynctcpsocket.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asynctcpsock...
webrtc/base/asynctcpsocket.cc:272: SignalPacketSent(this, GetRemoteAddress(),
sent_packet);
We have SignalPacketSent, but that doesn't signal when the packet gets dropped
above and is never sent.  Should we have a SignalPacketDropped or a field on
sent_packet that's ".dropped" (of course, then SentPacket would be a misnomer,
and we'd have to think of a different name)?  In other words, would webrtc::Call
want to know that we dropped a packet before it went out?

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:791: };
Can we just put one SentPacket type in webrtc/base so everyone can share one?

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:258: &P2PTransportChannel::OnPacketSent);
I think the OnX should have the same name as the SignalX.  So would OnSentPacket
make more sense?

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:319: connection->OnPacketSent(sent_packet);
{}s please

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:319: connection->OnPacketSent(sent_packet);
Do we really need to know which connection it went out on?  Can we just put
OnPacketSent on the Port and avoid doing a map lookup for every packet?

[stefan-webrtc, 2015-10-06 13:00:19]

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:48: virtual void OnPacketSent(const
webrtc::SentPacket& sent_packet) = 0;
On 2015/10/05 18:30:13, pthatcher1 wrote:
> Are we sure that packet_ids will not collide when we bundle audio and video
over
> the same transport?  If they do, this will cause problems.  If they don't,
then
> we're fine.

That is the point with this packet id. It's a sequence number which increases by
1 for each packet sent over the same port. I'm currently making the assumption
that all packets sent by a PC are sent on the same underlying port, which is the
same assumption we have been doing before for BWE.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asynctcpsock...
File webrtc/base/asynctcpsocket.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asynctcpsock...
webrtc/base/asynctcpsocket.cc:272: SignalPacketSent(this, GetRemoteAddress(),
sent_packet);
On 2015/10/05 18:30:14, pthatcher1 wrote:
> We have SignalPacketSent, but that doesn't signal when the packet gets dropped
> above and is never sent.  Should we have a SignalPacketDropped or a field on
> sent_packet that's ".dropped" (of course, then SentPacket would be a misnomer,
> and we'd have to think of a different name)?  In other words, would
webrtc::Call
> want to know that we dropped a packet before it went out?

This is not something I'm trying to achieve right now, but could potentially be
something we want to do in the future. I'd suggest that we make those changes
when/if we need them.

We could already infer that by identifying that a certain packet id didn't get a
SignalPacketSent().

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:319: connection->OnPacketSent(sent_packet);
On 2015/10/05 18:30:14, pthatcher1 wrote:
> Do we really need to know which connection it went out on?  Can we just put
> OnPacketSent on the Port and avoid doing a map lookup for every packet?

I think this is a good idea, now that I begin understand how these things are
connected. :)

Just to be sure, you are saying I should send a signal from Port(Interface) via
P2PTransportChannel and from there up to webrtc?

[pthatcher1, 2015-10-06 20:23:55]

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:48: virtual void OnPacketSent(const
webrtc::SentPacket& sent_packet) = 0;
On 2015/10/06 13:00:19, stefan-webrtc (holmer) wrote:
> On 2015/10/05 18:30:13, pthatcher1 wrote:
> > Are we sure that packet_ids will not collide when we bundle audio and video
> over
> > the same transport?  If they do, this will cause problems.  If they don't,
> then
> > we're fine.
> 
> That is the point with this packet id. It's a sequence number which increases
by
> 1 for each packet sent over the same port. I'm currently making the assumption
> that all packets sent by a PC are sent on the same underlying port, which is
the
> same assumption we have been doing before for BWE.

Each port only has one PC, yes.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asynctcpsock...
File webrtc/base/asynctcpsocket.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asynctcpsock...
webrtc/base/asynctcpsocket.cc:272: SignalPacketSent(this, GetRemoteAddress(),
sent_packet);
On 2015/10/06 13:00:19, stefan-webrtc (holmer) wrote:
> On 2015/10/05 18:30:14, pthatcher1 wrote:
> > We have SignalPacketSent, but that doesn't signal when the packet gets
dropped
> > above and is never sent.  Should we have a SignalPacketDropped or a field on
> > sent_packet that's ".dropped" (of course, then SentPacket would be a
misnomer,
> > and we'd have to think of a different name)?  In other words, would
> webrtc::Call
> > want to know that we dropped a packet before it went out?
> 
> This is not something I'm trying to achieve right now, but could potentially
be
> something we want to do in the future. I'd suggest that we make those changes
> when/if we need them.
> 
> We could already infer that by identifying that a certain packet id didn't get
a
> SignalPacketSent().
> 

Makes sense.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:319: connection->OnPacketSent(sent_packet);
On 2015/10/06 13:00:19, stefan-webrtc (holmer) wrote:
> On 2015/10/05 18:30:14, pthatcher1 wrote:
> > Do we really need to know which connection it went out on?  Can we just put
> > OnPacketSent on the Port and avoid doing a map lookup for every packet?
> 
> I think this is a good idea, now that I begin understand how these things are
> connected. :)
> 
> Just to be sure, you are saying I should send a signal from Port(Interface)
via
> P2PTransportChannel and from there up to webrtc?

Yes.

[pthatcher1, 2015-10-07 16:44:38]

Looking much better.

https://codereview.webrtc.org/1363573002/diff/120001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:1666: const webrtc::PacketOptions&
options) {
On 2015/10/05 18:30:13, pthatcher1 wrote:
> Would it be possible to have one PacketOptions struct instead of two?  If not,
> can we enumerate the reasons why not in the definition of the second one and
> then have a TODO saying when we should unify them?

?

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:63: cricket::TransportChannel*
transport_channel) override {
Needs a RTC_DCHECK(worker_thread_->IsCurrent());

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:70: }
Can't you do this?

if (!transport_channels_.insert(transport_channel)->second) {
  return;
}
transport_channel->SignalSentPacket.connect(this,
&MediaController::OnSentPacket);

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:52: cricket::TransportChannel*
transport_channel) = 0;
We might want to name these more explicitly:  ConnectToSignalSentPacket.  Also,
it has to be called on the worker thread, so it should be "_w".

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:54: cricket::TransportChannel*
transport_channel) = 0;
Can you put a comment explaining what these do, when they should be called, and
a TODO that we plan to remove them?

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:352:
media_controller_->ConnectTransportChannel(tc);
This isn't quite the right place to put this, since it will be called every time
bundle configuration changes.  I think it would be better to put it in
WebRtcSession::CreateChannels after the BaseChannels are created.  At that
point, get the transport channel from the audio and video channels.

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:361:
media_controller_->DisconnectTransportChannel(tc);
Woh, this won't work.  If two BaseChannels share a TransportChannel and one of
them is deleted, it will disconnect, and then the media controller will also
disconnect.

I think the easiest thing is to just not have a DisconnectTransportChannel
method.  The worst thing that will happen is that MediaController will have
dangling pointers in transport_channels_.  But as long as we document that, I
think it's fine.

[stefan-webrtc, 2015-10-07 16:46:46]

Cleanup

[stefan-webrtc, 2015-10-07 16:47:33]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-07 16:47:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/240001

[stefan-webrtc, 2015-10-07 16:55:25]

Could you take a look at the changes I have done to mediacontroller.h/.cc and
channel_unittest.cc? I'm not super proud of introducing the CallFactory, but at
the same time I think it's good to test the mechanism introduced in
MediaController this way. Moving construction of Call outside MediaController
didn't feel right either...

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:1491: false);
On 2015/10/05 18:30:13, pthatcher1 wrote:
> Can you make all the "nullptr, NULL" instances be "nullptr, nullptr"?  It's
just
> weird to see both right next to each other.

Done.

https://codereview.webrtc.org/1363573002/diff/120001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:1666: const webrtc::PacketOptions&
options) {
On 2015/10/05 18:30:13, pthatcher1 wrote:
> Would it be possible to have one PacketOptions struct instead of two?  If not,
> can we enumerate the reasons why not in the definition of the second one and
> then have a TODO saying when we should unify them?

I'd be happy to follow-up with a CL which does this, but since the
webrtc::Transport interface I think it's better to do it in a separate CL.

https://codereview.webrtc.org/1363573002/diff/120001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/talk/session/media/chann...
talk/session/media/channel.cc:488: return;
On 2015/10/05 18:30:13, pthatcher1 wrote:
> {}s please

This code has now been removed.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asyncpackets...
File webrtc/base/asyncpacketsocket.h (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/base/asyncpackets...
webrtc/base/asyncpacketsocket.h:124: SignalPacketSent;
On 2015/10/05 18:30:14, pthatcher1 wrote:
> On 2015/10/05 13:43:19, stefan-webrtc (holmer) wrote:
> > I'm not sure if I actually need SocketAddress here, I suspect not? It
doesn't
> > matter to me if the address changes, the packet ids are still the same. Or
can
> > multiple peer connections share the same socket?
> 
> You shouldn't care what socket it is, as long as the packet IDs don't overlap
> between different transports (different audio and video streams if they are
not
> bundled).  If the IDs don't overlap, then you're fine.  However, the typical
> style is to include the object as the first parameter.  So even though you
don't
> need it, it's consistent to include it.

Acknowledged.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/common_types.h#ne...
webrtc/common_types.h:791: };
On 2015/10/05 18:30:14, pthatcher1 wrote:
> Can we just put one SentPacket type in webrtc/base so everyone can share one?

Done.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:258: &P2PTransportChannel::OnPacketSent);
On 2015/10/05 18:30:14, pthatcher1 wrote:
> I think the OnX should have the same name as the SignalX.  So would
OnSentPacket
> make more sense?

Done. These should now all be aligned.

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc
File webrtc/p2p/base/port.cc (right):

https://codereview.webrtc.org/1363573002/diff/120001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:319: connection->OnPacketSent(sent_packet);
On 2015/10/06 20:23:55, pthatcher1 wrote:
> On 2015/10/06 13:00:19, stefan-webrtc (holmer) wrote:
> > On 2015/10/05 18:30:14, pthatcher1 wrote:
> > > Do we really need to know which connection it went out on?  Can we just
put
> > > OnPacketSent on the Port and avoid doing a map lookup for every packet?
> > 
> > I think this is a good idea, now that I begin understand how these things
are
> > connected. :)
> > 
> > Just to be sure, you are saying I should send a signal from Port(Interface)
> via
> > P2PTransportChannel and from there up to webrtc?
> 
> Yes.

Done.

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:361:
media_controller_->DisconnectTransportChannel(tc);
On 2015/10/07 16:44:38, pthatcher1 wrote:
> Woh, this won't work.  If two BaseChannels share a TransportChannel and one of
> them is deleted, it will disconnect, and then the media controller will also
> disconnect.
> 
> I think the easiest thing is to just not have a DisconnectTransportChannel
> method.  The worst thing that will happen is that MediaController will have
> dangling pointers in transport_channels_.  But as long as we document that, I
> think it's fine.  

Very good point. I will look at these comments later today or tomorrow.

[commit-bot: I haz the power, 2015-10-07 17:01:35]

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

[commit-bot: I haz the power, 2015-10-07 17:01:36]

Dry run: Try jobs failed on following builders:
  linux_msan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/5139)

[stefan-webrtc, 2015-10-08 12:52:56]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-08 12:53:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/280001

[stefan-webrtc, 2015-10-08 12:53:46]

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:63: cricket::TransportChannel*
transport_channel) override {
On 2015/10/07 16:44:37, pthatcher1 wrote:
> Needs a RTC_DCHECK(worker_thread_->IsCurrent());

Done.

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:70: }
On 2015/10/07 16:44:37, pthatcher1 wrote:
> Can't you do this?
> 
> if (!transport_channels_.insert(transport_channel)->second) {
>   return;
> }
> transport_channel->SignalSentPacket.connect(this,
> &MediaController::OnSentPacket);

Done.

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:52: cricket::TransportChannel*
transport_channel) = 0;
On 2015/10/07 16:44:38, pthatcher1 wrote:
> We might want to name these more explicitly:  ConnectToSignalSentPacket. 
Also,
> it has to be called on the worker thread, so it should be "_w".

Done.

https://codereview.webrtc.org/1363573002/diff/180001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:54: cricket::TransportChannel*
transport_channel) = 0;
On 2015/10/07 16:44:38, pthatcher1 wrote:
> Can you put a comment explaining what these do, when they should be called,
and
> a TODO that we plan to remove them?

Done.

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:352:
media_controller_->ConnectTransportChannel(tc);
On 2015/10/07 16:44:38, pthatcher1 wrote:
> This isn't quite the right place to put this, since it will be called every
time
> bundle configuration changes.  I think it would be better to put it in
> WebRtcSession::CreateChannels after the BaseChannels are created.  At that
> point, get the transport channel from the audio and video channels.

Ok, that won't cause problems if the transport channel changes somehow? Seems
like since we have BaseChannel::SetTransport we may create new transport
channels after WebRtcSession::CreateChannels has returned?

I guess if I connect the two after the call to EnableBundle we should be fine?

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:361:
media_controller_->DisconnectTransportChannel(tc);
On 2015/10/07 16:55:25, stefan-webrtc (holmer) wrote:
> On 2015/10/07 16:44:38, pthatcher1 wrote:
> > Woh, this won't work.  If two BaseChannels share a TransportChannel and one
of
> > them is deleted, it will disconnect, and then the media controller will also
> > disconnect.
> > 
> > I think the easiest thing is to just not have a DisconnectTransportChannel
> > method.  The worst thing that will happen is that MediaController will have
> > dangling pointers in transport_channels_.  But as long as we document that,
I
> > think it's fine.  
> 
> Very good point. I will look at these comments later today or tomorrow.

I assume media controller is guaranteed to outlive transport channels then?

[commit-bot: I haz the power, 2015-10-08 13:00:31]

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

[commit-bot: I haz the power, 2015-10-08 13:00:32]

Dry run: Try jobs failed on following builders:
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/10015)

[stefan-webrtc, 2015-10-08 13:51:45]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-08 13:52:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/300001

[commit-bot: I haz the power, 2015-10-08 13:56:50]

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

[commit-bot: I haz the power, 2015-10-08 13:56:54]

Dry run: Try jobs failed on following builders:
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/10017)

[stefan-webrtc, 2015-10-08 15:17:23]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-08 15:17:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/340001

[commit-bot: I haz the power, 2015-10-08 15:21:18]

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

[commit-bot: I haz the power, 2015-10-08 15:21:18]

Dry run: Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/2509)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/4809)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/4745)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/9943)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/8558)

[stefan-webrtc, 2015-10-08 15:32:06]

iOS deps fixed.

[stefan-webrtc, 2015-10-08 15:52:53]

More ios deps

[pthatcher1, 2015-10-08 20:27:55]

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:352:
media_controller_->ConnectTransportChannel(tc);
On 2015/10/08 12:53:45, stefan-webrtc (holmer) wrote:
> On 2015/10/07 16:44:38, pthatcher1 wrote:
> > This isn't quite the right place to put this, since it will be called every
> time
> > bundle configuration changes.  I think it would be better to put it in
> > WebRtcSession::CreateChannels after the BaseChannels are created.  At that
> > point, get the transport channel from the audio and video channels.
> 
> Ok, that won't cause problems if the transport channel changes somehow? Seems
> like since we have BaseChannel::SetTransport we may create new transport
> channels after WebRtcSession::CreateChannels has returned?
> 
> I guess if I connect the two after the call to EnableBundle we should be fine?

You can connect before or after EnableBundle.  The only way the transport
channel can change is if it changes to a transport channel from another
BaseChannel.   

That does mean we should worry about the case where they are bundled onto the
BaseChannel for data (the DataChannel).  In that case, you'd need to either
listen to the TransportChannel of the DataChannel or do it post-BUNDLE.  But
BUNDLE can happen at any time, so that's not really safe to rely on.

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:361:
media_controller_->DisconnectTransportChannel(tc);
On 2015/10/08 12:53:45, stefan-webrtc (holmer) wrote:
> On 2015/10/07 16:55:25, stefan-webrtc (holmer) wrote:
> > On 2015/10/07 16:44:38, pthatcher1 wrote:
> > > Woh, this won't work.  If two BaseChannels share a TransportChannel and
one
> of
> > > them is deleted, it will disconnect, and then the media controller will
also
> > > disconnect.
> > > 
> > > I think the easiest thing is to just not have a DisconnectTransportChannel
> > > method.  The worst thing that will happen is that MediaController will
have
> > > dangling pointers in transport_channels_.  But as long as we document
that,
> I
> > > think it's fine.  
> > 
> > Very good point. I will look at these comments later today or tomorrow.
> 
> I assume media controller is guaranteed to outlive transport channels then?

Yes, a MediaController has to outlive all the BaseChannels and once all the
BaseChannels are destroyed, then the TransportChannels are as well.

However, that is a bit subtle and probably easy to screw up in the future.  We
could fix it, but it would be a bit complex.  Now that I read your code in
WebRtcSession, I realize that it probably makes more sense to use WebRtcSession
as our temporary bind point and not MediaController.  I apologize for jumping
around on the design so much on this, but I think this will be a lot better.
Something like this:

1.  WebRtcSession connects to TransportChannel::OnSentPacket in three places:
  A.  At the end of CreateVoiceChannel.
  B.  At the end of CreateVideoChannel.
  C.  At the end of CreateDataChannel.
2.  WebRtcSession never disconnects from TransportChannel::OnSentPacket because
the session is guaranteed to outlive the TransportChannel since it owns the
TransportController.
3.  WebRtcSession::OnSentPacket_w can then call
media_controller_->OnSentPacket() or
media_controller_->call_w()->OnSentPacket(), whichever is easier.


This way, it's safer and the MediaController doesn't have to tack the
TransportChannel lifetimes.  


Sorry again for the jumping around to different designs.  But it looks like we
can end up in a good state.

https://codereview.webrtc.org/1363573002/diff/400001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1363573002/diff/400001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1824: if (voice_channel_created) {
It's not necessary to put this after the bundle code, since bundle may be
inactive at this point and then activated much later.  I think it would be more
simple to remove the "created" bools and just put the
ConnectToSignalSentPacket_w in the CreateVideoChannel and CreateVoiceChannel
methods.

Furthermore, we need to do one of the following:

A.  Call ConnectToSignalSentPacket for the DataChannel.
B.  Call ConnectToSignalSentPacket at the end of EnableBundle for both the
VoiceChannel and the VideoChannel. 

Otherwise if audio and video got bundled over the data channel's transport, the
signal wouldn't be hooked up.  In fact, we should probably have a unit test for
that scenario.

[stefan-webrtc, 2015-10-09 14:18:34]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[stefan-webrtc, 2015-10-09 14:18:45]

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:352:
media_controller_->ConnectTransportChannel(tc);
On 2015/10/08 20:27:55, pthatcher1 wrote:
> On 2015/10/08 12:53:45, stefan-webrtc (holmer) wrote:
> > On 2015/10/07 16:44:38, pthatcher1 wrote:
> > > This isn't quite the right place to put this, since it will be called
every
> > time
> > > bundle configuration changes.  I think it would be better to put it in
> > > WebRtcSession::CreateChannels after the BaseChannels are created.  At that
> > > point, get the transport channel from the audio and video channels.
> > 
> > Ok, that won't cause problems if the transport channel changes somehow?
Seems
> > like since we have BaseChannel::SetTransport we may create new transport
> > channels after WebRtcSession::CreateChannels has returned?
> > 
> > I guess if I connect the two after the call to EnableBundle we should be
fine?
> 
> You can connect before or after EnableBundle.  The only way the transport
> channel can change is if it changes to a transport channel from another
> BaseChannel.   
> 
> That does mean we should worry about the case where they are bundled onto the
> BaseChannel for data (the DataChannel).  In that case, you'd need to either
> listen to the TransportChannel of the DataChannel or do it post-BUNDLE.  But
> BUNDLE can happen at any time, so that's not really safe to rely on.

Acknowledged.

https://codereview.webrtc.org/1363573002/diff/180001/talk/session/media/chann...
talk/session/media/channel.cc:361:
media_controller_->DisconnectTransportChannel(tc);
On 2015/10/08 20:27:55, pthatcher1 wrote:
> On 2015/10/08 12:53:45, stefan-webrtc (holmer) wrote:
> > On 2015/10/07 16:55:25, stefan-webrtc (holmer) wrote:
> > > On 2015/10/07 16:44:38, pthatcher1 wrote:
> > > > Woh, this won't work.  If two BaseChannels share a TransportChannel and
> one
> > of
> > > > them is deleted, it will disconnect, and then the media controller will
> also
> > > > disconnect.
> > > > 
> > > > I think the easiest thing is to just not have a
DisconnectTransportChannel
> > > > method.  The worst thing that will happen is that MediaController will
> have
> > > > dangling pointers in transport_channels_.  But as long as we document
> that,
> > I
> > > > think it's fine.  
> > > 
> > > Very good point. I will look at these comments later today or tomorrow.
> > 
> > I assume media controller is guaranteed to outlive transport channels then?
> 
> Yes, a MediaController has to outlive all the BaseChannels and once all the
> BaseChannels are destroyed, then the TransportChannels are as well.
> 
> However, that is a bit subtle and probably easy to screw up in the future.  We
> could fix it, but it would be a bit complex.  Now that I read your code in
> WebRtcSession, I realize that it probably makes more sense to use
WebRtcSession
> as our temporary bind point and not MediaController.  I apologize for jumping
> around on the design so much on this, but I think this will be a lot better.
> Something like this:
> 
> 1.  WebRtcSession connects to TransportChannel::OnSentPacket in three places:
>   A.  At the end of CreateVoiceChannel.
>   B.  At the end of CreateVideoChannel.
>   C.  At the end of CreateDataChannel.
> 2.  WebRtcSession never disconnects from TransportChannel::OnSentPacket
because
> the session is guaranteed to outlive the TransportChannel since it owns the
> TransportController.
> 3.  WebRtcSession::OnSentPacket_w can then call
> media_controller_->OnSentPacket() or
> media_controller_->call_w()->OnSentPacket(), whichever is easier.
> 
> 
> This way, it's safer and the MediaController doesn't have to tack the
> TransportChannel lifetimes.  
> 
> 
> Sorry again for the jumping around to different designs.  But it looks like we
> can end up in a good state.

No problem, I prefer to do the right thing here and simplify for the future
improvements you have planned. And of course it would be better to avoid random
crashes... :) This is difficult to get right, and I wouldn't have been able to
do it in twice the time without your help.

I managed to get this working, and with a bit of code reading I managed to write
a test.

https://codereview.webrtc.org/1363573002/diff/400001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1363573002/diff/400001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1824: if (voice_channel_created) {
On 2015/10/08 20:27:55, pthatcher1 wrote:
> It's not necessary to put this after the bundle code, since bundle may be
> inactive at this point and then activated much later.  I think it would be
more
> simple to remove the "created" bools and just put the
> ConnectToSignalSentPacket_w in the CreateVideoChannel and CreateVoiceChannel
> methods.
> 
> Furthermore, we need to do one of the following:
> 
> A.  Call ConnectToSignalSentPacket for the DataChannel.
> B.  Call ConnectToSignalSentPacket at the end of EnableBundle for both the
> VoiceChannel and the VideoChannel. 
> 
> Otherwise if audio and video got bundled over the data channel's transport,
the
> signal wouldn't be hooked up.  In fact, we should probably have a unit test
for
> that scenario.
> 

Right, makes sense.

I didn't figure out a good way of testing this though. I'm not quite sure how to
setup a data channel and then later add video and audio and have them be bundled
at a later stage.

If you could take a look at the test I wrote for the regular case and based on
that give me some suggestions, that would be great. The WebRtcSession code is
fairly unknown territory to me, so figuring out how to write such a test takes a
lot of time. :)

[commit-bot: I haz the power, 2015-10-09 14:18:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/440001

[commit-bot: I haz the power, 2015-10-09 15:15:13]

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

[commit-bot: I haz the power, 2015-10-09 15:15:14]

Dry run: This issue passed the CQ dry run.

[pthatcher1, 2015-10-09 20:57:48]

Looking better.  The only thing I'm worried about now is how we get a fake thing
down into WebRtcSession.  Using a CallFactory is messy.  We might be able to
just pass in a MediaController into WebRtcSession, but that might be too much
refactoring to add on top of this CL.

So here's a third possibility:

1. Move OnSentPacket, as found on webrtc::Call into a  separate interface,
perhaps SentPacketObserver and have webrtc::Call implement it.

2. Add WebRtcSession::RegisterSentPacketObserver.

3. Pass webrtc::call into WebRtcSession::RegisterSentPacketObserver.


Now you don't anything fake.


Actually, this makes me wonder:  would it make sense to put a SentPacketObserver
in PacketOptions, and then just have the sender of the packet call OnSentPacket
on the observer?  Then we wouldn't need this even to be traced all the way
through all the code.  It would just exist in the packet options.  I think the
only thing we would have to worry about is that SentPacketObserver would need to
check which thread it gets called on and then hop threads if it needs to.  That
might be much cleaner after all.

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:78: std::set<cricket::TransportChannel*>
transport_channels_;
You don't need this any more, do you?

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:37: }  // namespace cricket
You don't need this any more, do you?

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:786:
channel_manager_->media_engine()->GetVoE()));
The addition of the CallFactory is really just so WebRtcSession can have a fake
thing that takes a packet, right?  Could we, instead, of have a
MediaControllerFactory passed into WebRtcSession?  Or, better yet, pass in a
MediaController into WebRtcSession instead of a channel_manager.  The
WebRtcSession can get the channel_manager from the MediaController.

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1169: this, &WebRtcSession::OnSentPacket);
Actually, if you listen to all of the BaseChannels (audio, video, and data), you
don't need this.  In fact, it might cause multiple call backs.

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:1228: SetupLoopbackCall();
For testing, this would be more thorough if we made sure it works even if
everything is bundled over the data channel transport.  The easiest way to do
that is to probably change the offer that comes from CreateOffer() in this file
such that offer->GetGroupByName(GROUP_TYPE_BUNDLE)->content_names()[0] ==
CN_DATA.  Right now [0] == CN_AUDIO and [2] == CN_DATA.  You could probably
accomplish that with:

ContentGroup* bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE);
if (bundle) {
  // Re-add audio and and video so that data shows up first.
  if(bundle->RemoveContentName(CN_AUDIO)) {
    bundle->AddContentName(CN_AUDIO);
  }
  if (bundle->RemoveContentName(CN_VIDEO)) {
    bundle->AddContentName(CN_VIDEO);
  }
}

[stefan-webrtc, 2015-10-09 21:27:21]

On 2015/10/09 20:57:48, pthatcher1 wrote:
> Looking better.  The only thing I'm worried about now is how we get a fake
thing
> down into WebRtcSession.  Using a CallFactory is messy.  We might be able to
> just pass in a MediaController into WebRtcSession, but that might be too much
> refactoring to add on top of this CL.
> 
> So here's a third possibility:
> 
> 1. Move OnSentPacket, as found on webrtc::Call into a  separate interface,
> perhaps SentPacketObserver and have webrtc::Call implement it.
> 
> 2. Add WebRtcSession::RegisterSentPacketObserver.
> 
> 3. Pass webrtc::call into WebRtcSession::RegisterSentPacketObserver.
> 
> 
> Now you don't anything fake.
> 
> 
> Actually, this makes me wonder:  would it make sense to put a
SentPacketObserver
> in PacketOptions, and then just have the sender of the packet call
OnSentPacket
> on the observer?  Then we wouldn't need this even to be traced all the way
> through all the code.  It would just exist in the packet options.  I think the
> only thing we would have to worry about is that SentPacketObserver would need
to
> check which thread it gets called on and then hop threads if it needs to. 
That
> might be much cleaner after all.

Not a bad idea, but I'm not completely convinced. The sender of the packet lives
in the browser process when running in Chrome. I have a feeling that it might
become a bit unclear? But maybe if it's well documented it could work.

> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
> File talk/app/webrtc/mediacontroller.cc (right):
> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
> talk/app/webrtc/mediacontroller.cc:78: std::set<cricket::TransportChannel*>
> transport_channels_;
> You don't need this any more, do you?
> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
> File talk/app/webrtc/mediacontroller.h (right):
> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
> talk/app/webrtc/mediacontroller.h:37: }  // namespace cricket
> You don't need this any more, do you?
> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> File talk/app/webrtc/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:786:
> channel_manager_->media_engine()->GetVoE()));
> The addition of the CallFactory is really just so WebRtcSession can have a
fake
> thing that takes a packet, right?  Could we, instead, of have a
> MediaControllerFactory passed into WebRtcSession?  Or, better yet, pass in a
> MediaController into WebRtcSession instead of a channel_manager.  The
> WebRtcSession can get the channel_manager from the MediaController.
> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:1169: this, &WebRtcSession::OnSentPacket);
> Actually, if you listen to all of the BaseChannels (audio, video, and data),
you
> don't need this.  In fact, it might cause multiple call backs.
> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> File talk/app/webrtc/webrtcsession_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession_unittest.cc:1228: SetupLoopbackCall();
> For testing, this would be more thorough if we made sure it works even if
> everything is bundled over the data channel transport.  The easiest way to do
> that is to probably change the offer that comes from CreateOffer() in this
file
> such that offer->GetGroupByName(GROUP_TYPE_BUNDLE)->content_names()[0] ==
> CN_DATA.  Right now [0] == CN_AUDIO and [2] == CN_DATA.  You could probably
> accomplish that with:
> 
> ContentGroup* bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE);
> if (bundle) {
>   // Re-add audio and and video so that data shows up first.
>   if(bundle->RemoveContentName(CN_AUDIO)) {
>     bundle->AddContentName(CN_AUDIO);
>   }
>   if (bundle->RemoveContentName(CN_VIDEO)) {
>     bundle->AddContentName(CN_VIDEO);
>   }
> }

[pthatcher1, 2015-10-09 22:50:33]

On 2015/10/09 21:27:21, stefan-webrtc (holmer) wrote:
> On 2015/10/09 20:57:48, pthatcher1 wrote:
> > Looking better.  The only thing I'm worried about now is how we get a fake
> thing
> > down into WebRtcSession.  Using a CallFactory is messy.  We might be able to
> > just pass in a MediaController into WebRtcSession, but that might be too
much
> > refactoring to add on top of this CL.
> > 
> > So here's a third possibility:
> > 
> > 1. Move OnSentPacket, as found on webrtc::Call into a  separate interface,
> > perhaps SentPacketObserver and have webrtc::Call implement it.
> > 
> > 2. Add WebRtcSession::RegisterSentPacketObserver.
> > 
> > 3. Pass webrtc::call into WebRtcSession::RegisterSentPacketObserver.
> > 
> > 
> > Now you don't anything fake.
> > 
> > 
> > Actually, this makes me wonder:  would it make sense to put a
> SentPacketObserver
> > in PacketOptions, and then just have the sender of the packet call
> OnSentPacket
> > on the observer?  Then we wouldn't need this even to be traced all the way
> > through all the code.  It would just exist in the packet options.  I think
the
> > only thing we would have to worry about is that SentPacketObserver would
need
> to
> > check which thread it gets called on and then hop threads if it needs to. 
> That
> > might be much cleaner after all.
> 
> Not a bad idea, but I'm not completely convinced. The sender of the packet
lives
> in the browser process when running in Chrome. I have a feeling that it might
> become a bit unclear? But maybe if it's well documented it could work.
> 
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
> > File talk/app/webrtc/mediacontroller.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
> > talk/app/webrtc/mediacontroller.cc:78: std::set<cricket::TransportChannel*>
> > transport_channels_;
> > You don't need this any more, do you?
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
> > File talk/app/webrtc/mediacontroller.h (right):
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
> > talk/app/webrtc/mediacontroller.h:37: }  // namespace cricket
> > You don't need this any more, do you?
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> > File talk/app/webrtc/webrtcsession.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> > talk/app/webrtc/webrtcsession.cc:786:
> > channel_manager_->media_engine()->GetVoE()));
> > The addition of the CallFactory is really just so WebRtcSession can have a
> fake
> > thing that takes a packet, right?  Could we, instead, of have a
> > MediaControllerFactory passed into WebRtcSession?  Or, better yet, pass in a
> > MediaController into WebRtcSession instead of a channel_manager.  The
> > WebRtcSession can get the channel_manager from the MediaController.
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> > talk/app/webrtc/webrtcsession.cc:1169: this, &WebRtcSession::OnSentPacket);
> > Actually, if you listen to all of the BaseChannels (audio, video, and data),
> you
> > don't need this.  In fact, it might cause multiple call backs.
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> > File talk/app/webrtc/webrtcsession_unittest.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
> > talk/app/webrtc/webrtcsession_unittest.cc:1228: SetupLoopbackCall();
> > For testing, this would be more thorough if we made sure it works even if
> > everything is bundled over the data channel transport.  The easiest way to
do
> > that is to probably change the offer that comes from CreateOffer() in this
> file
> > such that offer->GetGroupByName(GROUP_TYPE_BUNDLE)->content_names()[0] ==
> > CN_DATA.  Right now [0] == CN_AUDIO and [2] == CN_DATA.  You could probably
> > accomplish that with:
> > 
> > ContentGroup* bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE);
> > if (bundle) {
> >   // Re-add audio and and video so that data shows up first.
> >   if(bundle->RemoveContentName(CN_AUDIO)) {
> >     bundle->AddContentName(CN_AUDIO);
> >   }
> >   if (bundle->RemoveContentName(CN_VIDEO)) {
> >     bundle->AddContentName(CN_VIDEO);
> >   }
> > }

I'm not 100% convinced either. 

That's a good point about going over an IPC.  The caller of the IPC would need
to then keep track of the observer and fire it when then IPC comes back.  That
might be annoying.

[stefan-webrtc, 2015-10-10 15:32:52]

I would prefer not changing the approach to attaching a signal to the
PacketOptions, partly because I suspect it may be hard to understand the code
and how the signal works (I guess it will point to undefined memory when the
PacketOptions is copied over the IPC?). Going down a completely new path may
also reveal other complications...

Some questions / suggestions in the comments.

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:78: std::set<cricket::TransportChannel*>
transport_channels_;
On 2015/10/09 20:57:48, pthatcher1 wrote:
> You don't need this any more, do you?

Done.

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:37: }  // namespace cricket
On 2015/10/09 20:57:48, pthatcher1 wrote:
> You don't need this any more, do you?

Done.

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:786:
channel_manager_->media_engine()->GetVoE()));
On 2015/10/09 20:57:48, pthatcher1 wrote:
> The addition of the CallFactory is really just so WebRtcSession can have a
fake
> thing that takes a packet, right?  Could we, instead, of have a
> MediaControllerFactory passed into WebRtcSession?  Or, better yet, pass in a
> MediaController into WebRtcSession instead of a channel_manager.  The
> WebRtcSession can get the channel_manager from the MediaController.  

That might be an option, but after a quick look it seems to require a bit of
work to move the channel manager into the media controller. I'm a bit afraid
that this cl would start growing out of hand if I go down that path. Maybe a we
can settle with a MediaControllerFactory for now?

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1169: this, &WebRtcSession::OnSentPacket);
On 2015/10/09 20:57:48, pthatcher1 wrote:
> Actually, if you listen to all of the BaseChannels (audio, video, and data),
you
> don't need this.  In fact, it might cause multiple call backs.

Ah ok. I misunderstood you and thought that for instance a data channel could be
added later into session, requiring this.

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:1228: SetupLoopbackCall();
On 2015/10/09 20:57:48, pthatcher1 wrote:
> For testing, this would be more thorough if we made sure it works even if
> everything is bundled over the data channel transport.  The easiest way to do
> that is to probably change the offer that comes from CreateOffer() in this
file
> such that offer->GetGroupByName(GROUP_TYPE_BUNDLE)->content_names()[0] ==
> CN_DATA.  Right now [0] == CN_AUDIO and [2] == CN_DATA.  You could probably
> accomplish that with:
> 
> ContentGroup* bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE);
> if (bundle) {
>   // Re-add audio and and video so that data shows up first.
>   if(bundle->RemoveContentName(CN_AUDIO)) {
>     bundle->AddContentName(CN_AUDIO);
>   }
>   if (bundle->RemoveContentName(CN_VIDEO)) {
>     bundle->AddContentName(CN_VIDEO);
>   }
> }

I don't think that method works, since the ContentGroup returned by
SessionDescription::GetGroupByName is const. I tried building a group from
scratch instead, but it seems like my offer doesn't contain a data channel. I'm
currently stuck trying to offer both audio, video and data as I can't seem to
find any examples doing that in this file, and data doesn't seem to be added in
the same way audio and video is?

Any clues? :)

[stefan-webrtc, 2015-10-12 09:51:37]

Pass a media controller to webrtc session.

[stefan-webrtc, 2015-10-12 10:04:46]

Call OnSentPacket on worker thread.

[stefan-webrtc, 2015-10-12 10:11:14]

Cleanup

[stefan-webrtc, 2015-10-12 10:11:45]

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1363573002/diff/440001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:786:
channel_manager_->media_engine()->GetVoE()));
On 2015/10/10 15:32:51, stefan-webrtc (holmer) wrote:
> On 2015/10/09 20:57:48, pthatcher1 wrote:
> > The addition of the CallFactory is really just so WebRtcSession can have a
> fake
> > thing that takes a packet, right?  Could we, instead, of have a
> > MediaControllerFactory passed into WebRtcSession?  Or, better yet, pass in a
> > MediaController into WebRtcSession instead of a channel_manager.  The
> > WebRtcSession can get the channel_manager from the MediaController.  
> 
> That might be an option, but after a quick look it seems to require a bit of
> work to move the channel manager into the media controller. I'm a bit afraid
> that this cl would start growing out of hand if I go down that path. Maybe a
we
> can settle with a MediaControllerFactory for now?

I checked this again today and it turned out not to touch as much code as I
initially thought -- so done!

[stefan-webrtc, 2015-10-12 10:12:16]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-12 10:12:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/520001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/520001

[commit-bot: I haz the power, 2015-10-12 11:30:35]

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

[commit-bot: I haz the power, 2015-10-12 11:30:36]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2015-10-12 20:48:00]

glanced through the libjingle stuff (it scares me), reviewed the rest

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
File webrtc/base/asyncudpsocket.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
webrtc/base/asyncudpsocket.cc:63: int ret = socket_->Send(pv, cb);
This has the downside of you technically being able to receive a response from a
well-behaving client before SignalSentPacket gets triggered. Do you want to take
the timestamp before Send()? That has other downsides though (such as not
accounting for time within Send()).

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:519: ASSERT(channel == channel_);
Is this assert really good? If this can't be changed in runtime while things are
in the queue then what's the point of it being mutable?

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/video_engine/vie_...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/video_engine/vie_...
webrtc/video_engine/vie_channel_group.cc:442: if
(transport_feedback_adapter_.get() != nullptr) {
I think you can do if (transport_feedback_adapter_)

[pthatcher1, 2015-10-13 05:31:13]

https://codereview.webrtc.org/1363573002/diff/520001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1363573002/diff/520001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:111:
rtc::scoped_ptr<webrtc::MediaControllerInterface> media_controller_;
I think there's a problem here.  A MediaController (and a webrtc::Call) is a
per-PeerConnection object.  But a PeerConnectionFactory (and a ChannelManager)
is a cross-PeerConnection object.

Instead of factory->media_controller(), we should maybe have
factory->CreateMediaController().

https://codereview.webrtc.org/1363573002/diff/520001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:2199:
media_controller_->call_w()->OnSentPacket(sent_packet);
Two things:

1.  If you hop to the worker thread, shouldn't you remove the call to
OnSentPacket here?  Otherwise, you have two calls.

2.  Shouldn't OnSentPacket always happen on the worker thread?  A
TransportChannel lives on the worker thread and is only accessed via the worker
thread.  It shouldn't fire this except on the worker thread.  So I don't see why
the extra thread hop is needed.

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
File webrtc/base/asyncudpsocket.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
webrtc/base/asyncudpsocket.cc:63: int ret = socket_->Send(pv, cb);
On 2015/10/12 20:48:00, pbos-webrtc wrote:
> This has the downside of you technically being able to receive a response from
a
> well-behaving client before SignalSentPacket gets triggered. Do you want to
take
> the timestamp before Send()? That has other downsides though (such as not
> accounting for time within Send()).

What do you mean "receive a response"?  Do you mean a packet from the remote
side sent back in response to the packet sent here?  I think whatever algorithm
uses these timestamps at the end of the day should take into account that the
timestamp might be after.   That's just how things goes with concurrent
operations.

However, if you really want to be safe with what we stick in SignalSentPacket,
we could put in two timestamps:  before and after.  Then the algorithm that uses
the timestamps could always pick which one it wants and even detect when there's
a large gap.

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:519: ASSERT(channel == channel_);
On 2015/10/12 20:48:00, pbos-webrtc wrote:
> Is this assert really good? If this can't be changed in runtime while things
are
> in the queue then what's the point of it being mutable?

channel_ never changes, and never should.  I believe we could mark it const.

[stefan-webrtc, 2015-10-13 08:09:07]

Comments addressed

[stefan-webrtc, 2015-10-13 08:09:20]

https://codereview.webrtc.org/1363573002/diff/520001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1363573002/diff/520001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:111:
rtc::scoped_ptr<webrtc::MediaControllerInterface> media_controller_;
On 2015/10/13 05:31:13, pthatcher1 wrote:
> I think there's a problem here.  A MediaController (and a webrtc::Call) is a
> per-PeerConnection object.  But a PeerConnectionFactory (and a ChannelManager)
> is a cross-PeerConnection object.
> 
> Instead of factory->media_controller(), we should maybe have
> factory->CreateMediaController().

Yes, sorry about that... I must have stopped thinking.

https://codereview.webrtc.org/1363573002/diff/520001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:2199:
media_controller_->call_w()->OnSentPacket(sent_packet);
On 2015/10/13 05:31:13, pthatcher1 wrote:
> Two things:
> 
> 1.  If you hop to the worker thread, shouldn't you remove the call to
> OnSentPacket here?  Otherwise, you have two calls.
> 
> 2.  Shouldn't OnSentPacket always happen on the worker thread?  A
> TransportChannel lives on the worker thread and is only accessed via the
worker
> thread.  It shouldn't fire this except on the worker thread.  So I don't see
why
> the extra thread hop is needed.

You are right, it should. Keeping OnSentPacket_w.

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
File webrtc/base/asyncudpsocket.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
webrtc/base/asyncudpsocket.cc:63: int ret = socket_->Send(pv, cb);
On 2015/10/13 05:31:13, pthatcher1 wrote:
> On 2015/10/12 20:48:00, pbos-webrtc wrote:
> > This has the downside of you technically being able to receive a response
from
> a
> > well-behaving client before SignalSentPacket gets triggered. Do you want to
> take
> > the timestamp before Send()? That has other downsides though (such as not
> > accounting for time within Send()).
> 
> What do you mean "receive a response"?  Do you mean a packet from the remote
> side sent back in response to the packet sent here?  I think whatever
algorithm
> uses these timestamps at the end of the day should take into account that the
> timestamp might be after.   That's just how things goes with concurrent
> operations.
> 
> However, if you really want to be safe with what we stick in SignalSentPacket,
> we could put in two timestamps:  before and after.  Then the algorithm that
uses
> the timestamps could always pick which one it wants and even detect when
there's
> a large gap.

I'll take the timestamp before Send() as that is what I'm doing in Chrome,
otherwise I agree with pthatcher.

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:519: ASSERT(channel == channel_);
On 2015/10/13 05:31:13, pthatcher1 wrote:
> On 2015/10/12 20:48:00, pbos-webrtc wrote:
> > Is this assert really good? If this can't be changed in runtime while things
> are
> > in the queue then what's the point of it being mutable?
> 
> channel_ never changes, and never should.  I believe we could mark it const.

Done.

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/video_engine/vie_...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/video_engine/vie_...
webrtc/video_engine/vie_channel_group.cc:442: if
(transport_feedback_adapter_.get() != nullptr) {
On 2015/10/12 20:48:00, pbos-webrtc wrote:
> I think you can do if (transport_feedback_adapter_)

Done.

[stefan-webrtc, 2015-10-13 08:15:10]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-13 08:15:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/540001

[pthatcher1, 2015-10-15 05:19:52]

lgtm

[stefan-webrtc, 2015-10-15 06:53:06]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-15 06:53:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/560001

[commit-bot: I haz the power, 2015-10-15 06:54:24]

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

[commit-bot: I haz the power, 2015-10-15 06:54:25]

Dry run: Try jobs failed on following builders:
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/4897)
  linux_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)

[stefan-webrtc, 2015-10-15 08:27:06]

Rebase

[stefan-webrtc, 2015-10-15 08:27:54]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-15 08:28:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363573002/580001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363573002/580001

[stefan-webrtc, 2015-10-15 09:17:25]

Cleanups after merge

[pbos-webrtc, 2015-10-15 09:51:50]

lgtm

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
File webrtc/base/asyncudpsocket.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
webrtc/base/asyncudpsocket.cc:63: int ret = socket_->Send(pv, cb);
On 2015/10/13 08:09:20, stefan-webrtc (holmer) wrote:
> On 2015/10/13 05:31:13, pthatcher1 wrote:
> > On 2015/10/12 20:48:00, pbos-webrtc wrote:
> > > This has the downside of you technically being able to receive a response
> from
> > a
> > > well-behaving client before SignalSentPacket gets triggered. Do you want
to
> > take
> > > the timestamp before Send()? That has other downsides though (such as not
> > > accounting for time within Send()).
> > 
> > What do you mean "receive a response"?  Do you mean a packet from the remote
> > side sent back in response to the packet sent here?  I think whatever
> algorithm
> > uses these timestamps at the end of the day should take into account that
the
> > timestamp might be after.   That's just how things goes with concurrent
> > operations.
> > 
> > However, if you really want to be safe with what we stick in
SignalSentPacket,
> > we could put in two timestamps:  before and after.  Then the algorithm that
> uses
> > the timestamps could always pick which one it wants and even detect when
> there's
> > a large gap.
> 
> I'll take the timestamp before Send() as that is what I'm doing in Chrome,
> otherwise I agree with pthatcher.

Yeah, you'd need the signal to be sent before sending the packet for this to
work that way. That'd possibly add latency, so I'd avoid that.

https://codereview.webrtc.org/1363573002/diff/600001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/1363573002/diff/600001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:228: TransportChannelImpl* const
Comment at previous line

[stefan-webrtc, 2015-10-15 10:09:32]

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
File webrtc/base/asyncudpsocket.cc (right):

https://codereview.webrtc.org/1363573002/diff/520001/webrtc/base/asyncudpsock...
webrtc/base/asyncudpsocket.cc:63: int ret = socket_->Send(pv, cb);
On 2015/10/15 09:51:49, pbos-webrtc wrote:
> On 2015/10/13 08:09:20, stefan-webrtc (holmer) wrote:
> > On 2015/10/13 05:31:13, pthatcher1 wrote:
> > > On 2015/10/12 20:48:00, pbos-webrtc wrote:
> > > > This has the downside of you technically being able to receive a
response
> > from
> > > a
> > > > well-behaving client before SignalSentPacket gets triggered. Do you want
> to
> > > take
> > > > the timestamp before Send()? That has other downsides though (such as
not
> > > > accounting for time within Send()).
> > > 
> > > What do you mean "receive a response"?  Do you mean a packet from the
remote
> > > side sent back in response to the packet sent here?  I think whatever
> > algorithm
> > > uses these timestamps at the end of the day should take into account that
> the
> > > timestamp might be after.   That's just how things goes with concurrent
> > > operations.
> > > 
> > > However, if you really want to be safe with what we stick in
> SignalSentPacket,
> > > we could put in two timestamps:  before and after.  Then the algorithm
that
> > uses
> > > the timestamps could always pick which one it wants and even detect when
> > there's
> > > a large gap.
> > 
> > I'll take the timestamp before Send() as that is what I'm doing in Chrome,
> > otherwise I agree with pthatcher.
> 
> Yeah, you'd need the signal to be sent before sending the packet for this to
> work that way. That'd possibly add latency, so I'd avoid that.

Yes, agree.

https://codereview.webrtc.org/1363573002/diff/600001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransportchannel.h (right):

https://codereview.webrtc.org/1363573002/diff/600001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:228: TransportChannelImpl* const
On 2015/10/15 09:51:50, pbos-webrtc wrote:
> Comment at previous line

Done.

[stefan-webrtc, 2015-10-15 10:09:36]

Fixed comment.

[stefan-webrtc, 2015-10-15 13:37:08]

Add missing updated_options

[stefan-webrtc, 2015-10-15 13:39:25]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-10-15 13:39:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1363573002/#ps640001 (title:
"Add missing updated_options")

[commit-bot: I haz the power, 2015-10-15 13:39:44]

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

[commit-bot: I haz the power, 2015-10-15 13:44:36]

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

[commit-bot: I haz the power, 2015-10-15 13:44:37]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1241)

[stefan-webrtc, 2015-10-15 13:47:09]

stefan@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[stefan-webrtc, 2015-10-15 13:47:10]

...and one OWNER missing.

+mflodman for reviewing the root of webrtc/

[stefan-webrtc, 2015-10-15 13:49:43]

On 2015/10/15 13:47:10, stefan-webrtc (holmer) wrote:
> ...and one OWNER missing.
> 
> +mflodman for reviewing the root of webrtc/

Files in need of review are:
webrtc/call.h
webrtc/config.cc
webrtc/transport.h

[mflodman, 2015-10-15 13:58:22]

https://codereview.webrtc.org/1363573002/diff/640001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1363573002/diff/640001/webrtc/call.h#newcode19
webrtc/call.h:19: #include "webrtc/base/socket.h"
Is this really needed in this header?

It would be good to not include these "itnernal" files in an API file like this.

[mflodman, 2015-10-15 14:05:58]

lgtm

[stefan-webrtc, 2015-10-15 14:08:51]

https://codereview.webrtc.org/1363573002/diff/640001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1363573002/diff/640001/webrtc/call.h#newcode19
webrtc/call.h:19: #include "webrtc/base/socket.h"
On 2015/10/15 13:58:22, mflodman wrote:
> Is this really needed in this header?
> 
> It would be good to not include these "itnernal" files in an API file like
this.

Keeping as is for now. Chrome already depends on socket.h, so it's not really
internal. This will however, as you pointed out, force other users of Call to
include socket.h, which may be bad. In that case we will follow-up with a CL to
move it into a different or its own .h file.

[stefan-webrtc, 2015-10-15 14:09:09]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2015-10-15 14:09:30]

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

[commit-bot: I haz the power, 2015-10-15 14:26:13]

Committed patchset #31 (id:640001)

[commit-bot: I haz the power, 2015-10-15 14:26:29]

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