Stop sending the nick through data channels after some time #4182

danxuliu · 2020-09-19T01:43:35Z

Requires #4181

~~(Kept as draft until #4181 is merged, but ready to review nevertheless)~~

Since the introduction of the HPB the nick has been sent over and over again every second, as it is not possible to know when other participants have established a data channel connection (and neither to associate a signaling id with a Nextcloud id to get the nick from the participant list).

However, it is possible to know when other participants join the call, and it is safe to assume that the other participant established a connection with the local one after a "reasonable" amount of time.

As sending the nick every second is too aggressive (specially in calls with a lot of users) now the nick is sent when the local participant establishes a connection with the remote one and, as that does not guarantee that the remote one has established a connection with the local one, the nick is sent again several times (with an increasing interval) in the next 30 seconds. If a connection is established with another participant in the meantime the cycle starts again.

Additionally a nickChanged signaling message* is now sent along with the data channel message. The signaling message is needed for those cases in which there is no Peer object for a participant, as in that case it is not possible to send data channel messages. However, the data channel messages still need to be sent to keep compatibility with the mobile apps.

*An unknown signaling message is just ignored by the mobile apps, but it should not break anything; tested in iOS by @Ivansss, although just my guess from checking the Android app code (but I have not actually tested it).

danxuliu · 2020-09-19T01:43:49Z

/backport to stable20

nickvergessen · 2020-10-15T13:17:26Z

@Ivansss please check if this works with apps? (I guess they would need adjustments?)

Ivansss

Tested and works 👍

"startSendingNick" is only used when the HPB is used, and only with the own peer, so there is no need to provide the peer as a parameter. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

When the current participant is a guest the nick was always sent when another participant was connected. However, the nick is sent already (without HPB in the offer/answer, with HPB it is periodically sent), so there is no need to do it again. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Instead of sending "nickChanged" data channel messages from different places now an internal "nickChanged" event with the name is emitted, and its handler takes care of sending the data channel message as needed. This also fixes trying to send data channel messages to receiving only peers when the nick is changed and the HPB is used. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The function will be used to send both the media state and the nick, so its name is adjusted accordingly. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Since the introduction of the HPB the nick has been sent over and over again every second, as it is not possible to know when other participants have established a data channel connection. However, it is possible to know when other participants join the call, and it is safe to assume that the other participant established a connection with the local one after a "reasonable" amount of time. As sending the nick every second is too aggressive (specially in calls with a lot of users) now the nick is sent when the local participant establishes a connection with the remote one and, as that does not guarantee that the remote one has established a connection with the local one, the nick is sent again several times (with an increasing interval) in the next 30 seconds. If a connection is established with another participant in the meantime the cycle starts again. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The "userId" property was used only in CallParticipantModel, but as it is also set from the participant information provided by the signaling in most cases it is not needed. The only case in which it would be needed is if the Peer object is created and connected before the participant is found in the participant information provided by the signaling. However even in that rare case showing a user as a guest would happen very briefly and it will correct itself as soon as the signaling sends the participant information. Due to this, and as it will simplify also a "nickChanged" signaling message to be introduced later, the "userId" property is now ignored. Note that this only affects the internal "nick" event; the "userId" property still needs to be sent in the "nickChanged" data channel message as it is used by the Android app. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

When the HPB is used, if the local participant does not send audio nor video there is no Peer for the local participant, so it is not possible to send data channel messages (although they can be received if the other participant sends audio or video and thus has a Peer object). When the HPB is not used, if both the local participant and the remote one do not send audio nor video there is no Peer for their connection, so it is not possible to send or received data channel messages between those participants. The nick was only sent through data channel messages, so in the above cases the nick was not initially shown and neither updated if it was changed later. Now a "nickChanged" signaling message is introduced to ensure that the nick is transmitted even if there is no Peer for a participant. Even if the mobile apps do not currently handle the "nickChanged" event they will just ignore it without breaking havoc. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

When the HPB is not used the initial nick is sent along the offer and answer. However if there is no Peer between two participants there will be no offer/answer, so the nick needs to be explicitly sent in that case. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

nickvergessen · 2020-11-16T12:38:43Z

Status? shall we merge it in 21 only so we test it a bit more?

danxuliu · 2020-11-16T12:54:55Z

Status?

@PVince81 You self requested a review, should we wait for your review or should we merge? :-)

shall we merge it in 21 only so we test it a bit more?

I would backport it to 20 already, but I am also fine with waiting until the next maintenance release.

PVince81 · 2020-11-16T17:21:50Z

@danxuliu I've somehow deferred this as I wasn't sure whether you had other works in progress that would impact this PR, since you said you're going to revamp the "data channel" vs "signaling" vs ... stuff

danxuliu · 2020-11-16T17:33:41Z

Ah, no, no, the revamp and fixes were going to be done after this was merged (or before it was merged but on top of this anyway, not by amending anything here). Sorry for the misunderstanding.

PVince81

👍 code looks fine with my limited knowledge

tested with and without signaling server with a guest whose nick changes, it seemed to work fine

I've added some optional comments

PVince81 · 2020-11-17T09:13:48Z

src/utils/webrtc/models/CallParticipantModel.js

 			return
 		}

-		this.set('userId', data.userid || null)


Yes, see the commit description for details: e00f662

PVince81 · 2020-11-17T09:18:43Z

src/utils/webrtc/webrtc.js

+		// "webrtc.sendToAll" can not be used, as it only sends the signaling
+		// message to participants for which there is a Peer object, so the
+		// message may not be sent to participants without audio and video.
+		for (const sessionId in usersInCallMapping) {


is this something that we could put into a separate function to make it clearer ? or is this very specific to the nick use case ? (could be something for later)

Yes, I will address that when I fix the signaling messages for participants without Peer objects. So something for later :-)

src/utils/webrtc/webrtc.js

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu · 2020-11-18T20:27:33Z

@PVince81 Comment added; if you are satisfied with it feel free to merge, otherwise please push fixup commits to improve it ;-)

nickvergessen · 2020-12-09T10:51:10Z

/backport to stable20.1

danxuliu added 2. developing bug feature: call 📹 Voice and video calls labels Sep 19, 2020

danxuliu added this to the 💚 Next Major (21) milestone Sep 19, 2020

backportbot-nextcloud bot added the backport-request label Sep 19, 2020

danxuliu force-pushed the stop-sending-the-nick-through-data-channels-after-some-time branch from fd2dc4b to 8c0b33c Compare October 1, 2020 10:00

danxuliu mentioned this pull request Oct 2, 2020

[v8.0.x] Fix some issues related to guest participants nextcloud/talk-android#904

Merged

danxuliu force-pushed the stop-sending-the-nick-through-data-channels-after-some-time branch from 8c0b33c to 18cf0bc Compare October 15, 2020 10:21

danxuliu marked this pull request as ready for review October 15, 2020 10:22

danxuliu added 3. to review and removed 2. developing labels Oct 15, 2020

danxuliu requested review from Ivansss and nickvergessen October 15, 2020 10:22

Ivansss approved these changes Oct 23, 2020

View reviewed changes

danxuliu added 8 commits October 23, 2020 13:03

Specialize "startSendingNick" function to use only the own peer

0229496

"startSendingNick" is only used when the HPB is used, and only with the own peer, so there is no need to provide the peer as a parameter. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Generalize function name

1c865cc

The function will be used to send both the media state and the nick, so its name is adjusted accordingly. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu force-pushed the stop-sending-the-nick-through-data-channels-after-some-time branch from 18cf0bc to 03d269b Compare October 23, 2020 11:04

nickvergessen mentioned this pull request Oct 23, 2020

Missing display name during calls from participants with denied perms #4454

Open

PVince81 self-requested a review November 10, 2020 17:13

PVince81 approved these changes Nov 17, 2020

View reviewed changes

Add explicit documentation about payload of "nickChanged" message

259cb4e

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

PVince81 merged commit 6b7db03 into master Nov 19, 2020

PVince81 deleted the stop-sending-the-nick-through-data-channels-after-some-time branch November 19, 2020 12:21

backportbot-nextcloud bot mentioned this pull request Nov 19, 2020

[stable20] Stop sending the nick through data channels after some time #4649

Merged

backportbot-nextcloud bot removed the backport-request label Nov 19, 2020

backportbot-nextcloud bot added the backport-request label Dec 9, 2020

backportbot-nextcloud bot mentioned this pull request Dec 9, 2020

[stable20.1] Stop sending the nick through data channels after some time #4726

Merged

backportbot-nextcloud bot removed the backport-request label Dec 9, 2020

nickvergessen mentioned this pull request Dec 18, 2020

Add changelog for 11.0.0-alpha.1 #4802

Merged

nickvergessen mentioned this pull request Mar 18, 2021

Log debugging information in case of call quality degradation #5362

Closed

Stop sending the nick through data channels after some time #4182

Stop sending the nick through data channels after some time #4182

Uh oh!

Conversation

danxuliu commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danxuliu commented Sep 19, 2020

Uh oh!

nickvergessen commented Oct 15, 2020

Uh oh!

Ivansss left a comment

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented Nov 16, 2020

Uh oh!

danxuliu commented Nov 16, 2020

Uh oh!

PVince81 commented Nov 16, 2020

Uh oh!

danxuliu commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

danxuliu Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

danxuliu Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danxuliu commented Nov 18, 2020

Uh oh!

nickvergessen commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danxuliu commented Sep 19, 2020 •

edited

Loading

danxuliu commented Nov 16, 2020 •

edited

Loading