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

RTCIceCandidateComplete dictionary #207

Closed
robin-raymond opened this Issue May 26, 2015 · 12 comments

Comments

Projects
None yet
6 participants
@robin-raymond
Contributor

robin-raymond commented May 26, 2015

Testing for an empty object is not straight forward in JavaScript. It would be easier to add a name / value pairing like this when stringified:

{ "complete": true }

So we should have complete in the RTCIceCandidateComplete dictionary which is mandatory and set to true.

@robin-raymond

This comment has been minimized.

Contributor

robin-raymond commented May 26, 2015

Sample code please (in RTCIceGatherer)...

@aboba aboba removed the PR exists label Jun 3, 2015

@aboba aboba added the PR exists label Jun 11, 2015

robin-raymond pushed a commit that referenced this issue Jun 22, 2015

Robin Raymond
Addressed Philipp Hancke's review comments, as noted in: Issue #198
Added the "failed" state to RTCIceTransportState, as noted in: Issue #199
Added text relating to handling of incoming media packets prior to remote fingerprint verification, as noted in: Issue #200
Added a complete attribute to the RTCIceCandidateComplete dictionary, as noted in: Issue #207
Updated the description of RTCIceGatherer.close() and the "closed" state, as noted in: Issue #208
Updated Statistics API error handling to reflect proposed changes to the WebRTC 1.0 API, as noted in: Issue #214
Updated Section 10 (RTCDtmfSender) to reflect changes in the WebRTC 1.0 API, as noted in: Issue #215
Clarified state transitions due to consent failure, as noted in: Issue #216
Added a reference to [FEC], as noted in: Issue #217
@juberti

This comment has been minimized.

juberti commented Jun 24, 2015

This seems kind of bizarre, an event with a variable that is only set to a single value.

Can't this be handled entirely through IceGathererStateChange?

@robin-raymond

This comment has been minimized.

Contributor

robin-raymond commented Jun 24, 2015

We don't really need to generate RTCCandidateComplete at all because we know from the state, but how do we tell the remote RTCIceTransport that "candidates complete" is reached. We currently do that by adding this dictionary to "RTCIceTransport.addRemoteCandidate(candidateComplete)".

@pthatcherg

This comment has been minimized.

pthatcherg commented Jun 24, 2015

Could we add a method that is effectively RTCIceTransport.expectNoMoreCandidates()? That might be more simple.

Perhaps call it "RTCIceTransport.completeRemoteCandidates()"

@robin-raymond

This comment has been minimized.

Contributor

robin-raymond commented Jun 24, 2015

@pthatcher certainly can be done that way instead (think it's better).

@aboba aboba removed the PR exists label Jun 24, 2015

@pthatcherg

This comment has been minimized.

pthatcherg commented Jun 24, 2015

According to http://w3c.github.io/webrtc-pc/#widl-RTCPeerConnection-addIceCandidate-Promise-void--RTCIceCandidate-candidate

PeerConnection.addIceCandidate(IceCandidate) takes a non-nullable candidate. So going down the "overload addRemoteCandidate" path provides no similarity with WebRTC 1.0. That makes me more convinced we should go with a separate method of "RTCIceTransport.completeRemoteCandidates".

But we should write some sample code to make sure what we're coming up with makes sense.

@robin-raymond

This comment has been minimized.

Contributor

robin-raymond commented Sep 21, 2015

This same issue existing in WebRTC 1.0 (which does not have a nullable candidate option to addIceCandidate in 1.0); Not sure how 1.0 even does this at all...

@ibc

This comment has been minimized.

Contributor

ibc commented Dec 26, 2015

how do we tell the remote RTCIceTransport that "candidates complete" is reached. We currently do that by adding this dictionary to "RTCIceTransport.addRemoteCandidate(candidateComplete)".

IMHO that is valid as a network signaling fuideline and should not be defined by the spec. The very same can be achieved as follows:

// Sender:
mySignaling.send('no more candidates');

// Receiver:
mySignaling.on('msg', function(data) {
  if (data === 'no more candidates') {
    iceTransport.completeRemoteCandidates();
  }
}
@aboba

This comment has been minimized.

Contributor

aboba commented Mar 16, 2016

Currently there is a PR in WebRTC 1.0 to allow the null candidate as an argument to addIceCandidate():
w3c/webrtc-pc#527

It is awkward to have RTCIceCandidateComplete defined differently in WebRTC 1.0 and ORTC.

@robin-raymond

This comment has been minimized.

Contributor

robin-raymond commented Mar 16, 2016

@aboba If my memory serves me correctly, I believe it was Google who did not want to use "null" as an end of candidate scenario which is why we did the specialized "completed" candidate object. I don't personally care what we do but I prefer consistency.

CC: @taylor-b @pthatcherg

@taylor-b

This comment has been minimized.

taylor-b commented Mar 16, 2016

Well, in ORTC, an RTCIceCandidateComplete is emitted by onlocalcandidate and passed into addRemoteCandidate.

And in WebRTC 1.0, null is emitted by onicecandidate and passed into addIceCandidate.

So at least for both specs, the things that are emitted look the same as the things that are passed down.

I prefer the RTCIceCandidateComplete approach, but with WebRTC there are applications depending on the "null" candidate for indicating the end of gathering, which is why we decided to leave it that way, since no one had strong opinions. Though if we end up deciding to trickle the ufrag/password (to resolve ICE restart ambiguities), we'll probably want to trickle that with "end-of-candidates" too, in which case it would need to become a dictionary.

@aboba

This comment has been minimized.

Contributor

aboba commented Apr 7, 2016

@robin-raymond There seems to be a preference for leaving the current RTCIceCandidateComplete definition alone. Can we close this issue or is there more to do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment