Closed lminiero closed 1 year ago
apparently pion is hardcoded to assume that when the mid is called "audio" or "video" you're using the legacy plan-b rather than unified plan, even when that's clearly not the case.
I wonder if this is why I had to force Unified plan here: https://github.com/pion/example-webrtc-applications/pull/34
It'd be worth filing an issue upstream with pion if it's trivial to reproduce.
I think the pion developers know already, but anyway it makes sense for us to shorten those mid values anyway.
I think the pion developers know already, but anyway it makes sense for us to shorten those mid values anyway.
When I brought it up the response from @Sean-Der was "Woops! I thought I fixed that " :laughing: but yes it definitely makes sense to shorten here regardless.
Do you have an example SDP that's obviously not plan-b that pion misdetects as such?
Do you have an example SDP that's obviously not plan-b that pion misdetects as such?
I think @atoppi stumbled upon that behaviour recently, but I don't have any SDP myself.
Actually the use case that triggered the issue was quite complex:
1) use janus 0.x (so m=audio
and m=video
lines)
2) use UnifiedPlanWithFallback
SDP semantics in Pion
3) Register the mid extension in the pc mediaEngine
4) add a recv-only transceiver (through AddTransceiverFromKind
) before starting the negotiation
5) Receive the offer (that also contains the mid ext) from janus and complete the negotiation
What happens is that pion thinks that planB is being used since I'm using the unified plan with fallback semantics. As a consequence the transceiver added ahead of time at (4) will not be used and its mid will never be set.
The fix was just to switch to UnifiedPlan semantics (w/o fallback).
I'll try to collect sdps in the next few days.
I confirm the PR fixes the issue.
As I said, having set UnifiedPlanWithFallback
as semantics, pion will detect plan-b here because of audio
/video
mids and will miss the transceiver mid setting here.
@tmatth This is the remote offer from Janus.
v=0
o=- 1671620765108149 1 IN IP4 192.168.1.21
s=VideoRoom 100
t=0 0
a=group:BUNDLE audio video
a=extmap-allow-mixed
a=msid-semantic: WMS janus
m=audio 9 UDP/TLS/RTP/SAVPF 111
c=IN IP4 192.168.1.21
a=sendonly
a=mid:audio
a=rtcp-mux
a=ice-ufrag:0/qM
a=ice-pwd:awnMCfUNivInPYftR7uIef
a=ice-options:trickle
a=fingerprint:sha-256 C7:F9:AE:6F:FA:BF:EC:7B:0B:0F:1D:4D:AC:9A:1D:D8:1F:37:B1:ED:E7:A8:2B:EB:3E:C7:80:A1:F0:96:EE:6F
a=setup:actpass
a=rtpmap:111 opus/48000/2
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
a=rtcp-fb:111 transport-cc
a=msid:janus janusa0
a=ssrc:4030556587 cname:janus
a=ssrc:4030556587 msid:janus janusa0
a=ssrc:4030556587 mslabel:janus
a=ssrc:4030556587 label:janusa0
a=candidate:1 1 udp 2015363327 192.168.1.21 43373 typ host
a=end-of-candidates
m=video 9 UDP/TLS/RTP/SAVPF 96 97
c=IN IP4 192.168.1.21
a=sendonly
a=mid:video
a=rtcp-mux
a=ice-ufrag:0/qM
a=ice-pwd:awnMCfUNivInPYftR7uIef
a=ice-options:trickle
a=fingerprint:sha-256 C7:F9:AE:6F:FA:BF:EC:7B:0B:0F:1D:4D:AC:9A:1D:D8:1F:37:B1:ED:E7:A8:2B:EB:3E:C7:80:A1:F0:96:EE:6F
a=setup:actpass
a=rtpmap:96 VP8/90000
a=rtcp-fb:96 ccm fir
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli
a=rtcp-fb:96 goog-remb
a=rtcp-fb:96 transport-cc
a=extmap:1 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=rtpmap:97 rtx/90000
a=fmtp:97 apt=96
a=msid:janus janusv0
a=ssrc:2414377991 cname:janus
a=ssrc:2414377991 msid:janus janusv0
a=ssrc:2414377991 mslabel:janus
a=ssrc:2414377991 label:janusv0
a=candidate:1 1 udp 2015363327 192.168.1.21 43373 typ host
a=end-of-candidates
I confirm the PR fixes the issue.
Ack, merging then :v:
0.x
historically defaulted toaudio
,video
anddata
as mids in the SDP when not provided by the peer or the plugin. This was problematic for a couple of reasons:This patch addresses this by using much more compact defaults instead, namely
a
,v
andd
. This seems to be working as expected in the short test I made (RTP extensions are updated accordingly), but you may want to double check if this breaks anything for you. Planning to merge fairly quickly, so don't wait too long :wink: