-
Notifications
You must be signed in to change notification settings - Fork 164
Add bitrate priority control #897
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
Conversation
📝 WalkthroughWalkthroughAdds a Priority enum and per-encoding Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Encoding
participant Media as MediaEncoding
participant RTC as RTC Engine
participant Transport as Transport Config
rect rgba(200,230,255,0.5)
App->>Media: create encoding (maxBitrate, maxFps, bitratePriority, networkPriority)
end
rect rgba(220,255,220,0.5)
Media->>RTC: createRtpEncodingParameters(encoding)
note right of RTC: maps bitratePriority -> result.bitratePriority\nmaps networkPriority -> result.networkPriority
end
rect rgba(255,240,200,0.5)
App->>Transport: connect with ConnectOptions(isDscpEnabled)
Transport->>RTC: configureTransports(...)
note right of RTC: rtcConfiguration.enableDscp = connectOptions.isDscpEnabled
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Sources/LiveKit/Protocols/MediaEncoding.swift`:
- Around line 19-29: The new optional requirements on the public protocol
MediaEncoding (bitratePriority and networkPriority) will break downstream
conformers; add a public extension for MediaEncoding that provides default
implementations returning nil for bitratePriority and networkPriority (keeping
maxBitrate required) so existing conforming types remain source-compatible.
Implement the defaults in an extension on MediaEncoding that returns nil for
both bitratePriority and networkPriority.
🧹 Nitpick comments (3)
Sources/LiveKit/Types/AudioEncoding.swift (1)
26-44: Add DocC with a short example for the new priority initializer.The new public initializer and priority fields should include Swift DocC documentation with a short usage example. As per coding guidelines.
📝 Suggested DocC
- public init(maxBitrate: Int, bitratePriority: Priority?, networkPriority: Priority?) { + /// Creates an audio encoding with optional priority hints. + /// - Parameters: + /// - maxBitrate: Maximum bitrate in bits per second. + /// - bitratePriority: Priority for bandwidth allocation. + /// - networkPriority: Priority for DSCP marking. Requires ``ConnectOptions.isDscpEnabled``. + /// - Example: + /// let encoding = AudioEncoding(maxBitrate: 64_000, bitratePriority: .high, networkPriority: .medium) + public init(maxBitrate: Int, bitratePriority: Priority?, networkPriority: Priority?) { self.maxBitrate = maxBitrate self.bitratePriority = bitratePriority self.networkPriority = networkPriority }Sources/LiveKit/Types/VideoEncoding.swift (1)
27-47: Add DocC with a short example for the new priority initializer.The new public initializer and priority fields should include Swift DocC documentation with a short usage example. As per coding guidelines.
📝 Suggested DocC
- public init(maxBitrate: Int, maxFps: Int, bitratePriority: Priority?, networkPriority: Priority?) { + /// Creates a video encoding with optional priority hints. + /// - Parameters: + /// - maxBitrate: Maximum bitrate in bits per second. + /// - maxFps: Maximum frames per second. + /// - bitratePriority: Priority for bandwidth allocation. + /// - networkPriority: Priority for DSCP marking. Requires ``ConnectOptions.isDscpEnabled``. + /// - Example: + /// let encoding = VideoEncoding(maxBitrate: 2_000_000, maxFps: 30, bitratePriority: .high, networkPriority: .medium) + public init(maxBitrate: Int, maxFps: Int, bitratePriority: Priority?, networkPriority: Priority?) { self.maxBitrate = maxBitrate self.maxFps = maxFps self.bitratePriority = bitratePriority self.networkPriority = networkPriority }Sources/LiveKit/Types/Options/ConnectOptions.swift (1)
76-129: Add a short DocC example forisDscpEnabledusage.The new public flag should include a brief example at the entry point (e.g.,
ConnectOptionscreation) to align with documentation guidelines. As per coding guidelines.📝 Suggested DocC
/// Allows DSCP codes to be set on outgoing packets when network priority is used. /// Defaults to false. +/// - Example: +/// let options = ConnectOptions(isDscpEnabled: true) `@objc` public let isDscpEnabled: Bool
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Sources/LiveKit/Core/Room+Engine.swiftSources/LiveKit/Protocols/MediaEncoding.swiftSources/LiveKit/Types/AudioEncoding.swiftSources/LiveKit/Types/Options/ConnectOptions.swiftSources/LiveKit/Types/Priority.swiftSources/LiveKit/Types/VideoEncoding.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/LiveKit/Types/Priority.swift
🧰 Additional context used
📓 Path-based instructions (2)
**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
**/*.swift: UseDispatchQueue.liveKitWebRTC.sync { ... }for all WebRTC API calls to ensure thread safety
KeepPackage.swiftand.swift-versionin sync when adding dependencies, declaring the oldest supported version for backwards compatibility
Use#if swift(>=6.0)or#if compilerdirectives for version-specific constructs to maintain compatibility with the oldest supported Swift version
Use Swift 6 concurrency with data-race safety; perform async work on background threads, not the main thread (except for UI components like VideoView)
Useactorfor synchronization andStateSync<T>with locking for synchronous/nonisolated APIs; do not add new synchronization primitives
Minimize lock contention by grouping reads/writes under onestate.mutate { ... }call
UseAnyTaskCancellable(viatask.cancellable()) instead of manualTaskmanagement; long-running tasks require cooperative cancellation to avoid memory leaks
Use async primitives fromSupport/AsyncandSupport/Schedulerswhen operation order matters
Prefer native Swift async/await overCombinefor new code
Do not usefatalError()or similar assertions to crash consumer code; use defensive programming (retry, backoff, graceful failure) or propagate errors withthrowsandLiveKitError
Avoidassert()andprecondition()in favor of defensive programming and proper error handling
Anticipate invalid states at compile time using algebraic data types and typestates
Wrap unsafe APIs like subscript[0]with optional?to ensure safe access
Runswiftlintper.swiftlint.ymland do not introduce new warnings; attempt to remove// swiftlint:disablecomments from legacy files
Use/// Docstringsfor every public API using Swift markdown (- Note,- Warning,- SeeAlso, etc.) and add short code examples for new APIs at entry points
Use// Code commentssparingly; prefer better naming and structuring over trivial 'what' comments
UseLoggablelogs with.debugby default; use ...
Files:
Sources/LiveKit/Core/Room+Engine.swiftSources/LiveKit/Types/AudioEncoding.swiftSources/LiveKit/Types/Options/ConnectOptions.swiftSources/LiveKit/Types/VideoEncoding.swiftSources/LiveKit/Protocols/MediaEncoding.swift
Sources/LiveKit/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
Use
internal import LiveKitWebRTCto keep WebRTC types private from the public API
Files:
Sources/LiveKit/Core/Room+Engine.swiftSources/LiveKit/Types/AudioEncoding.swiftSources/LiveKit/Types/Options/ConnectOptions.swiftSources/LiveKit/Types/VideoEncoding.swiftSources/LiveKit/Protocols/MediaEncoding.swift
🧠 Learnings (3)
📚 Learning: 2026-01-27T10:27:58.186Z
Learnt from: CR
Repo: livekit/client-sdk-swift PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T10:27:58.186Z
Learning: Applies to Sources/LiveKit/Core/Transport.swift : Use `Core/Transport.swift` to wrap `LKRTCPeerConnection` and handle ICE candidates and SDP offer/answer negotiation
Applied to files:
Sources/LiveKit/Core/Room+Engine.swiftSources/LiveKit/Types/Options/ConnectOptions.swift
📚 Learning: 2026-01-27T10:27:58.186Z
Learnt from: CR
Repo: livekit/client-sdk-swift PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T10:27:58.186Z
Learning: Applies to Sources/LiveKit/Core/RTC.swift : Use `Core/RTC.swift` as the factory for creating WebRTC objects (peer connections, tracks, data channels, etc.)
Applied to files:
Sources/LiveKit/Core/Room+Engine.swift
📚 Learning: 2026-01-27T10:27:58.186Z
Learnt from: CR
Repo: livekit/client-sdk-swift PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T10:27:58.186Z
Learning: Applies to Sources/LiveKit/Extensions/RTC*.swift : Use `Extensions/RTC*.swift` files for convenience extensions on WebRTC types
Applied to files:
Sources/LiveKit/Core/Room+Engine.swift
🧬 Code graph analysis (1)
Sources/LiveKit/Types/VideoEncoding.swift (5)
Sources/LiveKit/Types/AudioEncoding.swift (1)
isEqual(48-53)Sources/LiveKit/Types/Options/ConnectOptions.swift (1)
isEqual(135-149)Sources/LiveKit/Types/Dimensions.swift (1)
isEqual(42-45)Sources/LiveKit/Types/VideoParameters.swift (1)
isEqual(65-69)Sources/LiveKit/Types/Options/BufferCaptureOptions.swift (1)
isEqual(43-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Build & Test (macos-26, 26.1, macOS, true)
- GitHub Check: Build & Test (macos-26, 26.1, macOS,variant=Mac Catalyst)
- GitHub Check: Build & Test (macos-26, 26.1, macOS, true)
- GitHub Check: Build & Test (macos-26, 26.1, tvOS Simulator,name=Apple TV,OS=26.1)
- GitHub Check: Build & Test (macos-26, 26.1, visionOS Simulator,name=Apple Vision Pro,OS=26.1)
- GitHub Check: Build & Test (macos-14, 15.4, macOS)
- GitHub Check: Build & Test (macos-26, 26.1, macOS, true)
- GitHub Check: Build & Test (macos-15, 16.4, iOS Simulator,name=iPhone 16 Pro,OS=18.5)
- GitHub Check: Build & Test (macos-14, 15.4, macOS,variant=Mac Catalyst)
- GitHub Check: Build & Test (macos-15, 16.4, macOS)
- GitHub Check: Build & Test (macos-26, 26.1, iOS Simulator,name=iPhone 17 Pro,OS=26.1, true)
- GitHub Check: Build & Test (macos-15, 16.4, tvOS Simulator,name=Apple TV,OS=18.5)
- GitHub Check: Build & Test (macos-15, 16.4, macOS,variant=Mac Catalyst)
- GitHub Check: Build & Test (macos-14, 15.4, iOS Simulator,name=iPhone 15 Pro,OS=17.5)
- GitHub Check: Build & Test (macos-14, 15.4, tvOS Simulator,name=Apple TV,OS=17.5)
- GitHub Check: Build & Test (macos-15, 16.4, visionOS Simulator,name=Apple Vision Pro,OS=2.5)
- GitHub Check: Build & Test (macos-26, 26.1, iOS Simulator,name=iPhone 17 Pro,OS=26.1, true)
- GitHub Check: Check Protocol
- GitHub Check: Lint
- GitHub Check: lint (macos-15, 16.4)
- GitHub Check: lint (macos-26, 26.1)
- GitHub Check: lint (macos-14, 15.4)
- GitHub Check: Analyze (swift)
🔇 Additional comments (4)
Sources/LiveKit/Types/AudioEncoding.swift (1)
48-60: Equality and hashing now include priority fields — looks good.Sources/LiveKit/Types/VideoEncoding.swift (1)
51-65: Equality and hashing now include priority fields — looks good.Sources/LiveKit/Types/Options/ConnectOptions.swift (1)
135-165: Equality and hashing updates include DSCP flag — looks good.Sources/LiveKit/Core/Room+Engine.swift (1)
120-142: The current implementation is correct and follows the established pattern in the codebase.
LKRTCConfigurationproperty mutations (such asiceServers,iceTransportPolicy, andenableDscp) do not require wrapping inDispatchQueue.liveKitWebRTC.sync. The sync queue is reserved for WebRTC API calls—specifically constructor invocations and factory methods that return WebRTC objects. This pattern is already established inRTCConfiguration.swift, whereliveKitDefault()wraps only the constructor (LKRTCConfiguration()) in sync, while subsequent property assignments occur outside the sync block. Similarly,RTC.createPeerConnection()wraps the factory method call (peerConnectionFactory.peerConnection(with:)) in sync. The configuration object, once constructed, can have its properties safely modified on any thread before being passed to factory methods that internally synchronize.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| public protocol MediaEncoding { | ||
| // | ||
| /// Maximum bitrate in bits per second. | ||
| var maxBitrate: Int { get } | ||
|
|
||
| /// Priority for bandwidth allocation. | ||
| var bitratePriority: Priority? { get } | ||
|
|
||
| /// Priority for DSCP marking. | ||
| /// Requires `ConnectOptions.isDscpEnabled` to be true. | ||
| var networkPriority: Priority? { get } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid source-breaking public protocol changes by providing defaults.
Adding new requirements to a public protocol breaks downstream conformers. Provide default implementations returning nil (or introduce a new protocol) to preserve source compatibility.
🔧 Proposed fix
public protocol MediaEncoding {
/// Maximum bitrate in bits per second.
var maxBitrate: Int { get }
/// Priority for bandwidth allocation.
var bitratePriority: Priority? { get }
/// Priority for DSCP marking.
/// Requires `ConnectOptions.isDscpEnabled` to be true.
var networkPriority: Priority? { get }
}
+
+public extension MediaEncoding {
+ var bitratePriority: Priority? { nil }
+ var networkPriority: Priority? { nil }
+}🤖 Prompt for AI Agents
In `@Sources/LiveKit/Protocols/MediaEncoding.swift` around lines 19 - 29, The new
optional requirements on the public protocol MediaEncoding (bitratePriority and
networkPriority) will break downstream conformers; add a public extension for
MediaEncoding that provides default implementations returning nil for
bitratePriority and networkPriority (keeping maxBitrate required) so existing
conforming types remain source-compatible. Implement the defaults in an
extension on MediaEncoding that returns nil for both bitratePriority and
networkPriority.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.