Send updated offers to subscribers after publisher renegotiations

[danxuliu]

Although related, this is independent from #191.

Tested also when running against a proxy :-)

When a publisher has a connection the publisher can update the connection (for example, to add a video track to an audio only connection) by sending an updated offer to Janus. Janus detects that, adjusts the connection and then sends back an answer. Once the publisher connection is updated Janus starts a renegotiation for the subscribers and generates the offers to be sent to them.

The signaling server did not handle the event, so the offers were not sent and the subscriber connections were not updated. Now the offers are sent as needed, which makes possible for the renegotiation to be completed by the clients.

In this case the offer message will also include an update parameter so clients can differentiate between offers to create new connections or update the existing one.

A new feature flag, publisher-update, is also introduced to notify clients whether updating publisher connections is supported or not (supported as in it will cause renegotiation offers to be sent for the subscribers). This is needed for the clients to differentiate if they can just send an updated offer or if they need to force a reconnection. However, would it be better to drop the flag here and add a single flag about renegotiations once #191 is ready?

Some things I am not very sure about (I am quite unsure about any change made by me to this code, but specially about these :-P ):

• OnOffer should be called OnUpdatedSubscriberOffer, OnConfiguredSubscriber or something like that? OnOffer may be a bit misleading, as it looks like it would be called for any offer, and not only when a negotiation is needed in Janus for a subscriber and thus the offer needs to be sent to the other peer.
• Second parameter of OnOffer is defined as map[string]interface{}, as it carries the offer provided by Janus in event.Jsep (mcu_janus.go), which (currently) is a string indexed map with fields sdp and type. However, in mcu_proxy.go msg.Payload["offer"] is interface{}, so the compiler complains that it needs to be type asserted to map[string]interface{}. Is there a better way to do it? Note that if the second parameter of OnOffer is changed to interface{} then the type assertion will be needed in clientsession.go in Payload:offer.
• Could there be any possible race condition if a new subscriber joins when the publisher is updated?

[coveralls]

Pull Request Test Coverage Report for Build 1897268595

• 3 of 48 (6.25%) changed or added relevant lines in 5 files are covered.
• 94 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+3.3%) to 52.522%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
hub.go	2	4	50.0%
mcu_proxy.go	0	2	0.0%
mcu_janus.go	0	6	0.0%
clientsession.go	0	35	0.0%

Files with Coverage Reduction	New Missed Lines	%
mcu_janus.go	1	5.46%
geoip.go	93	22.29%

Totals
Change from base Build 1852558795:	3.3%
Covered Lines:	5050
Relevant Lines:	9615

---

💛 - Coveralls

[fancycode]

Thanks! Some initial feedback on the code. Didn't test yet (see nextcloud/spreed#6896 (comment)).

OnOffer should be called OnUpdatedSubscriberOffer, OnConfiguredSubscriber or something like that?

What about OnUpdateOffer?

Second parameter of OnOffer is defined as map[string]interface{}, [...]

I updated the code to match what is done for candidates. Didn't test yet if the format for the clients is the same though.

Could there be any possible race condition if a new subscriber joins when the publisher is updated?

That should IMHO be handled by Janus (which is the source of truth for SDPs).

[fancycode]

Is the Update really needed? A client knows if it already has a connection with another peer and can with that determine if it's an update or not. Or am I missing something?

[danxuliu]

If an RTCPeerConnection handles an offer that is not an update it will end failing, but depending on the implementation it can take up to 30 seconds to fail (and in some cases I think that I saw it getting stuck, but I am not 100% sure).

In general this should not happen, but as offers are periodically requested until a connection is established in some corner cases with delayed messages and flaky connections it could happen that an offer was requested and just before receiving it a new request was made. The original connection is established, but Janus closes it due to receiving the new offer request. However, in some cases the browser could keep the connection open, so when the new offer is received it would be handled by the existing connection, rather than immediately establishing a new one.

Due to this Talk's WebUI closes the previous connection and starts a new one when an offer for the same participant is received, and this is why it needs to differentiate between normal offers, which closes the connection and start a new one, and updated offers, which apply to the existing one.

[fancycode]

Also I would combine this with #191 and introduce a single feature flag like renegotiate or update-sdp.

[danxuliu]

OnOffer should be called OnUpdatedSubscriberOffer, OnConfiguredSubscriber or something like that?

What about OnUpdateOffer?

It is your code, so I will rename it to whatever you tell me ;-) I have added a fixup for that.

Second parameter of OnOffer is defined as map[string]interface{}, [...]

I updated the code to match what is done for candidates. Didn't test yet if the format for the clients is the same though.

That is the reason why I initially used map[string]interface{}. The payload of the "normal" offer directly contains the sdp and type returned by Janus in the Jsep, it does not have an offer sublevel. Therefore, using

		Payload:  map[string]interface{}{
			"offer": offer,
		},

would make the payload to be different depending on the offer type. Due to this I did not add a fixup for this yet.

Could there be any possible race condition if a new subscriber joins when the publisher is updated?

That should IMHO be handled by Janus (which is the source of truth for SDPs).

Yes, I would expect Janus to properly handle that (let's hope it does :-P ). I meant race condition in how the signaling server handles the events from Janus, as I just copy-pasted and adjusted existing code without a deep understanding ;-) Due to this I do not know if it could happen for example that the signaling server takes too long to create a subscriber object, Janus emits the configured event and then the updated offer is lost because although the subscriber exists in Janus it did not exist yet in the signaling server.

Also I would combine this with #191 and introduce a single feature flag like renegotiate or update-sdp.

👍 Should everything be combined in a single pull request? Or just work independently in the features and then add the final flag once everything is ready?

[fancycode]

Tested with nextcloud/spreed#6896 and works. Nice!

It is your code, so I will rename it to whatever you tell me ;-) I have added a fixup for that.

👍 though I had no strong preference on the name.

[...] would make the payload to be different depending on the offer type. Due to this I did not add a fixup for this yet.

Agreed, we can keep it as-is then.

Yes, I would expect Janus to properly handle that (let's hope it does :-P ). I meant race condition in how the signaling server handles the events from Janus, as I just copy-pasted and adjusted existing code without a deep understanding ;-) Due to this I do not know if it could happen for example that the signaling server takes too long to create a subscriber object, Janus emits the configured event and then the updated offer is lost because although the subscriber exists in Janus it did not exist yet in the signaling server.

The signaling server will first create the internal subscriber object and then send the subscription request to Janus, so there should be no race condition here.

Also I would combine this with #191 and introduce a single feature flag like renegotiate or update-sdp.

+1 Should everything be combined in a single pull request? Or just work independently in the features and then add the final flag once everything is ready?

I don't care if you want to keep it in two PRs or want to combine it - do what fits your workflow best.

[fancycode]

Note to self: de-duplicate with

nextcloud-spreed-signaling/hub.go

Line 1873 in d141775

offer_message := &AnswerOfferMessage{

once this has landed.

[danxuliu]

The signaling server will first create the internal subscriber object and then send the subscription request to Janus, so there should be no race condition here.

Perfect!

I don't care if you want to keep it in two PRs or want to combine it - do what fits your workflow best.

Then I would prefer to keep it as two separate PRs. Nevertheless, feel free to merge this one once it is ready or wait until #191 is ready too, I have no preference on that :-)

[fancycode]

Looks good now, thanks!

Could you please change the feature id to be more generic in preparation for combining this with #191 and then squash your commits?

[danxuliu]

Could you please change the feature id to be more generic in preparation for combining this with #191 and then squash your commits?

Done. I squashed the fixups in their respective commits, but I was not sure if you wanted all the commits squased in a single one or not, so for now I kept the commit to add the feature and the commit to add the common flag separated. If you want me to squash the commit that adds the flag into the first commit too let me know :-)

Indepedently of that, I can also rebase on current master to use the latest CI changes. Again, just let me know ;-)

[fancycode]

Perfect, thanks!