Allow applications to limit the ICE check rate through RTCConfiguration

Allow applications to limit the ICE check rate through RTCConfiguration

If an application sets a non-null value in RTCConfiguration.iceCheckMinInterval, we do not sent STUN pings more often than that. This is useful for bandwidth constrained scenarios.

This CL also increases the maximum STUN ping timeout to 60 seconds up from its previous value of 5 (which meant that a ping response received 5 seconds later would not be counted), and allows the RTT estimate to go up to 60 seconds from its previous limit of 3. RTTs above 3 seconds are possible on mobile links. (webrtc:7109)

This CL was originally written by pthatcher@, I am just submitting it after a minor cleanup.

BUG=webrtc:7082, webrtc:7109
Review-Url: https://codereview.webrtc.org/2670053002
Cr-Commit-Position: refs/heads/master@{#16421}
Committed: https://chromium.googlesource.com/external/webrtc/+/5107246d4bf4048ca3eb428107bfccf7d757d667

[skvlad, 2017-02-02 05:15:58]

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

[commit-bot: I haz the power, 2017-02-02 05:16:02]

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-02-02 05:27:12]

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

[commit-bot: I haz the power, 2017-02-02 05:27:13]

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

[skvlad, 2017-02-02 06:02:46]

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

[commit-bot: I haz the power, 2017-02-02 06:02:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[skvlad, 2017-02-02 06:19:39]

skvlad@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, glaznev@webrtc.org, pthatcher@webrtc.org

[commit-bot: I haz the power, 2017-02-02 06:39:48]

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

[commit-bot: I haz the power, 2017-02-02 06:39:49]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-02-02 07:03:55]

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:384: // (STUN pings), in milliseconds.
Nit: The variable could be named in milliseconds (ice_check_min_interval_ms) to
make this more clear

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/jseptrans...
File webrtc/p2p/base/jseptransport.h (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:192: // Measure in milliseconds.
nit: This can match the other comment

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:193: rtc::Optional<int> min_ping_interval;
nit: Would prefer more consistent naming between "ice_check_min_interval" and
"min_ping_interval"

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1220: const int
kWriteTimeoutDelay = 8000;
Why was this needed?

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

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/port.cc#n...
webrtc/p2p/base/port.cc:63: const int MAXIMUM_RTT = 60000;  // 60 seconds
See other comment

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

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:78: static const int CONNECTION_RESPONSE_TIMEOUT = 60 *
1000;  // 60 seconds
Would prefer if this was in a separate CL, or at least called out in the
description

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/pc/peerconnection....
webrtc/pc/peerconnection.cc:508: rtc::Optional<int> ice_check_min_interval;
Out of curiosity, did my scheme here work, or did you remember to update
operator== by yourself?

[skvlad, 2017-02-02 08:51:25]

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:384: // (STUN pings), in milliseconds.
On 2017/02/02 07:03:55, Taylor Brandstetter wrote:
> Nit: The variable could be named in milliseconds (ice_check_min_interval_ms)
to
> make this more clear

None of the other timeout/interval parameters are. If you prefer clarity over
consistency, I can rename it.

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/jseptrans...
File webrtc/p2p/base/jseptransport.h (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/jseptrans...
webrtc/p2p/base/jseptransport.h:193: rtc::Optional<int> min_ping_interval;
On 2017/02/02 07:03:55, Taylor Brandstetter wrote:
> nit: Would prefer more consistent naming between "ice_check_min_interval" and
> "min_ping_interval"

Renamed to ice_check_min_interval. "ICE" is really redundant here since it's
IceSettings, but "check_min_interval" doesn't seem to mean anything.

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1220: const int
kWriteTimeoutDelay = 8000;
On 2017/02/02 07:03:55, Taylor Brandstetter wrote:
> Why was this needed?

The connection becomes FAILED when a certain set of conditions is met:
 - we have not received a ping response within CONNECTION_WRITE_CONNECT_TIMEOUT
(5 s)
 - there were at least CONNECTION_WRITE_CONNECT_FAILURES (5) failed pings
 - at least RTT has elapsed since the last ping was sent

The RTT estimation was capped at 3000 ms (actually 3000/2, because of this line:
https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/port.cc?rcl=...
- that I couldn't find an explanation for). This CL changes the maximum RTT to
60 000 ms. The test results an RTT of 2250, that previously would be capped at
1500 and now (correctly) is not.

This increased the time it takes for this particular test to go to the FAILED
state from ~5500ms to ~7500ms (the state checks only happen when the next ping
is due to be sent). This made the 6000 ms timeout no longer sufficient.

Since the test is meant to verify UMA metrics and not ICE behavior, the exact
value of the timeout isn't essential - it needs to wait as long as necessary for
the state to become FAILED. However, increasing the (simulated) timeout is
sufficient in this case.

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

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:78: static const int CONNECTION_RESPONSE_TIMEOUT = 60 *
1000;  // 60 seconds
On 2017/02/02 07:03:55, Taylor Brandstetter wrote:
> Would prefer if this was in a separate CL, or at least called out in the
> description

I'll mention it in the description. Probably not worth a CL of its own.

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/pc/peerconnection....
webrtc/pc/peerconnection.cc:508: rtc::Optional<int> ice_check_min_interval;
On 2017/02/02 07:03:55, Taylor Brandstetter wrote:
> Out of curiosity, did my scheme here work, or did you remember to update
> operator== by yourself?

Peter did this change, I'll let him answer :)

[Taylor Brandstetter, 2017-02-02 18:16:01]

lgtm

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:384: // (STUN pings), in milliseconds.
On 2017/02/02 08:51:24, skvlad wrote:
> On 2017/02/02 07:03:55, Taylor Brandstetter wrote:
> > Nit: The variable could be named in milliseconds (ice_check_min_interval_ms)
> to
> > make this more clear
> 
> None of the other timeout/interval parameters are. If you prefer clarity over
> consistency, I can rename it.

Nah, consistency is better.

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2670053002/diff/20001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1220: const int
kWriteTimeoutDelay = 8000;
On 2017/02/02 08:51:25, skvlad wrote:
> On 2017/02/02 07:03:55, Taylor Brandstetter wrote:
> > Why was this needed?
> 
> The connection becomes FAILED when a certain set of conditions is met:
>  - we have not received a ping response within
CONNECTION_WRITE_CONNECT_TIMEOUT
> (5 s)
>  - there were at least CONNECTION_WRITE_CONNECT_FAILURES (5) failed pings
>  - at least RTT has elapsed since the last ping was sent
> 
> The RTT estimation was capped at 3000 ms (actually 3000/2, because of this
line:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/port.cc?rcl=...
> - that I couldn't find an explanation for). This CL changes the maximum RTT to
> 60 000 ms. The test results an RTT of 2250, that previously would be capped at
> 1500 and now (correctly) is not.
> 
> This increased the time it takes for this particular test to go to the FAILED
> state from ~5500ms to ~7500ms (the state checks only happen when the next ping
> is due to be sent). This made the 6000 ms timeout no longer sufficient.
> 
> Since the test is meant to verify UMA metrics and not ICE behavior, the exact
> value of the timeout isn't essential - it needs to wait as long as necessary
for
> the state to become FAILED. However, increasing the (simulated) timeout is
> sufficient in this case.

Alright, makes sense.

[Taylor Brandstetter, 2017-02-02 18:18:08]

On 2017/02/02 18:16:01, Taylor Brandstetter wrote:
> lgtm

Though remember to update the description to include the timeout and maximum RTT
changing, and preferably make a bug for that as well.

[AlexG, 2017-02-02 18:19:42]

lgtm

[skvlad, 2017-02-02 18:52:48]

Description was changed from

==========
Allow applications to limit the ICE check rate through RTCConfiguration

If an application sets a non-null value in RTCConfiguration.iceCheckMinInterval,
we do not sent STUN pings more often than that. This is useful for bandwidth
constrained scenarios.

This CL was originally written by pthatcher@, I am just submitting it after a
minor cleanup.

BUG=webrtc:7082
==========

to

==========
Allow applications to limit the ICE check rate through RTCConfiguration

If an application sets a non-null value in RTCConfiguration.iceCheckMinInterval,
we do not sent STUN pings more often than that. This is useful for bandwidth
constrained scenarios.

This CL also increases the maximum STUN ping timeout to 60 seconds up from its
previous value of 5 (which meant that a ping response received 5 seconds later
would not be counted), and allows the RTT estimate to go up to 60 seconds from
its previous limit of 3. RTTs above 3 seconds are possible on mobile links.

This CL was originally written by pthatcher@, I am just submitting it after a
minor cleanup.

BUG=webrtc:7082
==========

[skvlad, 2017-02-02 18:56:32]

Description was changed from

==========
Allow applications to limit the ICE check rate through RTCConfiguration

If an application sets a non-null value in RTCConfiguration.iceCheckMinInterval,
we do not sent STUN pings more often than that. This is useful for bandwidth
constrained scenarios.

This CL also increases the maximum STUN ping timeout to 60 seconds up from its
previous value of 5 (which meant that a ping response received 5 seconds later
would not be counted), and allows the RTT estimate to go up to 60 seconds from
its previous limit of 3. RTTs above 3 seconds are possible on mobile links.

This CL was originally written by pthatcher@, I am just submitting it after a
minor cleanup.

BUG=webrtc:7082
==========

to

==========
Allow applications to limit the ICE check rate through RTCConfiguration

If an application sets a non-null value in RTCConfiguration.iceCheckMinInterval,
we do not sent STUN pings more often than that. This is useful for bandwidth
constrained scenarios.

This CL also increases the maximum STUN ping timeout to 60 seconds up from its
previous value of 5 (which meant that a ping response received 5 seconds later
would not be counted), and allows the RTT estimate to go up to 60 seconds from
its previous limit of 3. RTTs above 3 seconds are possible on mobile links.
(webrtc:7109)

This CL was originally written by pthatcher@, I am just submitting it after a
minor cleanup.

BUG=webrtc:7082, webrtc:7109
==========

[skvlad, 2017-02-02 18:56:40]

The CQ bit was checked by skvlad@webrtc.org

[commit-bot: I haz the power, 2017-02-02 18:56:43]

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

[tkchin_webrtc, 2017-02-02 19:43:47]

On 2017/02/02 18:56:43, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

Do we need the ObjC changes in RTCConfiguration.h as well?

[skvlad, 2017-02-02 19:47:40]

On 2017/02/02 19:43:47, tkchin_webrtc wrote:
> On 2017/02/02 18:56:43, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
> 
> Do we need the ObjC changes in RTCConfiguration.h as well?

Yes, please review https://codereview.webrtc.org/2674663002/.

[commit-bot: I haz the power, 2017-02-02 19:50:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486061800769190,
"parent_rev": "e5bd70223d603c99ef4043313c523d7276330a8e", "commit_rev":
"5107246d4bf4048ca3eb428107bfccf7d757d667"}

[commit-bot: I haz the power, 2017-02-02 19:50:18]

Description was changed from

==========
Allow applications to limit the ICE check rate through RTCConfiguration

If an application sets a non-null value in RTCConfiguration.iceCheckMinInterval,
we do not sent STUN pings more often than that. This is useful for bandwidth
constrained scenarios.

This CL also increases the maximum STUN ping timeout to 60 seconds up from its
previous value of 5 (which meant that a ping response received 5 seconds later
would not be counted), and allows the RTT estimate to go up to 60 seconds from
its previous limit of 3. RTTs above 3 seconds are possible on mobile links.
(webrtc:7109)

This CL was originally written by pthatcher@, I am just submitting it after a
minor cleanup.

BUG=webrtc:7082, webrtc:7109
==========

to

==========
Allow applications to limit the ICE check rate through RTCConfiguration

If an application sets a non-null value in RTCConfiguration.iceCheckMinInterval,
we do not sent STUN pings more often than that. This is useful for bandwidth
constrained scenarios.

This CL also increases the maximum STUN ping timeout to 60 seconds up from its
previous value of 5 (which meant that a ping response received 5 seconds later
would not be counted), and allows the RTT estimate to go up to 60 seconds from
its previous limit of 3. RTTs above 3 seconds are possible on mobile links.
(webrtc:7109)

This CL was originally written by pthatcher@, I am just submitting it after a
minor cleanup.

BUG=webrtc:7082, webrtc:7109

Review-Url: https://codereview.webrtc.org/2670053002
Cr-Commit-Position: refs/heads/master@{#16421}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5107246d4bf4048ca3eb42810...
==========

[commit-bot: I haz the power, 2017-02-02 19:50:19]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/5107246d4bf4048ca3eb42810...

[pthatcher2, 2017-02-02 20:02:41]

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

[pthatcher2, 2017-02-02 20:02:42]

lgtm :)

Nice job turning that thing into an Int and fixing the tests.

Taylor, as for your trick: it didn't help because the comment you left in the
struct caught me first.  Good job on the comment :).