MapPortHandler Callback for container support

[xinze-zheng]

Description

Expose a handler func(candidate Candidate) int handler that will be called upon addCandidate. The handler will set the mappedPort private field. During unmarshal, if mappedPort is not 0, it will replace the port in ICE candidate string.

Reference issue

pion/webrtc#3155

[codecov]

Codecov Report

❌ Patch coverage is 80.95238% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.33%. Comparing base (4da5bc1) to head (3ded026).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
gather.go	77.77%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #831      +/-   ##
==========================================
+ Coverage   88.27%   88.33%   +0.06%     
==========================================
  Files          43       43              
  Lines        5509     5538      +29     
==========================================
+ Hits         4863     4892      +29     
  Misses        451      451              
  Partials      195      195              

Flag	Coverage Δ
go	88.33% <80.95%> (+0.06%)	⬆️

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.

[JoTurk]

Thank you so much, I like this direction let me know when it's ready for test and review.

[JoTurk]

We've stopped adding new configurations to the AgentConfig struct since it's planned for deprecation in favor of a more Pion-aligned API. this feature should instead be exposed as an option, WithMapPortHandler, and used with the new NewAgentWithConfig API. Users who want to use this should migrate to the new API.

we did the same thing recently with the renomination API: #822 (comment)

[JoTurk]

question, do we have to make this public?

[xinze-zheng]

Should we? Will webrtc care about this? I was thinking for design of separted ICE (maybe the future ion) this will be somehow used. What do you think?

[JoTurk]

I think we should keep it private for now, unless we need it, or someone requests that we expose it later.

[JoTurk]

@xinze-zheng I'm sorry about the internal API change, we had to fix the options so we can upgrade webrtc to use them.

[JoTurk]

I think it would be better if we can change the test to use the new Public api instead, NewAgentWithOptions, so it doesn't break if we change the internal API in the future, and so it matches the user contract.
But this is up to you.

[JoTurk]

@xinze-zheng hello, we merged the options API and added the new Address rewrite API if you want to continue working on this.

[JoTurk]

@xinze-zheng I'll try to test and review this today or by tomorrow, thank you a lot :)

[xinze-zheng]

@JoTurk Hi I've been looking into integrating this feature with webrtc. Seems we need to replace webrtc.ICECandidate.Port with mappedPort in webrtc.newICECandidateFromICE (or the information of mappedPort is lost). I think we can implement via

	port := candidate.Port()
	if candidate.GetMappedPort() != 0 {
		port = candidate.GetMappedPort()
	}

we just need to make GetMappedPort public in ice.candidate.

[JoTurk]

@xinze-zheng I think the only clean path if we go this route is to make a custom extension for mapped ports but i think it's maybe too much. I changed webrtc's candidates so it keeps ICE extensions when converting. Not sure about exposing mapped ports as a helper (but we can do this if no other solution).

I'm maybe missing something, but why we can't overwrite candidates the same we do with addresses?

[xinze-zheng]

@JoTurk Hello, I've changed the behavior of mapPort to overwrite the port field of a potential ice candidate.

[JoTurk]

I think we'll either need to remove the candidate type filter or make it possible to add multiple filters for different candidates types.
I think removing is better because the user filter have access to the candidate type.

[xinze-zheng]

Agree! Updated.