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

Allowing subscription settings to update before fully subscribed #200

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/LiveKit/Core/SignalClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ internal extension SignalClient {
$0.width = UInt32(settings.dimensions.width)
$0.height = UInt32(settings.dimensions.height)
$0.quality = settings.videoQuality.toPBType()
$0.fps = UInt32(settings.preferredFPS)
$0.fps = UInt32(settings.fps)
}
}

Expand Down
31 changes: 27 additions & 4 deletions Sources/LiveKit/Publications/RemoteTrackPublication.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class RemoteTrackPublication: TrackPublication {
// MARK: - Private

// user's preference to subscribe or not
private var preferSubscribed: Bool?
private var preferSubscribed: Bool
private var metadataMuted: Bool = false

// adaptiveStream
Expand All @@ -47,6 +47,8 @@ public class RemoteTrackPublication: TrackPublication {
track: Track? = nil,
participant: Participant) {

preferSubscribed = participant.room.engine._state.connectOptions.autoSubscribe

super.init(info: info,
track: track,
participant: participant)
Expand Down Expand Up @@ -115,11 +117,24 @@ public class RemoteTrackPublication: TrackPublication {
public func set(preferredFPS newValue: UInt) -> Promise<Void> {
// no-op if already the desired value
let trackSettings = _state.trackSettings
guard trackSettings.preferredFPS != newValue else { return Promise(()) }
guard trackSettings.fps != newValue else { return Promise(()) }

guard userCanModifyTrackSettings else { return Promise(TrackError.state(message: "adaptiveStream must be disabled and track must be subscribed")) }

let settings = trackSettings.copyWith(preferredFPS: newValue)
let settings = trackSettings.copyWith(fps: newValue)
// attempt to set the new settings
return send(trackSettings: settings)
}

@discardableResult
public func set(preferredDimensions newValue: Dimensions) -> Promise<Void> {
// no-op if already the desired value
let trackSettings = _state.trackSettings
guard trackSettings.dimensions != newValue else { return Promise(()) }

guard userCanModifyTrackSettings else { return Promise(TrackError.state(message: "adaptiveStream must be disabled and track must be subscribed")) }

let settings = trackSettings.copyWith(dimensions: newValue)
// attempt to set the new settings
return send(trackSettings: settings)
}
Expand Down Expand Up @@ -184,7 +199,15 @@ private extension RemoteTrackPublication {

var userCanModifyTrackSettings: Bool {
// adaptiveStream must be disabled and must be subscribed
!isAdaptiveStreamEnabled && subscribed
if kind == .video && isAdaptiveStreamEnabled {
log("Adaptive stream is enabled, cannot change video track settings", .warning)
return false
}
if !(preferSubscribed || subscribed) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is no longer true. they should be able to change settings anytime

log("Cannot update track settings when not subscribed", .warning)
return false
}
return true
}
Comment on lines 200 to 211
Copy link
Member Author

Choose a reason for hiding this comment

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

@davidzhao Do you mean this whole check is not required anymore ?

Copy link
Member

Choose a reason for hiding this comment

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

I just mean the subscribed check. isAdaptiveStreamEnabled is still needed. though re-reading the code, you now have preferSubscribed which actually covers it. sorry about the false alarm.

}

Expand Down
13 changes: 7 additions & 6 deletions Sources/LiveKit/Types/TrackSettings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,26 +21,27 @@ internal struct TrackSettings: Equatable {
let enabled: Bool
let dimensions: Dimensions
let videoQuality: VideoQuality
let preferredFPS: UInt
let fps: UInt

init(enabled: Bool = false,
dimensions: Dimensions = .zero,
videoQuality: VideoQuality = .low,
preferredFPS: UInt = 0) {
videoQuality: VideoQuality = .high,
fps: UInt = 0) {

self.enabled = enabled
self.dimensions = dimensions
self.videoQuality = videoQuality
self.preferredFPS = preferredFPS
self.fps = fps
}

func copyWith(enabled: Bool? = nil,
dimensions: Dimensions? = nil,
videoQuality: VideoQuality? = nil,
preferredFPS: UInt? = nil) -> TrackSettings {
fps: UInt? = nil) -> TrackSettings {

TrackSettings(enabled: enabled ?? self.enabled,
dimensions: dimensions ?? self.dimensions,
videoQuality: videoQuality ?? self.videoQuality,
preferredFPS: preferredFPS ?? self.preferredFPS)
fps: fps ?? self.fps)
}
}
2 changes: 1 addition & 1 deletion Sources/LiveKit/Types/VideoQuality.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import Foundation

internal enum VideoQuality {
public enum VideoQuality {
case low
case medium
case high
Expand Down