Fix an issue that causes missing video due to track arriving before the participant info has arrived #1585
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
WebRTC before the participant info has arrived
SDK Size Comparison 📏
|
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
Outdated
Show resolved
Hide resolved
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
Show resolved
Hide resolved
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
Outdated
Show resolved
Hide resolved
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
Outdated
Show resolved
Hide resolved
...o-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt
Show resolved
Hide resolved
WalkthroughThe changes add an optional Changes
Sequence DiagramssequenceDiagram
participant WebRTC as WebRTC Connection
participant Sub as Subscriber
participant RtcSess as RtcSession
participant Evt as Event System
Note over WebRTC,Evt: Scenario: Track arrives before Participant
WebRTC->>Sub: onAddStream(stream)<br/>[track received]
Sub->>Sub: lookup session/trackType
Sub->>RtcSess: setTrack(sessionId, trackType, track)<br/>[participant not yet created]
RtcSess->>RtcSess: registerOrphanedTrack()<br/>[store track in orphanedTracks]
Note over WebRTC,Evt: Scenario: Participant joins or created
Evt->>RtcSess: TrackPublishedEvent with<br/>participant or<br/>ParticipantJoinedEvent
RtcSess->>RtcSess: create/update participant
RtcSess->>RtcSess: reconcileOrphanedTracks()<br/>[retrieve stored tracks]
RtcSess->>RtcSess: setTrack() for each<br/>orphaned track
Note over WebRTC,Evt: Scenario: Stream removal
WebRTC->>Sub: onRemoveStream(stream)
Sub->>Sub: lookup sessionId, trackType
Sub->>Sub: clean internal maps<br/>(tracks, mappings)
Sub->>Sub: emit RemovedMediaStream
Sub->>RtcSess: removedStreamsFlow
RtcSess->>RtcSess: cleanupOrphanedTracks<br/>ForSessionAndType()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (1)
1262-1289: Verify potential duplicate reconciliation.Lines 1274 and 1288 both call
reconcileOrphanedTracks(event.sessionId)within the sameTrackPublishedEventhandler. Line 1274 is inside theif (event.participant != null)block, and line 1288 is at the end of the handler.This means when
event.participant != null, reconciliation happens twice. While not incorrect (reconciliation is idempotent viatakeOrphanedTracks), it may be inefficient.Consider whether the reconciliation at line 1288 should be in an
elseblock or removed, since line 1274 already handles the reconciliation when participant info is present.Apply this diff if reconciliation at line 1288 should only happen when participant is not included:
updatePublishState( userId = event.userId, sessionId = event.sessionId, trackType = event.trackType, videoEnabled = true, audioEnabled = true, paused = false, ) - - // Reconcile orphaned tracks for this participant - // The track might have arrived before the participant was created - reconcileOrphanedTracks(event.sessionId) }stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt (1)
607-668: Consider refactoring to reduce duplication.The implementation correctly handles track removal and cleanup, but the logic for audio and video tracks is duplicated. Consider extracting a helper function to reduce duplication:
private fun processRemovedTrack(trackId: String, trackLabel: String) { val sessionId = trackIdToParticipant[trackId] val trackType = trackIdToTrackType[trackId] if (sessionId != null && trackType != null) { logger.i { "[onRemoveStream] #sfu; #track; #orphaned-track; Removing $trackLabel track for " + "sessionId=$sessionId, trackType=$trackType, trackId=$trackId" } // Remove from internal tracking tracks[sessionId]?.remove(trackType) trackIdToParticipant.remove(trackId) trackIdToTrackType.remove(trackId) // Emit removal event removedStreamsFlow.tryEmit(RemovedMediaStream(sessionId, trackType)) } else { logger.w { "[onRemoveStream] #sfu; #track; #orphaned-track; Could not find sessionId or trackType " + "for removed $trackLabel track: $trackId" } } } override fun onRemoveStream(stream: MediaStream?) { super.onRemoveStream(stream) if (stream == null) return logger.i { "[onRemoveStream] #sfu; #track; #orphaned-track; stream: $stream" } stream.audioTracks.forEach { track -> processRemovedTrack(track.id(), "audio") } stream.videoTracks.forEach { track -> processRemovedTrack(track.id(), "video") } }However, the current implementation is clear and correct, so this is optional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
stream-video-android-core/api/stream-video-android-core.api(2 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt(9 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt(3 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/signal/socket/RTCEventMapper.kt(2 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/SfuDataEvent.kt(1 hunks)stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/OrphanedTracksTest.kt(1 hunks)stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/SubscriberTest.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt (1)
setTrack(236-239)
⏰ 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). (6)
- GitHub Check: compare-sdk-sizes / Compare SDK sizes
- GitHub Check: Build / compose apks
- GitHub Check: Spotless check
- GitHub Check: API check
- GitHub Check: Unit Tests
- GitHub Check: build
🔇 Additional comments (19)
stream-video-android-core/api/stream-video-android-core.api (2)
11028-11037: LGTM! Consistent API extension for TrackPublishedEvent.The addition of the optional
Participantparameter toTrackPublishedEventis well-structured:
- The synthetic constructor (line 11029) maintains backward compatibility for existing callers
- All data class methods (component4, copy, getter) are properly updated
- This change enables the race condition fix by allowing participant info to arrive with track events
Ensure that consumers of this API handle the nullable participant gracefully, particularly in scenarios where tracks arrive before participant information.
11046-11055: LGTM! Parallel changes to TrackUnpublishedEvent.The
TrackUnpublishedEventreceives the same treatment asTrackPublishedEvent, maintaining consistency:
- Synthetic constructor for backward compatibility (line 11047)
- Complete data class method updates (component4, copy, getter)
- Same optional participant parameter design
The parallel API design ensures both track publication and unpublication events can carry participant information, enabling proper orphaned track reconciliation in both directions.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (8)
265-290: LGTM! Clean orphaned tracks infrastructure.The
OrphanedTrackdata class andConcurrentHashMapstorage provide a clean, thread-safe foundation for handling tracks that arrive before participants. The key format and synchronization strategy are well-documented.
315-344: LGTM! Proper orphaned track handling in setTrack.The modification correctly handles the race condition by checking for participant existence before attaching tracks. The early return and clear logging make the flow easy to follow.
346-359: LGTM! Simple and thread-safe registration.The implementation is straightforward and correctly uses
ConcurrentHashMap.put()for atomic registration. The logging includes helpful metrics for debugging.
361-389: LGTM! Proper synchronization for compound operation.The
synchronizedblock correctly protects the compound read-and-remove operation. WhileConcurrentHashMapprovides atomic single operations, the iteration and conditional removal requires external synchronization, which is properly applied here.Based on learnings, this addresses the past review discussion about synchronization strategy.
391-407: LGTM! Clean reconciliation logic.The reconciliation flow is straightforward: retrieve orphaned tracks, then attach them via
setTrack(). Since the participant now exists,setTrack()will succeed. The early return for empty cases is efficient.
409-428: LGTM! Proper cleanup for removed streams.This cleanup correctly handles the case where a WebRTC track is removed while still orphaned. The atomic
ConcurrentHashMap.remove()is thread-safe, and the JS SDK equivalence is well-documented.
539-549: LGTM! Proper integration with removal flow.The listener correctly subscribes to
removedStreams()and triggers cleanup of orphaned tracks. This prevents memory leaks when streams are removed before participant info arrives.
951-959: LGTM! Comprehensive cleanup prevents memory leaks.The cleanup correctly clears any remaining orphaned tracks during session cleanup. The warning log helps identify cases where tracks were never reconciled, which is useful for debugging.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/SfuDataEvent.kt (1)
104-126: LGTM! Clean API extension for large calls optimization.The optional
participantfield is a clean addition that maintains backward compatibility via the defaultnullvalue. The KDoc clearly explains when the field is present and how it should be used, making the API easy to understand.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/signal/socket/RTCEventMapper.kt (1)
79-95: LGTM! Correct event mapping with participant context.The mapper correctly passes through the optional
participantfield from SFU events. The inline comments clearly explain the purpose of this field for large call optimization.stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/SubscriberTest.kt (1)
391-433: LGTM! Comprehensive test coverage for removal functionality.The three new tests provide good coverage:
- Happy path: verifies internal state cleanup
- Null handling: ensures robustness
- Unknown tracks: prevents exceptions on unexpected input
The tests correctly verify both the
tracksmap andtrackIdToParticipantmap cleanup.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt (2)
111-120: LGTM! Clean data class for removed streams.The
RemovedMediaStreamdata class mirrors the design ofReceivedMediaStream, providing consistency in the API. The documentation clearly describes its purpose.
536-545: LGTM! Proper flow setup for removal events.The
removedStreamsFlowconfiguration matches thestreamsFlowpattern with appropriate buffer settings. The publicremovedStreams()accessor provides a clean API for observers.stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/OrphanedTracksTest.kt (4)
45-94: LGTM! Good integration test structure.The test correctly simulates the race condition scenario: checking participant doesn't exist, creating it via
getOrCreateParticipant, and verifying state. The delay allows async reconciliation to complete. The comment about WebRTC track simulation limitations is helpful context.
96-183: LGTM! Excellent coverage of edge cases.These tests properly verify:
- Multiple tracks per participant are all reconciled
- Different participants' orphaned tracks don't interfere with each other
The assertions verify both video and audio enabled flags, demonstrating proper track type handling.
185-263: LGTM! Tests cover large call optimization flow.These tests correctly verify the large call optimization where
TrackPublishedEventandTrackUnpublishedEventinclude participant info. The tests demonstrate both participant creation and state updates work correctly.
265-308: LGTM! Normal flow test provides baseline.This test verifies the normal case (participant exists before track arrives) still works correctly. This is important to ensure the orphaned tracks mechanism doesn't break the common path.
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/OrphanedTracksTest.kt
Show resolved
Hide resolved
|
Goal
Fix a race condition where video tracks are permanently lost when they arrive before participant information, resulting in users seeing infinite loading spinners instead of video.
Implementation
Implemented an orphaned tracks mechanism that stores tracks arriving before participants exist, then automatically reconciles them when participant information arrives via any event (JoinResponse, ParticipantJoined, TrackPublished/Unpublished with optional participant field).
Testing
Created comprehensive integration tests covering all race condition scenarios including track-before-participant ordering.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.