Add flakiness check if there is no received packets in a certain period.

Add flakyness check based on the recently received packets.

BUG=4901
R=pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/54360510ff9b7c61fc906d3ed360b06a5824bbf1

[honghaiz3, 2015-06-24 16:42:54]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-06-24 16:43:06]

Patchset #3 (id:60001) has been deleted

[honghaiz3, 2015-06-24 16:43:13]

Patchset #2 (id:40001) has been deleted

[honghaiz3, 2015-06-24 16:43:23]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2015-06-24 23:01:10]

Patchset #1 (id:80001) has been deleted

[honghaiz3, 2015-06-24 23:01:19]

Patchset #1 (id:100001) has been deleted

[honghaiz3, 2015-06-24 23:01:27]

Patchset #1 (id:120001) has been deleted

[honghaiz3, 2015-06-24 23:04:28]

Patchset #1 (id:140001) has been deleted

[honghaiz3, 2015-06-24 23:14:15]

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

[honghaiz3, 2015-06-24 23:14:15]

This is a firstcut of the CL. 
I have tested that flakiness was able to be passed to the app.

[honghaiz3, 2015-06-24 23:15:52]

Patchset #1 (id:160001) has been deleted

[pthatcher1, 2015-06-24 23:42:52]

I like it, I'm just not sold on two things:

1.  The name "flaky".

2.  The fact that we have "readable" and "flaky".  Could we get away with just
"readable"?

https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/peerconnection_jni.cc:253: void
OnIceFlakinessChange(bool flaky) override {
We need to think of a better name than "IceFlakiness" :).

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

https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:152: void OnIceFlakinessChange(bool flaky)
override;
Can you make them all "override", then, to be consistent?

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

https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:492: ice_flaky_(false),
Same here: we need a better name than ice_flaky.

https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1413: if (ice_flaky_ != flaky) {
Please use an early return.

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

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:467: if (dtls_state_ == STATE_NONE ||
dtls_state_ == STATE_OPEN) {
Why not just always set it, regardless of state?

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

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1450: last_data_received_ >= min_last_recv_time;
Instead of using this method, we could just have methods for
last_ping_received(), last_ping_response_received(), and last_data_received(),
and let the p2ptransportchannel.cc handle it from there.

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:66: enum TransportOperation {
Maybe TransportStateType?

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:67: TRANSPORT_READABLE = 0,
TRANSPORT_OPERATION_READABLE or TRANSPORT_READABLE_STATE

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:185: }
Is all_channels_flaky() ever used?  If not, we should leave it out.

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.cc (right):

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.cc:35: if (flaky_ != flaky) {
Please use an early return.

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.h (right):

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:158: bool flaky_;
I'm not completely sure we want to have flaky_ be different from readable_.  
I'd like to think about that some more.

[honghaiz3, 2015-06-25 23:24:42]

On 2015/06/24 23:42:52, pthatcher1 wrote:
> I like it, I'm just not sold on two things:
> 
> 1.  The name "flaky".
> 
> 2.  The fact that we have "readable" and "flaky".  Could we get away with just
> "readable"?
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/java/jni...
> File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/java/jni...
> talk/app/webrtc/java/jni/peerconnection_jni.cc:253: void
> OnIceFlakinessChange(bool flaky) override {
> We need to think of a better name than "IceFlakiness" :).
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/peerconn...
> File talk/app/webrtc/peerconnection.h (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/peerconn...
> talk/app/webrtc/peerconnection.h:152: void OnIceFlakinessChange(bool flaky)
> override;
> Can you make them all "override", then, to be consistent?
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/webrtcse...
> File talk/app/webrtc/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:492: ice_flaky_(false),
> Same here: we need a better name than ice_flaky.
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:1413: if (ice_flaky_ != flaky) {
> Please use an early return.
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/dtlstran...
> File webrtc/p2p/base/dtlstransportchannel.cc (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/dtlstran...
> webrtc/p2p/base/dtlstransportchannel.cc:467: if (dtls_state_ == STATE_NONE ||
> dtls_state_ == STATE_OPEN) {
> Why not just always set it, regardless of state?
> 
> https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/port.cc
> File webrtc/p2p/base/port.cc (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/port.cc#...
> webrtc/p2p/base/port.cc:1450: last_data_received_ >= min_last_recv_time;
> Instead of using this method, we could just have methods for
> last_ping_received(), last_ping_response_received(), and last_data_received(),
> and let the p2ptransportchannel.cc handle it from there.
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transport.h
> File webrtc/p2p/base/transport.h (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
> webrtc/p2p/base/transport.h:66: enum TransportOperation {
> Maybe TransportStateType?
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
> webrtc/p2p/base/transport.h:67: TRANSPORT_READABLE = 0,
> TRANSPORT_OPERATION_READABLE or TRANSPORT_READABLE_STATE
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
> webrtc/p2p/base/transport.h:185: }
> Is all_channels_flaky() ever used?  If not, we should leave it out.
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
> File webrtc/p2p/base/transportchannel.cc (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
> webrtc/p2p/base/transportchannel.cc:35: if (flaky_ != flaky) {
> Please use an early return.
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
> File webrtc/p2p/base/transportchannel.h (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
> webrtc/p2p/base/transportchannel.h:158: bool flaky_;
> I'm not completely sure we want to have flaky_ be different from readable_.  
> I'd like to think about that some more.
I addressed other comments except for the naming. It will be great if you can
have a suggestion. 

For using readable to replace flaky, 
as we read yesterday at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
when the read_state becomes READ_STATE_TIMEOUT, it may trigger ICE restart. 
But we want the app to have an option to decide whether to trigger ICE restart
or not when flakiness is detected.

[pthatcher1, 2015-06-26 19:24:03]

I think I would be happy with this CL if it used the name "receiving", for which
I left far too many comments about places where it would fit (although there are
many more on which I did not comment;  please just follow the pattern).

Also, please address the method in port.cc.

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

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:152: void OnIceFlakinessChange(bool flaky)
override;
How about "OnIceConnectionReceivingChange(bool receiving)"?

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:305: void OnTransportFlaky(cricket::Transport*
transport) override;
How about OnTransportReceiving?

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:350: void SetIceFlaky(bool flaky);
How about SetIceConnectionReceiving(bool receiving)?

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:388: bool ice_flaky_;
How about "bool ice_connection_receiving_"?

https://codereview.webrtc.org/1207563002/diff/200001/talk/examples/android/sr...
File talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1207563002/diff/200001/talk/examples/android/sr...
talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java:901:
public void onIceFlakinessChange(boolean flaky) {
How about "onIceConnectionRecevingChange(boolean receiving)"?

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

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:216: void OnFlakyState(TransportChannel*
channel);
How about OnRecevingState?

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

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:28: MSG_CHECK_FLAKINESS
MSG_CHECK_RECEIVING

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1163: OnCheckFlakiness();
OnCheckReceiving

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:248: 
How about 


check_receiving_delay_;
receiving_timeout_;

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

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1450: last_data_received_ >= min_last_recv_time;
Return max(last_ping_received_, last_ping_response_received_,
last_data_received_);

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

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:563: bool CheckReceiving(uint32 receiving_timeout);
I think it would be better to just have a last_received() method that returns
the max of the three values we store and let the caller  compare that with the
current time and timeout value.

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/session.h
File webrtc/p2p/base/session.h (right):

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/session....
webrtc/p2p/base/session.h:371: virtual void OnTransportFlaky(Transport*
transport) {
OnTransportReceiving

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:69: TRANSPORT_FLAKY_STATE
TRANSPORT_RECEIVING_STATE

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:380: void OnChannelFlakyState(TransportChannel*
channel);
OnChannelReceivingState

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:451: TransportState flaky_;
receiving_;

[pthatcher1, 2015-06-26 19:27:22]

Oh, and we need some unit tests.

[honghaiz3, 2015-07-06 17:46:59]

Patchset #3 (id:220001) has been deleted

[honghaiz3, 2015-07-06 17:51:46]

Fixed the names following the suggestions except slight difference in
peerconnection.h. 
Let me know if that looks fine or you want the longer name. 
Working on added tests.

[honghaiz3, 2015-07-06 17:54:52]

Plus, I changed the default check_receiving_delay_ to 250ms so that it will have
smaller impact on cpu consumption by default.

[honghaiz3, 2015-07-06 18:08:06]

Patchset #3 (id:240001) has been deleted

[honghaiz3, 2015-07-06 19:24:20]

Patchset #3 (id:260001) has been deleted

[pthatcher1, 2015-07-06 22:04:47]

Unit tests?

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

https://codereview.webrtc.org/1207563002/diff/280001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1413:
SetIceConnectionReceiving(transport->no_channel_receiving());
The session is receiving if no channel is receiving?  Should there be a "!"
somewhere in there?

Also, the logic here is a little more tricky than this.  There are potentially 3
transports: audio, video, and data.  We should probably only be "receiving" if
all transports are receiving.  For example:

int receiving = 0;
int not_receiving = 0;
for (const auto& kv : transport_proxies()) {
  cricket::Transport* transport = kv.second->impl();
  if (transport && transport->HasChannels()) {
    if (transport->no_channel_receiving()) {
      not_receiving++;
    } else {
      receiving++;
    }
  }
}


SetIceConnectionReceiving((receiving > 0) && (not_receiving == 0));

https://codereview.webrtc.org/1207563002/diff/280001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1207563002/diff/280001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:88: // conform to the w3c standard.
Just one TODO is sufficient.  You can remove this one.

https://codereview.webrtc.org/1207563002/diff/280001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:102: // packets or at least one channel changes
back.
Might be more clear with something like:

"Called with receiving=true when packets are first received, then called with
receiving=false when no packets are received for a certain time period, and then
called whenever the state changes between receiving and not receiving."

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

https://codereview.webrtc.org/1207563002/diff/280001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:201:
receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50) {
It seems like we should add a set_receiving_timeout method to allow apps to
change the timeout.  Perhaps you were planning on adding that in a future CL?  I
think it would make sense in this CL.

[honghaiz3, 2015-07-07 18:46:56]

Patchset #5 (id:320001) has been deleted

[honghaiz3, 2015-07-07 18:47:07]

Patchset #4 (id:300001) has been deleted

[honghaiz3, 2015-07-07 18:54:16]

Patchset #4 (id:340001) has been deleted

[honghaiz3, 2015-07-07 18:54:51]

Thanks! added tests. PTAL.

[pthatcher1, 2015-07-07 20:49:56]

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

https://codereview.webrtc.org/1207563002/diff/360001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1410: void
WebRtcSession::OnTransportReceiving(cricket::Transport* transp) {
transp?

https://codereview.webrtc.org/1207563002/diff/360001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1421:
SetIceConnectionReceiving(!not_receiving);
Actually, now that I see it, I think it would be more clear in the reverse:

// The ice connection is considered receiving if any
// transport has channels and is receiving.

bool receiving = false;
for (const auto& kv : transport_proxies()) {
  cricket::Transport* transport = kv.second->impl();
  if (transport && transport->HasChannels() &&
!transport->no_channel_receiving()) {
    receiving = true;
    break;
  }
}
SetIceConnectionReceiving(receiving);



Or better yet:

bool receiving = false;
for (const auto& kv : transport_proxies()) {
  cricket::Transport* transport = kv.second->impl();
  if (transport && transport->any_channel_receiving()) {
    receiving = true;
    break;
  }
}
SetIceConnectionReceiving(receiving);

https://codereview.webrtc.org/1207563002/diff/360001/talk/examples/android/sr...
File talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1207563002/diff/360001/talk/examples/android/sr...
talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java:902:
Log.d(TAG, "IceConnectionReceiving changes to " + receiving);
changes to => changed to

https://codereview.webrtc.org/1207563002/diff/360001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1207563002/diff/360001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1880: }
Can you add a test for when set_receiving_timeout isn't called?

[honghaiz3, 2015-07-07 21:40:15]

On 2015/07/07 20:49:56, pthatcher1 wrote:
>
https://codereview.webrtc.org/1207563002/diff/360001/talk/app/webrtc/webrtcse...
> File talk/app/webrtc/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/360001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:1410: void
> WebRtcSession::OnTransportReceiving(cricket::Transport* transp) {
> transp?
> 
>
https://codereview.webrtc.org/1207563002/diff/360001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:1421:
> SetIceConnectionReceiving(!not_receiving);
> Actually, now that I see it, I think it would be more clear in the reverse:
> 
> // The ice connection is considered receiving if any
> // transport has channels and is receiving.
> 
> bool receiving = false;
> for (const auto& kv : transport_proxies()) {
>   cricket::Transport* transport = kv.second->impl();
>   if (transport && transport->HasChannels() &&
> !transport->no_channel_receiving()) {
>     receiving = true;
>     break;
>   }
> }
> SetIceConnectionReceiving(receiving);
> 
> 
> 
> Or better yet:
> 
> bool receiving = false;
> for (const auto& kv : transport_proxies()) {
>   cricket::Transport* transport = kv.second->impl();
>   if (transport && transport->any_channel_receiving()) {
>     receiving = true;
>     break;
>   }
> }
> SetIceConnectionReceiving(receiving);
> 
>
https://codereview.webrtc.org/1207563002/diff/360001/talk/examples/android/sr...
> File talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java
> (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/360001/talk/examples/android/sr...
> talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java:902:
> Log.d(TAG, "IceConnectionReceiving changes to " + receiving);
> changes to => changed to
> 
>
https://codereview.webrtc.org/1207563002/diff/360001/webrtc/p2p/base/p2ptrans...
> File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/360001/webrtc/p2p/base/p2ptrans...
> webrtc/p2p/base/p2ptransportchannel_unittest.cc:1880: }
> Can you add a test for when set_receiving_timeout isn't called?

PTAL.

[pthatcher1, 2015-07-07 22:01:40]

lgtm with the change to the unit tests

https://codereview.webrtc.org/1207563002/diff/380001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1207563002/diff/380001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1887:
ch.set_receiving_timeout(500);
On second though, just delete the previous test and add this:

EXPECT_GE(1000, ch.receiving_timeout());
EXPECT_GE(200, ch.check_receiving_delay());
ch.set_receiving_timeout(500);
EXPECT_EQ(500, ch.receiving_timeout());
EXPECT_EQ(200, ch.check_receiving_delay());

[honghaiz3, 2015-07-07 22:16:44]

On 2015/07/07 22:01:40, pthatcher1 wrote:
> lgtm with the change to the unit tests
> 
>
https://codereview.webrtc.org/1207563002/diff/380001/webrtc/p2p/base/p2ptrans...
> File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1207563002/diff/380001/webrtc/p2p/base/p2ptrans...
> webrtc/p2p/base/p2ptransportchannel_unittest.cc:1887:
> ch.set_receiving_timeout(500);
> On second though, just delete the previous test and add this:
> 
> EXPECT_GE(1000, ch.receiving_timeout());
> EXPECT_GE(200, ch.check_receiving_delay());
> ch.set_receiving_timeout(500);
> EXPECT_EQ(500, ch.receiving_timeout());
> EXPECT_EQ(200, ch.check_receiving_delay());
Done. Thanks!

[honghaiz3, 2015-07-07 23:14:09]

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

[honghaiz3, 2015-07-07 23:14:10]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1207563002/#ps400001 (title:
"Address more comments")

[commit-bot: I haz the power, 2015-07-07 23:14:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207563002/400001

[commit-bot: I haz the power, 2015-07-07 23:14:27]

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

[commit-bot: I haz the power, 2015-07-07 23:14:28]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[honghaiz3, 2015-07-08 15:18:34]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2015-07-08 15:18:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207563002/400001

[commit-bot: I haz the power, 2015-07-08 15:18:53]

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

[commit-bot: I haz the power, 2015-07-08 15:18:54]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[pthatcher1, 2015-07-08 18:08:44]

Committed patchset #6 (id:400001) manually as
54360510ff9b7c61fc906d3ed360b06a5824bbf1 (presubmit successful).

[guoweis_webrtc, 2015-07-20 16:58:56]

On 2015/07/08 18:08:44, pthatcher1 wrote:
> Committed patchset #6 (id:400001) manually as
> 54360510ff9b7c61fc906d3ed360b06a5824bbf1 (presubmit successful).

would be nice if we specify bug number along with this CL.

[honghaiz3, 2015-08-05 23:56:57]

These are some draft responses to the comments which I did not publish earlier.
You can safely ignore them. (Need to publish them so that its state will change
to closed).

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

https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:152: void OnIceFlakinessChange(bool flaky)
override;
On 2015/06/24 23:42:52, pthatcher1 wrote:
> Can you make them all "override", then, to be consistent?

Done.

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

https://codereview.webrtc.org/1207563002/diff/180001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1413: if (ice_flaky_ != flaky) {
On 2015/06/24 23:42:52, pthatcher1 wrote:
> Please use an early return.

Done.

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

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.cc:467: if (dtls_state_ == STATE_NONE ||
dtls_state_ == STATE_OPEN) {
On 2015/06/24 23:42:52, pthatcher1 wrote:
> Why not just always set it, regardless of state?
For both set_readable and set_writable, they only do it when the dtls state is
none or open. 
I suppose, if this is in the middle a DTLS exchange, it is better not to fire
the event (which may trigger ICE restart).

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

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1450: last_data_received_ >= min_last_recv_time;
On 2015/06/24 23:42:52, pthatcher1 wrote:
> Instead of using this method, we could just have methods for
> last_ping_received(), last_ping_response_received(), and last_data_received(),
> and let the p2ptransportchannel.cc handle it from there.
If we do that, we will have to expose three public methods to (instead of one
now). 
For this reason, I slightly prefer to the current one which only exposes one
public method.

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:66: enum TransportOperation {
On 2015/06/24 23:42:52, pthatcher1 wrote:
> Maybe TransportStateType?

Done.

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:67: TRANSPORT_READABLE = 0,
On 2015/06/24 23:42:52, pthatcher1 wrote:
> TRANSPORT_OPERATION_READABLE or TRANSPORT_READABLE_STATE

Done. Took the latter one.

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:185: }
On 2015/06/24 23:42:52, pthatcher1 wrote:
> Is all_channels_flaky() ever used?  If not, we should leave it out.

Yes in webrtcsession.cc

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.cc (right):

https://codereview.webrtc.org/1207563002/diff/180001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.cc:35: if (flaky_ != flaky) {
On 2015/06/24 23:42:52, pthatcher1 wrote:
> Please use an early return.

Done.

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

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:152: void OnIceFlakinessChange(bool flaky)
override;
On 2015/06/26 19:24:02, pthatcher1 wrote:
> How about "OnIceConnectionReceivingChange(bool receiving)"?
That's a pretty long name. Plus, that may raise more confusion with
OnIceConnectionChange. 
So I choose  OnIceReceivingChange. Let me know if you still prefer to using the
longer name.

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:305: void OnTransportFlaky(cricket::Transport*
transport) override;
On 2015/06/26 19:24:02, pthatcher1 wrote:
> How about OnTransportReceiving?

Done.

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:350: void SetIceFlaky(bool flaky);
On 2015/06/26 19:24:02, pthatcher1 wrote:
> How about SetIceConnectionReceiving(bool receiving)?

Done, although not sure if I should just change it to SetIceReceiving().

https://codereview.webrtc.org/1207563002/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:388: bool ice_flaky_;
On 2015/06/26 19:24:02, pthatcher1 wrote:
> How about "bool ice_connection_receiving_"?

Done.

https://codereview.webrtc.org/1207563002/diff/200001/talk/examples/android/sr...
File talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1207563002/diff/200001/talk/examples/android/sr...
talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java:901:
public void onIceFlakinessChange(boolean flaky) {
On 2015/06/26 19:24:02, pthatcher1 wrote:
> How about "onIceConnectionRecevingChange(boolean receiving)"?

Done (without "Connection").

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

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransportchannel.h:216: void OnFlakyState(TransportChannel*
channel);
On 2015/06/26 19:24:02, pthatcher1 wrote:
> How about OnRecevingState?

Done.

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

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:28: MSG_CHECK_FLAKINESS
On 2015/06/26 19:24:02, pthatcher1 wrote:
> MSG_CHECK_RECEIVING

Done.

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:1163: OnCheckFlakiness();
On 2015/06/26 19:24:02, pthatcher1 wrote:
> OnCheckReceiving

Done.

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:248: 
On 2015/06/26 19:24:02, pthatcher1 wrote:
> How about 
> 
> 
> check_receiving_delay_;
> receiving_timeout_;

Done.

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

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/port.cc#...
webrtc/p2p/base/port.cc:1450: last_data_received_ >= min_last_recv_time;
On 2015/06/26 19:24:02, pthatcher1 wrote:
> Return max(last_ping_received_, last_ping_response_received_,
> last_data_received_);

Done.

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

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:563: bool CheckReceiving(uint32 receiving_timeout);
On 2015/06/26 19:24:02, pthatcher1 wrote:
> I think it would be better to just have a last_received() method that returns
> the max of the three values we store and let the caller  compare that with the
> current time and timeout value.

Done.

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/session.h
File webrtc/p2p/base/session.h (right):

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/session....
webrtc/p2p/base/session.h:371: virtual void OnTransportFlaky(Transport*
transport) {
On 2015/06/26 19:24:02, pthatcher1 wrote:
> OnTransportReceiving

Done.

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/transport.h
File webrtc/p2p/base/transport.h (right):

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:69: TRANSPORT_FLAKY_STATE
On 2015/06/26 19:24:03, pthatcher1 wrote:
> TRANSPORT_RECEIVING_STATE

Done.

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:380: void OnChannelFlakyState(TransportChannel*
channel);
On 2015/06/26 19:24:03, pthatcher1 wrote:
> OnChannelReceivingState

Done.

https://codereview.webrtc.org/1207563002/diff/200001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transport.h:451: TransportState flaky_;
On 2015/06/26 19:24:03, pthatcher1 wrote:
> receiving_;

Done.

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

https://codereview.webrtc.org/1207563002/diff/280001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1413:
SetIceConnectionReceiving(transport->no_channel_receiving());
On 2015/07/06 22:04:47, pthatcher1 wrote:
> The session is receiving if no channel is receiving?  Should there be a "!"
> somewhere in there?
> 
> Also, the logic here is a little more tricky than this.  There are potentially
3
> transports: audio, video, and data.  We should probably only be "receiving" if
> all transports are receiving.  For example:
> 
> int receiving = 0;
> int not_receiving = 0;
> for (const auto& kv : transport_proxies()) {
>   cricket::Transport* transport = kv.second->impl();
>   if (transport && transport->HasChannels()) {
>     if (transport->no_channel_receiving()) {
>       not_receiving++;
>     } else {
>       receiving++;
>     }
>   }
> }
> 
> 
> SetIceConnectionReceiving((receiving > 0) && (not_receiving == 0));

Done. Modified as discussed.

https://codereview.webrtc.org/1207563002/diff/280001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1207563002/diff/280001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:88: // conform to the w3c standard.
On 2015/07/06 22:04:47, pthatcher1 wrote:
> Just one TODO is sufficient.  You can remove this one.
I kept this one and removed the other one because this is the definition of the
interface.

https://codereview.webrtc.org/1207563002/diff/280001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:102: // packets or at least one channel changes
back.
On 2015/07/06 22:04:47, pthatcher1 wrote:
> Might be more clear with something like:
> 
> "Called with receiving=true when packets are first received, then called with
> receiving=false when no packets are received for a certain time period, and
then
> called whenever the state changes between receiving and not receiving."
Maybe enough just with the last sentence.

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

https://codereview.webrtc.org/1207563002/diff/280001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.cc:201:
receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50) {
On 2015/07/06 22:04:47, pthatcher1 wrote:
> It seems like we should add a set_receiving_timeout method to allow apps to
> change the timeout.  Perhaps you were planning on adding that in a future CL? 
I
> think it would make sense in this CL.

Done. Yes. This is needed for the tests.

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

https://codereview.webrtc.org/1207563002/diff/360001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1410: void
WebRtcSession::OnTransportReceiving(cricket::Transport* transp) {
On 2015/07/07 20:49:55, pthatcher1 wrote:
> transp?
The parameter is not used and I wanted to avoid the name shadowing, although it
probably does not matter. Change to transport.

https://codereview.webrtc.org/1207563002/diff/360001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1421:
SetIceConnectionReceiving(!not_receiving);
On 2015/07/07 20:49:55, pthatcher1 wrote:
> Actually, now that I see it, I think it would be more clear in the reverse:
> 
> // The ice connection is considered receiving if any
> // transport has channels and is receiving.
> 
> bool receiving = false;
> for (const auto& kv : transport_proxies()) {
>   cricket::Transport* transport = kv.second->impl();
>   if (transport && transport->HasChannels() &&
> !transport->no_channel_receiving()) {
>     receiving = true;
>     break;
>   }
> }
> SetIceConnectionReceiving(receiving);
> 
> 
> 
> Or better yet:
> 
> bool receiving = false;
> for (const auto& kv : transport_proxies()) {
>   cricket::Transport* transport = kv.second->impl();
>   if (transport && transport->any_channel_receiving()) {
>     receiving = true;
>     break;
>   }
> }
> SetIceConnectionReceiving(receiving);
> 
> 

Done. This is better. Thanks!

https://codereview.webrtc.org/1207563002/diff/360001/talk/examples/android/sr...
File talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/1207563002/diff/360001/talk/examples/android/sr...
talk/examples/android/src/org/appspot/apprtc/PeerConnectionClient.java:902:
Log.d(TAG, "IceConnectionReceiving changes to " + receiving);
On 2015/07/07 20:49:55, pthatcher1 wrote:
> changes to => changed to

Done.

https://codereview.webrtc.org/1207563002/diff/360001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1207563002/diff/360001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1880: }
On 2015/07/07 20:49:55, pthatcher1 wrote:
> Can you add a test for when set_receiving_timeout isn't called? 

Done.

https://codereview.webrtc.org/1207563002/diff/380001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/1207563002/diff/380001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1887:
ch.set_receiving_timeout(500);
On 2015/07/07 22:01:40, pthatcher1 wrote:
> On second though, just delete the previous test and add this:
> 
> EXPECT_GE(1000, ch.receiving_timeout());
> EXPECT_GE(200, ch.check_receiving_delay());
> ch.set_receiving_timeout(500);
> EXPECT_EQ(500, ch.receiving_timeout());
> EXPECT_EQ(200, ch.check_receiving_delay());

Done.