Fix unneeded messages when sending initial state#16420
Conversation
| * restarted with each new participant that joins. | ||
| * | ||
| * Similarly, in the case of signaling messages it is not possible either to | ||
| * know when the remote participants have "seen" the local participant and thus |
There was a problem hiding this comment.
Can't you just listen on the join event and when a new session joins, send it the data? With that you should not need to sent it multiple times. If you received the join event for a session, it is connected and in the room (and will be able to receive events).
There was a problem hiding this comment.
If I am not mistaken the join event is sent when a participant joins the room, so it can not be used here as the messages should be sent only to the participants that joined the call.
Nevertheless, the equivalent would be participants->update, but if I recall correctly the problem is that the clients might ignore the messages with the state of other participants if they are sent "too soon":
- the client sends the HTTP request to join the call
- another client receives the signaling message with the participants update
- that other client sends the state with a signaling message
- the client that just joined has not received yet the list of participants in the call -> the signaling message with the state is ignored
I think there were other possible scenarios, like getting the signaling message before receiving the HTTP response so the client also ignored the messages (that was fixed in the WebUI for an unrelated issue, but I am not sure about other clients).
Of course all that could be solved by ensuring that all clients keep track of the state of remote participants as soon as the client begins the HTTP request to join the call. However, the current changes should be an intermediate step until a better overall solution, most likely using the transient states feature of the signaling server :-) so adjusting all the clients is better deferred until then.
fec164d to
018e796
Compare
ebb35eb to
8fbe5ca
Compare
a6124de to
c962e49
Compare
This will make possible to explicitly call it from other objects rather than having to emit events in the WebRTC object. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This will make possible to decouple the messages sent for the initial media state from the messages sent when the media state changes during the call. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Currently only changes in the audio, speaking and video state are notified, although in the future it should also notify about the nick, the raised hand or any other state. Similarly, although right now it only notifies of changes in the state it will also take care of notifying other participants about the current state when they join the call (or the local participant joins). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This will make possible to use module augmentation to provide a TypeScript definition of the class that includes the methods added by the EmitterMixin. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This will make possible to explicitly call them from other objects rather than having to emit events in the WebRTC object. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Besides changing where they are sent in the code the new code also reduces the number of signaling messages, as now it sends them directly to each participant rather than to every participant. Moreover, when Janus is used, the state is sent now when the remote participant joins the call rather than when a connection was established with that participant (or joined, if that participant was not a publisher, as in that case no connection was established with it). Sending the state when a connection was established with the remote participant did not guarantee in any way that the signaling message would be received by the remote participant (hence the repeated sending with an exponential backoff), so sending it instead when the participant joins has essentially the same result (as the repeated sending with an exponential backoff is kept), but simplifies the code and makes more sense conceptually (as before the state of the publisher was sent when a connection with a subscriber was established). Similarly, when Janus is not used the state is sent now the first time that a connection is established with the remote participant, but not on every change to the connected state, as doing it only the first time is actually needed. Data channel messages are also sent now directly to the other participant when Janus is not used. However, when Janus is used they are still sent to all participants, as Janus does not support sending data channel messages to a specific participant. Unfortunately that means that data channel messages are still repeated for every participant in the call when another remote participant joins it. This is specially bad when the local participant joins a call, as in that case all the remote participants joined at the same time from the point of view of the local participant. Independently of that, note that when Janus is used the media state is always sent even if the local participant is not publishing, but if Janus is not used it will not be sent if neither the local participant nor the remote participant are publishing. While it should be possible to not sent the media state in all cases if the local participant is not publishing (because the audio and video will be implicitly disabled in that case) this is not properly handled by the iOS client yet, so for now the media state is still sent in that case. Note that this means that without Janus and when neither the local nor remote participants are publishing the iOS client will show the audio as available. In the previous code this was less problematic, because as the media state was sent to all participants, the media state could be received by a remote participant (through a signaling message) even if neither the remote participant nor the local participant was publishing if the local participant established a connection with a different remote participant that was publishing. This is no longer the case, but is nevertheless something to be fixed in the iOS client (and not a big deal anyway, as limited publishing permissions are unlikely to be used in a setup without Janus). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The attribute stores the display name of the local participant no matter if it is a guest or a regular user. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Similarly to the initial sending of the media state the nick is now sent using the LocalStateBroadcaster subclasses as part of the current state. Like for the media state the new code reduces the messages sent by sending them directly to each participant instead of to all participants, except for data channels using Janus which are still sent to all participants. When Janus is not used the nick is sent only if no connection will be established with the remote participant, as otherwise the nick is sent as part of the offer/answer. Nevertheless, the detection of the peer being set to null does not properly work due to the peer attribute being initialized to "null", but that will be fixed in a follow up commit. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
If a null peer is set the CallParticipantModel did not trigger a "change:peer" event, as the peer was originally initialized to null. To work around that now "set:peer" is emitted whenever the peer attribute is set, no matter its value. "peer" (and "screenPeer") attribute(s) should be instead initialized to "undefined", like other unknown attributes, and the standard "change:peer" should be listened to. However, for simplicity and "safety" for the time being "set:peer" is emitted instead as it is needed only for the LocalStateBroadcasterNoMcu and changing the initial value may, even if it should not, cause unexpected issues. Moreover, the semantics of undefined attributes may require further changes, like special handling when removing a screen peer. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
c962e49 to
63e26b3
Compare
|
/backport to stable33 please |
|
/backport to stable32 please |
Antreesy
left a comment
There was a problem hiding this comment.
Read through, smoke tested with janus+hpb in local docker env. Don't have much comments on that.
Follow-up: need to look into boilerplate binding for functions and webrtc listeners, maybe we could avoid that in future
TL;DR; this pull request introduces specific classes to send the media state and name to other participants, and while doing so it changes how they are sent, greatly reducing their number.
When the media state (audio/video enabled/disabled, speaking/not speaking) and the name of the local participant changes the rest of participants in the call need to be notified. This is done through data channel and signaling messages. Similarly, when a remote participant joins the call the local participant needs to sent the current media state and name to the remote participant so it can set its initial state. If the local participant is the one joining the call it needs to notify the state to all the other participants already in the call.
Until now in all cases the notification was done to all the remote participants in the call. However, although that is needed when the state changes, it was overkill when sending the initial state, as the messages should have been sent only to the specific participant that joined. Now the initial state is sent only to specific participants rather than to all participants (except when using Janus and data channel messages, as Janus does not support sending data channel messages to specific participants).
Nevertheless, when using Janus the messages are still sent with an exponential backoff, as it is not possible to know when the remote participant has "seen" the local participant. However, before that was done when a subscriber connection was established with a remote participant (or a remote participant joined, if it was not publishing). That did not guarantee that the message would be received by the remote participant (hence the exponential backoff), and it was actually a bit messy (as the state of a publisher was sent based on a subscriber). Moreover, it sent many unneeded messages, as they were sent whenever the connection changed to the connected state, so it would be also sent if the subscriber WebRTC connection briefly disconnected and recovered by itself. Due to all that when using Janus now the state to the remote participants are sent just when they join the call.
When not using Janus the state is still sent when the connection is established, but only when it is established for the first time. Similarly, doing it whenever the connection changed to the connected state was unneeded. Note that without Janus the exponential backoff is not needed, because the connections are peer connections between the participants, so if it is established it is guaranteed that the remote participant has "seen" the local participant.
For further details please refer to the commit messages and the documentation in the classes.
All the above changes can greatly reduce the number of messages (specially signaling messages) that are sent in large calls. For reference, when using Janus, in a best case scenario with each participant joining just once, before these changes, in a webinar with 200 participants there would have been, at least, ~48M signaling messages just from sending the initial state (because the messages were sent to all participants, and it was sent every time a participant established a connection / joined, so it was a summatory from 1 to n of n*n states, and each state is sent in 3 separate messages and 6 times for each due to the exponential backoff). With these changes, in a best case scenario, there would be ~720k signaling messages from the initial state (because the messages are now sent directly to the participant that joined, although the exponential backoff and separate messages are still used).
Follow ups