paullouisageneau / libdatachannel

C/C++ WebRTC network library featuring Data Channels, Media Transport, and WebSockets
https://libdatachannel.org/
Mozilla Public License 2.0
1.74k stars 354 forks source link

Application mid has a fixed value, which can override other media #585

Closed pimterry closed 2 years ago

pimterry commented 2 years ago

When present, it seems like the application stream always uses mid '0', even if that mid is already in use. If there's a conflict, the other stream is not included in the offer SDP.

I'm reproducing this with node-datachannel v0.2.4, but I'm fairly sure it's coming from here internally. Repro:

NDC = require('node-datachannel');
p = new NDC.PeerConnection('test', { iceServers: [] });

p.addTrack(new NDC.Video("0", "SencRecv"));
p.createDataChannel('test');

console.log(p.localDescription().sdp);

This prints the following SDP:

v=0
o=rtc 1573040060 0 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE 0
a=msid-semantic:WMS *
a=setup:actpass
a=ice-ufrag:JhWK
a=ice-pwd:2FrMgprkaYb13pcs4OLNLE
a=fingerprint:sha-256 98:CB:ED:D3:78:CC:5C:5A:DE:D7:D2:93:71:D7:4B:64:6F:4E:D4:F9:BB:54:48:EA:C9:74:59:DC:7C:30:D9:D6
m=application 40549 UDP/DTLS/SCTP webrtc-datachannel
c=IN IP4 172.16.3.224
a=bundle-only
a=mid:0
a=sendrecv
a=sctp-port:5000
a=max-message-size:262144
a=candidate:1 1 UDP 2122317823 172.16.3.224 40549 typ host
a=candidate:2 1 UDP 2122317567 172.22.0.1 40549 typ host
a=candidate:3 1 UDP 2122317311 172.19.0.1 40549 typ host
a=candidate:4 1 UDP 2122317055 172.18.0.1 40549 typ host
a=candidate:5 1 UDP 2122316799 172.20.0.1 40549 typ host
a=candidate:6 1 UDP 2122316543 172.21.0.1 40549 typ host
a=candidate:7 1 UDP 2122316287 192.168.104.248 40549 typ host
a=end-of-candidates

Running it again, but using p.addTrack(new NDC.Video("1234", "SencRecv")) instead (i.e. using "1234" as the mid for the video stream instead of "0") returns the below:

v=0
o=rtc 2364435378 0 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE 0 1234
a=group:LS 1234
a=msid-semantic:WMS *
a=setup:actpass
a=ice-ufrag:MxSH
a=ice-pwd:rIn/QCXOaF8aIX/McdSqui
a=fingerprint:sha-256 DD:D9:8E:84:50:90:F3:99:AA:DA:A7:0E:EC:AC:ED:13:EA:CE:6D:DA:DF:7B:BE:97:10:8D:1F:26:B0:0D:89:57
m=application 56041 UDP/DTLS/SCTP webrtc-datachannel
c=IN IP4 172.16.3.224
a=bundle-only
a=mid:0
a=sendrecv
a=sctp-port:5000
a=max-message-size:262144
a=candidate:1 1 UDP 2122317823 172.16.3.224 56041 typ host
a=candidate:2 1 UDP 2122317567 172.22.0.1 56041 typ host
a=candidate:3 1 UDP 2122317311 172.19.0.1 56041 typ host
a=candidate:4 1 UDP 2122317055 172.18.0.1 56041 typ host
a=candidate:5 1 UDP 2122316799 172.20.0.1 56041 typ host
a=candidate:6 1 UDP 2122316543 172.21.0.1 56041 typ host
a=candidate:7 1 UDP 2122316287 192.168.104.248 56041 typ host
a=end-of-candidates
m=video 56041 UDP/TLS/RTP/SAVPF
c=IN IP4 172.16.3.224
a=bundle-only
a=mid:1234
a=rtcp-mux

Note the 5 extra lines at the end for the video stream that are lost in the previous example.

This affects my use case because I'm proxying WebRTC streams between other clients, I'd like to preserve the mids selected by those clients, and Chrome at least happily uses mid 0 for its video streams. Seems like this would affect anybody who explicitly uses "0" as their media mid in an offer though.

It would be good if the application stream used an unused mid, instead of always using 0. More generaly, it'd would be nice to have a way to set an explicit mid for the application stream, both because that would provide a workaround for this and more generally.

(Sorry I'm filing so many issues! NodeDataChannel/libdatachannel is working really well for me and I'm just getting into the meat of it at the moment, I'll having so many complicated issues soon :smile:. I hope this one is actually a real bug!)

pimterry commented 2 years ago

Found a further strange edge case here in fact.

This first example below, which uses a non-conflicting 1234 mid, does correctly create SDP with both video & application media, which is good:

NDC = require('node-datachannel');
p = new NDC.PeerConnection('test', { iceServers: [] });

p.addTrack(new NDC.Video("1234", "SencRecv"));
p.createDataChannel('test');

console.log(p.localDescription().sdp); // Includes m=application and m=video

If you swap the track/channel setup lines though so that the channel is created first, as below, then it doesn't include the video at all:

NDC = require('node-datachannel');
p = new NDC.PeerConnection('test', { iceServers: [] });

p.createDataChannel('test');
p.addTrack(new NDC.Video("1234", "SencRecv"));

console.log(p.localDescription().sdp); // Only includes m=application

Is there a good reason for that? Seems very odd to me.

paullouisageneau commented 2 years ago

When present, it seems like the application stream always uses mid '0', even if that mid is already in use. If there's a conflict, the other stream is not included in the offer SDP.

Indeed, the library prevents conflicts with existing remote tracks, but not local ones, which is an oversight. It should be fixed by https://github.com/paullouisageneau/libdatachannel/pull/586

(Sorry I'm filing so many issues! NodeDataChannel/libdatachannel is working really well for me and I'm just getting into the meat of it at the moment, I'll having so many complicated issues soon smile. I hope this one is actually a real bug!)

On the contrary, it is great that you test corner cases like this one and take the time to write such detailed bug reports, thank you!

If you swap the track/channel setup lines though so that the channel is created first, as below, then it doesn't include the video at all:

NDC = require('node-datachannel');
p = new NDC.PeerConnection('test', { iceServers: [] });

p.createDataChannel('test');
p.addTrack(new NDC.Video("1234", "SencRecv"));

console.log(p.localDescription().sdp); // Only includes m=application

Is there a good reason for that? Seems very odd to me.

This is actually an expected behavior. By default, createDataChannel() calls setLocalDescription() internally while addTrack() does not, as it would prevent adding multiple tracks. Therefore, you need to call createDataChannel() (possibly multiple times) after addTrack() (possibly multiple time).

To disable this behavior, there is a flag called disableAutoNegotiation in the PeerConnection configuration. When it is enabled, the user is responsible for calling setLocalDescription() after createDataChannel(), addTrack(), and setRemoteDescription(). However, I'm seeing now that this flag appears to be missing in the node-datachannel wrapper.

pimterry commented 2 years ago

It should be fixed by https://github.com/paullouisageneau/libdatachannel/pull/586

Perfect, thanks!

To disable this behavior, there is a flag called disableAutoNegotiation in the PeerConnection configuration. When it is enabled, the user is responsible for calling setLocalDescription() after createDataChannel(), addTrack(), and setRemoteDescription(). However, I'm seeing now that this flag appears to be missing in the node-datachannel wrapper.

That all makes sense, and I see you've already opened https://github.com/murat-dogan/node-datachannel/pull/96 to fix it, thank you! Manually managing negotiation is definitely going to work better for my use case, that's great.

paullouisageneau commented 2 years ago

You're welcome, I'm closing this as it looks solved.