Permissions API #37

davidliu · 2022-01-06T03:21:16Z

Android seems to have some crashes after repeated enabling/disabling of permissions, but that might be a sample app issue rather than SDK issue. I'll look into that separately. JS seems to handle it fine.

davidliu · 2022-01-06T03:29:53Z

One question I had was whether tracks should be dropped from the publication when disallowed/unsubscribed? Noticed that we seem not to be doing this at the moment for android/JS. Looks like we properly get the tracks again through WebRTC when the permissions are regranted.

livekit-android-sdk/src/main/java/io/livekit/android/room/Room.kt

livekit-android-sdk/src/main/java/io/livekit/android/room/participant/LocalParticipant.kt

boks1971 · 2022-01-06T03:47:38Z

livekit-android-sdk/src/main/java/io/livekit/android/room/track/RemoteTrackPublication.kt

     */
    fun setSubscribed(subscribed: Boolean) {
+        if (subscribed && !subscriptionAllowed) {


Is this preventing sending an UpdateSubscriptions message? Sorry, I maybe mistaking code here. But, sending UpdateSubscriptions should not be prevented. Server will send the SubscriptionPermissionUpdate when there are no permissions. Note that permissions can be revoked when a track is actively subscribed. So, the server could send that update mid stream too. So, clients should always send subscription message and listen for the update from the server.

I think this code is not preventing sending that message, but just wanted to elaborate.

Mmm, the case I was trying to cover here was if we had previously received a subscription permission update disallowing a track, and the client decides to attempt to subscribe anyways. Should we still attempt the subscription in that case?

Got it. Maybe, the log message can be something like already subscribed, but currently does not have permissions?

I think we should still let the client update the server, because it's indicating a desired state.

It does make the API a bit inconsistent, which I don't love:

.subscribed indicates if it's desired to be subscribed and successfully subscribed

.setSubscribed updates the desired state

Any alternatives?

Discussed with @boks1971, the proposal is to turn .subscribed into an enum describing the current subscription status, with "subscribed, not allowed" and "subscribed" as two distinct states.

Thinking about the setSubscribed part, I think it might be more descriptive of user intent to have separate subscribe() and unsubscribe() functions, which would help clear up the inconsistency you note.

that sounds good. though I'm wary about breaking current users. Especially on JS. we could perhaps deprecate .subscribed, but still provide it as a way of returning subscribeState == Subscribed ?

boks1971 · 2022-01-06T03:50:01Z

One question I had was whether tracks should be dropped from the publication when disallowed/unsubscribed? Noticed that we seem not to be doing this at the moment for android/JS. Looks like we properly get the tracks again through WebRTC when the permissions are regranted.

Yes @davidliu . As permissions can be granted/revoked at any time by the publishing end, server keeps track of state and reconciles the subscriptions with changing permissions and pauses/resumes tracks as necessary. The SubscriptionPermissionUpdate message is the mechanism through which server will signal permission changes and whether the associated track will be pausing/resuming. The tracks are not removed from publication.

davidliu · 2022-01-06T04:32:54Z

One question I had was whether tracks should be dropped from the publication when disallowed/unsubscribed? Noticed that we seem not to be doing this at the moment for android/JS. Looks like we properly get the tracks again through WebRTC when the permissions are regranted.

Yes @davidliu . As permissions can be granted/revoked at any time by the publishing end, server keeps track of state and reconciles the subscriptions with changing permissions and pauses/resumes tracks as necessary. The SubscriptionPermissionUpdate message is the mechanism through which server will signal permission changes and whether the associated track will be pausing/resuming. The tracks are not removed from publication.

Sorry, to be clear, is that a yes, we should drop/remove the tracks from the client, or yes, we should keep them around?

boks1971 · 2022-01-06T04:46:38Z

livekit-android-sdk/src/main/java/io/livekit/android/room/track/RemoteTrackPublication.kt

    val isAutoManaged: Boolean
        get() = (track as? RemoteVideoTrack)?.autoManageVideo ?: false

    override val subscribed: Boolean
        get() {
-            if (unsubscribed) {
+            if (unsubscribed || !subscriptionAllowed) {


Wondering if this should return false. The track is subscribed to, but just that it does not have permissions. Feels like this needs to return an enum which gives a better picture of subscription state.

hmm good question, I think technically it's desired to be subscribed, but tricky because clients use subscribed to indicate if subscription is successful and ready to be streamed.

returning false probably makes more sense than true in this case.

boks1971 · 2022-01-06T04:51:17Z

One question I had was whether tracks should be dropped from the publication when disallowed/unsubscribed? Noticed that we seem not to be doing this at the moment for android/JS. Looks like we properly get the tracks again through WebRTC when the permissions are regranted.

Yes @davidliu . As permissions can be granted/revoked at any time by the publishing end, server keeps track of state and reconciles the subscriptions with changing permissions and pauses/resumes tracks as necessary. The SubscriptionPermissionUpdate message is the mechanism through which server will signal permission changes and whether the associated track will be pausing/resuming. The tracks are not removed from publication.

Sorry, to be clear, is that a yes, we should drop/remove the tracks from the client, or yes, we should keep them around?

Sorry David. I meant, yes - keep the publications. But, I am not saying a definitive yes without knowing all the client intricacies. Basically, the idea is that certain tracks exist in the system and a subscriber has subscribed to it. But those tracks could be paused/resumed based on publisher side permissions. So, if the subscriber checks what tracks are available in the session, the state will say tracks are available, but they may not be pulled down due to permissions. But, I think this is something to discuss with @davidzhao and @hiroshihorie also to find the right approach.

davidzhao · 2022-01-06T06:28:38Z

livekit-android-sdk/src/main/java/io/livekit/android/room/Room.kt

+
+            // Unsubscribe if become disallowed.
+            if (!pub.subscriptionAllowed && pub.subscribed) {
+                pub.setSubscribed(false)


do we have to manually unset? or would server send down an unsubscribed event?

I think setSubscribed should be reserved for user intent, so that if a subscription is revoked, and the intent is still there, server can send down the track again when subscription is restored.

Nope, this was a misunderstanding on my part and has been removed.

davidzhao · 2022-01-06T07:08:44Z

livekit-android-sdk/src/main/java/io/livekit/android/room/track/RemoteTrackPublication.kt

    val isAutoManaged: Boolean
        get() = (track as? RemoteVideoTrack)?.autoManageVideo ?: false

    override val subscribed: Boolean
        get() {
-            if (unsubscribed) {
+            if (unsubscribed || !subscriptionAllowed) {


hmm good question, I think technically it's desired to be subscribed, but tricky because clients use subscribed to indicate if subscription is successful and ready to be streamed.

returning false probably makes more sense than true in this case.

davidzhao · 2022-01-06T07:11:18Z

livekit-android-sdk/src/main/java/io/livekit/android/room/track/RemoteTrackPublication.kt

     */
    fun setSubscribed(subscribed: Boolean) {
+        if (subscribed && !subscriptionAllowed) {


I think we should still let the client update the server, because it's indicating a desired state.

It does make the API a bit inconsistent, which I don't love:

.subscribed indicates if it's desired to be subscribed and successfully subscribed

.setSubscribed updates the desired state

Any alternatives?

… of ParticipantEvent

davidzhao · 2022-01-06T21:44:56Z

livekit-android-sdk/src/main/java/io/livekit/android/room/track/TrackPublication.kt

@@ -22,10 +22,6 @@ open class TrackPublication(
        private set
    open var muted: Boolean = false
        internal set
-    open val subscribed: Boolean


this is kind of useful when you have a TrackPublication, and don't care if it's remote or local, but just need to quickly check isSubscribed then decide to render.

davidliu added 4 commits January 5, 2022 12:50

permissions API

df255a2

naming

8ce461d

don't add track if disallowed

45084b5

hold on to track for now

17df42c

davidliu requested review from davidzhao, boks1971 and hiroshihorie January 6, 2022 03:22

boks1971 approved these changes Jan 6, 2022

View reviewed changes

boks1971 reviewed Jan 6, 2022

View reviewed changes

davidliu added 2 commits January 5, 2022 20:53

room event for subscription permission update

2c01b4b

update test

cd274c0

davidzhao reviewed Jan 6, 2022

View reviewed changes

davidliu added 4 commits January 6, 2022 01:35

language cleanup

9386629

Move track permission handling to RemoteParticipant to allow emission…

6569874

… of ParticipantEvent

Subscription status enum

5cc3a22

revert preventing subscription to disallowed track

3e25249

davidzhao reviewed Jan 6, 2022

View reviewed changes

davidliu added 2 commits January 7, 2022 02:33

keep subscribe boolean and add extra subscriptionStatus

bf9d864

fix build

6132331

davidliu merged commit 0a334e7 into master Jan 7, 2022

davidliu deleted the dl/permissions branch January 7, 2022 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permissions API #37

Permissions API #37

davidliu commented Jan 6, 2022 •

edited

Loading

davidliu commented Jan 6, 2022 •

edited

Loading

boks1971 Jan 6, 2022

davidliu Jan 6, 2022

boks1971 Jan 6, 2022

davidzhao Jan 6, 2022

davidliu Jan 6, 2022

davidzhao Jan 6, 2022

boks1971 commented Jan 6, 2022

davidliu commented Jan 6, 2022

boks1971 Jan 6, 2022

davidzhao Jan 6, 2022

boks1971 commented Jan 6, 2022

davidzhao Jan 6, 2022

davidliu Jan 6, 2022

davidzhao Jan 6, 2022

davidzhao Jan 6, 2022

davidzhao Jan 6, 2022

Permissions API #37

Permissions API #37

Conversation

davidliu commented Jan 6, 2022 • edited Loading

davidliu commented Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boks1971 commented Jan 6, 2022

davidliu commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boks1971 commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidliu commented Jan 6, 2022 •

edited

Loading

davidliu commented Jan 6, 2022 •

edited

Loading