Add multi codec negotiation feature

[itzmanish]

Description

This extends PR #2632 and Adds the capability to have multi-codec negotiation just by setting a flag in the media engine.

Reference issue

Fixes #...

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.70%. Comparing base (c79463d) to head (36f4d04).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
settingengine.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3018      +/-   ##
==========================================
- Coverage   78.71%   78.70%   -0.01%     
==========================================
  Files          91       91              
  Lines       11347    11361      +14     
==========================================
+ Hits         8932     8942      +10     
- Misses       1928     1933       +5     
+ Partials      487      486       -1     

Flag	Coverage Δ
go	80.60% <86.66%> (-0.02%)	⬇️
wasm	63.84% <ø> (ø)

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JoeTurki]

I'm thinking maybe the original continue specifically intended for data sections, making it safe to consider adding this without a flag? not sure, I'll have to check, but thanks again Manish!

[nils-ohlmeier]

I updated the PR with a bugfix: since the MediaEngine gets copied for every new PeerConnection this new setting needs explicitly been taken care of in the copy() function.

I'm thinking maybe the original continue specifically intended for data sections, making it safe to consider adding this without a flag? not sure, I'll have to check, but thanks again Manish!

Even though this PR makes Pion more RFC compliant I would suggest to start with an optional setting, like in this PR. Followed by eventually switching the default of the setting. And hopefully if nobody opens any issues explaining why they depend on the old, non-RFC compliant behavior, we can remove the setting.

[JoeTurki]

Thanks You guys, would have been cooler if we could add this setting to SettingEngine instead, but i don't think that there is a clean way to do that :(

[nils-ohlmeier]

@JoeTurki here is an alternative proposal:

• we add a new multi codec setting to the SettingEngine
• here https://github.com/pion/webrtc/blob/master/peerconnection.go#L158 we manually go through the mediaEngine related settings in SettingEngine and call setters on the new copy of MediaEngine.

What I'm not clear on is if we should do this only if disabledMediaEngineCopy is false, or if we should do that regardless of the value of disableMediaEngineCopy.

[nils-ohlmeier]

The latest commit has my alternative proposal. Let me know which version everyone likes better.

[JoeTurki]

Thank you so much @nils-ohlmeier this makes so much sense, @itzmanish what do you think? i think we should make the mediaEngine setting private and expose it through Nils proposal via the settings engine?

[Sean-Der]

What would it break if we turned this on by default?

if a remote didn’t actually enable a codec in a media section things wouldn’t work?

[Sean-Der]

I’m curious what people think is best for users. If it’s not on by default people won’t benefit/have to debug and then will turn it on

[nils-ohlmeier]

My only concern here is that this changes how Pion so far handled things. Maybe there are services/implementations out there which for some reason rely on the fact that Pion does answer with the same codec for all media sections. Personally I would find that pretty strange, but you never know.

I guess the more likely scenario is that folks learned to work around the current behavior some way. For example I'm not sure if renegotiating one media section after another, each going through a full SDP offer/answer round, would allow you to choose different codecs. But if people have developed such workarounds I guess it is pretty unlikely that these would break.

So maybe you are right and we could make this the new default and only leave the new setting in as a way to get back to the old behavior if someone needs it.

[Sean-Der]

That's a great analysis :)

My gut says it is better for users to have this on by default. I would even leave the setting out! Until someone complains then we add it back. Otherwise it turns into one of those things that is in the code base forever that never gets cleaned up.

Maybe I should be more conservative. I feel bad we are shipping something today that is subpar, I just want users to git pull and get the better version without jumping through hoops. Especially if no one is disrupted!

[itzmanish]

what do you think? i think we should make the mediaEngine setting private and expose it through Nils proposal via the settings engine?

yeah, because not everyone uses custom MediaEngine struct and pass on the API to create PeerConnection and it would be nice to have this as a config in setting engines so everyone can use it easily.

[itzmanish]

A different issue I have noticed, though I am not sure how this works with the spec guideline. The updateHeaderExtensionFromMediaSection is called from the default block of the 2nd switch case as well as at the end of the method https://github.com/pion/webrtc/pull/3018/files#diff-045bc12c2f054653bc94bdbcbdd27f63b14536b7b5b34f451633f14ea1d8c1b0L677.

Now even if we don't consider multi codec enabled scenario, the default case will be hit for all subsequent codecs in offer even the audio/video is negotiated. In that case if the offer have different extensionId for header extensions they will be stored in negotiatedHeaderExtension map which is being used by peerconnection when doing handleIncomingSSRC. This may pick up wrong extension id for the track on the incoming MID.

Questions -

1. Is it allowed to have a different extension ID for the same extension in different media sections?
2. Should Pion handle different extension IDs for the same extension for different media sections and correctly use the extension ID for a MID's incoming track?

[nils-ohlmeier]

That's a great analysis :)

My gut says it is better for users to have this on by default. I would even leave the setting out! Until someone complains then we add it back. Otherwise it turns into one of those things that is in the code base forever that never gets cleaned up.

Maybe I should be more conservative. I feel bad we are shipping something today that is subpar, I just want users to git pull and get the better version without jumping through hoops. Especially if no one is disrupted!

Alright. I see your point for making this the new default behavior. Makes sense.

I would suggest to have the setting in there for folks to easily disable if needed. I can put a reminder in my calendar to remove it, or maybe we could put it on the todo list for the next release after the next? But if you @Sean-Der prefer to config option I'll remove it. Just let me know.

[nils-ohlmeier]

A different issue I have noticed, though I am not sure how this works with the spec guideline. The updateHeaderExtensionFromMediaSection is called from the default block of the 2nd switch case as well as at the end of the method https://github.com/pion/webrtc/pull/3018/files#diff-045bc12c2f054653bc94bdbcbdd27f63b14536b7b5b34f451633f14ea1d8c1b0L677.

Now even if we don't consider multi codec enabled scenario, the default case will be hit for all subsequent codecs in offer even the audio/video is negotiated. In that case if the offer have different extensionId for header extensions they will be stored in negotiatedHeaderExtension map which is being used by peerconnection when doing handleIncomingSSRC. This may pick up wrong extension id for the track on the incoming MID.

Questions -

1. Is it allowed to have a different extension ID for the same extension in different media sections?
2. Should Pion handle different extension IDs for the same extension for different media sections and correctly use the extension ID for a MID's incoming track?

Yes you are right that this is a complicated and related topic.

TL;DR: within the same bundle group the extension IDs need to the same across all the media sections, which belong to the same bundle group.
That being said each media section can negotiate it's own set of extensions.

I'm not entirely sure that even all browsers handle all of the edge cases for this properly.

I would suggest we open a new ticket to tackle that topic.

[itzmanish]

yeah, let's discuss that seperately #3087 to avoid getting out of track from the scope of this PR.

[Sean-Der]

@nils-ohlmeier go for it! Sounds like a great plan to me.

I really appreciate everyone being a conservative. A breaking release causes a lot of trouble. It’s just so tempting to get improvements out as fast as possible

[nils-ohlmeier]

@JoeTurki @itzmanish could you take another look at the PR to double check that I didn't screw up something with my last commits?

[3DRX]

Just a small question, why is setMultiCodecNegotiation called only when disableMediaEngineCopy is false?

[JoeTurki]

Good question if you look at nils comment:

What I'm not clear on is if we should do this only if disabledMediaEngineCopy is false, or if we should do that regardless of the value of disableMediaEngineCopy.

I think we should set it regardless, We're currently waiting on @itzmanish review for the changes!

[nils-ohlmeier]

Indeed I'm unsure on how to handle that one.
One argument is that if copying is disable no settings from the initial MediaEngine should be transferred over.
I think the other view is: this is now a global setting and thus should get applied to all MediaEngine instances.

I'm okay doing it either way.

[itzmanish]

Since this is a global setting now, I think we should always respect that when copying the mediaengine. However api have setting engine inside it and it won't get changed once the api is made, I don't think we need to explicitly set the multiCodecNegotiation flag from setting engine because it would be same.

Also currently It's not possible to set mediaengine's multicodec flag directly because they are now private field which means if you want to disable multicodec in the middle (which is weird case where you want multicodec for some instances of peerconnection but not for other), you have to change the settingengine directly.

[itzmanish]

@JoeTurki The changes are fine from my side and let's merge this.