Conversation
📝 WalkthroughWalkthroughAdded bitrate priority controls: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
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🎯 4 (Complex) | ⏱️ ~45 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/src/types/video_encoding.dart`:
- Around line 42-53: The copyWith implementation on VideoEncoding prevents
callers from clearing priorities because null means "use existing"; update
VideoEncoding.copyWith to accept explicit clearing flags (e.g., add bool
clearBitratePriority = false and bool clearNetworkPriority = false) and change
the logic in VideoEncoding.copyWith so that if clearBitratePriority is true it
sets bitratePriority to null, else if bitratePriority param is non-null it uses
that, otherwise it keeps this.bitratePriority; do the analogous change for
networkPriority. This keeps backward-compatible behavior for callers who omit
the flags while letting callers explicitly unset priorities.
🧹 Nitpick comments (1)
lib/src/types/audio_encoding.dart (1)
38-47: Consider:copyWithcannot reset nullable fields tonull.This pattern using
??prevents callers from explicitly clearingbitratePriorityornetworkPriorityback tonullonce set. If this is intentional (priority should never need clearing), this is fine. Otherwise, consider using a sentinel wrapper pattern.Given that these are opt-in priority overrides, the current pattern is likely acceptable for most use cases.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/types/audio_encoding.dartlib/src/types/other.dartlib/src/types/video_encoding.dartlib/src/utils.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/types/other.dart
- lib/src/utils.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). (7)
- GitHub Check: Build for Flutter Web WASM
- GitHub Check: Build for Flutter Windows
- GitHub Check: Build for Flutter Linux
- GitHub Check: Build for Flutter iOS
- GitHub Check: Build for Flutter macOS
- GitHub Check: Build for Flutter Web
- GitHub Check: Build for Flutter Android
🔇 Additional comments (8)
lib/src/types/audio_encoding.dart (3)
20-36: LGTM!The class design is clean: proper
@immutableannotation, allfinalfields, clear documentation, and appropriate constructor parameters withrequiredformaxBitrateand optional nullable priorities.
49-62: LGTM!Equality and
hashCodeare correctly implemented with consistent field coverage. Theidentical()short-circuit is a good optimization.
64-78: LGTM!Presets provide sensible default bitrates for common audio quality levels. The preset list is complete and well-organized.
lib/src/types/video_encoding.dart (5)
23-40: Solid addition of priority fields and constructor wiring.
Clear docs and immutable fields make the new priorities easy to reason about.
55-57: toString update looks good.
Includes new fields, keeping debugging output complete.
63-72: Equality and hashCode updates are correct.
Good inclusion of priority fields to keep value semantics consistent.
78-91: Comparable ordering is consistent with equality.
Nice tie-breakers on priority fields to avoid inconsistent ordering.
108-109: No issue here. The code correctly defaultsprioritytoRTCPriorityType.lowwhenbitratePriorityis null, which matches flutter_webrtc's own default (the field is non-nullable and defaults tolow). This preserves the platform default behavior rather than diverging from it.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| VideoEncoding copyWith({ | ||
| int? maxFramerate, | ||
| int? maxBitrate, | ||
| Priority? bitratePriority, | ||
| Priority? networkPriority, | ||
| }) => | ||
| VideoEncoding( | ||
| maxFramerate: maxFramerate ?? this.maxFramerate, | ||
| maxBitrate: maxBitrate ?? this.maxBitrate, | ||
| bitratePriority: bitratePriority ?? this.bitratePriority, | ||
| networkPriority: networkPriority ?? this.networkPriority, | ||
| ); |
There was a problem hiding this comment.
Allow copyWith to explicitly clear priorities.
As written, passing null keeps the existing value, so callers can’t reset a priority back to “unset.” Consider a sentinel-based pattern.
Suggested fix
class VideoEncoding implements Comparable<VideoEncoding> {
+ static const Object _unset = Object();
...
VideoEncoding copyWith({
int? maxFramerate,
int? maxBitrate,
- Priority? bitratePriority,
- Priority? networkPriority,
+ Object? bitratePriority = _unset,
+ Object? networkPriority = _unset,
}) =>
VideoEncoding(
maxFramerate: maxFramerate ?? this.maxFramerate,
maxBitrate: maxBitrate ?? this.maxBitrate,
- bitratePriority: bitratePriority ?? this.bitratePriority,
- networkPriority: networkPriority ?? this.networkPriority,
+ bitratePriority: identical(bitratePriority, _unset)
+ ? this.bitratePriority
+ : bitratePriority as Priority?,
+ networkPriority: identical(networkPriority, _unset)
+ ? this.networkPriority
+ : networkPriority as Priority?,
);🤖 Prompt for AI Agents
In `@lib/src/types/video_encoding.dart` around lines 42 - 53, The copyWith
implementation on VideoEncoding prevents callers from clearing priorities
because null means "use existing"; update VideoEncoding.copyWith to accept
explicit clearing flags (e.g., add bool clearBitratePriority = false and bool
clearNetworkPriority = false) and change the logic in VideoEncoding.copyWith so
that if clearBitratePriority is true it sets bitratePriority to null, else if
bitratePriority param is non-null it uses that, otherwise it keeps
this.bitratePriority; do the analogous change for networkPriority. This keeps
backward-compatible behavior for callers who omit the flags while letting
callers explicitly unset priorities.
| this.encoding, | ||
| this.dtx = true, | ||
| this.red = true, | ||
| this.audioBitrate = AudioPreset.music, |
There was a problem hiding this comment.
Do you follow semver? Because this was a breaking change.
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.