New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable trickling of end-of-candidates through addIceCandidate. #527

Merged
merged 6 commits into from Mar 17, 2016

Conversation

Projects
None yet
7 participants
@taylor-b
Copy link
Contributor

taylor-b commented Feb 29, 2016

A null parameter in addIceCandidate now represents end-of-candidates
for all media sections.

The definitions of the failed and completed ICE connection states
have been updated so that they only occur when there are no more
remote candidates.

Also updated the example code that trickled candidates, such that the
null candidate is trickled rather than ignored.

Fixes issues #483 and #442.

webrtc.html Outdated
end-of-candidates indication for said media section as
defined in [[TRICKLE-ICE]], unless an ICE restart occurred
for said media section since setting the last remote
description.</p>

This comment has been minimized.

@taylor-b

taylor-b Feb 29, 2016

Author Contributor

This caveat exists to avoid the race condition where one peer sets a local offer triggering an ICE restart at the same time as another peer signals end-of-candidates. In this situation, an "end-of-candidates" received before the remote answer refers to the previous "generation" of ICE candidates, so it should be ignored.

This comment has been minimized.

@pthatcherg

pthatcherg Mar 8, 2016

Contributor

Is ignoring it the right thing to do? If the local description for the ICE restart is rolled back, we'd still want the end-of-candidates to have applied.

You already specified that the end-of-candidates applies to the last applied remote description, so that part seems fine. But perhaps we should specify that it applies to all successfully applied remote descriptions, not just the last one. That way, if there are two concurrent remote gathering generations, both will be affected by the end-of-candidates.

This comment has been minimized.

@pthatcherg

pthatcherg Mar 10, 2016

Contributor

Perhaps a better way of saying this is: We need to make it clear what happens when this is done:

pc.setRemoteDescription(remote_offer_with_ice_restart);
pc.addIceCandidate(null);
pc.setRemoteDescription({type: rollback});

or this:

pc.setLocalDescription(local_offer_with_ice_restart);
pc.addIceCandidate(null);
pc.setLocalDescription({type: rollback});

Can the pc now go to completed/failed?

This comment has been minimized.

@taylor-b

taylor-b Mar 10, 2016

Author Contributor

I created two JSEP issues for these scenarios. rtcweb-wg/jsep#251 for the scenario above, and rtcweb-wg/jsep#250 for the one below.

webrtc.html Outdated
<p>Passing <code>null</code> into <code><a href=
"#dom-peerconnection-addicecandidate">addIceCandidate()</a></code>.</p>
</li>
</ul>

This comment has been minimized.

@taylor-b

taylor-b Feb 29, 2016

Author Contributor

Should this be moved to JSEP (at least, the first two bullets)? I'm just not sure where in JSEP it would fit.

This comment has been minimized.

@pthatcherg

pthatcherg Mar 8, 2016

Contributor

Yes, it seems like the first two bullet points should be, if not all three.

This comment has been minimized.

@taylor-b

taylor-b Mar 10, 2016

Author Contributor

It's still not clear to me where in JSEP this should go. Should it be a standalone section, or spread across "Applying a Remote Description" and "addIceCandidate"? For now I'm at least adding a note.

This comment has been minimized.

@fluffy

fluffy Mar 14, 2016

Contributor

I think it should be spread across the sections you mentioned

This comment has been minimized.

@taylor-b

taylor-b Mar 16, 2016

Author Contributor

Ok. I removed the stuff that should go in JSEP, and created this PR: rtcweb-wg/jsep#254

Note that there's still a TODO for describing "applying a candidate" process in JSEP.

webrtc.html Outdated
following steps:</p>
<ol>
<li>
<p>For each media section in the last successfully applied

This comment has been minimized.

@adam-be

adam-be Mar 3, 2016

Member

Please use 'media description' (instead of 'media section') as it's defined with a link to the RFC.

This comment has been minimized.

@taylor-b

taylor-b Mar 10, 2016

Author Contributor

Done. The link to the RFC is in the "Terminology" section which this now links to.

@alvestrand

This comment has been minimized.

Copy link
Contributor

alvestrand commented Mar 3, 2016

Looks good, but needs rebase.

@taylor-b taylor-b force-pushed the taylor-b:issue483_trickle_end_of_candidates branch from b230e82 to d4cff9b Mar 3, 2016

@taylor-b

This comment has been minimized.

Copy link
Contributor Author

taylor-b commented Mar 3, 2016

Alright, rebased.

webrtc.html Outdated
</li>
<li>
<p>Return a promise resolved with
<code>undefined</code>.</p>

This comment has been minimized.

@pthatcherg

pthatcherg Mar 8, 2016

Contributor

Should it resolve immediately, or once the processing is done? It seems like it shouldn't resolve until it's done.

This comment has been minimized.

@taylor-b

taylor-b Mar 10, 2016

Author Contributor

You're right. I fixed this.

network. Alternatively, the <a>ICE Agent</a> has finished checking
all existing candidates pairs and failed to find a connection for
at least one component, but is still gathering and/or waiting for
additional remote candidates.</dd>

This comment has been minimized.

@pthatcherg

pthatcherg Mar 8, 2016

Contributor

Does this mean that when we have more than one component, we will go new -> disconnected -> completed or new -> disconnected -> failed?

This comment has been minimized.

@taylor-b

taylor-b Mar 10, 2016

Author Contributor

No, there would be an intermediary state of "checking".

@alvestrand

This comment has been minimized.

Copy link
Contributor

alvestrand commented Mar 10, 2016

@taylor-b ping

taylor-b added some commits Feb 29, 2016

Enable trickling of end-of-candidates through addIceCandidate.
A null parameter in addIceCandidate now represents end-of-candidates
for all media sections.

The definitions of the `failed` and `completed` ICE connection states
have been updated so that they only occur when there are no more
remote candidates.

Also updated the example code that trickled candidates, such that the
null candidate is trickled rather than ignored.

@taylor-b taylor-b force-pushed the taylor-b:issue483_trickle_end_of_candidates branch from 70b65a8 to 13b1a68 Mar 10, 2016

taylor-b added some commits Mar 16, 2016

Remove the addIceCandidate ICE restart caveat.
This should be addressed on a broader scale, for both candidates and
end-of-candidates notifications, in JSEP.
See: rtcweb-wg/jsep#250
@aboba

This comment has been minimized.

Copy link
Contributor

aboba commented Mar 17, 2016

LGTM.

@elagerway

This comment has been minimized.

Copy link
Contributor

elagerway commented Mar 17, 2016

LGTM

alvestrand added a commit that referenced this pull request Mar 17, 2016

Merge pull request #527 from taylor-b/issue483_trickle_end_of_candidates
Enable trickling of end-of-candidates through addIceCandidate.

@alvestrand alvestrand merged commit 054249c into w3c:master Mar 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment