Add PeerConnection::setConfiguration() method

[murillo128]

This PR allows changing the peerconnection configuration before the ICE transport has been inited on sLD or sRD.

This is needed in order to support setting the STUN/TURN servers received on the POST answer of a WHIP call.

[paullouisageneau]

Thank you for the PR, it looks good, basically my only comment is that I think setConfiguration() should accept the configuration by value and move it instead of accepting a reference.

[murillo128]

In order to fully support the WHIP stun/turn configuration I would need to add another change, but I am not sure exactly how to proceed.

In browsers, the ICE gathering phase is started when the SLD is called, so the flow is as follows:

• create PC with emtpy config
• create sdp offer
• send POST with sdp offer
• receive POST response with ice server info and sdp anser
• update pc config with ice servers
• set local description
• set remote description

[murillo128]

But in libdatachannel the createOffer and sLD are merged into a single method, so the flow is as follow:

• create PC with emtpy config
• create sdp offer and sLD
• send POST with sdp offer
• receive POST response with ice server info and sdp anser
• update pc config with ice servers
• set remote description

The sLD call creates and starts the ICE transport, so not sure what happens to the ice gathering if there have not been any server specified in the config and if we can just update the ice servers before setting the set remote description and trigger the ice gathering phase or if we need to split the ICE transport creation (to get the offer) and the ICE transport start (to start the ICE gathering phase) in two phases.

What do you think it is the best approach?

[paullouisageneau]

The sLD call creates and starts the ICE transport, so not sure what happens to the ice gathering if there have not been any server specified in the config and if we can just update the ice servers before setting the set remote description and trigger the ice gathering phase or if we need to split the ICE transport creation (to get the offer) and the ICE transport start (to start the ICE gathering phase) in two phases.

What do you think it is the best approach?

@murillo128 Indeed libdatachannel features a simplified API where createOffer() and createAnswer() do not exist, and setLocalDescription() only supports being called with implicit description. It makes the implementation way simpler as you don't have to re-setup things according the set local description.

If I understand correctly, the WHIP flow in browsers prevents from transmitting ICE candidates from client to server, so you have to rely only on peer reflexive candidates on server-side. Is my understanding correct? In this case, it sounds like STUN servers won't make any difference as the server reflexive candidates will be ignored. Is this mechanism actually designed for TURN servers only?

Currently ICE gathering starts on setLocalDescription(), if no server is specified in the config then only host candidates are gathered. So I think what you want could be achieved with two retro-compatible modifications:

• Adding an option to prevent gathering on setLocalDescription(), for instance disableAutoGathering, in the configuration (on the model of disableAutoNegotiation), and a dedicated method void gatherLocalCandidates(std::vector<IceServer> additionalIceServers = {}) to start the process manually, optionally with additional ICE servers.
• Changing the ICE transport to accept additional ICE servers in impl::IceTransport::gatherLocalCandidates(). For libnice, this is easy as I guess nice_agent_set_relay_info() may be called at any point before gathering, but for libjuice it requires adding a new API function to add TURN servers after agent creation.

Choosing this approach however makes this PR unnecessary.

[murillo128]

If I understand correctly, the WHIP flow in browsers prevents from transmitting ICE candidates from client to server, so you have to rely only on peer reflexive candidates on server-side. Is my understanding correct? In this case, it sounds like STUN servers won't make any difference as the server reflexive candidates will be ignored. Is this mechanism actually designed for TURN servers only?

That is not exactly how WHIP is intended to work. WHIP servers are meant to be either on the same private network as the WHIP client or with an reachable public ip address.

So the WHIP server will return an SDP answer containing the ICE candidates required by the WHIP client to connect to. In case that the client is behind a firewall, it will not be able to connect to the server ICE candidates, and that's were TURN helps.

When the WHIP client gathers the TURN reflexive candidates, it should send a STUN request to the ICE candidates, so the WHIP server will learn dynamically about the client new candidate without even needing to signal it via a subsequent PATCH request.

For completeness, the WHIP spec also allows the server to be behind a firewall (although I think this is an edge case), so that's when STUN makes sense. After exchanging the SDP O/A in the POST request, the WHIP client will send the gathered local candidates back to the WHIP server in an HTTP PATCH request. This way, the WHIP server can send back an STUN request to those candidates to open the NAT ports on its side.

[murillo128]

Regarding this PR, I think it still makes sense as one of the strong points of libdatachannel is that is has a similar API than the w3c one. So I would more prone of hiding the details internally. What would you think about this:

• Do not start the ice gathering phase if the iceServers are empty when calling the sLD
• Allow updating the iceServer in the pc.setConfiguration if the current iceServers are empty
• Start the ICE gathering automatically (if not already started) on the sRD

The behavior and apis would be quite similar to chrome/webrtc one (with the nits of missing createOffer/createAnswer)

[murillo128]

I have added an initial version of how updating the servers on setConfiguration would look like. Should work for libnice.

[paullouisageneau]

Regarding this PR, I think it still makes sense as one of the strong points of libdatachannel is that is has a similar API than the w3c one. So I would more prone of hiding the details internally. What would you think about this:

* Do not start the ice gathering phase if the iceServers are empty when calling the sLD

* Allow updating the iceServer in the pc.setConfiguration if the current iceServers are empty

* Start the ICE gathering automatically (if not already started) on the sRD

The behavior and apis would be quite similar to chrome/webrtc one (with the nits of missing createOffer/createAnswer)

I'm not really comfortable with fitting this in by changing the current behavior of sLD/sRD as I would break other people's use cases. For instance, users expect sLD to always start gathering and may rely on the gathering complete event to get the non-trickled description, and it is a perfectly normal use case to set no ICE servers when you know the peer is reachable. I'd also like to maintain a consistent behavior with datachannel-wasn, which calls the browser implementation under the hood.

Updating the ice servers could indeed be done with setConfiguration(), however there are a couple drawbacks: the rest of the configuration is ignored, and allowing additional servers would require to dedupe the list.

In order to stick to an API closer to the W3C without requiring architectural changes, I think it could be a good compromise to add a createOffer() method which initializes the ICE transport if not initialized already, generates and returns the local offer description, but does not set it and does not start gathering. setLocalDescription() would still work as usual with implicit description and start gathering, and could be called after changing the ice servers. What do you think?

[murillo128]

I am fine for adding an flag for controlling if sLD triggerst the ice gathering or not, adding a new createOffer woiuld be ideal, although I would need more support from your side in order to understand the required changes.

Regarding changing the rest of parameters in setConfiguration(), in the webrtc w3c api can be done, but it will trigger an ICE restart if some configuration parameters are changed.

What we can do is to check if the configuration is actually changing any other parameter and raise an exception if so.
The usage of setConfiguration() would work as follows for updating the ice servers:

auto config = pc.getConfiguration().
config.iceServers.push(....);
pc.setConfiguration(config);

regarding turn dedup, are you performing any at the moment? what prevents a client inserting the same turn server twice?

[paullouisageneau]

I am fine for adding an flag for controlling if sLD triggerst the ice gathering or not, adding a new createOffer woiuld be ideal, although I would need more support from your side in order to understand the required changes.

If I'm not mistaken, it would boil down to:

namespace rtc {
string PeerConnection::createOffer() {
	auto iceTransport = impl()->initIceTransport();
	if (!iceTransport)
		throw std::runtime_error("Peer connection is closed");

	Description offer = iceTransport->getLocalDescription(Description::Type::Offer);
	return impl()->populateLocalDescription(std::move(offer));
}
}

where impl::PeerConnection::populateLocalDescription() would do the operations in the first part of impl::PeerConnection::processLocalDescription() until description.setFingerprint().

What we can do is to check if the configuration is actually changing any other parameter and raise an exception if so. The usage of setConfiguration() would work as follows for updating the ice servers:

auto config = pc.getConfiguration().
config.iceServers.push(....);
pc.setConfiguration(config);

This works for me!

regarding turn dedup, are you performing any at the moment? what prevents a client inserting the same turn server twice?

Nothing prevents it, it should deduped in libjuice in the end (because otherwise the TURN server would get confused about the same client authentifying twice). I'm not sure about libnice though.

[Sean-Der]

@paullouisageneau I have updated this PR with your suggested API

config.disableAutoGathering = true;
....
...

std::vector<rtc::IceServer> iceServers = {};
iceServers.emplace_back("localhost", 3478, "foo", "bar");
peer_connection->gatherLocalCandidates(iceServers);

[Sean-Der]

@paullouisageneau can I get another review please, thank you!

[Sean-Der]

Ok last one I hope @paullouisageneau I think I got it this time :)

[paullouisageneau]

Thank you for the work, it will be great to introduce TURN support for WHIP/WHEP!

[Sean-Der]

@paullouisageneau whats left for a new tag? I can take on the tasks!

[paullouisageneau]

@Sean-Der I just released v0.21.0!