-
Notifications
You must be signed in to change notification settings - Fork 21
Always include track id in MSID/SSRC attributes #221
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #221 +/- ##
==========================================
- Coverage 87.70% 87.63% -0.08%
==========================================
Files 50 50
Lines 2684 2693 +9
==========================================
+ Hits 2354 2360 +6
- Misses 330 333 +3
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
lib/ex_webrtc/rtp_transceiver.ex
Outdated
[%ExSDP.Attribute.SSRC{id: ssrc, attribute: "msid", value: "- #{id}"}] | ||
|
||
nil -> | ||
[%ExSDP.Attribute.SSRC{id: ssrc, attribute: "msid", value: "- -"}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit worried that - -
may hit us in the future but as long as it works...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you suggest instead? Chrome, for example, uses some random ID that imitates a track ID (that doesn't exist).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the code responsible for parsing MSID in chromium: https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/webrtc_sdp.cc;l=1812-1820?q=msid:&ss=chromium
Indeed, they don't allow for duplicate track_id
We could either try to add random track id or revert these changes at all. Would be nice to check how browser generates this random id. Found this: https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/webrtc_sdp.cc;l=1386?q=msid:&ss=chromium
but this code seems to assume there is always a track? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is how they create RTPVideoSender: https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/peer_connection.cc;l=1175-1176;drc=b8b5768e5d0d1c2a84fe4896eae884d97fd1131e;bpv=1;bpt=1
They just generate random UUID as id for this sender. This ID, then, seems to be used as track_id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They seem to generate sdp offer from something called MediaDescriptionOptions
This MediaDescriptionOptions has two methods: AddAudioSender and AddVideoSender. Both of them take track_id as an argument.
Looking for their references I only found this: https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/sdp_offer_answer.cc;l=739;drc=b8b5768e5d0d1c2a84fe4896eae884d97fd1131e;bpv=1;bpt=1
which suggests using plan B, which is suspicious, but on the other hand, as track id they pass sender->id(). This id seems to be random uuid generated in the link in the previous comment.
I can't find when and how they use track id though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to sum up, I think we could try to use our sender's id. The only question is whether its current form is good enough as we use :crypto.strong_rand_bytes(12) to generate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we use the same function for track_id so we should be good with using sender's id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose the approach with sender_id as the default value.
747f3be
to
b22d21c
Compare
Closes #219