onicecandidate handling about node-webrtc

[modeswitch]

I don't think I wrote code to handle a null candidate, so that's definitely an issue. The fix is straightforward though.

[modeswitch]

Actually, I'm not sure the underlying libjingle API behaves that way. The code I have would probably fail in that case, which I haven't seen. I'll investigate.

[DamonOehlman]

Cool - thanks. I've not touched the underlying libjingle code so you may well be right.

Another thing I just noticed is that the signature of the onicecandidate event argument differs from what I am used to handling in the browser. I've attempted to copy and paste an RTCIceCandidateEvent instance from chrome and format it correctly for markdown:

RTCIceCandidateEvent
  - bubbles: false
  - cancelBubble: false
  - cancelable: false
  - candidate: RTCIceCandidate
    - candidate: "a=candidate:2063663814 1 udp 2113937151 192.168.1.198 56333 typ host generation 0
    ↵"
    - sdpMLineIndex: 0
    - sdpMid: "audio"
    - __proto__: RTCIceCandidate
  - clipboardData: undefined
  - currentTarget: RTCPeerConnection
  - defaultPrevented: false
  - eventPhase: 0
  - returnValue: true
  - srcElement: RTCPeerConnection
  - target: RTCPeerConnection
  - timeStamp: 1388732135817
  - type: "icecandidate"
  - __proto__: RTCIceCandidateEvent

Basically, the difference in the browser there is a double nesting of candidate information:

RTCIceCandidateEvent
  - candidate
     - candidate
     - sdpMid
     - sdpMLineIndex

whereas in the current implementation of node-webrtc candidate, sdpMid and sdpMLineIndex are all top level members of the event object:

{ candidate: 'a=candidate:4199089266 2 udp 1845501695 101.164.36.236 45493 typ srflx raddr 192.168.1.198 rport 45493 generation 0\r\n',
  sdpMid: 'audio',
  sdpMLineIndex: 0 }

I agree it probably seems a bit ridiculous but for maximum compatibility with the browser API I would probably suggest changing the mock event object to:

{
  candidate: {
    candidate: 'a=candidate:4199089266 2 udp 1845501695 101.164.36.236 45493 typ srflx raddr 192.168.1.198 rport 45493 generation 0\r\n',
    sdpMid: 'audio',
    sdpMLineIndex: 0 
  }
}

[DamonOehlman]

PS. A couple of commented lines in the current connect.js tests show how nice tape is for quickly switching between testing functionality in the browser vs testing from the command line:

https://github.com/DamonOehlman/node-webrtc/blob/tape-tests/test/connect.js#L2

Basically I uncomment those lines and comment out the node-webrtc peerconnection.js include and run the file with beefy (beefy test/connect.js) to check that the code behaves as I would expect in the browser and look to see if we achieve parity in node-webrtc.

[modeswitch]

I can fix the event format while I'm at it.

Re: tape, that's pretty nice :)

[modeswitch]

@DamonOehlman the icecandidate structure is fixed now

[DamonOehlman]

Thanks Alan - I saw some progress on the repo, just hadn't gotten around to trying it out again. Will have a look soon.

[modeswitch]

I think the best thing here is to handle this on the JS side by emitting a null candidate event when the ICE gathering state changes. Thoughts?

[DamonOehlman]

Sounds good to me.

[cjb]

Hi! I'm still seeing this (onicecandidate() is never called with a null argument) with the current HEAD of the develop branch.

I think that @modeswitch's proposal earlier in the thread -- "handle this on the JS side by emitting a null candidate event when the ICE gathering state changes" -- might not work. The reason is that onicegatheringstatechange() fires three times before ICE is complete here, yet the null call is only supposed to come when ICE is finished (and that's the use case I'd be comparing against null for).

Or maybe you meant that you'll only fire onicecandidate(null) when onicegatheringstatechange() fires and the state is newly connected. That sounds like it would follow the API.

[DamonOehlman]

@cjb As far as browser behaviour, the browser doesn't trigger the event with a null argument but rather a null value for the .candidate attribute of the event. So generally, your handling code looks something like:

pc.onicecandidate = function(evt) {
  if (evt.candidate) {
    // send the candidate via a signalling channel to your peer
  }
  else {
    // we're done :)
  }
};

It's been a while since I've had a look at the node-webrtc project but I'll pick it up again and have another look to see if this is the way it behaves also.

[cjb]

@DamonOehlman Thanks, you're right. I can confirm that node-webrtc never passes through a null value for the .candidate attribute of the event -- every callback gets a filled candidate attribute here.

[DamonOehlman]

OK, if that's the case then a lot of code that works with WebRTC in the browser won't be compatible. It was definitely working at one point in time (I recall from when I originally wrote the tape test suite), so I wonder if @modeswitch has opted for a different approach...

[modeswitch]

I haven't changed this. Can you confirm that the tests cover this? Also, if either of you can submit a patch with a fix I'll merge it.

On June 6, 2014 11:43:39 PM EDT, Damon Oehlman [email protected] wrote:

OK, if that's the case then a lot of code that works with WebRTC in the
browser won't be compatible. It was definitely working at one point in
time (I recall from when I originally wrote the tape test suite), so I
wonder if @modeswitch has opted for a different approach...

---

Reply to this email directly or view it on GitHub:
#44 (comment)

[DamonOehlman]

I just had a hunt through and it looks like I implemented a timer to monitor the iceGatheringState in the tests to get around the fact that it isn't implemented:

https://github.com/js-platform/node-webrtc/blob/develop/test/helpers/capture-candidates.js

If @cjb doesn't beat me to it, I'll look at implementing the patch.

[cjb]

@DamonOehlman 👍, works great.

[modeswitch]

Great! I noticed that the state events got messed up somewhere so I'll fix those and merge as soon as I'm back at my computer.

On June 7, 2014 1:02:08 PM EDT, Chris Ball [email protected] wrote:

@DamonOehlman 👍, works great.

---

Reply to this email directly or view it on GitHub:
#44 (comment)

[DamonOehlman]

The first commit in the PR cleans up a few copy and paste bugs that look to have been introduced at some stage. I think that's what you are referring to, and yes you should definitely double check that I've done the right thing, but I'm 99.9% I have :)

[modeswitch]

I think this is fixed now.