Re: [webrtc-pc] State gaps in connectionState algorithm (#2827)

Wow, it's been years since I looked at this.  

With fresh eyes, it does indeed lack a default case, and your idea of reversing "connecting" and "connected" and making "connecting" be the default makes sense.  But is there a risk of having it be "connecting" in cases where it doesn't make sense?  Is there something missing?  

Why don't we look and see how libwebrtc implements it ... (https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/jsep_transport_controller.cc;l=1229;drc=f37b11fe3cfd8d5f8294ef27d5c6c723a4028d89;bpv=0;bpt=1)

Here is a simplified version of it: 

```
  if (ice_failed_count + dtls_failed_count > 0) {
     return Failed;
  } else if (ice_disconnected_count > 0) {
     return Disconnected;
  } else if (ice_new_count + ice_closed_count + dtls_new_count + dtls_closed_count == ice_total_count + dtls_total_count) {
    return New;
  } else if (ice_new_count + ice_checking_count + dtls_new_count + dtls_connecting_count > 0) {
    return Connecting;
  } else if (ice_connected_count + ice_completed_count + ice_closed_count + dtls_connected_count  + dtls_closed_count == ice_total_count + dtls__total_count) {
    return Connected;
  } else {
    RTC_DCHECK_NOTREACHED();
    return New;
  }
```

Some observations:
- This matches the algorithm in the spec.
- The default is expected to not happen, but if it does, the default is "new".

So, how could this happen?  It could happen if not one of the cases match, which means:

```
((ice_failed + dtls_failed) == 0) &&
(ice_disconnected == 0) &&
((ice_new + ice_closed + dtls_new  + dtls_closed) < (ice_total + dtls_total)) &&
((ice_new + ice_checking + dtls_new + dtls_connecting) == 0) &&
((ice_connected + ice_completed + ice_closed + dtls_connected  + dtls_closed) < (ice_total + dtls_total))
```

Rearranging a bit, this means:
```
(ice_new = 0) &&
(ice_checking == 0) &&
(ice_disconnected == 0) &&
(ice_failed == 0) &&
(dtls_new == 0) &&
(dtls_connecting == 0) &&
(dtls_failed == 0) &&
((ice_closed + dtls_closed) < (ice_total + dtls_total)) &&
((ice_connected + ice_completed + ice_closed + dtls_connected  + dtls_closed) < (ice_total + dtls_total))
```

But the first 7 lines means that:
- All ICE states are in this set: (connected, completed, closed)
- All DTLS states are in this set: (connected, closed)

Which means the last line cannot be true.  
Which means this could never happen.


So...  let's go back to your cases:

- All ICE transports are in "new" or "closed", and any DTLS transport is in "connected", or
- Any ICE transports are in "new" or "closed", and all DTLS transports are "connected"

In both cases, since there is an ICE transport is in the "new" state, it would trigger the rule for "connecting", and the algorithm in the spec would return "connecting", and that is what libwebrtc does.

What would happen if we removed the "connecting" rule and made it the default, as you suggest?    That would give the exact same results as the existing algorithm in the spec as long as this is guaranteed to be false (that the "connecting" rule and the "connected" rule cannot be true at the same time):

```
(ice_new_count + ice_checking_count + dtls_new_count + dtls_connecting_count > 0) &&
(ice_connected_count + ice_completed_count + ice_closed_count + dtls_connected_count  + dtls_closed_count == ice_total_count + dtls_total_count)
```

And this can be rewritten as:
```
(ice_new_count > 0 || ice_checking_count > 0 || dtls_new_count > 0 || dtls_connecting_count > 0) &&
(ice_new_count == 0 && ice_checking_count == 0 && dtls_new_count == 0 && dtls_connecting_count == 0)
```

Which always false, which means the "connecting" rule and "connected" rule are never both true, which means we can reorder them and drop the "connecting" rule and make it the default.


TL;DR:  That's a fine modification to the algorithm if you think it makes it more readable/implementable.  But the algorithm matches all possible cases, including the ones you brought up, which should return "connected".

But it's fairly complex, so maybe I got something wrong :).



-- 
GitHub Notification of comment by pthatcher
Please view or discuss this issue at https://github.com/w3c/webrtc-pc/issues/2827#issuecomment-1433657734 using your GitHub account


-- 
Sent via github-notify-ml as configured in https://github.com/w3c/github-notify-ml-config

Received on Thursday, 16 February 2023 20:13:29 UTC