Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reconnections on single media permission changes #7034

Merged

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Mar 22, 2022

Fixes #7048

Until now the WebUI code treated audio and video permissions as a combined permission: either both were enabled or both were disabled. This was inherited from the initial implementation, but it is no longer the case; it is possible to have audio permissions without video permissions and the other way around.

Besides that this pull request also fixes some other related issues.

Unfortunately there are a lot of possible combinations between permissions and devices, as can be seen by the scenarios below (which only include those that currently fail, there are even more 😢).

Pending:

  • Call setAudioAllowed and setVideoAllowed depending on the permission change
  • Test without HPB

How to test (scenario 1a)

  • Setup the HPB
  • Remove audio and video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with both audio and video (although both will be blocked once joined)
  • In the original window, grant video permissions to the guest

Result with this pull request

In the private window a forced reconnection was triggered, and if the video is enabled it will be visible in the original window

Result without this pull request

In the private window a forced reconnection was triggered, but the participant is not able to establish the connection again

How to test (scenario 1b)

  • Setup the HPB
  • Remove audio and video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with audio but not video (although audio will be blocked once joined)
  • In the original window, grant video permissions to the guest

Result with this pull request

In the private window nothing happened

Result without this pull request

In the private window a forced reconnection was triggered, but the participant is not able to establish the connection again

How to test (scenario 1c)

  • Setup the HPB
  • Remove video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with both audio and video (although the video will be blocked once joined)
  • In the original window, grant video permissions to the guest

Result with this pull request

In the private window a forced reconnection was triggered, and if the video is enabled it will be visible in the original window

Result without this pull request

In the private window nothing happened

How to test (scenario 1d)

  • Setup the HPB
  • Remove video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with video but not audio (although the video will be blocked once joined)
  • In the original window, grant video permissions to the guest

Result with this pull request

In the private window a forced reconnection was triggered, and if the video is enabled it will be visible in the original window

Result without this pull request

In the private window nothing happened

How to test (scenario 2)

  • Setup the HPB
  • Remove video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call without audio nor video devices
  • Open the device settings and select a camera

Result with this pull request

Nothing happens

Result without this pull request

In the private window a forced reconnection is triggered, although the connection is prevented by the HPB and never finishes

How to test (scenario 3a)

  • Setup the HPB
  • Enable audio and video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with both audio and video
  • In the original window, revoke video permissions for the guest

Result with this pull request

In the private window a forced reconnection was triggered and video is no longer sent (OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders() returns sender only for audio)

Result without this pull request

In the private window a forced reconnection is triggered, although the connection never finishes; This track is already set on a sender error is printed to browser console

How to test (scenario 3b)

  • Setup the HPB
  • Enable audio and video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with audio but not video
  • In the original window, revoke video permissions for the guest

Result with this pull request

In the private window nothing happened

Result without this pull request

In the private window a forced reconnection is triggered, although the connection never finishes; This track is already set on a sender error is printed to browser console

How to test (scenario 3c)

  • Setup the HPB
  • Enable audio and video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with video but not audio
  • In the original window, revoke video permissions for the guest

Result with this pull request

In the private window a forced reconnection was triggered and video is no longer sent (OCA.Talk.SimpleWebRTC.webrtc.peers[0].pc.getSenders() is empty)

Result without this pull request

In the private window a forced reconnection is triggered, although the connection never finishes; This track is already set on a sender error is printed to browser console

How to test (scenario 4a)

  • Setup the HPB
  • Disable video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with both audio and video (although the video will be blocked once joined)
  • In the original window, revoke audio permissions but grant video permissions for the guest

Result with this pull request

In the private window a forced reconnection was triggered, audio is no longer sent but video is

Result without this pull request

In the private window a forced reconnection is triggered, although the connection never finishes; This track is already set on a sender error is printed to browser console

How to test (scenario 4b)

  • Setup the HPB
  • Disable video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with audio but not video
  • In the original window, revoke audio permissions but grant video permissions for the guest

Result with this pull request

In the private window a forced reconnection was triggered, audio is no longer sent

Result without this pull request

In the private window a forced reconnection is triggered, although the connection never finishes; This track is already set on a sender error is printed to browser console

How to test (scenario 4c)

  • Setup the HPB
  • Disable video permissions by default in a conversation
  • Start a call as a moderator
  • In a private window, open the conversation as a guest
  • Join the call with video but not audio (although the video will be blocked once joined)
  • In the original window, revoke audio permissions but grant video permissions for the guest

Result with this pull request

In the private window a forced reconnection was triggered, audio is no longer sent but video is

Result without this pull request

In the private window a forced reconnection is triggered, although the connection never finishes; This track is already set on a sender error is printed to browser console

How to test (scenario 5)

  • Start a call with audio and video
  • Open the device settings and select no microphone and no camera
  • In a private window, join the call
  • In the original window, select a microphone or camera

Result with this pull request

A forced reconnection is triggered in the original window and the original participant can be heard or seen in the private window

Result without this pull request

Nothing happens and the original participant can not be heard or seen in the private window

@danxuliu
Copy link
Member Author

/backport to stable23

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the fix-reconnections-on-single-media-permission-changes branch from 50898c9 to 650baf0 Compare March 31, 2022 07:47
@danxuliu danxuliu marked this pull request as ready for review March 31, 2022 07:51
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work in general and fix the issues mentioned

@Ivansss
Copy link
Member

Ivansss commented Mar 31, 2022

Didn't work for me.

Test I did:

  • Create a conversation and set restricted permissions
  • Start the call with moderator
  • Join with as a guest
  • Give microphone permissions to everyone
  • Guest did not rejoin the call

Screenshot 2022-03-31 at 15 24 56

@nickvergessen
Copy link
Member

That was without HPB, might be related

@danxuliu
Copy link
Member Author

Didn't work for me.

I have tested those steps with and without HPB and in both cases it worked as expected.

While working on this pull request I noticed that, sometimes, a guest failed to connect again after a forced reconnection. Apparently it only happened with guests, but I checked and it also happened before this pull request; I am not sure if I saw it only without HPB or also with HPB. I was not able to found yet why it happens, though, but it is very likely that your issue is that one, and therefore unrelated to the changes here.

Independently of that I added a last fixup to add back some code that I was too eager to remove (the event handler still needs to be executed in case of an error to remove the event handler for the successful start).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The constraints are used in the caller code for things like setting the
current call flags. The constraints were modified by the
MediaDevicesSource and the MediaDevicesManager depending on the
available devices, but they did not reflect things like a node further
in the pipeline failing to set the track (for example, if the
VirtualBackground failed hard) and causing the final track to be null.
Basing the constraints on the tracks instead should address those
(corner) cases while behaving the same in the general cases.

Besides that, this will make possible to remove the constraints from
"MediaDevicesSource.start()" in a following commit.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The constraints given to "MediaDevicesSource.start()" were used to limit
the media to be got. However, this only applied in the initial start; if
a new device from a media kind initially disallowed was selected later
the node got a track from it. Now "setAudioAllowed(bool)" and
"setVideoAllowed(bool)" are introduced to restrict the media to be got
whenever the node is active.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
If media is allowed the track will be started (if a device is
available), and if media is disallowed the current track of its kind
will be stopped.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
These methods will be used to allow or disallow media based on the
permission changes.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Until now the audio and video publishing permissions were handled as a
whole; either the local participant was able to publish both audio and
video or not. However, the permissions API is actually finer grained,
and it is possible to have one without the other. Now the allowed state
of local media is updated based on the publishing permissions, which
automatically starts and stops the tracks as needed. Nevertheless, in
some cases forced reconnections need to be explicitly triggered to
remove tracks from the senders, as well as to start and stop the local
media.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
If publishing media is disallowed now the media is also disabled to
ensure that, if the media is then allowed again, it will not be
unexpectedly enabled.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When the local participant joins a call in most cases the call flags
already reflect the current media being published. However, if the
active media changed since the request to join the call was sent (for
example, if a track started during a forced reconnection, which could
happen if audio was disallowed and video allowed at the same time) the
call flags would need to be updated.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When tracks are replaced the forced reconnections were based on whether
the call was started with local media active or not. However, in some
cases a forced reconnection was needed even if the call was started with
local media active. For example, if after joining a call the user
removed the audio and video devices and then added them again, as if
another participant joined the call in the meantime that participant
would not try to establish a connection after the devices were selected
again, as currently the clients only establish a connection if another
participant has media when the list of participants in the call change.

Besides that, when the flags are updated now the flags are based only in
the current tracks rather than on the previous flags, as if the flags
were updated again before the previous request finished the second
update would be based on the original flags.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the fix-reconnections-on-single-media-permission-changes branch from 495440e to b511244 Compare March 31, 2022 15:10
@nickvergessen nickvergessen merged commit a6f26b2 into master Apr 4, 2022
14 checks passed
@nickvergessen nickvergessen deleted the fix-reconnections-on-single-media-permission-changes branch April 4, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection can not be established without camera permission
3 participants