Wire up RTP keep-alive in ortc api.

Wire up RTP keep-alive in ortc api.

[This CL is work in progress.]

Wire up the rtp keep-alive in webrtc::Call::Config using new
SetRtpTransportParameters() method on RtpTransportInterface.

BUG=webrtc:7907
Review-Url: https://codereview.webrtc.org/2981513002
Cr-Commit-Position: refs/heads/master@{#19287}
Committed: https://chromium.googlesource.com/external/webrtc/+/db2a9fc6ec119052139d42f8897412bba7fa848b

[pthatcher1, 2017-07-11 18:25:09]

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

[pthatcher1, 2017-07-11 18:25:09]

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
webrtc/api/ortc/rtptransportinterface.h:58: RtpKeepAliveConfig keepalive_config;
Can we just call this "keepalive" (or keep_alive?)?

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters
GetRtpTransportParameters() const = 0;
I think we should merge these into one set of get/set:

SetParameters(RtpTransportParameters)
RtpTransportParameters GetParameters()


Where RtpTransportParameters has RtcpParameters:

struct RtpTrnsportParameters {
  RtcpParameters rtcp;
  RtpKeepAliveParameters keepalive;
}

[sprang_webrtc, 2017-07-11 19:54:05]

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
webrtc/api/ortc/rtptransportinterface.h:58: RtpKeepAliveConfig keepalive_config;
On 2017/07/11 18:25:09, pthatcher1 wrote:
> Can we just call this "keepalive" (or keep_alive?)?

Sure.

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters
GetRtpTransportParameters() const = 0;
On 2017/07/11 18:25:09, pthatcher1 wrote:
> I think we should merge these into one set of get/set:
> 
> SetParameters(RtpTransportParameters)
> RtpTransportParameters GetParameters()
> 
> 
> Where RtpTransportParameters has RtcpParameters:
> 
> struct RtpTrnsportParameters {
>   RtcpParameters rtcp;
>   RtpKeepAliveParameters keepalive;
> }

I pondered that, but then we're mixing a transport level feature with a
stream-level one. It got strange to be forced to pass along unused
RtcpParameters in places were just the RtpTransportParameters made sense.
Having a third "RtpRtcpParameters" or whatever struct joining these two also
feels a bit overkill.

[sprang_webrtc, 2017-07-12 14:14:28]

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

[sprang_webrtc, 2017-07-12 14:14:29]

Input appreciated. 
Will start adding tests.

[Taylor Brandstetter, 2017-07-12 16:19:50]

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/call/call.h
File webrtc/call/call.h (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/call/call.h#newcod...
webrtc/call/call.h:209: // only before creating and send streams. Returns true
on success.
I think "and" was meant to be "any" here

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.cc:192:
rtp_transport_controller_->SetRtpTransportParameters(parameters);
Ok, so setting RTP transport parameters on one RtpTransport will affect the
entire transport controller (in other words, the whole Call)? This limitation
should be called out in the applicable header file(s) in webrtc/api/ortc/.

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.h (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.h:88: RtpTransportAdapter(const RtcpParameters&
rtcp_parameters,
Since the constructor (and factory methods) take RtcpParameters, I think they
should take the RtpTransportParameters as well for consistency. Maybe for the
keepalive options it doesn't matter, but it seems likely we could add things to
RtpTransportParameters in the future which we would want to have available at
construction time.

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:140: if
(rtp_transport_parameters_.keepalive.timeout_interval_ms > 0 &&
The parameters in this class don't appear to be used. So it may be better to
leave this method as a TODO and just return an error, so people don't wrongly
assume things are wired up already.

Note that this class is currently only used in the PeerConnection object model
(hence it being in "pc/"), but it's what we plan to eventually use for both the
ORTC and PeerConnection code paths, replacing "RtpTransportAdapter" with it.

[pthatcher1, 2017-07-12 22:45:21]

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters
GetRtpTransportParameters() const = 0;
On 2017/07/11 19:54:05, sprang_webrtc wrote:
> On 2017/07/11 18:25:09, pthatcher1 wrote:
> > I think we should merge these into one set of get/set:
> > 
> > SetParameters(RtpTransportParameters)
> > RtpTransportParameters GetParameters()
> > 
> > 
> > Where RtpTransportParameters has RtcpParameters:
> > 
> > struct RtpTrnsportParameters {
> >   RtcpParameters rtcp;
> >   RtpKeepAliveParameters keepalive;
> > }
> 
> I pondered that, but then we're mixing a transport level feature with a
> stream-level one. It got strange to be forced to pass along unused
> RtcpParameters in places were just the RtpTransportParameters made sense.
> Having a third "RtpRtcpParameters" or whatever struct joining these two also
> feels a bit overkill.

Ah, that's a good point.  I was thrown off by the fact that we use it here as
transport-level parameters.  I presume we will change that in the future to be
per-stream.

In that case, I'm fine with an additional getter/setter.  But can you call it
GetParameters and SetParameters? 
RtpTransport::SetParameters(RtpTransportParameters) makes a lot more sense than
RtpTransport::SetRtpTransportParameters(RtpTransportParameters).

[Taylor Brandstetter, 2017-07-12 23:15:50]

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters
GetRtpTransportParameters() const = 0;
On 2017/07/11 19:54:05, sprang_webrtc wrote:
> On 2017/07/11 18:25:09, pthatcher1 wrote:
> > I think we should merge these into one set of get/set:
> > 
> > SetParameters(RtpTransportParameters)
> > RtpTransportParameters GetParameters()
> > 
> > 
> > Where RtpTransportParameters has RtcpParameters:
> > 
> > struct RtpTrnsportParameters {
> >   RtcpParameters rtcp;
> >   RtpKeepAliveParameters keepalive;
> > }
> 
> I pondered that, but then we're mixing a transport level feature with a
> stream-level one. It got strange to be forced to pass along unused
> RtcpParameters in places were just the RtpTransportParameters made sense.
> Having a third "RtpRtcpParameters" or whatever struct joining these two also
> feels a bit overkill.

Wait, what feature here is stream-level? The keepalives? But I thought the
reason we're putting it here (as opposed to on RtpSender) is that it's
conceptually transport-level.

I'd generally agree with Peter about merging the two structs. It's more
consistent with the ORTC programming model, where if you need to change
something you're expected to do "getParameters, modify  stuff, setParameters".
And all the objects defined thus far have used a single "parameters" structure.

This seems sensible to me, and consistent with other things:

struct RtpTransportParameters {
  RtcpParameters rtcp;
  RtpKeepAliveConfig keepalive_config;
};

[pthatcher1, 2017-07-12 23:49:57]

On 2017/07/12 23:15:50, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
> File webrtc/api/ortc/rtptransportinterface.h (right):
> 
>
https://codereview.webrtc.org/2981513002/diff/20001/webrtc/api/ortc/rtptransp...
> webrtc/api/ortc/rtptransportinterface.h:104: virtual RtpTransportParameters
> GetRtpTransportParameters() const = 0;
> On 2017/07/11 19:54:05, sprang_webrtc wrote:
> > On 2017/07/11 18:25:09, pthatcher1 wrote:
> > > I think we should merge these into one set of get/set:
> > > 
> > > SetParameters(RtpTransportParameters)
> > > RtpTransportParameters GetParameters()
> > > 
> > > 
> > > Where RtpTransportParameters has RtcpParameters:
> > > 
> > > struct RtpTrnsportParameters {
> > >   RtcpParameters rtcp;
> > >   RtpKeepAliveParameters keepalive;
> > > }
> > 
> > I pondered that, but then we're mixing a transport level feature with a
> > stream-level one. It got strange to be forced to pass along unused
> > RtcpParameters in places were just the RtpTransportParameters made sense.
> > Having a third "RtpRtcpParameters" or whatever struct joining these two also
> > feels a bit overkill.
> 
> Wait, what feature here is stream-level? The keepalives? But I thought the
> reason we're putting it here (as opposed to on RtpSender) is that it's
> conceptually transport-level.

The RtcpParameters.  

> 
> I'd generally agree with Peter about merging the two structs. It's more
> consistent with the ORTC programming model, where if you need to change
> something you're expected to do "getParameters, modify  stuff, setParameters".
> And all the objects defined thus far have used a single "parameters"
structure.
> 
> This seems sensible to me, and consistent with other things:
> 
> struct RtpTransportParameters {
>   RtcpParameters rtcp;
>   RtpKeepAliveConfig keepalive_config;
> };

[pthatcher1, 2017-07-12 23:57:09]

I just talked to Taylor about it, and he corrected me that RtcpParameters are
transport-level.  And since this new keep alive thing is transport-level.  So
nothing is stream-level, and I go back to my original argument that we should
combine them into one RtpTransportParameter that combines RTCP and RTP keep
alives and has one getter and setter.

[sprang_webrtc, 2017-07-16 09:34:02]

sprang@webrtc.org changed reviewers:
+ ilnik@webrtc.org

[sprang_webrtc, 2017-07-16 09:34:03]

Moved RtcpParameters into RtpTransportParameters. I also propagate that struct
further. Maybe I took that a bit too far now?

+ilnik, could you work with pthatcher@ and deadbeef@ in getting this into a
landable state while I'm OOO?

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/call/call.h
File webrtc/call/call.h (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/call/call.h#newcod...
webrtc/call/call.h:209: // only before creating and send streams. Returns true
on success.
On 2017/07/12 16:19:50, Taylor Brandstetter wrote:
> I think "and" was meant to be "any" here

Done.

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.cc:192:
rtp_transport_controller_->SetRtpTransportParameters(parameters);
On 2017/07/12 16:19:50, Taylor Brandstetter wrote:
> Ok, so setting RTP transport parameters on one RtpTransport will affect the
> entire transport controller (in other words, the whole Call)? This limitation
> should be called out in the applicable header file(s) in webrtc/api/ortc/.

That was the idea, since the keep-alive is about keeping the underlying
transport (ie udp socket) alive, rather than and individual rtp transport. Hope
I'm not confusing terminology here.
Right now, the implementation is handling the keep-alive on a stream-basis, but
ultimately it would probably be better to do it on a call basis, that's why I
moved the config to webrtc::Call::Config

Do you think it would make more sense to move it back?

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.h (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.h:88: RtpTransportAdapter(const RtcpParameters&
rtcp_parameters,
On 2017/07/12 16:19:50, Taylor Brandstetter wrote:
> Since the constructor (and factory methods) take RtcpParameters, I think they
> should take the RtpTransportParameters as well for consistency. Maybe for the
> keepalive options it doesn't matter, but it seems likely we could add things
to
> RtpTransportParameters in the future which we would want to have available at
> construction time.

Done.

[pthatcher1, 2017-07-17 22:43:27]

lgtm

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/rtptransp...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/rtptransp...
webrtc/api/ortc/rtptransportinterface.h:99: // altering the payload type of
timeout interval after this point is not
of timeout => or timeout

[Taylor Brandstetter, 2017-07-17 23:13:11]

Looks great so far. Just needs some tests:

- Test that the keepalive parameters get applied correctly at the "media engine"
level, using a fake media engine to accomplish this. See for example
"SetRtcpParametersAppliesParametersToMediaEngine".

- Tests for the API contract related to the keepalive parameters, including
testing that errors are thrown for things that are documented as unsupported.
See for example "GetAndSetRtcpParameters".

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.cc:192:
rtp_transport_controller_->SetRtpTransportParameters(parameters);
On 2017/07/16 09:34:03, sprang_webrtc wrote:
> On 2017/07/12 16:19:50, Taylor Brandstetter wrote:
> > Ok, so setting RTP transport parameters on one RtpTransport will affect the
> > entire transport controller (in other words, the whole Call)? This
limitation
> > should be called out in the applicable header file(s) in webrtc/api/ortc/.
> 
> That was the idea, since the keep-alive is about keeping the underlying
> transport (ie udp socket) alive, rather than and individual rtp transport.
Hope
> I'm not confusing terminology here.
> Right now, the implementation is handling the keep-alive on a stream-basis,
but
> ultimately it would probably be better to do it on a call basis, that's why I
> moved the config to webrtc::Call::Config
> 
> Do you think it would make more sense to move it back?

We may be confusing terminology.

Here's what the objects in this API represent, conceptually:

RtpTransport = RTP transport layer on top of one underlying 5-tuple (or ICE
component, if using ICE). Equates to a "media transport" as defined in RFC7656.

RtpTransportController = Controlling multiple RTP transports between the same
pair of "endpoints". Some things, like bandwidth estimation and A/V sync,
operate at this level. Most closely equates to a "multimedia session" as defined
in RFC7656, or a PeerConnection in the WebRTC 1.0 object model.

So "Call" is conceptually more like RtpTransportController. It encompasses
*multiple* underlying transports (when BUNDLE is not used).

There's unfortunately not an object that maps easily to "RtpTransport" at the
"Call" level. The closest thing is RtpStreamReceiverController, but that's only
for the receive side. So, I'd be fine with what you've done, as long as we call
out the limitation. Or you could move the config out of webrtc::Call::Config,
and leave it per-stream for now, until we have a nice "transport" abstraction to
use.

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto...
File webrtc/api/ortc/ortcfactoryinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto...
webrtc/api/ortc/ortcfactoryinterface.h:118: // later through
SetRtpTransportParameters.
The method ended up just being called SetParameters (at Peter's suggestion)

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto...
webrtc/api/ortc/ortcfactoryinterface.h:125: const RtpTransportParameters&
rtcp_parameters,
Need to replace "rtcp_parameters" with "rtp_parameters" here and elsewhere.

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/ortc/rtptransportc...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/ortc/rtptransportc...
webrtc/ortc/rtptransportcontrolleradapter.h:105: RTCError
SetRtpTransportParameters(const RtpTransportParameters& parameters);
Do we need two methods here or just one?

[ilnik, 2017-07-21 12:21:19]

I have implemented the comments.

Now working on tests - there are some problems.
How can I on a level of rtptransport_unittest force some streams to be created?
I've experimented around, but somehow, no streams are created.

Also, I can't upload anything to this issue, so my patch is there:
https://codereview.webrtc.org/2984793002

I've added you both as reviewers there too.

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.cc:192:
rtp_transport_controller_->SetRtpTransportParameters(parameters);
On 2017/07/17 23:13:11, Taylor Brandstetter wrote:
> On 2017/07/16 09:34:03, sprang_webrtc wrote:
> > On 2017/07/12 16:19:50, Taylor Brandstetter wrote:
> > > Ok, so setting RTP transport parameters on one RtpTransport will affect
the
> > > entire transport controller (in other words, the whole Call)? This
> limitation
> > > should be called out in the applicable header file(s) in webrtc/api/ortc/.
> > 
> > That was the idea, since the keep-alive is about keeping the underlying
> > transport (ie udp socket) alive, rather than and individual rtp transport.
> Hope
> > I'm not confusing terminology here.
> > Right now, the implementation is handling the keep-alive on a stream-basis,
> but
> > ultimately it would probably be better to do it on a call basis, that's why
I
> > moved the config to webrtc::Call::Config
> > 
> > Do you think it would make more sense to move it back?
> 
> We may be confusing terminology.
> 
> Here's what the objects in this API represent, conceptually:
> 
> RtpTransport = RTP transport layer on top of one underlying 5-tuple (or ICE
> component, if using ICE). Equates to a "media transport" as defined in
RFC7656.
> 
> RtpTransportController = Controlling multiple RTP transports between the same
> pair of "endpoints". Some things, like bandwidth estimation and A/V sync,
> operate at this level. Most closely equates to a "multimedia session" as
defined
> in RFC7656, or a PeerConnection in the WebRTC 1.0 object model.
> 
> So "Call" is conceptually more like RtpTransportController. It encompasses
> *multiple* underlying transports (when BUNDLE is not used).
> 
> There's unfortunately not an object that maps easily to "RtpTransport" at the
> "Call" level. The closest thing is RtpStreamReceiverController, but that's
only
> for the receive side. So, I'd be fine with what you've done, as long as we
call
> out the limitation. Or you could move the config out of webrtc::Call::Config,
> and leave it per-stream for now, until we have a nice "transport" abstraction
to
> use.

Done.

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2981513002/diff/40001/webrtc/pc/rtptransport.cc...
webrtc/pc/rtptransport.cc:140: if
(rtp_transport_parameters_.keepalive.timeout_interval_ms > 0 &&
On 2017/07/12 16:19:50, Taylor Brandstetter wrote:
> The parameters in this class don't appear to be used. So it may be better to
> leave this method as a TODO and just return an error, so people don't wrongly
> assume things are wired up already.
> 
> Note that this class is currently only used in the PeerConnection object model
> (hence it being in "pc/"), but it's what we plan to eventually use for both
the
> ORTC and PeerConnection code paths, replacing "RtpTransportAdapter" with it.

Acknowledged.

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto...
File webrtc/api/ortc/ortcfactoryinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto...
webrtc/api/ortc/ortcfactoryinterface.h:118: // later through
SetRtpTransportParameters.
On 2017/07/17 23:13:11, Taylor Brandstetter wrote:
> The method ended up just being called SetParameters (at Peter's suggestion)

Done.

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/ortcfacto...
webrtc/api/ortc/ortcfactoryinterface.h:125: const RtpTransportParameters&
rtcp_parameters,
On 2017/07/17 23:13:11, Taylor Brandstetter wrote:
> Need to replace "rtcp_parameters" with "rtp_parameters" here and elsewhere.

Done.

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/rtptransp...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/api/ortc/rtptransp...
webrtc/api/ortc/rtptransportinterface.h:99: // altering the payload type of
timeout interval after this point is not
On 2017/07/17 22:43:27, pthatcher1 wrote:
> of timeout => or timeout

Done.

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/ortc/rtptransportc...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2981513002/diff/60001/webrtc/ortc/rtptransportc...
webrtc/ortc/rtptransportcontrolleradapter.h:105: RTCError
SetRtpTransportParameters(const RtpTransportParameters& parameters);
On 2017/07/17 23:13:11, Taylor Brandstetter wrote:
> Do we need two methods here or just one?

I think one method is enough here. Removed SetRtcpParameters and moved it's code
to the SetRtpTransportParameters.

[Taylor Brandstetter, 2017-07-21 17:02:17]

On 2017/07/21 12:21:19, ilnik wrote:
> I have implemented the comments.
> 
> Now working on tests - there are some problems.
> How can I on a level of rtptransport_unittest force some streams to be
created?
> I've experimented around, but somehow, no streams are created.
> 

You need to create an RtpSender prepare it for sending to cause a send stream to
be created. For example:

auto sender = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_AUDIO,
rtp_transport_.get());
ASSERT_NE(nullptr, sender.get());
ASSERT_TRUE(sender->Send(MakeMinimalVp8Parameters().ok());

[ilnik, 2017-07-21 17:33:23]

On 2017/07/21 17:02:17, Taylor Brandstetter wrote:
> On 2017/07/21 12:21:19, ilnik wrote:
> > I have implemented the comments.
> > 
> > Now working on tests - there are some problems.
> > How can I on a level of rtptransport_unittest force some streams to be
> created?
> > I've experimented around, but somehow, no streams are created.
> > 
> 
> You need to create an RtpSender prepare it for sending to cause a send stream
to
> be created. For example:
> 
> auto sender = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_AUDIO,
> rtp_transport_.get());
> ASSERT_NE(nullptr, sender.get());
> ASSERT_TRUE(sender->Send(MakeMinimalVp8Parameters().ok());

Strange... in the test, there's already rtpsender created like that for video
and send called (lines 239-245 in rtptransport_unittest.cc), yet underlying call
have absolutely no streams (then second SetParameters gets down to
rtptransportcontrolleradapter.cc:224, the call_ doesn't have any streams and
keepalive settings are changed).

[Taylor Brandstetter, 2017-07-21 17:59:06]

On 2017/07/21 17:33:23, ilnik wrote:
> On 2017/07/21 17:02:17, Taylor Brandstetter wrote:
> > On 2017/07/21 12:21:19, ilnik wrote:
> > > I have implemented the comments.
> > > 
> > > Now working on tests - there are some problems.
> > > How can I on a level of rtptransport_unittest force some streams to be
> > created?
> > > I've experimented around, but somehow, no streams are created.
> > > 
> > 
> > You need to create an RtpSender prepare it for sending to cause a send
stream
> to
> > be created. For example:
> > 
> > auto sender = ortc_factory_->CreateRtpSender(cricket::MEDIA_TYPE_AUDIO,
> > rtp_transport_.get());
> > ASSERT_NE(nullptr, sender.get());
> > ASSERT_TRUE(sender->Send(MakeMinimalVp8Parameters().ok());
> 
> Strange... in the test, there's already rtpsender created like that for video
> and send called (lines 239-245 in rtptransport_unittest.cc), yet underlying
call
> have absolutely no streams (then second SetParameters gets down to
> rtptransportcontrolleradapter.cc:224, the call_ doesn't have any streams and
> keepalive settings are changed).

Oh, the issue is that we're verifying the application of parameters by using
"FakeMediaEngine". But there's still a real "Call", which ends up not getting
used at all aside from the SetRtpKeepaliveParameters method.

So, we'll need to either:

1. Move the API off of "Call" (which was suggested earlier for other reasons).
2. Add a way to test with a FakeCall. Wouldn't be too much work, just need a new
test-only "OrtcFactory::Create" method.
3. Just leave this untested until we've done more refactoring (pulling RTP stuff
out of Call).

[sprang_webrtc, 2017-08-01 11:50:41]

Had to do some unpleasant hacking to get the test going, but at least it's
working now... ptal

[Taylor Brandstetter, 2017-08-02 00:12:56]

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:228: // SetParameters should set keepalive
for all rtp transports.
nit: Capitalize "RTP"

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:230: TEST_F(RtpTransportTest,
CantChangeKeepAliveAfterCreatedSendStreams) {
From a "unit test" perspective, I'd think of this as testing two things:

1. That Call::SetRtpKeepAliveConfig fails if video send streams are created.
2. That RtpTransport returns INTERNAL_ERROR if Call::SetRtpKeepAliveConfig
fails.

#1 can be tested with a Call unit test, #2 can be tested here using a FakeCall,
but the combination doesn't really belong here, since that's more of an
integration test. Note the comment at the top:

// This test uses fake packet transports and a fake media engine, in order to
// test the RtpTransport at only an API level. Any end-to-end test should go in
// ortcfactory_integrationtest.cc instead.

I think we can avoid the need for this test in the first place though, if we
make RtpTransport responsible for ensuring the keepalive settings don't change,
and return INVALID_MODIFICATION. Then this test could work with just the fake
media engine.

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:240: // Make sure real video channels are
create, so that webrtc::Call code is
nit: "are created"

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:250: // Need to explicitly call
SetParameters() to actually set keepalive.
Even though the keepalive parameters were passed into CreateRtpTransport? That's
pretty unintuitive; can that be fixed?

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:268: 
Is there way to have another test that verifies the keepalive settings reach
Call? See my comment from the previous round of review. "Just leave this
untested until we've done more refactoring" is still a valid option.

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:103: // |parameters.keepalive| will
be set for ALL rtp transports in the call.
nit: Capitalize RTP.

[sprang_webrtc, 2017-08-02 16:45:08]

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:228: // SetParameters should set keepalive
for all rtp transports.
On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> nit: Capitalize "RTP"

Done.

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:230: TEST_F(RtpTransportTest,
CantChangeKeepAliveAfterCreatedSendStreams) {
On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> From a "unit test" perspective, I'd think of this as testing two things:
> 
> 1. That Call::SetRtpKeepAliveConfig fails if video send streams are created.
> 2. That RtpTransport returns INTERNAL_ERROR if Call::SetRtpKeepAliveConfig
> fails.
> 
> #1 can be tested with a Call unit test, #2 can be tested here using a
FakeCall,
> but the combination doesn't really belong here, since that's more of an
> integration test. Note the comment at the top:
> 
> // This test uses fake packet transports and a fake media engine, in order to
> // test the RtpTransport at only an API level. Any end-to-end test should go
in
> // ortcfactory_integrationtest.cc instead.
> 
> I think we can avoid the need for this test in the first place though, if we
> make RtpTransport responsible for ensuring the keepalive settings don't
change,
> and return INVALID_MODIFICATION. Then this test could work with just the fake
> media engine.

I've removed the "real" parts and added validation in
RtpTransportControllerAdapter instead. This is a little clunky, but otherwise we
can't tell if a stream has been created.

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:240: // Make sure real video channels are
create, so that webrtc::Call code is
On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> nit: "are created"

removed

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:250: // Need to explicitly call
SetParameters() to actually set keepalive.
On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> Even though the keepalive parameters were passed into CreateRtpTransport?
That's
> pretty unintuitive; can that be fixed?

Yes, wired that up now.

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:268: 
On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> Is there way to have another test that verifies the keepalive settings reach
> Call? See my comment from the previous round of review. "Just leave this
> untested until we've done more refactoring" is still a valid option.

The previous test with the real stream did (since RtpTransportControllerAdapter
has an actual Call instance), but now it's a little more work.

Do you want an ortc integration test?

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:103: // |parameters.keepalive| will
be set for ALL rtp transports in the call.
On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> nit: Capitalize RTP.

Done.

[Taylor Brandstetter, 2017-08-03 01:18:04]

By the way, sorry if this code is confusing. I tried to make it as clear as I
could, but as mentioned in various comments, it's all hacks on top of
BaseChannel/MediaChannel, so it's going to be difficult to work with until
that's fixed (which should hopefully be some time this year).

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:250: // Need to explicitly call
SetParameters() to actually set keepalive.
On 2017/08/02 16:45:08, sprang_webrtc wrote:
> On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> > Even though the keepalive parameters were passed into CreateRtpTransport?
> That's
> > pretty unintuitive; can that be fixed?
> 
> Yes, wired that up now.

Where is that wired up? I don't see it.

I was thinking the most sensible thing would be to call
RtpTransportControllerAdapter::SetRtpTransportParameters from
RtpTransportAdapter::CreateProxied (bubbling up the error if it fails).

Meaning, if you did:

1. controller = CreateRtpTransportController()
2. transport1 = CreateRtpTransport(keepalive_params_1, controller, ...)
3. sender1 = CreateRtpSender(track1, transport1, ...)
4. sender1.Send(...)
5. transport2 = CreateRtpTransport(keepalive_params_2, controller, ...)

... The second CreateRtpTransport would return an error, because it's implicitly
trying to change the keepalive params for the Call after sending has started.

https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:268: 
On 2017/08/02 16:45:08, sprang_webrtc wrote:
> On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> > Is there way to have another test that verifies the keepalive settings reach
> > Call? See my comment from the previous round of review. "Just leave this
> > untested until we've done more refactoring" is still a valid option.
> 
> The previous test with the real stream did (since
RtpTransportControllerAdapter
> has an actual Call instance), but now it's a little more work.
> 
> Do you want an ortc integration test?

The previous test verifies that the setting can't be changed, but it doesn't
verify that the correct value actually gets applied to the stream. If you don't
think that's worth the effort to test, I'm ok with that.

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans...
webrtc/api/ortc/rtptransportinterface.h:98: // RTP keep-alive settings need to
be set before creating any send-streams,
nit: Someone using this API probably doesn't know what a "send stream" is; maybe
"before an RtpSender has started sending" instead (if that's accurate)?

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.h
File webrtc/ortc/ortcfactory.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory....
webrtc/ortc/ortcfactory.h:47: CreateRtpTransportController(const
RtpTransportParameters& parameters);
We shouldn't add this method; the fact that RtpTransportControllerAdapter knows
about RtpTransportParameters is an implementation detail. From an API
perspective, each RtpTransport has its own RtpTransportParameters, just like how
each RtpSender/RtpReceiver has its own RtpParameters.

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:232:
RTC_DCHECK(call_->SetRtpKeepAliveConfig(parameters.keepalive));
If this is in a DCHECK, it will only be executed in debug builds, right?

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:105: RTCError
SetRtpTransportAndParameters(
Can remove "And" from this name.

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.c...
webrtc/pc/rtptransport.cc:126: "RTP keep-alive parameters not supported by this
channel.");
Why is this message changing?

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport_u...
File webrtc/pc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport_u...
webrtc/pc/rtptransport_unittest.cc:44: TEST(RtpTransportTest,
SetRtpTransportKeepAliveNotSupported) {
Can you add a comment explaining that this is a limitation of the current
implementation, and may be fixed later?

[sprang_webrtc, 2017-08-03 13:06:14]

> By the way, sorry if this code is confusing. I tried to make it as clear as I
> could, but as mentioned in various comments, it's all hacks on top of
> BaseChannel/MediaChannel, so it's going to be difficult to work with until
> that's fixed (which should hopefully be some time this year).
> 
>
https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
> File webrtc/ortc/rtptransport_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2981513002/diff/100001/webrtc/ortc/rtptransport...
> webrtc/ortc/rtptransport_unittest.cc:250: // Need to explicitly call
> SetParameters() to actually set keepalive.
> On 2017/08/02 16:45:08, sprang_webrtc wrote:
> > On 2017/08/02 00:12:56, Taylor Brandstetter wrote:
> > > Even though the keepalive parameters were passed into CreateRtpTransport?
> > That's
> > > pretty unintuitive; can that be fixed?
> > 
> > Yes, wired that up now.
> 
> Where is that wired up? I don't see it.
> 
> I was thinking the most sensible thing would be to call
> RtpTransportControllerAdapter::SetRtpTransportParameters from
> RtpTransportAdapter::CreateProxied (bubbling up the error if it fails).

That makes sense. I did the wireup via
RtpTransportControllerAdapter ctor and Init() call. This had the benefit of
allowing construction time parameters for the Call instance, but has other
problems. Removed it a changed so we set parameters through the proper
setters when we create the proxied instead.


> Meaning, if you did:
> 
> 1. controller = CreateRtpTransportController()
> 2. transport1 = CreateRtpTransport(keepalive_params_1, controller, ...)
> 3. sender1 = CreateRtpSender(track1, transport1, ...)
> 4. sender1.Send(...)
> 5. transport2 = CreateRtpTransport(keepalive_params_2, controller, ...)
> 
> ... The second CreateRtpTransport would return an error, because it's
implicitly
> trying to change the keepalive params for the Call after sending has started.
> 

Creating another transport (bound to the same controller) is now a different
thing. I've added unit test for that as well. I don't think it even makes
sense to allow creating another transport with new keepalive config even if
there
have been no sending started.

[sprang_webrtc, 2017-08-03 13:08:15]

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans...
webrtc/api/ortc/rtptransportinterface.h:98: // RTP keep-alive settings need to
be set before creating any send-streams,
On 2017/08/03 01:18:03, Taylor Brandstetter wrote:
> nit: Someone using this API probably doesn't know what a "send stream" is;
maybe
> "before an RtpSender has started sending" instead (if that's accurate)?

How about changing "send stream" to "media channel"?

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.h
File webrtc/ortc/ortcfactory.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory....
webrtc/ortc/ortcfactory.h:47: CreateRtpTransportController(const
RtpTransportParameters& parameters);
On 2017/08/03 01:18:03, Taylor Brandstetter wrote:
> We shouldn't add this method; the fact that RtpTransportControllerAdapter
knows
> about RtpTransportParameters is an implementation detail. From an API
> perspective, each RtpTransport has its own RtpTransportParameters, just like
how
> each RtpSender/RtpReceiver has its own RtpParameters.

True, that's why I didn't add it to the interface. What if we just make it
private? I did this since it allows setting the call config on construction.

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:232:
RTC_DCHECK(call_->SetRtpKeepAliveConfig(parameters.keepalive));
On 2017/08/03 01:18:03, Taylor Brandstetter wrote:
> If this is in a DCHECK, it will only be executed in debug builds, right?

Oops, right. Let's do a check instead then.

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.h:105: RTCError
SetRtpTransportAndParameters(
On 2017/08/03 01:18:03, Taylor Brandstetter wrote:
> Can remove "And" from this name.

Done.

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.c...
webrtc/pc/rtptransport.cc:126: "RTP keep-alive parameters not supported by this
channel.");
On 2017/08/03 01:18:04, Taylor Brandstetter wrote:
> Why is this message changing?

This is a new message. I just verify that you don't try to set keep-alive
config, since it's not wired up with this implementation.

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport_u...
File webrtc/pc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport_u...
webrtc/pc/rtptransport_unittest.cc:44: TEST(RtpTransportTest,
SetRtpTransportKeepAliveNotSupported) {
On 2017/08/03 01:18:04, Taylor Brandstetter wrote:
> Can you add a comment explaining that this is a limitation of the current
> implementation, and may be fixed later?

Done.

[Taylor Brandstetter, 2017-08-03 16:50:37]

lgtm, assuming nits about comments and error messages are addressed

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans...
webrtc/api/ortc/rtptransportinterface.h:98: // RTP keep-alive settings need to
be set before creating any send-streams,
On 2017/08/03 13:08:14, sprang_webrtc wrote:
> On 2017/08/03 01:18:03, Taylor Brandstetter wrote:
> > nit: Someone using this API probably doesn't know what a "send stream" is;
> maybe
> > "before an RtpSender has started sending" instead (if that's accurate)?
> 
> How about changing "send stream" to "media channel"?

Again, we don't define what a "media channel" is in this API.

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory.h
File webrtc/ortc/ortcfactory.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory....
webrtc/ortc/ortcfactory.h:47: CreateRtpTransportController(const
RtpTransportParameters& parameters);
On 2017/08/03 13:08:14, sprang_webrtc wrote:
> On 2017/08/03 01:18:03, Taylor Brandstetter wrote:
> > We shouldn't add this method; the fact that RtpTransportControllerAdapter
> knows
> > about RtpTransportParameters is an implementation detail. From an API
> > perspective, each RtpTransport has its own RtpTransportParameters, just like
> how
> > each RtpSender/RtpReceiver has its own RtpParameters.
> 
> True, that's why I didn't add it to the interface. What if we just make it
> private? I did this since it allows setting the call config on construction.

I don't see that allowing setting the call config on construction buys much,
since you need to be able to set it after construction anyway, in the normal
case where the RtpTransport is created after the RtpTransportController. I also
can't find where this constructor is called (or even defined).

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:230: "media streams.");
nit: Someone using this API may interpret "media stream" as referring to a
MediaStream object, which isn't what this is talking about. "Stream" is a pretty
ambiguous term for us, since it has different meanings in the contexts of
WebRTC, SDP and RTP.

Can you rephrase this message in the terminology used by this API? Which I think
would be "RtpSender has started sending"?

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.cc
File webrtc/pc/rtptransport.cc (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/pc/rtptransport.c...
webrtc/pc/rtptransport.cc:126: "RTP keep-alive parameters not supported by this
channel.");
On 2017/08/03 13:08:14, sprang_webrtc wrote:
> On 2017/08/03 01:18:04, Taylor Brandstetter wrote:
> > Why is this message changing?
> 
> This is a new message. I just verify that you don't try to set keep-alive
> config, since it's not wired up with this implementation.

Sorry, I thought this was RtpTransportAdapter when reviewing, carry on.

https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:229: // It is impossible to modify
keepalive parameters if any streams are created.
Can you comment that this is an limitation of the existing implementation (since
the keepalive settings go on Call, instead of a more granular object), and may
be fixed later?

https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:256: TEST_F(RtpTransportTest,
KeepAliveMustBeSameAcrossTransportController) {
Again, can you comment that this is a limitation of the existing implementation,
and the test should be updated when it's fixed? Can you also document this
limitation in one of the API headers? rtptransportinterface.h says:

// RTP keep-alive settings need to be set before creating any media channels,
// altering the payload type or timeout interval after this point is not
// supported.

But this doesn't talk about "must be same across controller".

https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:272: // Update the parameters, and crate
another transport for the same controller.
nit: "crate"

[sprang_webrtc, 2017-08-07 09:42:45]

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

[commit-bot: I haz the power, 2017-08-07 09:42:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-07 09:45:22]

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

[commit-bot: I haz the power, 2017-08-07 09:45:23]

Dry run: Try jobs failed on following builders:
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/14365)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/23630)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/27915)

[sprang_webrtc, 2017-08-07 10:34:23]

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

[commit-bot: I haz the power, 2017-08-07 10:34:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[sprang_webrtc, 2017-08-07 10:39:36]

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans...
File webrtc/api/ortc/rtptransportinterface.h (right):

https://codereview.webrtc.org/2981513002/diff/120001/webrtc/api/ortc/rtptrans...
webrtc/api/ortc/rtptransportinterface.h:98: // RTP keep-alive settings need to
be set before creating any send-streams,
On 2017/08/03 16:50:36, Taylor Brandstetter wrote:
> On 2017/08/03 13:08:14, sprang_webrtc wrote:
> > On 2017/08/03 01:18:03, Taylor Brandstetter wrote:
> > > nit: Someone using this API probably doesn't know what a "send stream" is;
> > maybe
> > > "before an RtpSender has started sending" instead (if that's accurate)?
> > 
> > How about changing "send stream" to "media channel"?
> 
> Again, we don't define what a "media channel" is in this API.

Alright, "before an RtpSender has started sending" it is then.

https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransport_unittest.cc (right):

https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:229: // It is impossible to modify
keepalive parameters if any streams are created.
On 2017/08/03 16:50:36, Taylor Brandstetter wrote:
> Can you comment that this is an limitation of the existing implementation
(since
> the keepalive settings go on Call, instead of a more granular object), and may
> be fixed later?

Done.

https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:256: TEST_F(RtpTransportTest,
KeepAliveMustBeSameAcrossTransportController) {
On 2017/08/03 16:50:37, Taylor Brandstetter wrote:
> Again, can you comment that this is a limitation of the existing
implementation,
> and the test should be updated when it's fixed? Can you also document this
> limitation in one of the API headers? rtptransportinterface.h says:
> 
> // RTP keep-alive settings need to be set before creating any media channels,
> // altering the payload type or timeout interval after this point is not
> // supported.
> 
> But this doesn't talk about "must be same across controller".

Done.

https://codereview.webrtc.org/2981513002/diff/140001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransport_unittest.cc:272: // Update the parameters, and crate
another transport for the same controller.
On 2017/08/03 16:50:37, Taylor Brandstetter wrote:
> nit: "crate"

Done.

[commit-bot: I haz the power, 2017-08-07 10:40:00]

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

[commit-bot: I haz the power, 2017-08-07 10:40:01]

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

[sprang_webrtc, 2017-08-07 10:48:12]

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

[commit-bot: I haz the power, 2017-08-07 10:48:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-07 10:54:10]

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

[commit-bot: I haz the power, 2017-08-07 10:54:11]

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

[sprang_webrtc, 2017-08-07 11:18:38]

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

[commit-bot: I haz the power, 2017-08-07 11:18:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-07 11:24:11]

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

[commit-bot: I haz the power, 2017-08-07 11:24:12]

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

[sprang_webrtc, 2017-08-07 16:15:34]

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

[commit-bot: I haz the power, 2017-08-07 16:16:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-07 16:42:52]

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

[commit-bot: I haz the power, 2017-08-07 16:42:53]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-08-07 17:18:15]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-08-07 17:18:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2981513002/#ps260001 (title:
"Add workaround for gcc")

[commit-bot: I haz the power, 2017-08-07 17:18:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-07 17:24:03]

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

[commit-bot: I haz the power, 2017-08-07 17:24:06]

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

[sprang_webrtc, 2017-08-07 18:22:17]

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

[sprang_webrtc, 2017-08-07 18:22:18]

+stefan, can you please review common_types.h and call/* ?

[Taylor Brandstetter, 2017-08-07 19:59:37]

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.cc:84: std::move(params_result));
nit: I think you should be able to just return "std::move(params_result)".
RTCErrorOr can be constructed implicitly from an RTCError&&, for exactly these
kinds of situations.

[stefan-webrtc, 2017-08-08 08:13:06]

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:1148: // Can be called by RtpTransportController not on the
configuration thread.
This comment will become confusing. Call currently has an
RtpTransportControllerSend object, which is what I'd expect that this refers to.
Should this method be added to that class instead? That way the send streams can
ask the RtpTransportControllerSend whether or not to enable keepalive. Later on,
the RtpTransportController will be responsible for RTP and also keepalive.

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.h
File webrtc/call/call.h (right):

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.h#newco...
webrtc/call/call.h:210: virtual bool SetRtpKeepAliveConfig(const
RtpKeepAliveConfig& config) = 0;
Is there a reason why we have to set this after we create Call? Seems like a
step in the wrong direction.

[sprang_webrtc, 2017-08-08 08:15:13]

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.cc:84: std::move(params_result));
On 2017/08/07 19:59:36, Taylor Brandstetter wrote:
> nit: I think you should be able to just return "std::move(params_result)".
> RTCErrorOr can be constructed implicitly from an RTCError&&, for exactly these
> kinds of situations.

Right, thanks!

[sprang_webrtc, 2017-08-08 08:53:30]

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:1148: // Can be called by RtpTransportController not on the
configuration thread.
On 2017/08/08 08:13:06, stefan-webrtc wrote:
> This comment will become confusing. Call currently has an
> RtpTransportControllerSend object, which is what I'd expect that this refers
to.
> Should this method be added to that class instead? That way the send streams
can
> ask the RtpTransportControllerSend whether or not to enable keepalive. Later
on,
> the RtpTransportController will be responsible for RTP and also keepalive.

OK, so this badly phrased comment referred to the RtpTransportControllerAdapter
in the ortc api, which is not quite the same.

I've updated that class to actually configure this on the configuration thread
as well. So removed comment and added back thread check.

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.h
File webrtc/call/call.h (right):

https://codereview.webrtc.org/2981513002/diff/260001/webrtc/call/call.h#newco...
webrtc/call/call.h:210: virtual bool SetRtpKeepAliveConfig(const
RtpKeepAliveConfig& config) = 0;
On 2017/08/08 08:13:06, stefan-webrtc wrote:
> Is there a reason why we have to set this after we create Call? Seems like a
> step in the wrong direction.

I know, it was my intention to have this at construction time, but due to where
the Call instance lives (in the RtpTransportControllerAdapter), it get's
complicated as we want the parameters to be set on the RtpTransport interface of
the ORTC API...

[sprang_webrtc, 2017-08-08 13:51:12]

After offline discussion with stefan@, I've moved the keep-alive config away
from (supposedly transport-agnostic) Call::Config and put in the fledgling
RtpTransportControllerSendInterface instead.
This should make it easier to wire it up for audio streams as well (?) and in
the future make a truly transport-level feature. It however means I had to
remove the Call unit tests, and now rely on the ORTC classes to verify that we
don't unexpectedly update change settings.

ptal

[sprang_webrtc, 2017-08-08 13:52:08]

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

[commit-bot: I haz the power, 2017-08-08 13:52:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-08 13:54:24]

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

[commit-bot: I haz the power, 2017-08-08 13:54:26]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/27885)
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/28321)

[Taylor Brandstetter, 2017-08-08 23:14:17]

Patchset 17 lgtm.

[stefan-webrtc, 2017-08-09 07:23:09]

This is much better IMO, thanks! :)

One comment that I'd like to hear your opinion on before landing.

https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:235: "transport controller.");
Just a thought; if it's not possible to set after creating streams or
transports, isn't it safe to simply recreate Call and the
RtpTransportControllerSend when this API is called, instead of having a setter?

[stefan-webrtc, 2017-08-09 07:23:10]

This is much better IMO, thanks! :)

One comment that I'd like to hear your opinion on before landing.

https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:235: "transport controller.");
Just a thought; if it's not possible to set after creating streams or
transports, isn't it safe to simply recreate Call and the
RtpTransportControllerSend when this API is called, instead of having a setter?

[sprang_webrtc, 2017-08-09 07:58:59]

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

[commit-bot: I haz the power, 2017-08-09 07:59:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[sprang_webrtc, 2017-08-09 07:59:24]

https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:235: "transport controller.");
On 2017/08/09 07:23:10, stefan-webrtc wrote:
> Just a thought; if it's not possible to set after creating streams or
> transports, isn't it safe to simply recreate Call and the
> RtpTransportControllerSend when this API is called, instead of having a
setter?

Agree the would be nice (at least from the Call perspective), but seems to be
somewhat in conflict with the ortc api and how it's used. See comment here
https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory....

Maybe we can clean this up when the ortc implementation matures?

[commit-bot: I haz the power, 2017-08-09 08:01:17]

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

[commit-bot: I haz the power, 2017-08-09 08:01:19]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/22584)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/22363)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/26846)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/27979)

[stefan-webrtc, 2017-08-09 12:38:22]

On 2017/08/09 07:59:24, sprang_webrtc wrote:
>
https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport...
> File webrtc/ortc/rtptransportcontrolleradapter.cc (right):
> 
>
https://codereview.webrtc.org/2981513002/diff/320001/webrtc/ortc/rtptransport...
> webrtc/ortc/rtptransportcontrolleradapter.cc:235: "transport controller.");
> On 2017/08/09 07:23:10, stefan-webrtc wrote:
> > Just a thought; if it's not possible to set after creating streams or
> > transports, isn't it safe to simply recreate Call and the
> > RtpTransportControllerSend when this API is called, instead of having a
> setter?
> 
> Agree the would be nice (at least from the Call perspective), but seems to be
> somewhat in conflict with the ortc api and how it's used. See comment here
>
https://codereview.webrtc.org/2981513002/diff/120001/webrtc/ortc/ortcfactory....
> 
> Maybe we can clean this up when the ortc implementation matures?

sgtm

lgtm

[sprang_webrtc, 2017-08-09 12:45:57]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-08-09 12:45:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2981513002/#ps380001 (title:
"deps, again")

[commit-bot: I haz the power, 2017-08-09 12:46:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-09 13:42:33]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1502282757319950,
"parent_rev": "3e69e5c2c0450002a03be5184f28647efccbb47a", "commit_rev":
"db2a9fc6ec119052139d42f8897412bba7fa848b"}

[commit-bot: I haz the power, 2017-08-09 13:42:39]

Description was changed from

==========
Wire up RTP keep-alive in ortc api.

[This CL is work in progress.]

Wire up the rtp keep-alive in webrtc::Call::Config using new
SetRtpTransportParameters() method on RtpTransportInterface.

BUG=webrtc:7907
==========

to

==========
Wire up RTP keep-alive in ortc api.

[This CL is work in progress.]

Wire up the rtp keep-alive in webrtc::Call::Config using new
SetRtpTransportParameters() method on RtpTransportInterface.

BUG=webrtc:7907

Review-Url: https://codereview.webrtc.org/2981513002
Cr-Commit-Position: refs/heads/master@{#19287}
Committed:
https://chromium.googlesource.com/external/webrtc/+/db2a9fc6ec119052139d42f88...
==========

[commit-bot: I haz the power, 2017-08-09 13:42:42]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/external/webrtc/+/db2a9fc6ec119052139d42f88...