Skip to content

Conversation

@danxuliu
Copy link
Member

@danxuliu danxuliu commented Jun 1, 2021

Updated version of #3222
Works better with #4686 (as otherwise subscriber-only participants can not properly raise their hand)

Pending:

  • Fix guests - Guests can not fetch the participant list, so the property was added too to the conversation like other participant properties that should be always available, like the id
  • Automatically enable or disable media and force a reconnection when permissions are granted or revoked during a call
  • Rename publishingAllowed to publishingPermissions and change the constants from NEVER and ALWAYS to NONE, AUDIO, VIDEO and SCREENSHARING
    • Initially it would be easier to treat the permissions as all or nothing, without differentiating between the device types
  • Fix media wrongly available or unavailable in certain sequences of switching devices and granting/revoking permissions (unfortunately I do not know the sequences, I just noticed that in some cases it does strange things)

Follow ups:

@PVince81
Copy link
Member

PVince81 commented Jun 1, 2021

ideas for "publishing allowed":

  • publishing allowed => speaker or presenter
  • publishing disallowed => listener

@PVince81
Copy link
Member

PVince81 commented Jun 1, 2021

or "mediaPermissions" or something, this could be something like flags "video publish allowed/disallowed, audio publish allowed/disallowed". in the case of classrooms a teacher might want to prevent video publishing but manually allow audio for a single person when someone raises hand. having "mediaPermissions" or "mediaPermissionFlags" would allow for this.

unless I'm mixing this up with a different concept

class="video"
:is-grid="true"
:fit-video="isStripe"
:token="token"
Copy link
Member

Choose a reason for hiding this comment

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

Can't you read it from the store instead of passing it through X layers?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a matter of taste, I guess 🤷 I prefer to pass data from the parent views rather than using the store. Anyway I can change it.

@danxuliu
Copy link
Member Author

danxuliu commented Jun 1, 2021

or "mediaPermissions" or something, this could be something like flags "video publish allowed/disallowed, audio publish allowed/disallowed". in the case of classrooms a teacher might want to prevent video publishing but manually allow audio for a single person when someone raises hand. having "mediaPermissions" or "mediaPermissionFlags" would allow for this.

Initially I did not realize that there could be use cases in which only allowing audio but not video could be interesting, so that is why I used a single setting for both (and screensharing). But yes, that sounds like a valid use case, so I will rename publishing_allowed to publishing_permissions and treat it like a bitwise value for none, audio, video and screensharing.

How to differentiate between a persistent publisher and a temporary publisher (that is, participants that are always publishing or participants that are publishing only when they speak) would still need to be addressed, but that would be something for later.

@danxuliu danxuliu force-pushed the add-publishing-allowed-property-for-participants branch from 4d5e3c4 to 3e7f756 Compare June 7, 2021 14:12
@danxuliu danxuliu marked this pull request as ready for review June 7, 2021 14:13
},
isAudioAllowed() {
return this.conversation.publishingPermissions === PARTICIPANT.PUBLISHING_PERMISSIONS.ALL
Copy link
Member

Choose a reason for hiding this comment

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

All is a binary flag list. So is publishingPermissions.
I guess a bit comparison for the respective flag makes more sense and ensures it works in the future if we ever split it?

Suggested change
return this.conversation.publishingPermissions === PARTICIPANT.PUBLISHING_PERMISSIONS.ALL
return this.conversation.publishingPermissions & PARTICIPANT.PUBLISHING_PERMISSIONS.AUDIO

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not check against the specific flags because for now the UI treats the permissions as a whole, all or nothing, so checking against all ensures that the whole UI behaviour would be consistent. I think that should be changed only once the rest of the UI is updated to handle specific flags too.

@danxuliu danxuliu force-pushed the add-publishing-allowed-property-for-participants branch 2 times, most recently from 072e1c8 to b7e830c Compare June 9, 2021 10:00
@danxuliu
Copy link
Member Author

danxuliu commented Jun 9, 2021

Fixed up and rebased. The two commits that added the menu options to grant or revoke permissions were moved to #5708.

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.

Something we can do in a follow up:

  • In CallController check the publishing permissions and cross the callFlag with it, so you only show up with what you can

@danxuliu
Copy link
Member Author

danxuliu commented Jun 9, 2021

Something we can do in a follow up:

* [ ]  In CallController check the publishing permissions and cross the callFlag with it, so you only show up with what you can

If I understand it correctly it is there already, see 7fb8dac But it was done on the service rather than the controller to ensure that it was always respected, even if the call flags are changed for some reason from somewhere else besides the controller.

danxuliu added 20 commits June 11, 2021 09:07
Guests are not able to fetch the participant list. Due to this the value
is also returned in the room information so clients can know their own
publishing permissions.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Only moderators of group and public conversations can set the value.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a participant does not have publishing permissions the HPB will
block signaling messages related to establishing a connection, like
sending the candidates. This would prevent participants using clients
not supporting yet the publishing permissions (and thus still trying to
publish even if they are not allowed to) from sending media in a call.

Unfortunately the lack of permissions only prevents the connection from
being established. If a participant is already sending media revoking
the publishing permissions will not cause the connection to be stopped
by the HPB.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The requested call flags can be different of the actual call flags used
if the audio or video devices are not available. This is not a big
issue, as the flags will be updated with the proper value as soon as the
participant list is requested again. Nevertheless, it is better to set
the right ones :-)

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The participant call flags are now honoured as media constraints when
joining a call. Therefore, now only the media specified in the flags is
allowed to be sent (although whether it is sent or not depends on the
available media).

Note that the default constraints now need to be hardcoded in
"LocalMedia.start". Otherwise if no constraints are given the object
that contains the default ones could get modified depending on the
available media (for example, if there is no camera connected), and the
updated ones will be used from that moment on even if the media has
become available.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a constraint object with audio and video properties is given to
"startLocalVideo()" that object will be modified inside
"localMedia.start()" and "mediaConstraints" will contain the actual
constraints used. However, if no object was given "localMediaStarted"
was emitted with an undefined constraints value.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
This will allow the clients to react to "publishingPermissions" changes
directly from the signaling messages, without needing to fetch again the
participants (although they may need to fetch them again nevertheless
for UI updates).

The internal signaling server also needs to listen to changes on the
property to be able to know when to send the message (the external
signaling server already listened to the changes to be able to update
the permission flags internally).

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Until now the "localMediaStarted" and "localMediaError" were emitted
only when the local participant joined a call. Therefore it was possible
to listen to all the events when waiting for the media to be ready
before joining a call.

However, in order to be able to call "startLocalVideo()" again during a
call (for example, if publishing permissions were granted) the events
must be listened to only once when waiting for the media to be started
to then join a call.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
This will be needed to explicitly stop the local media during a call (if
the local media is implicitly stopped by switching devices the
properties were already adjusted when handling the stream changes).

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The previous media state was restored when the local media was started.
Until now that happened only when starting a call, but the media will
need to be started during a call too if publishing permissions are
granted. In that case the media state should not be restored and the
media must start always disabled, as granting the publishing permissions
is done by other users, not by the one that will be publishing.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Until now it was only possible to know if there were active local
streams, but not whether the local media as a whole was active or not.
That is, if new streams would be automatically started if media devices
were switched. This will be needed to properly react to changes in the
publishing permissions to ensure that even if there are no streams no
streams will be started at a later point when there are no publishing
permissions.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
For now the UI treats the permissions as a whole, all or nothing, so the
"isXXXAllowed" methods do not differentiate between each individual
permission.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The audio and video buttons were enabled and disabled based on whether
audio and video was available. This roughly matched the publishing
permissions, but after a permission change there could be a small lapse
of time in which audio and video was available but publishing was not
allowed, which caused a mismatch between the button being enabled and
the tooltip stating that enabling audio/video was not allowed.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@nickvergessen nickvergessen force-pushed the add-publishing-allowed-property-for-participants branch from b7e830c to 9517a57 Compare June 11, 2021 07:08
@nickvergessen
Copy link
Member

Rebased due to a conflict in docs/capabilities.md

@nickvergessen nickvergessen merged commit c863183 into master Jun 11, 2021
@nickvergessen nickvergessen deleted the add-publishing-allowed-property-for-participants branch June 11, 2021 07:33
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.

4 participants