Split call participants and peer connections #2600

danxuliu · 2022-11-30T05:57:42Z

Fixes #2544
~~Requires #2590 (so the first commit of this pull request is Handle the raw ICE connection state in the views)~~

It is highly recommended to review this commit by commit rather than all at once ;-) Commit messages may also provide additional context.

Model classes for call participants were added. CallParticipant is the main model class, which observes the changes in its peer connections and data channels and updates a data object that keeps track of the state*. This data object is the one exposed to the view classes through a read-only object (CallParticipantModel) that can be also observed for changes on specific threads. Thanks to that the ParticipantDisplayItems are now automatically updated in the main thread when the model changes, and they also propagate the changes to the adapter (so notifiyDataSetChanged is automatically called when any item changed).

*In the future it should also listen to signaling messages, for example for raised hands, but also for audio/video enabled/disabled events, as they are also sent through signaling messages besides the data channels just in case the data channel connection could not be established.

Besides that, a helper class is now used to keep track of the participants in the call based on the signaling messages, instead of based on the peer connections. For now it just uses the same signaling messages as before, but in the future this could be used to also handle the message sent by the external signaling server when a participant leaves the room, which implicitly means that the participant left the call (if a participant abruptly leaves the call and the room at the same time no message for leaving the call is sent, only about leaving the room).

With all this now call participants are added and removed when they join and leave the call, and peer connections are only started when the joined participant is publishing media (according to the call flags of the participant for audio/video, and based on the offers received for screen shares). Updating the peer connections of an existing call participant when the flags change during the call is still needed for proper renegotiation support, and it will be done in a follow up.

The steps below should be tested with HPB; without HPB, as the Android app is publishing media, a peer connection is anyway needed to send it to the other participant even if that participant is not publishing.

How to test

Start a call without a microphone or camera selected in the browser (that is, not just disabled, but not selected), or without audio and video permissions
Join the call in the Android app

Result with this pull request

The Android app does not try to establish a peer connection with the other participant, and no progess bar is shown on that other participant

Result without this pull request

The Andoid app tries to establish a peer connection with the other participant to receive media (and the external signaling server logs eventually shows that an offer could not be requested), and a progress bar is always shown on that other participant

danxuliu · 2022-12-29T19:16:46Z

The two new issues reported by SpotBugs are both related to this code:
https://github.com/nextcloud/talk-android/pull/2600/files#diff-50dc4e0167d557f36af93be1eeb64df0cbd347b3a31a4da9d8a2f6a12514b9a5R94-R99

The Performance - PSC: Presize Collection - Method does not presize the allocation of a collection seems to assume that left can be initialized to the same size as knownCallParticipantsNotFound, because the loop adds the contents from knownCallParticipantsNotFound to left. However, left will not have the same size as knownCallParticipantsNotFound, and in most cases it will be much lower, as the known call participants that are still in the call are removed from knownCallParticipantsNotFound. Of course left could be initialized anyway to the same size as knownCallParticipantsNotFound, but in most cases it will just reserve unneeded space, so to fix the performance issue reported by SpotBugs a performance "issue" might be introduced :-) (although in most cases it will be just a few items, so it should not matter much in practice).

The Dodgy code - UAA: Use Add All - Method uses simple loop to copy contents of one collection to another is technically right, but it seems to ignore that the loop does other things with the added elements. Therefore, even if left.addAll(knownCallParticipantsNotFound) was used a loop would still need to be used anyway, and I think that the current loop is clearer as it shows in a more cohesive way the changes done to callParticipant before adding it to left (but, of course, that is just a matter of taste, so also fine by me to change it ;-) ).

mahibi · 2023-01-02T10:52:15Z

/rebase

mahibi · 2023-01-03T15:17:31Z

/rebase

mahibi · 2023-01-05T14:04:23Z

tested with sermo, removed mic and video permissions of caller and also set mic and video to "none" when starting the call.
there is still a progress shown in android..
we can have a look together whenever you want @danxuliu

timkrueger

Spinning wheel still exists.

danxuliu · 2023-01-10T17:11:52Z

Fixed. I do not know what I was thinking on 🤦 Besides the scenario described above now the connection is not established either in voice only calls if the remote participant is publishing video but not audio.

I was going to prevent establishing screen sharing connections in voice only calls, but the problem is that since some versions ago screen shares can also include sound (for example, if the participant is sharing a browser tab with sound). It is not possible to differentiate from the call flags between a video-only screen share and an audio and video screen share in order to establish or not to establish the connection, so... for now the connection is also established in voice only calls when a participant shares the screen, although in most cases the result will be just a loading spinner because the screen share connection only has a video track and that video track was explicitly stopped due to being in a voice only call 🤷

It might be possible to detect that specific case and prevent showing the loading spinner, but... something for the future ;-)

mahibi · 2023-01-10T18:00:36Z

calling from my local machine without hpb with the "none" devices scenario, then the avatar of the android participant is spinning in the webbrowser

danxuliu · 2023-01-12T19:03:03Z

calling from my local machine without hpb with the "none" devices scenario, then the avatar of the android participant is spinning in the webbrowser

Partially fixed (when the session ID of the Android app participant is higher than the session ID of the browser participant, which was a regression introduced in this pull request). When the session ID of the browser participant is the higher one the connection will still fail if the browser participant is not sending media; this was an existing issue and requires further refactorings so, for now, it is not fixed and tracked in #2679

mahibi · 2023-01-16T09:05:02Z

/rebase

Rather than just providing a coarse "connected" or "not connected" value now the views receive the raw ICE connection state. Combined with other properties this will make possible to show a finer grained status (like done in the WebUI), although for now just "connected" or "not connected" is still shown as before. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Note that the session ID, user ID and the stream type attributes are still kept, as they can be useful to identify the instance when debugging. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

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

Generic final attributes first, followed by object specific final attributes and then other object attributes. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Instead of explicitly calling "notifyDataSetChanged" after setting values on a ParticipantDisplayItem now the adapter observes all its items and calls "notifyDataSetChanged" automatically when any of them changes. Although this adds some boilerplate code it will make possible to update the ParticipantDisplayItems and automatically propagate the changes to the adapter when a model changes, rather than having to explicitly do it from the CallActivity. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Clients that modify the model would define the variables using the mutable subclass, while clients that only need to access the model are expected to use the read-only base class. The read-only class provides an observer; as it is expected that the model will be modified from background threads but observed from the main thread the observer can be registered along a handler to be notified on its thread, independently of on which thread the values were set. Currently there does not seem to be a need to observe each value on its own, so the observer is notified in a coarse way when any value changes. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Instead of explicitly setting the values on the ParticipantDisplayItems now the values are set on the CallParticipantModels, and the items are automatically updated from their model when they change. Different items are still used for the audio/video and screen shares of the same participant, so the type is used to select from which properties of the model is the item updated. As the model may be updated from background threads it is explicitly observed by the items from the main thread using a Handler shared by all the items. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

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

CallParticipant provides a read-only CallParticipantModel and internally handles the data channel and peer connection events that modify the model. Nevertheless, the CallParticipant requires certain properties to be externally set, like the userId or the peer connections. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The listeners for call participant messages and for the call participant nick provided by offers / answers were created and destroyed based on the peer connections, although they were implicitly associated to a call participant. Now they are explicitly created and destroyed based on its associated call participant. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

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

The ParticipantDisplayItems were created and destroyed based on the peer connections. Now a ParticipantDisplayItem of "video" type is associated to a call participant, while an additional item is created and destroyed depending on the state of the screen peer connection of the call participant. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The connection state changes to "closed" only when the connection is closed. However, closing a connection does not fire any event (not even the "iceConnectionStateChanged" event), so the event handler can be removed as it will never be executed. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The observers were created for any peer connection, but after recent changes they ignored all changes but those from the self peer connection. Therefore it is enough to just add an explicit listener on that peer connection rather than on all of them. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

For now only the same signaling messages that were already handled are still handled; in the future it could be extended to handle other messages, like the one sent by the external signaling server when a participant leaves the room (in some cases no participants update message is sent if the participant leaves the call and room at the same time, which causes the participants to still be seen as in call until a new update is received). Signed-off-by: Daniel Calviño Sánchez <[email protected]>

As CallParticipantList starts listening on the signaling messages as soon as it is created it needs to be created and destroyed right before entering and exiting a call. Otherwise it could receive messages on other states (for example, while the "connection timeout" message is shown) and thus once the local participant joined the event would not include the other participants already in the call as joined (although they would be anyway reported as unchanged). Signed-off-by: Daniel Calviño Sánchez <[email protected]>

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

Instead of trying to create a video peer connection for any joined participant now only a call participant is created for any joined participant, and a video peer connection is created only for those participants that are publishing audio or video. If a call participants does not have a video peer connection the call participant is now seen as "connected" from the UI, as there is no need to show a progress bar for that participant. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The peer connections will be of either "video" or "screen" type, so they can be simply removed based on the session id and an explicit type. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

github-actions · 2023-01-16T09:16:50Z

Codacy

Lint

Type	master	PR
Warnings	111	111
Errors	0	0

SpotBugs

Category	Base	New
Correctness	12	12
Dodgy code	173	174
Internationalization	5	5
Malicious code vulnerability	3	3
Performance	9	10
Security	2	2
Total	204	206

SpotBugs increased!

github-actions · 2023-01-16T09:17:46Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/2600-talk.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

mahibi · 2023-01-16T09:51:44Z

/backport to stable-15.1

danxuliu added 2. developing Work in progress feature: call ☎️ labels Nov 30, 2022

danxuliu force-pushed the split-call-participants-and-peer-connections branch 2 times, most recently from 8308d06 to c31299c Compare November 30, 2022 23:20

This was referenced Nov 30, 2022

Fix handling offers in already established connections #1823

Draft

Start and end connections based on "inCall" flags changes #2610

Draft

mahibi added this to the 15.2.0 milestone Dec 27, 2022

danxuliu force-pushed the split-call-participants-and-peer-connections branch 2 times, most recently from 0f357b0 to 7549569 Compare December 29, 2022 18:58

danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 29, 2022

danxuliu marked this pull request as ready for review December 29, 2022 19:14

danxuliu requested a review from timkrueger December 29, 2022 19:14

nextcloud-command force-pushed the split-call-participants-and-peer-connections branch from 7549569 to 695de97 Compare January 2, 2023 10:52

nextcloud-command force-pushed the split-call-participants-and-peer-connections branch from 695de97 to 07e0b6b Compare January 3, 2023 15:18

mahibi self-requested a review January 5, 2023 14:04

timkrueger suggested changes Jan 6, 2023

View reviewed changes

danxuliu force-pushed the split-call-participants-and-peer-connections branch from 07e0b6b to f439825 Compare January 10, 2023 16:59

mahibi approved these changes Jan 12, 2023

View reviewed changes

danxuliu force-pushed the split-call-participants-and-peer-connections branch from f439825 to 35ed03a Compare January 12, 2023 18:30

danxuliu added 2 commits January 16, 2023 09:05

Remove unused getters and setters

e887fde

Note that the session ID, user ID and the stream type attributes are still kept, as they can be useful to identify the instance when debugging. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu added 17 commits January 16, 2023 09:05

Declare attributes set just once in constructor as "final"

5fe9154

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

Reorder attributes

d67b04d

Generic final attributes first, followed by object specific final attributes and then other object attributes. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Keep track of the stream in the peer connection

4aef76e

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

Rename methods to add and remove ParticipantDisplayItems

e17a999

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

Extract methods to add and remove call participants

2cb7572

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

Simplify ending the peer connections

67e259f

The peer connections will be of either "video" or "screen" type, so they can be simply removed based on the session id and an explicit type. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

nextcloud-command force-pushed the split-call-participants-and-peer-connections branch from 35ed03a to 67e259f Compare January 16, 2023 09:05

mahibi merged commit a87f2fb into master Jan 16, 2023

backportbot-nextcloud bot added the backport-request label Jan 16, 2023

backportbot-nextcloud bot mentioned this pull request Jan 16, 2023

[stable-15.1] Split call participants and peer connections #2693

Merged

backportbot-nextcloud bot removed the backport-request label Jan 16, 2023

AndyScherzinger deleted the split-call-participants-and-peer-connections branch January 8, 2026 13:07

Uh oh!

Split call participants and peer connections #2600

Split call participants and peer connections #2600

Uh oh!

Conversation

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

Uh oh!

How to test

Result with this pull request

Result without this pull request

Uh oh!

danxuliu commented Dec 29, 2022

Uh oh!

mahibi commented Jan 2, 2023

Uh oh!

mahibi commented Jan 3, 2023

Uh oh!

mahibi commented Jan 5, 2023

Uh oh!

timkrueger left a comment

Choose a reason for hiding this comment

Uh oh!

danxuliu commented Jan 10, 2023

Uh oh!

mahibi commented Jan 10, 2023

Uh oh!

danxuliu commented Jan 12, 2023

Uh oh!

mahibi commented Jan 16, 2023

Uh oh!

github-actions bot commented Jan 16, 2023

Codacy

Lint

SpotBugs

SpotBugs increased!

Uh oh!

github-actions bot commented Jan 16, 2023

Uh oh!

mahibi commented Jan 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danxuliu commented Nov 30, 2022 •

edited

Loading