Skip to content
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

How do I indicate the end of remote candidates? #1952

Closed
boldt opened this issue Aug 1, 2018 · 16 comments
Closed

How do I indicate the end of remote candidates? #1952

boldt opened this issue Aug 1, 2018 · 16 comments
Assignees
Labels

Comments

@boldt
Copy link

boldt commented Aug 1, 2018

Following the spec, it's not clear to me how to indicate the end of remote candidates.

You write:

This method can also be used to indicate the end of remote candidates when called with an empty string for the candidate member

and

This can be indicated by calling addIceCandidate with a candidate value whose candidate property is set to an empty string

and

If candidate.candidate is an empty string, process candidate as an end-of-candidates indication

and

If this RTCIceCandidate represents an end-of-candidates indication, candidate is an empty string.

How do you mean this?

  1. pc.addIceCandidate('');
  2. pc.addIceCandidate({candidate:''});
  3. pc.addIceCandidate(new IceCandidate(''));
  4. pc.addIceCandidate(new IceCandidate({candidate:''}));

Non of them works for me in Chrome 67. Probably its not implemented yet?

@ibc
Copy link

ibc commented Aug 1, 2018

So much artifact makes me crazy. APIs are free and semantics is good:

pc.endCandidates();

@alvestrand
Copy link
Contributor

The complexity is compounded by the fact that end-of-candidates needs to specify which media section and generation they end.

@ibc
Copy link

ibc commented Aug 2, 2018

The complexity is compounded by the fact that end-of-candidates needs to specify which media section and generation they end.

Let's forget SDP, "media sections", transceivers and other artifacts.

It's clear that end-of-candidates should just affect a specific RtcIceTransport, right? And it happens that, in WebRTC, the ICE transport is the lowest layer in-the-wire. On top of the ICE transport other layers are carried (such as media layers).

So iceTransport.endRemoteCandidates().

@jan-ivar
Copy link
Member

jan-ivar commented Aug 2, 2018

  1. and 3. are wrong. addIceCandidate does not take a string as argument, and neither does the mostly-legacy RTCIceCandidate constructor.

@aboba
Copy link
Contributor

aboba commented Aug 2, 2018

The text examples all appear consistent with each other. So perhaps we can add an example (2).

@alvestrand
Copy link
Contributor

Yes, Chrome has a bug.

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-addIceCandidate-expected.txt?q=addIceCandidate+file:wpt&sq=package:chromium&l=6&dr=C

FAIL Add with empty candidate string (end of candidate) should succeed promise_test: Unhandled rejection with value: object "OperationError: Failed to execute 'addIceCandidate' on 'RTCPeerConnection': Error processing ICE candidate"

@boldt
Copy link
Author

boldt commented Aug 2, 2018

@aboba An example will clarify how to do the end-of-candidates.

@jan-ivar Why is it example (2)? Reading the spec tells me Promise<void> addIceCandidate((RTCIceCandidateInit or RTCIceCandidate) candidate);, which yields to my example (4) - or do I miss something? Also developer.mozilla.org tells me to use new RTCIceCandidate(...).

@alvestrand Thanks for the response with the Chrome bug.

To give you a context: In my applitcation I signal offer, answer and ICE candidates. The PeerConnection.iceConnectionState for the answerer (callee) is new, checking and connected, but it never reaches the completed state. I assume, its required to inform the callee about the end-of-candidates to get in the completed state.

@jan-ivar
Copy link
Member

jan-ivar commented Aug 2, 2018

@boldt Thanks for pointing to the MDN example. It had wrong info in it, and I've fixed it now. PTAL.

Hopefully that should clarify its intended use. In short, as a user of the API, signaling of end-of-candidates should happen automatically, provided you blindly forward what onicecandidate produces until it returns null. You're probably hitting that browsers haven't all implemented this yet.

  1. Is incorrect.
  2. pc.addIceCandidate({candidate:''}); signals end of candidates.
  3. Is incorrect.
  4. is legacy. Please avoid.

@boldt
Copy link
Author

boldt commented Aug 3, 2018

@alvestrand Do you know, if Chrome provides any other legacy functionallity for end-of-candidates?

@jan-ivar I still wonder about the spec (see my previous previous comment).

Let's start with the eventhanlder onicecandidate event. It is fired, when a new RTCIceCandidate is made available to the script.. Looking into the debug console, all ICE candidates objects are of type RTCIceCandidate (not plain JS objects):

bildschirmfoto vom 2018-08-03 10 34 25

Looking into the spec, you write:

Promise<void> addIceCandidate((RTCIceCandidateInit or RTCIceCandidate) candidate);

The Promise expects an RTCIceCandidate. So, why is RTCIceCandidate now plain JS object {candidate:'...'} and not new RTCIceCandidate({candidate:'...'}) as thrown by the onicecandidate event? And why is it legacy?

I also added the end-of-candidates example (2) to MDN. Thus we have the correct version documented there as well.

@jan-ivar
Copy link
Member

jan-ivar commented Aug 3, 2018

@boldt I think you mean the function expects certain arguments. The promise is just the return value from the function, and has nothing to do with the input arguments to the function.

The function expects EITHER RTCIceCandidate OR an RTCIceCandidateInit, a plain dictionary/JS object.

An RTCIceCandidate instance is valid input to a function expecting RTCIceCandidateInit, because they have the same members. E.g.

let c1 = new RTCIceCandidate(new RTCIceCandidate({candidate: '...'}));

This is a common copy-constructor pattern in JS for data holder interfaces:

let e = new Event("test2", new Event("test", {bubbles: true}));
console.log(e.bubbles); // true

Come to think of it, this means we can simplify the spec. I'll file an issue. Thanks!

@jan-ivar
Copy link
Member

jan-ivar commented Aug 3, 2018

@boldt Unsure about your MDN change, because users aren't meant to have to signal end-of-candidates manually. Once browsers fix their bugs, onicecandidate will automatically emit the end-of-candidate candidate as a signal to the other side. One browser to another.

Users should typically not interfere with this, and merely forward candidates opaquely.

@boldt
Copy link
Author

boldt commented Aug 20, 2018

Users should typically not interfere with this, and merely forward candidates opaquely.

I totally agree.

Once browsers fix their bugs, onicecandidate will automatically emit the end-of-candidate candidate as a signal to the other side

So, how should/will the events on onicecandidates finally look like? Will an empty candidate be fired? The note in the spec says:

The null candidate event is fired to ensure legacy compatibility. New code should monitor the gathering state of RTCIceTransport and/or RTCPeerConnection.

From a users perspective, I don't want to monitor the states and detect the end of the geathering and send a "proprietary" end-of-candidates. I just want to signal the candidates fired by onicecandidate, where the last one signals the end. Thus, I expect as onicecandidate events:

  • {candidate: '...'} (non empty string)
  • ...
  • {candidate: '...'} (non empty string)
  • '' (empty string as end-of-candidate / gathering is finshed)

Thus I can feed them 1:1 into addIceCandidate():

  • addIceCandidate({candidate: '...'}) (non empty string)
  • ...
  • addIceCandidate({candidate: '...'}) (non empty string)
  • addIceCandidate('') (corresponds to my example 1)

@jan-ivar
Copy link
Member

@boldt Again, addIceCandidate does not take a string as argument.

From this spec note, you'll see the following on event.candidate (given two real candidates):

  • {candidate: '...'} (non empty string)
  • {candidate: '...'} (non empty string)
  • {candidate: ''} (empty string indicates end of generation of candidates)
  • null (null indicates iceGatheringState is "complete")

Web developers are expected to opaquely serialize everything except null to addIceCandidate():

  • addIceCandidate({candidate: '...'}) (non empty string)
  • addIceCandidate({candidate: '...'}) (non empty string)
  • addIceCandidate({candidate: ''}) (empty string)

... which completes the iceGatherningState.

@boldt
Copy link
Author

boldt commented Aug 28, 2018

You're right, since you wrote 1.and 3. are wrong.". Thus, your example is exactly what I am expecting. Thanks!

@aboba
Copy link
Contributor

aboba commented Aug 30, 2018

What is the next step on this issue?

@aboba aboba added the question label Aug 30, 2018
@jan-ivar
Copy link
Member

We've updated the MDN example, so I think this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants