Include sdpMid and sdpMLineIndex for ICECandidates returned by OneICECandidate

[joeturki]

Description

Currently, Pion returns an empty sdpMid and a 0 sdpMLineIndex. This PR ensures Pion returns the corresponding sdpMid and sdpMLineIndex for ICE candidates for clients that expects it. Fixes trickle issues.

Changes

1. ICECandidates: New fields SDPMid and SDPMLineIndex.
2. ICEGatherer: SetMediaStreamIdentification and return the correct SDPMid and SDPMLineIndex.
3. extractICEDetails: Return a struct instead of multiple values.
4. extractICEDetails refactored the media description selection to a different function.
5. Added new tests.

Reference issue

Fixes #2690
Fixes #1833

[codecov]

Codecov Report

Attention: Patch coverage is 94.84536% with 5 lines in your changes missing coverage. Please review.

Project coverage is 77.96%. Comparing base (c50ca41) to head (58d8c7b).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
sdp.go	93.47%	2 Missing and 1 partial ⚠️
peerconnection.go	81.81%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2990      +/-   ##
==========================================
+ Coverage   77.76%   77.96%   +0.20%     
==========================================
  Files          89       89              
  Lines       10550    10578      +28     
==========================================
+ Hits         8204     8247      +43     
+ Misses       1853     1841      -12     
+ Partials      493      490       -3     

Flag	Coverage Δ
go	79.53% <94.79%> (+0.21%)	⬆️
wasm	63.68% <100.00%> (+0.17%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Sean-Der]

@joeturki This is the highest quality PR I have seen in a long time. This is really impressive stuff.

When I read this I feel like I am reading code of someone who has been intimately involved with WebRTC/Go for years. I am so excited for you to be more involved :)

[Sean-Der]

Mind squashing everything into one commit and address the comments I left! Then we can merge this right away :)

[joeturki]

Thanks :) I think it's ready.

[finnhoeck]

Hey,

my company is currently working on our Webcaster product and we encountered an issue that seems related.
We are creating a peerconnection and add transceivers to it via AddTransceiverFromKind().
We were hoping to be able to call SetMid() on the tranceiver and get ice candidates emitted that have the SDPMid property set accordingly, but that does not seem to happen. The MID property is always an empty string there.

@joeturki I checked out the latest commit from your branch fix/trickle-ice-in and created a test to check if your PR will fix our issue, but it does not seem so.
A gist of that test can be found here: https://gist.github.com/finnhoeck/894298df51d694a66c7255c9530a14be
I simply copied the TestPeerConnectionTrickleMediaStreamIdentification test and changed it a bit so it uses AddTransceiverFromKind.

It would be nice if you could share your thoughts :)

Thanks and best regards
Finn

[joeturki]

@finnhoeck Yeah, this PR primarily for the issue with the answer peer, but I'll go ahead and fix this too today. Thank you for the test.

[joeturki]

@finnhoeck Can you test it again? I think this is a better approach for now, and it works for both offer and answer.

(Until i ship the multiple m= section gatherer soon).

[finnhoeck]

Hey @joeturki

I tested it in our project. I was wondering that we only receive IceCandidates for the transceiver of kind "audio" and not of kind "video". So I added a check to the TestTranceiverMediaStreamIdentification test and it seems that IceCandidates are only emitted for the first transceiver added. Please see the gist here: https://gist.github.com/finnhoeck/9fa1069670693d9e0f57b86e93634d88
When switching the AddTransceiverFromKind calls around, I see either the audio candidates not being present, or the video candidates not being present.

Does that maybe have to do with the the multiple m= section gatherer that you mentioned?

Thank you very much for your work! If I can further help with testing, please just ping me :)

[joeturki]

Yes, It's just how pion work right-now, but it's not related to this unless you have a non-bundle client. and it shouldn't be an issue.

When using ICE, a candidate needs to be gathered for each port. This takes approximately 20 ms extra for each extra "m=" section due to the NAT pacing requirements. All of this gathering can be overlapped with other things while, e.g., a web page is loading to minimize the impact. If the client only wants to generate Traversal Using Relays around NAT (TURN) or STUN ICE candidates for one of the "m=" lines and then use Trickle ICE [RFC8838] to get the non-host ICE candidates for the rest of the "m=" sections, it MAY do that and will not need any additional gathering time. rfc8843.

[joeturki]

@finnhoeck do you have a PlanB client, do you just name your audio mid audio? can you share your sdp?

[finnhoeck]

@joeturki No, we use Unified Plan. I am naming the mids for better readability. But with "audio" and "video" I meant the AddTransceiverFromKind calls. Sorry if my message was misleading.
Sure, I will share a SDP later today, I am currently in meetings

[joeturki]

@jech It's now a single function, I will fix the missing mid in the rest of the code and add tests there, I think i will do it in extractICEDetailsFromMedia too for example.

Edit: I fixed the issues. and squashed.

[jech]

Joe has requested that I review this commit, and I'm extremely embarassed: while all of the code looks correct to me, I have no idea what it does. As far as I can tell, we're carrying around some extra data, but the commit does not ever change Pion's behaviour in the case of a correct client.

So while I'm glad to declare nihil obstat, I'm not competent to order imprimatur.

[joeturki]

Joe has requested that I review this commit, and I'm extremely embarassed: while all of the code looks correct to me, I have no idea what it does. As far as I can tell, we're carrying around some extra data, but the commit does not ever change Pion's behaviour in the case of a correct client.

@jech I asked you to review because i fixed the comments you made, you were correct :)

Thanks.

[finnhoeck]

@joeturki Here is the SDP that I promised to share: https://gist.github.com/finnhoeck/5581f4214a3bad99f5304208b53f79f0

Not sure if it helps with anything, though :)

[joeturki]

@finnhoeck works as intended. a=group:BUNDLE audio video Pion only take ICE detail from bundle master section, audio in your case :)

[joeturki]

@Sean-Der it's ready for merge :)

[Sean-Der]

@joeturki fantastic :)

go ahead and merge! If someone else approves you should have a write bit, check if it works!