Add bitrate priority control #969
Conversation
📝 WalkthroughWalkthroughAdded bitrate priority control: new Priority enum and AudioEncoding type; VideoEncoding and AudioPublishOptions now support bitrate/network priorities; local publish logic and RTCRtpEncoding construction use these encodings; RTCConfiguration gains enableDscp; exports and WebRTC deps updated. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant Local as LocalParticipant
participant Enc as AudioEncoding
participant RTC as WebRTC (RTCRtpSender)
App->>Local: publishAudioTrack(publishOptions)
Local->>Enc: select encoding (publishOptions.encoding or presetMusic)
Local->>Local: enc.toRTCRtpEncoding() -> sendEncodings
Local->>RTC: addTransceiver / setSenderEncodings (maxBitrate, priority, networkPriority)
RTC-->>Local: transceiver negotiated / sender returned
Local-->>App: publish completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/types/other.dart (1)
144-187: Remove or gateenableDscpto Android only—it will silently fail on all other platforms.Lines 144–187:
enableDscpis not a standard WebRTC RTCConfiguration option and will be ignored on Flutter Web (browser) and typically on iOS and desktop. It is supported only on Android (via native WebRTC'sPeerConnection.RTCConfiguration.enableDscp). The code currently passes it unconditionally to all platforms, causing silent failures. Either:
- Gate it to Android only before adding to the configuration map, or
- Document explicitly that it is Android-only and will be ignored elsewhere.
Standard WebRTC DSCP/QoS control is per-RTP encoding via
RTCRtpSender.setParameters()withnetworkPriority, not a globalRTCConfigurationproperty.lib/src/types/video_encoding.dart (1)
77-86: Comparable contract drift: compareTo ignores priority fields.The
compareTo()method only compares bitrate and framerate, but the==operator andhashCodeinclude all four fields (maxBitrate, maxFramerate, bitratePriority, networkPriority). Two instances with identical bitrate/framerate but different priorities will return 0 fromcompareTo()while==returns false, violating the Comparable contract. This can cause unexpected behavior in TreeSet/TreeMap where objects with compareTo() == 0 won't insert as distinct elements, and breaks the consistency guarantee between equality and ordering.Consider adding priority tie-breakers to
compareTo()to maintain consistency:Suggested fix
`@override` int compareTo(VideoEncoding other) { // compare bitrates - final result = maxBitrate.compareTo(other.maxBitrate); - // if bitrates are the same, compare by fps - if (result == 0) { - return maxFramerate.compareTo(other.maxFramerate); - } - - return result; + var result = maxBitrate.compareTo(other.maxBitrate); + if (result != 0) return result; + + result = maxFramerate.compareTo(other.maxFramerate); + if (result != 0) return result; + + result = (bitratePriority?.index ?? -1).compareTo(other.bitratePriority?.index ?? -1); + if (result != 0) return result; + + return (networkPriority?.index ?? -1).compareTo(other.networkPriority?.index ?? -1); }
🧹 Nitpick comments (1)
lib/src/participant/local.dart (1)
128-134: Consider centralizing audio RTCRtpEncoding construction to avoid drift.The two RTCRtpEncoding blocks are nearly identical; a small helper (or AudioEncodingExt) will reduce duplication and keep the maxBitrate guard consistent across both code paths.
♻️ Suggested refactor
- final List<rtc.RTCRtpEncoding> encodings = [ - rtc.RTCRtpEncoding( - maxBitrate: audioEncoding.maxBitrate, - priority: (audioEncoding.bitratePriority ?? Priority.low).toRtcpPriorityType(), - networkPriority: audioEncoding.networkPriority?.toRtcpPriorityType(), - ) - ]; + final encodings = [ + if (audioEncoding.maxBitrate > 0) audioEncoding.toRTCRtpEncoding(), + ]; ... - sendEncodings: [ - if (audioEncoding.maxBitrate > 0) - rtc.RTCRtpEncoding( - maxBitrate: audioEncoding.maxBitrate, - priority: (audioEncoding.bitratePriority ?? Priority.low).toRtcpPriorityType(), - networkPriority: audioEncoding.networkPriority?.toRtcpPriorityType(), - ), - ], + sendEncodings: [ + if (audioEncoding.maxBitrate > 0) audioEncoding.toRTCRtpEncoding(), + ],Also applies to: 171-176
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.changes/priority-controllib/livekit_client.dartlib/src/options.dartlib/src/participant/local.dartlib/src/track/web/_audio_analyser.dartlib/src/types/audio_encoding.dartlib/src/types/other.dartlib/src/types/priority.dartlib/src/types/video_encoding.dartlib/src/utils.dartpubspec.yaml
⏰ 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). (8)
- GitHub Check: Build for Flutter Web
- GitHub Check: Build for Flutter Web WASM
- GitHub Check: Dart Analyze Check
- GitHub Check: Build for Flutter iOS
- GitHub Check: Build for Flutter macOS
- GitHub Check: Build for Flutter Android
- GitHub Check: Build for Flutter Windows
- GitHub Check: Build for Flutter Linux
🔇 Additional comments (9)
lib/src/track/web/_audio_analyser.dart (1)
5-5: Import consolidation looks good.Line 5: clean and consistent with current usage.
.changes/priority-control (1)
1-1: Changelog entry looks good.Clear and concise.
lib/livekit_client.dart (1)
62-66: Exports look good.Line 62–Line 66: exposing the new types is appropriate for the public API.
lib/src/utils.dart (1)
432-434: No changes needed. Both the legacy SVC path (lines 432–434) and the non-legacytoRTCRtpEncoding()method already use identical logic for priority:(bitratePriority ?? Priority.low).toRtcpPriorityType(). The priority handling is already consistent between the two paths. The suggested diff would introduce inconsistency rather than alignment.Likely an incorrect or invalid review comment.
pubspec.yaml (1)
49-50: flutter_webrtc 1.3.0 and dart_webrtc 1.7.0 support priority/networkPriority APIs; enableDscp is not exposed by dart_webrtc.Both libraries support the priority/networkPriority APIs you're using (flutter_webrtc 1.3.0 added
priorityandnetworkPrioritytoRTCRtpEncoding; dart_webrtc 1.7.0 added Priority control APIs). No breaking changes affect existing platforms.However,
enableDscpis a native WebRTC configuration field (part ofRTCConfigurationon Android/iOS), not a surface exposed by dart_webrtc's API wrapper. If you're relying on DSCP tagging, ensure your native WebRTC implementation (via platform channels or native code) handles it directly.Likely an incorrect or invalid review comment.
lib/src/types/priority.dart (1)
17-39: LGTM: clear Priority enum and mapping.Nice, minimal enum plus conversion helper; the switch is exhaustive and readable.
lib/src/options.dart (1)
309-353: LGTM: AudioPublishOptions now exposes AudioEncoding.Constructor, copyWith, and toString are updated consistently.
lib/src/types/audio_encoding.dart (1)
20-88: LGTM: AudioEncoding presets + RTC conversion are cohesive.The type is immutable, presets are sensible, and the conversion helper is clean.
lib/src/types/video_encoding.dart (1)
18-73: LGTM: priority fields are wired through copyWith/equality and RTC conversion.Nice, consistent propagation of the new priority fields.
Also applies to: 92-105
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/types/other.dart (1)
162-167:⚠️ Potential issue | 🔴 CriticalThe
enableDscpsetting will likely be ignored by flutter_webrtc.
The flutter_webrtc plugin does not expose DSCP configuration in its documented Dart API—only ICE server configuration is supported. While the underlying libwebrtc library supports DSCP viaenable_dscp, flutter_webrtc does not plumb this setting from Dart down to the native layer. The map key will be silently ignored at runtime unless flutter_webrtc is forked or patched to support it.
🤖 Fix all issues with AI agents
In `@lib/src/types/audio_encoding.dart`:
- Around line 83-87: The toRTCRtpEncoding() methods in AudioEncoding and
VideoEncoding currently force a default Priority.low when bitratePriority is
null; change the priority mapping to preserve null by calling
bitratePriority?.toRtcpPriorityType() instead of (bitratePriority ??
Priority.low).toRtcpPriorityType(), leaving networkPriority as-is
(networkPriority?.toRtcpPriorityType()); update both
AudioEncoding.toRTCRtpEncoding and VideoEncoding.toRTCRtpEncoding accordingly so
presets that omit bitratePriority remain unset.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/src/types/audio_encoding.dartlib/src/types/other.dartlib/src/types/video_encoding.dart
⏰ 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). (8)
- GitHub Check: Build for Flutter iOS
- GitHub Check: Build for Flutter Web WASM
- GitHub Check: Build for Flutter Web
- GitHub Check: Dart Analyze Check
- GitHub Check: Build for Flutter macOS
- GitHub Check: Build for Flutter Windows
- GitHub Check: Build for Flutter Linux
- GitHub Check: Build for Flutter Android
🔇 Additional comments (5)
lib/src/types/other.dart (2)
144-154: DSCP toggle wiring looks good.
Nice, clean addition of the field and constructor parameter.
174-187: copyWith update is consistent.
Propagates the new field in the same pattern as existing options.lib/src/types/video_encoding.dart (2)
18-73: Priority fields are integrated cleanly.
Constructor/copyWith/equality/hash/toString updates are consistent.
92-105: Priority field already defaults to low per WebRTC spec; no behavior changeThis is a newly created file, so no prior behavior exists. According to the WebRTC priority specification, when the
priorityfield is leftnull, WebRTC defaults to"low"automatically. The current explicit(bitratePriority ?? Priority.low)achieves the same result as the proposednullapproach and is consistent with the existing pattern inaudio_encoding.dartandutils.dart. No major issue.Likely an incorrect or invalid review comment.
lib/src/types/audio_encoding.dart (1)
20-79: AudioEncoding value object and presets look solid.
Clear API surface and consistent immutability.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| rtc.RTCRtpEncoding toRTCRtpEncoding() => rtc.RTCRtpEncoding( | ||
| maxBitrate: maxBitrate, | ||
| priority: (bitratePriority ?? Priority.low).toRtcpPriorityType(), | ||
| networkPriority: networkPriority?.toRtcpPriorityType(), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the current state of audio_encoding.dart
cd lib/src/types && head -100 audio_encoding.dart | tail -30Repository: livekit/client-sdk-flutter
Length of output: 1148
🏁 Script executed:
# Search for video_encoding to compare similar concerns
fd -t f "video_encoding.dart" lib/Repository: livekit/client-sdk-flutter
Length of output: 103
🏁 Script executed:
# Look for RTCRtpEncoding usage and understand the context better
rg "RTCRtpEncoding" -A 5 -B 2 lib/src/types/Repository: livekit/client-sdk-flutter
Length of output: 1562
🏁 Script executed:
# Check git history for this file to understand what changed
git log --oneline lib/src/types/audio_encoding.dart | head -20Repository: livekit/client-sdk-flutter
Length of output: 95
🏁 Script executed:
# Look for bitratePriority usage and how it was handled before
rg "bitratePriority" -B 3 -A 3 lib/src/types/audio_encoding.dartRepository: livekit/client-sdk-flutter
Length of output: 1681
🏁 Script executed:
# Check video_encoding.dart to see how it handles priority
cat lib/src/types/video_encoding.dart | grep -A 20 "extension VideoEncodingExt"Repository: livekit/client-sdk-flutter
Length of output: 610
🏁 Script executed:
# Get more recent git history with diffs
git log -p --max-count=5 -- lib/src/types/audio_encoding.dartRepository: livekit/client-sdk-flutter
Length of output: 52
🏁 Script executed:
# Check the Priority enum definition
rg "enum Priority" -A 5 lib/Repository: livekit/client-sdk-flutter
Length of output: 291
🏁 Script executed:
# Look for RTCRtpEncoding definition in flutter_webrtc or understand what happens when priority is null
rg "class RTCRtpEncoding" -A 10Repository: livekit/client-sdk-flutter
Length of output: 52
🏁 Script executed:
# Check if there are any tests or usage of AudioEncoding that depend on the priority default
rg "AudioEncoding\(" -B 2 -A 2 --max-count=15 lib/src/Repository: livekit/client-sdk-flutter
Length of output: 1576
Fix priority default to preserve null behavior when not configured.
Both AudioEncoding and VideoEncoding presets (presetTelephone, presetSpeech, etc.) are created without specifying bitratePriority, indicating priority should remain unset by default. The current code forces Priority.low when null, which changes this intended behavior. The networkPriority field already uses the correct pattern networkPriority?.toRtcpPriorityType() to preserve null values.
🛠️ Proposed adjustment (preserve default behavior)
- priority: (bitratePriority ?? Priority.low).toRtcpPriorityType(),
+ priority: bitratePriority?.toRtcpPriorityType(),This change should also be applied to VideoEncoding which has the identical issue.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rtc.RTCRtpEncoding toRTCRtpEncoding() => rtc.RTCRtpEncoding( | |
| maxBitrate: maxBitrate, | |
| priority: (bitratePriority ?? Priority.low).toRtcpPriorityType(), | |
| networkPriority: networkPriority?.toRtcpPriorityType(), | |
| ); | |
| rtc.RTCRtpEncoding toRTCRtpEncoding() => rtc.RTCRtpEncoding( | |
| maxBitrate: maxBitrate, | |
| priority: bitratePriority?.toRtcpPriorityType(), | |
| networkPriority: networkPriority?.toRtcpPriorityType(), | |
| ); |
🤖 Prompt for AI Agents
In `@lib/src/types/audio_encoding.dart` around lines 83 - 87, The
toRTCRtpEncoding() methods in AudioEncoding and VideoEncoding currently force a
default Priority.low when bitratePriority is null; change the priority mapping
to preserve null by calling bitratePriority?.toRtcpPriorityType() instead of
(bitratePriority ?? Priority.low).toRtcpPriorityType(), leaving networkPriority
as-is (networkPriority?.toRtcpPriorityType()); update both
AudioEncoding.toRTCRtpEncoding and VideoEncoding.toRTCRtpEncoding accordingly so
presets that omit bitratePriority remain unset.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.