822159 - Fix trickle ICE to start checking when new candidates come in

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Fix trickle ICE to start checking when new candidates come in

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 692812 [details] [diff] [review]
Fix trickle ICE to start checking when new candidates come in

Review of attachment 692812 [details] [diff] [review]:
-----------------------------------------------------------------

The current code has a bug where if you have two streams and one succeeds before the other gets any candidates, the other never gets started. This fixes that.

Note that if a stream fails with some candidate subset it will not restart even if it gets a new valid candidate that might work.

Comment 3

[Adam Roach [:abr]]

Comment on attachment 692812 [details] [diff] [review]
Fix trickle ICE to start checking when new candidates come in

Review of attachment 692812 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me -- minor comments only.

::: media/mtransport/test/ice_unittest.cpp
@@ +159,5 @@
> +        WrapRunnableRet(streams_[stream], &NrIceMediaStream::ParseTrickleCandidate,
> +                        candidates[j],
> +                        &res), NS_DISPATCH_SYNC);
> +      
> +      ASSERT_TRUE(NS_SUCCEEDED(res));

Formatting nits: Trailing WS on lines 156, 162. Line 159 > 80 chars.

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +333,5 @@
>        nr_ice_candidate_pair_start(pair->pctx,pair); /* Ignore failures */
>        NR_ASYNC_TIMER_SET(timer_val,nr_ice_media_stream_check_timer_cb,cb_arg,&stream->timer);
>      }
> +    else {
> +      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): no pairs for %s",stream->pctx->label,stream->label);

Wrap line

@@ +510,5 @@
>        nr_ice_media_stream_states[str->ice_state],
>        nr_ice_media_stream_states[state]);
>      
> +    if(str->ice_state != NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE &&
> +       state == NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)

With the "make no-change a no-op" check above, isn't this redundant? We are guaranteed that str->ice_state cannot be equal to state; so if state == ACTIVE, then we are assured that str->ice_state must not.

I think we want to be consistent between this check and the following one: either check *both* str->ice_state and state, or check only one of them.

Propose reworking to read "if (state == NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)"

::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
@@ +252,5 @@
> +       components.
> +
> +       Note that this is not compliant with RFC 5245, but consistent with
> +       the libjingle trickle ICE behavior. Note that we will not restart
> +       checks if they have all failed.

This comment needs some clarification, since the logic to avoid restarting from failed isn't contained in the code below; perhaps something like "...if they have all failed, since the switch statement above will abort for the FAILED state."

@@ +258,5 @@
> +       TODO([e-mail]): update when the trickle ICE RFC is published
> +    */
> +    if (!pstream->timer) {
> +      if(r=nr_ice_media_stream_start_checks(pctx, pstream)) {
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s): peer (%s), stream(%s) failed to start checks",pctx->ctx->label,pctx->label,stream->label);

Wrap line

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 692812 [details] [diff] [review]
Fix trickle ICE to start checking when new candidates come in

Review of attachment 692812 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. Will fix the things I agree iwth.

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +333,5 @@
>        nr_ice_candidate_pair_start(pair->pctx,pair); /* Ignore failures */
>        NR_ASYNC_TIMER_SET(timer_val,nr_ice_media_stream_check_timer_cb,cb_arg,&stream->timer);
>      }
> +    else {
> +      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): no pairs for %s",stream->pctx->label,stream->label);

nICEr style is not to wrap.

@@ +510,5 @@
>        nr_ice_media_stream_states[str->ice_state],
>        nr_ice_media_stream_states[state]);
>      
> +    if(str->ice_state != NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE &&
> +       state == NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)

Good point. I think these changes were made before I added the no-op check.

::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
@@ +252,5 @@
> +       components.
> +
> +       Note that this is not compliant with RFC 5245, but consistent with
> +       the libjingle trickle ICE behavior. Note that we will not restart
> +       checks if they have all failed.

OK.

@@ +258,5 @@
> +       TODO([e-mail]): update when the trickle ICE RFC is published
> +    */
> +    if (!pstream->timer) {
> +      if(r=nr_ice_media_stream_start_checks(pctx, pstream)) {
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s): peer (%s), stream(%s) failed to start checks",pctx->ctx->label,pctx->label,stream->label);

nICEr style is not to wrap

Comment 5

[Adam Roach [:abr]]

Committed upstream:

http://svn.resiprocate.org/viewsvn/resiprocate?diff_format=l&view=revision&revision=9918

Comment 6

[Adam Roach [:abr]]

Note to self: should land interdiff patch upstream when nits are addressed to ease future changes.

Comment 7

[Eric Rescorla (:ekr)]

Attachment: Fix trickle ICE to start checking when new candidates come in.

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 697750 [details] [diff] [review]
Fix trickle ICE to start checking when new candidates come in.

Review of attachment 697750 [details] [diff] [review]:
-----------------------------------------------------------------

carried forward r=abr

Comment 9

[Eric Rescorla (:ekr)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c778cd83a1

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f0c778cd83a1