Add a sort by source in the LivestreamPlayer default implementation#1681
Merged
aleksandar-apostolov merged 1 commit intoMay 13, 2026
Merged
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
aleksandar-apostolov
changed the title
LivestreamPlayer default implementationLivestreamPlayer default implementation
PratimMallick
approved these changes
May 13, 2026
WalkthroughLivestreamPlayer's default host video selection logic is refactored to prioritize participants by source rank. A new ChangesLivestream Host Video Selection Priority
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (2)
stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/livestream/LivestreamPlayer.kt (2)
338-345: 💤 Low valueConsider adding a comment to document the priority ordering rationale.
The ranking function establishes RTMP as highest priority and WebRTC as lowest, but the reasoning isn't immediately clear from the code. A brief comment explaining why this ordering was chosen would help future maintainers understand the design decision.
📝 Suggested documentation addition
+// Ranks participant sources by priority for livestream host selection. +// Lower values = higher priority. RTMP sources are preferred for reliability and quality. private fun participantSourceRank(s: ParticipantSource): Int = when (s) { ParticipantSource.PARTICIPANT_SOURCE_RTMP -> 0 ParticipantSource.PARTICIPANT_SOURCE_WHIP -> 1 ParticipantSource.PARTICIPANT_SOURCE_RTSP -> 2 ParticipantSource.PARTICIPANT_SOURCE_SRT -> 3 ParticipantSource.PARTICIPANT_SOURCE_WEBRTC_UNSPECIFIED -> 4 ParticipantSource.PARTICIPANT_SOURCE_SIP -> 2 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/livestream/LivestreamPlayer.kt` around lines 338 - 345, The participantSourceRank function assigns numeric priority to ParticipantSource values but lacks documentation; add a concise comment above participantSourceRank explaining the rationale for the ordering (e.g., why RTMP is highest priority, WebRTC lowest, and why SIP/RTSP share a rank), mention any tie-breaking or expected behavior when priorities equal, and reference the mapped constants (ParticipantSource.PARTICIPANT_SOURCE_RTMP, _WHIP, _RTSP, _SRT, _WEBRTC_UNSPECIFIED, _SIP) so future maintainers understand the design decision and can update the mapping consistently.
338-345: ⚡ Quick winClarify the duplicate rank assignment or assign unique priorities.
Both
PARTICIPANT_SOURCE_RTSPandPARTICIPANT_SOURCE_SIPare assigned rank2. When multiple participants with the same rank exist,minByOrNullwill return the first one encountered, making the selection non-deterministic. If equal priority is intentional, consider adding a comment explaining why. Otherwise, assign unique ranks to ensure deterministic selection.📋 Suggested fix to assign unique ranks
private fun participantSourceRank(s: ParticipantSource): Int = when (s) { ParticipantSource.PARTICIPANT_SOURCE_RTMP -> 0 ParticipantSource.PARTICIPANT_SOURCE_WHIP -> 1 ParticipantSource.PARTICIPANT_SOURCE_RTSP -> 2 ParticipantSource.PARTICIPANT_SOURCE_SRT -> 3 ParticipantSource.PARTICIPANT_SOURCE_WEBRTC_UNSPECIFIED -> 4 - ParticipantSource.PARTICIPANT_SOURCE_SIP -> 2 + ParticipantSource.PARTICIPANT_SOURCE_SIP -> 5 }Or, if equal priority is intentional, add a clarifying comment:
private fun participantSourceRank(s: ParticipantSource): Int = when (s) { ParticipantSource.PARTICIPANT_SOURCE_RTMP -> 0 ParticipantSource.PARTICIPANT_SOURCE_WHIP -> 1 + // RTSP and SIP have equal priority ParticipantSource.PARTICIPANT_SOURCE_RTSP -> 2 ParticipantSource.PARTICIPANT_SOURCE_SRT -> 3 ParticipantSource.PARTICIPANT_SOURCE_WEBRTC_UNSPECIFIED -> 4 ParticipantSource.PARTICIPANT_SOURCE_SIP -> 2 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/livestream/LivestreamPlayer.kt` around lines 338 - 345, The participantSourceRank function assigns the same rank (2) to PARTICIPANT_SOURCE_RTSP and PARTICIPANT_SOURCE_SIP which makes minByOrNull selection non-deterministic; update participantSourceRank to give each source a unique priority (e.g., change PARTICIPANT_SOURCE_SIP to a distinct rank) or, if equal priority is intentional, add a clarifying comment inside participantSourceRank explaining why RTSP and SIP share rank 2 and that tie-breaking order is acceptable; ensure references to ParticipantSource.PARTICIPANT_SOURCE_RTSP and ParticipantSource.PARTICIPANT_SOURCE_SIP are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/livestream/LivestreamPlayer.kt`:
- Around line 338-345: The participantSourceRank function assigns numeric
priority to ParticipantSource values but lacks documentation; add a concise
comment above participantSourceRank explaining the rationale for the ordering
(e.g., why RTMP is highest priority, WebRTC lowest, and why SIP/RTSP share a
rank), mention any tie-breaking or expected behavior when priorities equal, and
reference the mapped constants (ParticipantSource.PARTICIPANT_SOURCE_RTMP,
_WHIP, _RTSP, _SRT, _WEBRTC_UNSPECIFIED, _SIP) so future maintainers understand
the design decision and can update the mapping consistently.
- Around line 338-345: The participantSourceRank function assigns the same rank
(2) to PARTICIPANT_SOURCE_RTSP and PARTICIPANT_SOURCE_SIP which makes
minByOrNull selection non-deterministic; update participantSourceRank to give
each source a unique priority (e.g., change PARTICIPANT_SOURCE_SIP to a distinct
rank) or, if equal priority is intentional, add a clarifying comment inside
participantSourceRank explaining why RTSP and SIP share rank 2 and that
tie-breaking order is acceptable; ensure references to
ParticipantSource.PARTICIPANT_SOURCE_RTSP and
ParticipantSource.PARTICIPANT_SOURCE_SIP are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b0f587ea-2c86-4e46-a621-e9f80f21c609
📒 Files selected for processing (1)
stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/livestream/LivestreamPlayer.kt
Contributor
SDK Size Comparison 📏
|
|
aleksandar-apostolov
deleted the
fix/livestream-player-participant-sorting
branch
Collaborator
|
🚀 Available in v1.23.1 |
aleksandar-apostolov
added a commit
that referenced
this pull request
May 19, 2026
* test(core): add failing regression test for sortedParticipants channelFlow bug SortedParticipantsState wraps its sort logic in a channelFlow whose builder block launches two coroutines on the outer scope and returns immediately. The channel closes as soon as the block returns, so all subsequent trySend calls from inside the launches go to a closed channel and are silently dropped. The existing tests in CallStateTest pass because the global DispatcherRule installs UnconfinedTestDispatcher, which runs launched coroutines inline up to first suspension and lets the initial sort squeak through before the channel closes. Under any realistic production dispatcher (Main, Default, IO), the bug surfaces: sortedParticipants never emits, OrientationVideoRenderer always falls back to the unsorted participants map for >6-participant calls. This test isolates SortedParticipantsState with StandardTestDispatcher to surface the production behavior. Expected red on current develop; will go green after the StateFlow migration. * feat(core): add sort comparator primitives and visibility-aware decorators Introduces a React-parity sorting toolkit under stream-video-android-core/sorting: - combineComparators / conditional / ifInvisible / ifInvisibleOrUnknown composable building blocks that return -1, 0, +1 directly (no compareBy(Boolean) inversion bug) - dominantSpeaker, speaking, screenSharing, publishingVideo, publishingAudio, raisedHand, byReactionType, byName, byJoinedAt, byRole, bySourcePriority primitive comparators on ParticipantState - pinned (internal) — local pin > server pin > pinnedAt recency bySourcePriority replaces the previous participantSourceRank, fixing the SIP=RTSP=2 collision by taking a vararg priority list and ranking by argument position. ifInvisible vs ifInvisibleOrUnknown lets presets choose how strict the visibility guard is — UNKNOWN counts as visible for the former, as invisible for the latter. Adds 25 unit tests (15 primitive truth tables, 10 combinator/decorator behaviors) verified against the React equivalents in stream-video-js. Test fixtures support the A-F React fixture shape and 15-participant rolling-scroll scenarios. * feat(core): add SortPreset sealed interface with Default, SpeakerLayout, AudioRoom Three named presets for the participant grid: - Default: screenSharing → pinned → ifInvisibleOrUnknown(dominantSpeaker → speaking → raisedHand → ingressSource → publishingVideo → publishingAudio). The continuous- scroll grid choice — UNKNOWN-as-invisible so off-screen tiles get ordered by activity before they render. - SpeakerLayout: screenSharing → pinned → dominantSpeaker → ifInvisible(...). The dominant speaker is outside the visibility guard so the spotlight always reflects who's talking. - AudioRoom: ifInvisibleOrUnknown(...) → byRole(admin, host, speaker). Role-aware ordering for livestream-style rooms. The pinned comparator is internal (its PinUpdateAtTime parameter type is internal). Presets resolve via an internal `SortPreset.build(pins)` extension invoked by the sorter before each pass, so the latest local/server pin map participates in the ordering. Tests cover the React-parity composed-comparator scenarios, three Default-preset rolling-scroll scenarios (15 participants, visible window P8..P12), the SpeakerLayout visibility-guard difference, and the AudioRoom role priority. 13 tests total, all green. * fix(core): rewrite SortedParticipantsState to back sortedParticipants with StateFlow The previous implementation wrapped sorting in a channelFlow whose builder body launched two coroutines on the outer scope and returned immediately. The channel closed before any collector subscribed, so subsequent trySend calls hit a closed channel and the public sortedParticipants flow never emitted under any non- Unconfined dispatcher. OrientationVideoRenderer's `if (size > 6) sortedParticipants else participants` always took the fallback path; the renderer effectively never applied sorting in production. The rewrite: - Backs the public list with a MutableStateFlow<List<ParticipantState>>. - Drives sorting from a combine over (participants, pinned-detailed, preset, custom-comparator) plus a separate launch for call.events. Both write into the same _sortedParticipants.value via a synchronous resort() that preserves the previous-order stability trick and skips emission when the order is unchanged. - Accepts a SortPreset (default SortPreset.Default) and exposes setPreset(...) for runtime switching. updateComparator(Comparator) stays as an escape hatch for ad-hoc orderings. - CallState now derives an internal _pinnedParticipantsDetailed flow combining _localPins + _serverPins while preserving PinUpdateAtTime (which carries the PinType). Local pins override server pins on key collision. The public pinnedParticipants StateFlow keeps its existing Map<String, OffsetDateTime> shape via mapState — no surface-level change. The earlier regression test now passes (was red on develop). Two existing CallStateTest sorting tests adjusted: switched stateIn target to backgroundScope to avoid runTest leak detection, and updated the expected order to reflect the new Default preset behavior (screen-sharer leads pinned because screenSharing is the first comparator in the chain). * refactor(compose): LivestreamPlayer consumes sortedParticipants; remove duplicated source rank The default livestream picker now reads call.state.sortedParticipants and selects the first participant with videoEnabled. Because the Default SortPreset's bySourcePriority sits inside the visibility guard and viewers in a livestream context typically render as UNKNOWN, the ingress host (RTMP / WHIP / SRT / RTSP) naturally sorts first and the picker resolves to them. This deletes the local participantSourceRank(s: ParticipantSource) helper added in PR #1681, which duplicated the rank table in DefaultSortOrder.kt and carried the same SIP=RTSP=2 collision. The canonical bySourcePriority(vararg) replaces it. DefaultSortOrder.kt is deleted entirely — its behavior is subsumed by SortPresets. * test(core): add SortedParticipantsState flow-behavior tests Six tests covering the reactive surface of the new StateFlow-backed SortedParticipantsState under StandardTestDispatcher: - participants flow change emits an updated sorted list - pinned flow change pushes the pinned participant to the front - preset switch triggers a resort using the new preset - custom comparator overrides the active preset until cleared - repeated identical sorts are coalesced (same list instance returned, no redundant emission) - call event triggers a resort against current participant + pin state, catching state changes that bypass the participants map (e.g. event handlers flipping dominantSpeaker on a ParticipantState directly) * chore(core): apiDump for sorting changes + spotless formatting Updates stream-video-android-core.api to reflect the new public sorting surface: - SortPreset sealed interface with Default, SpeakerLayout, AudioRoom data objects - Top-level comparator primitives (dominantSpeaker, speaking, screenSharing, publishingVideo, publishingAudio, raisedHand, byName, byJoinedAt) - combineComparators, conditional, ifInvisible, ifInvisibleOrUnknown combinators - bySourcePriority, byRole, byReactionType comparator factories - CallState.setSortPreset(...) — runtime preset switching - CallState.getSortedParticipants() return type narrowed from Flow to StateFlow The Flow → StateFlow narrowing is a binary-compat break for Java consumers but source-compatible for Kotlin callers (StateFlow IS-A Flow). Acceptable under the v2 breaking-change allowance per CLAUDE.md. Spotless reflowed ComparatorsTest and SortPresetsTest line breaks; no logic changes. * refactor(core): consolidate participants and sortedParticipants into a single sorted flow CallState.participants now sources from _sortedParticipantsState.sortedParticipants — it always returns the list ordered by the active SortPreset. The old sortedParticipants property is kept as a @deprecated WARNING alias that points at participants, so existing callers compile and run unchanged with a migration nudge. This collapses the dual-flow architecture that was the root cause of a renderer bug discovered during visual smoke testing: OrientationVideoRenderer's val callParticipants by remember(participants) { derivedStateOf { if (sortedParticipants.size > 6) sortedParticipants else participants } } held two separate StateFlow subscriptions for the same conceptual list, with remember keyed only on `participants`. The dual subscription created subtle staleness and lifecycle interactions with the DisposableEffect setVisibility calls that drive SFU track subscriptions — tiles rendered in the correct sort order but their video tracks failed to display. With a single sorted flow, OrientationVideoRenderer collapses to: val callParticipants by call.state.participants.collectAsStateWithLifecycle() remoteParticipants also moves to derive from the sorted participants flow (filtering out the local sessionId), so it's consistently sorted with participants. Lazy-initialized to defer the forward reference to _sortedParticipantsState. LivestreamPlayer's default livestreamFlow and the existing CallStateTest sorting tests switched off the deprecated alias in the same pass. Net effect: - One conceptual list, one StateFlow subscription, no dual-flow staleness - Strictly fewer recompositions in scroll-heavy paths thanks to the sorter's lastSortOrder coalescing (visibility flips that don't change order no longer emit downstream) - React parity restored — both SDKs expose participants as the single sorted list - The previously-dead `if (size > 6) sortedParticipants` branch is gone * test(core): cover visible-block stability end-to-end through the live flow Two reactive-path tests added to SortedParticipantsStateTest: - `visible-block internal order survives off-screen dominant-speaker promotion`: 15 participants with P8..P12 marked VISIBLE, P15 (off-screen) gains dominantSpeaker. The call event drives a resort. Assertions: P15 jumps to index 0, the visible block keeps [P8, P9, P10, P11, P12] internal order. - `visible block stays stable when a visible participant gains a signal`: P10 (inside the viewport) becomes dominantSpeaker. The visible/visible predicate returns 0 for every pair in the visible block, so the list does NOT reshuffle — P10 stays at its original index instead of jumping to the top. These exercise the full reactive path that the existing SortPresetsTest suite skips: direct field mutation on ParticipantState (mirroring the SDK's DominantSpeakerChangedEvent handler), the call.events trigger, the combine collect, the lastSortOrder coalescing, and the final StateFlow emission. Closes the verification gap between the static comparator tests and the visual smoke check. * feat(core): add byUserId tiebreaker to all sort presets, mirroring iOS Appends `ifInvisibleOrUnknown(byUserId)` (or `ifInvisible(byUserId)` for SpeakerLayout) as a trailing decorator on each preset. Matches iOS's pattern of `ifInvisibleBy(userId)` as the last comparator in every preset. Effect: when two participants share every other signal (no dominant-speaker, no pin, no raised hand, no ingress source advantage, equal track state), the sort falls back to lexicographic userId compare instead of stable input order. The visible block keeps its internal relative order (visible/visible pairs return 0 regardless of byUserId), and the invisible tail becomes deterministic across re-sorts and across sessions. In production, userIds are UUIDs so the resulting tail order is meaningless to humans — but it's STABLE. That's the value: no surprise reshuffles when a sort fires with no meaningful state change. Tests: - New `byUserId` primitive truth-table tests in ParticipantComparatorsTest. - Updated `Default preset - two off-screen promotions bubble...` expectation to the new lex-ordered tail, with an explanatory comment. - Updated `custom comparator overrides preset until cleared` to expect [a,b,c] after setPreset(Default) — the tiebreaker actively orders by userId now, replacing the previous "all signals 0 → stable input order" behavior. For AudioRoom, the tiebreaker runs AFTER byRole(...) so role priority still wins on equal participants — the test `AudioRoom - host role beats regular participant` now passes without modification. API additive: `getByUserId()` exposed alongside the other primitive comparators. * fix(core): preserve binary compatibility of sortedParticipants return type The previous commit narrowed CallState.sortedParticipants from Flow to StateFlow, which is binary-incompatible for Java callers compiled against the v1 API. Reverts the public return type of the deprecated `sortedParticipants` alias back to `Flow<List<ParticipantState>>`. The underlying value is still a StateFlow (returned via `get() = participants`), so consumers see identical runtime behavior; only the declared method signature stays on the wider Flow type. apiCheck against develop now shows only additive changes: setSortPreset(...), the SortPreset sealed interface + 3 data objects, and the new comparator primitives/combinators in the io.getstream.video.android.core.sorting package. No removed or modified public symbols. * refactor(compose): keep LivestreamPlayer's source-priority sort self-contained Re-applies bySourcePriority(RTMP, WHIP, SRT, RTSP) inside the LivestreamPlayer's default livestreamFlow before picking the first video-enabled participant. The grid-level preset already orders ingress sources first, but the explicit local sort isolates the picker from any future change to the active grid preset — swapping the grid preset cannot accidentally affect which participant the livestream player picks as the host. Stable sort preserves the upstream grid order among same-source participants, so no additional ordering rules need to be encoded here. Uses the canonical bySourcePriority comparator from the sorting package; no local rank table. * refactor(core): wire CallType.sortPreset and rename AudioRoom preset Adds an open `sortPreset` property on the `CallType` sealed class so each call type can declare its default participant sort preset, matching React's `CallType` → `sortParticipantsBy` association. `CallType.Livestream` overrides to `SortPreset.LivestreamOrAudioRoom`; other types stay on `SortPreset.Default`. `CallState` resolves the preset on construction via `CallType.fromName(call.type)?.sortPreset ?: SortPreset.Default` and passes it as `initialPreset` to `SortedParticipantsState`. Callers can still override at runtime via `setSortPreset(...)` or `updateParticipantSortingOrder(...)`. `SortPreset.AudioRoom` renamed to `SortPreset.LivestreamOrAudioRoom` for cross-platform naming parity (matches iOS `livestreamOrAudioRoomSortPreset` and React `livestreamOrAudioRoomSortPreset`). Rename is internal to this PR — nothing on develop referenced the old name. `CallType.AudioCall` (Android-only 1:1 audio concept) intentionally stays on the default preset; a dedicated `CallType.AudioRoom` for the actual group-audio call type will land in a follow-up PR. `LivestreamPlayer`'s local source-priority sort stays in place as defense in depth — if anything changes the active preset at runtime, the host picker is still correct. 6 new CallTypeSortPresetTest cases verifying the resolution path. apiCheck against develop: still additive only. * fix(core): address CodeRabbit review - mutex + defensive preset fallback - Adds an `else` branch to `SortPreset.build(...)` so unknown SortPreset implementations (Java consumers can implement Kotlin sealed interfaces on JVM targets below 17) fall back to Default rather than throwing NoWhenBranchMatchedException. Sorting must never crash a call. - Wraps SortedParticipantsState.resort() body in a Mutex. The state combine collector and the call.events collector both call resort(), which mutates lastSortOrder and _sortedParticipants. Production CallState scope today is single-threaded (Dispatchers.Main), but the contract should hold regardless of dispatcher choice. Mutex contention is negligible — resort body is microseconds of sync work. Tests: all 55 sorting cases still pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
LivestreamPlayerdefault implementation does not consider the rank of the participant by source. This causes a WebRTC participant to be sometimes chosen instead of an RRMP participant.Implementation
Sort by source when choosing participant with video instead of always taking the first one.
Testing
Run a livestream with 2 publishers one RTMP and other WebRTC. The RTMP one should be shown in the UI.
Summary by CodeRabbit