chore(llc): improved join call time #1126
Conversation
WalkthroughAdds global clientDetails, moves client-details setup into StreamVideo initialization, pre-warms generic SDP cache in RtcManager, and converts several non-critical awaits in call lifecycle paths to fire-and-forget to reduce join latency. Changes
Sequence DiagramsequenceDiagram
participant App as App Startup / Test
participant SV as StreamVideo
participant Globals as globals.dart
participant RM as RtcManager
participant Call as Call.join()
participant CS as CallSession
App->>SV: StreamVideo.create(options)
SV->>Globals: set global clientDetails (via _setClientDetails)
SV->>RM: cacheGenericSdp() [unawaited]
RM->>RM: generate RecvOnly & SendOnly SDPs and populate _cachedGenericSdp
RM-->>SV: returns (fire-and-forget)
App->>Call: call.join()
Call->>CS: construct session (reads global clientDetails)
CS->>CS: skip local client detail gathering
par Non-critical (fire-and-forget)
Call->>Call: unawaited(_applyConnectOptions())
Call->>Call: unawaited(_sfuStatsReporter.sendSfuStats())
and Critical path
Call->>RM: getGenericSdp(direction) -> returns cached SDP
end
Call-->>App: join() completes while non-critical tasks continue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/stream_video/lib/globals.dart (1)
4-5: Document and constrain lifecycle of globalclientDetailsThe new internal global is fine for centralizing client metadata, but it would help to:
- Add a short doc comment explaining when and by whom
clientDetailsis initialized.- Clarify (in docs or invariant) that it’s effectively write-once at startup, so later reads from
CallSessioncan safely assume it’s either fully populated or intentionallynull.This keeps the implicit contract around this shared mutable state explicit.
Also applies to: 21-22
packages/stream_video/lib/src/webrtc/rtc_manager.dart (1)
112-120: SDP caching looks good; consider error semantics and minor concurrency nitThe caching approach is sound and should meaningfully reduce repeated generic SDP generation. Two small suggestions:
- If
offer.sdpever ends upnull, returning''keeps the join flow running with an invalid SDP and hides the failure behind later errors. Consider making this a hard failure (e.g. throwing or returning a dedicated error type) so callers can surface a clear “failed to generate SDP” message instead of sending an empty SDP.- Very minor: if you ever care about avoiding duplicate work under concurrent calls, you could cache
Future<String>or useputIfAbsentwith an async initializer so simultaneousgetGenericSdp(direction)calls share the same computation. Right now you may do redundant work, but it’s harmless.Also applies to: 121-123, 126-130, 148-155
packages/stream_video/lib/src/call/session/call_session.dart (1)
254-266: GlobalclientDetailsusage: confirm expected null/initialization behaviorSwitching from a per-session
_clientDetailsto the globalclientDetailssimplifies wiring and avoids repeated device-info collection, which is good for join latency. A couple of behavior points to be explicit about:
clientDetailsis now effectively best-effort: if join/fastReconnect is initiated before the global has been initialized, SFU will seeclientDetails == nullrather than the SDK blocking to compute it. If that’s intentional, it’s worth treating “no client details” as an acceptable, non-fatal case on the backend.- Because this is global, tests or edge flows that construct
CallSessionwithout going through the normalStreamVideoinitialization will also sendnullclient details. That’s functionally safe, but you may want a debug log/trace the first time you send a join withclientDetails == nullto make this more observable.If you ever need stricter guarantees, an alternative would be to thread a
ClientDetails?into theCallSessionconstructor instead of reading globals, but the current approach is fine if the looser contract is acceptable.Also applies to: 296-304, 318-328, 437-451
packages/stream_video/lib/src/call/call.dart (2)
1048-1053: Fire‑and‑forget SFU stats: behavior change looks intentionalAll these call sites now treat
_sfuStatsReporter?.sendSfuStats(...)as best‑effort telemetry rather than part of the control flow, which aligns with the PR goal of reducing latency and avoiding failures due to non‑critical reporting.Assuming
SfuStatsReporter.sendSfuStatsis:
- internally robust to concurrent invocations, and
- catching/logging its own errors,
this change is reasonable and should improve responsiveness without affecting correctness.
Also applies to: 1346-1347, 1396-1397, 1532-1533, 1653-1658
1310-1321: Deferring_applyConnectOptionsis fine; be aware of “connected vs. configured” windowMoving
_applyConnectOptions()to a fire‑and‑forget call withcatchErrorinsideonRtcManagerCreatedCallbackmeans:
session.start()/join can now complete before camera/mic/screen‑share and audio device preferences have been fully applied.- Failures to apply those preferences are logged but no longer fail the join.
This is a good trade‑off for join latency, but it does introduce a brief window where the call is “connected” while local media configuration is still catching up. If that’s acceptable UX‑wise, you’re good; otherwise you might want to:
- clearly document that
connectOptionsare applied asynchronously after join, and/or- gate specific UI transitions on lightweight signals that media setup has finished.
📜 Review details
Configuration used: CodeRabbit UI
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 (6)
packages/stream_video/CHANGELOG.md(1 hunks)packages/stream_video/lib/globals.dart(2 hunks)packages/stream_video/lib/src/call/call.dart(7 hunks)packages/stream_video/lib/src/call/session/call_session.dart(4 hunks)packages/stream_video/lib/src/stream_video.dart(4 hunks)packages/stream_video/lib/src/webrtc/rtc_manager.dart(2 hunks)
⏰ 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: analyze
- GitHub Check: build
- GitHub Check: stream_video
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video_flutter
- GitHub Check: iOS Build (Dogfooding)
🔇 Additional comments (3)
packages/stream_video/CHANGELOG.md (1)
1-6: Changelog entry matches implementationThe performance note correctly reflects the async deferrals and SDP caching added in this PR. No issues from a code/API perspective.
packages/stream_video/lib/src/stream_video.dart (2)
11-14: LGTM!The new imports are correctly added to support the expanded client details collection and SDP caching functionality.
Also applies to: 66-66
1266-1356: LGTM!The refactored
_setClientDetails()comprehensively collects platform-specific device information and constructs theClientDetailsprotobuf model correctly. The centralized approach (moving this fromCallSessionto a global variable) is a sound optimization for join call time.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/stream_video/lib/src/stream_video.dart (1)
1358-1365: Add defensive checks for version string parsing.The version parsing still assumes exactly 3 dot-separated parts. If
streamVideoVersionhas a format like"1.0"or includes build metadata like"1.0.0+1",skip(1).firstwill throw. Although the outer try-catch handles this, defensive parsing would prevent silent failures in client details initialization.Apply this diff to add safe version parsing:
final versionSplit = streamVideoVersion.split('.'); clientDetails = sfu_models.ClientDetails( sdk: sfu_models.Sdk( type: sfu_models.SdkType.SDK_TYPE_FLUTTER, - major: versionSplit.first, - minor: versionSplit.skip(1).first, - patch: versionSplit.last, + major: versionSplit.isNotEmpty ? versionSplit[0] : '0', + minor: versionSplit.length > 1 ? versionSplit[1] : '0', + patch: versionSplit.length > 2 ? versionSplit[2] : '0', ),
📜 Review details
Configuration used: CodeRabbit UI
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 (5)
packages/stream_video/lib/src/call/call.dart(7 hunks)packages/stream_video/lib/src/stream_video.dart(7 hunks)packages/stream_video/test/src/call/call_allow_multiple_active_calls_test.dart(2 hunks)packages/stream_video/test/src/call/call_apply_settings_test.dart(1 hunks)packages/stream_video/test/src/core/client_state_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_video/lib/src/call/call.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). (9)
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video
- GitHub Check: stream_video_screen_sharing
- GitHub Check: stream_video_flutter
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: analyze
- GitHub Check: build
- GitHub Check: analyze_legacy_version
- GitHub Check: iOS Build (Dogfooding)
🔇 Additional comments (5)
packages/stream_video/test/src/call/call_apply_settings_test.dart (1)
59-59: LGTM! Test appropriately disables SDP precaching.The addition of
precacheGenericSdps: falseis appropriate for the test context, where performance optimizations are not needed and may add unnecessary complexity or timing issues.packages/stream_video/test/src/call/call_allow_multiple_active_calls_test.dart (1)
81-81: LGTM! Consistent test configuration.Both test groups appropriately disable SDP precaching for deterministic test behavior.
Also applies to: 209-209
packages/stream_video/test/src/core/client_state_test.dart (1)
66-66: LGTM! Test configuration aligned with other test files.Disabling SDP precaching maintains consistency across the test suite.
Also applies to: 187-187
packages/stream_video/lib/src/stream_video.dart (2)
11-11: LGTM! Imports properly support new functionality.The new imports for
system_info2, protobuf models, andrtc_managerare appropriately added to support client details gathering and SDP precaching.Also applies to: 14-14, 66-66
131-131: LGTM! Parameter properly added to API surface.The
precacheGenericSdpsparameter is correctly threaded through the factory and private constructor with a sensible default value oftruefor production use.Also applies to: 141-141, 154-154
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1126 +/- ##
=====================================
Coverage 6.29% 6.30%
=====================================
Files 593 593
Lines 40887 40894 +7
=====================================
+ Hits 2574 2578 +4
- Misses 38313 38316 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.