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

Conversation

hiroshihorie
Copy link
Member

@hiroshihorie hiroshihorie commented Apr 11, 2023

livekit/client-sdk-js#471

also allow to set videoQuality.

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

Sources/LiveKit/Publications/RemoteTrackPublication.swift Outdated Show resolved Hide resolved
Comment on lines 200 to 211
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) {
log("Cannot update track settings when not subscribed", .warning)
return false
}
return true
}
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants