Move audio switching logic to execute on another thread#1648
Conversation
…switching off the main-thread
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
|
@coderabbitai Analyse this PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the PR details and diff to conduct a thorough analysis. 🏁 Script executed: Length of output: 15463
🔍 PR Analysis: Move Audio Switching Logic to Another Thread✅ What's Good
|
WalkthroughThe changes migrate audio handling from Android main-thread Handler/Looper callbacks to coroutine-based execution. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 (4)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioHandler.kt (3)
29-29: TODO comments flagged by static analysis.These TODO comments indicate planned refactoring work. Consider creating tracking issues to ensure they're addressed:
- Line 29: Make
AudioHandlerinternal withResult<>return types- Line 43: Merge
AudioSwitchHandlerintoMicrophoneManagerWould you like me to help draft GitHub issues to track these technical debt items?
Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioHandler.kt` at line 29, The TODOs indicate technical debt: make the AudioHandler class internal and change its public APIs to return Result<> instead of throwing/using nullable returns, so update the AudioHandler declaration to internal and refactor its public methods (e.g., start/stop/prepare functions in AudioHandler) to return Result<T> wrapping success/error; additionally merge the AudioSwitchHandler functionality into MicrophoneManager by moving/combining methods and state from AudioSwitchHandler into MicrophoneManager and removing the separate AudioSwitchHandler type, ensuring callers now use MicrophoneManager for device switching and preserving existing behavior and error propagation via Result<>.
69-82: Fire-and-forget coroutine launches may complicate cleanup sequencing.The
start(),stop(), andselectDevice()methods usescope.launchwithout awaiting completion. Forstop()specifically, callers likeMicrophoneManager.cleanup()cannot guarantee the audio switch is fully stopped before proceeding.Given the single-threaded dispatcher and the PR's goal of eliminating frame drops, this trade-off is reasonable. However, if deterministic cleanup becomes necessary in the future, consider returning a
Jobor providing a suspending variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioHandler.kt` around lines 69 - 82, The methods start(), stop(), and selectDevice() currently fire-and-forget with scope.launch which prevents callers (e.g., MicrophoneManager.cleanup()) from deterministically waiting for controller.start()/controller.stop()/controller.selectDevice() to finish; change these APIs to either return the Job from scope.launch (e.g., return scope.launch { controller.stop() }) or provide suspending variants (suspend fun stop() { ensureScope()?.let { it.launch... } or directly call controller.stop() if already suspending) so callers can await completion; keep ensureScope() and controller.* calls intact and update call sites like MicrophoneManager.cleanup() to await the returned Job or call the suspending variant.
58-67: Silent no-op whenStreamVideoinstance is unavailable.If
StreamVideo.instanceOrNull()returnsnullor isn't aStreamVideoClient,ensureScope()returnsnullandstart()/stop()/selectDevice()silently do nothing. This could make debugging difficult if audio issues arise due to incorrect SDK initialization order.Consider adding a log warning when the instance is unavailable:
💡 Add logging for debugging
private fun ensureScope(): CoroutineScope? { val ctx = (StreamVideo.instanceOrNull() as? StreamVideoClient) - ?.getAudioContext() ?: return null + ?.getAudioContext() + + if (ctx == null) { + StreamLog.w(TAG) { "[ensureScope] StreamVideoClient not available, audio operations will be skipped" } + return null + } if (scope == null) { scope = ctx.createChildScope() } return scope }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioHandler.kt` around lines 58 - 67, ensureScope() currently returns null when StreamVideo.instanceOrNull() is missing or not a StreamVideoClient, causing start()/stop()/selectDevice() to silently no-op; update ensureScope (in class AudioHandler) to emit a clear warning log (e.g., Log.w or your existing logger) right before returning null that states StreamVideo is unavailable or not a StreamVideoClient and that audio operations will be skipped, and also log when creating the child scope (when ctx.createChildScope() runs) so initialization failures are visible.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioExecutionContext.kt (1)
43-46: Potential crash ifrelease()is called before lazy properties are accessed.If
release()is called when neitherscopenordispatcherhave been initialized (e.g., early cleanup path),scope.cancel()will force lazy initialization, then immediately cancel. This is wasteful but not harmful. However, callingdispatcher.close()on a just-initialized dispatcher that was never used is safe.More importantly, after
release()is called, subsequent calls tocreateChildScope()will throwIllegalStateExceptionbecause the dispatcher is closed. Consider adding a guard or documenting this constraint.💡 Optional: Add closed state tracking
+private var released = false + fun createChildScope(): CoroutineScope { + check(!released) { "AudioExecutionContext has been released" } return CoroutineScope(scope.coroutineContext + Job(scope.coroutineContext[Job])) } fun release() { + if (released) return + released = true scope.cancel() dispatcher.close() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioExecutionContext.kt` around lines 43 - 46, Add a closed/released flag and guard lazy initialization: introduce a private var released = false, set released = true at the start of release(), and only cancel/close scope and dispatcher if they are already initialized (avoid forcing lazy init — convert the lazy delegates to checkable forms or use nullable/backing properties so you can test initialization before accessing). Also update createChildScope() to check released and throw a clear IllegalStateException (or return a no-op/closed scope) when released is true to avoid post-release usage. Ensure references include release(), scope, dispatcher, and createChildScope().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioExecutionContext.kt`:
- Around line 39-41: createChildScope() currently builds a standalone Job() so
children aren't cancelled when release() cancels the parent scope; change it to
create a Job that is a child of the existing scope's job (e.g., construct the
child Job with the parent Job from scope.coroutineContext or use
scope.coroutineContext.plus(Job(parentJob))) so the CoroutineScope returned by
createChildScope() inherits the parent's lifecycle and will be cancelled when
scope is cancelled; update createChildScope() to obtain the parent Job from
scope.coroutineContext and pass it as the parent to the new Job.
---
Nitpick comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioExecutionContext.kt`:
- Around line 43-46: Add a closed/released flag and guard lazy initialization:
introduce a private var released = false, set released = true at the start of
release(), and only cancel/close scope and dispatcher if they are already
initialized (avoid forcing lazy init — convert the lazy delegates to checkable
forms or use nullable/backing properties so you can test initialization before
accessing). Also update createChildScope() to check released and throw a clear
IllegalStateException (or return a no-op/closed scope) when released is true to
avoid post-release usage. Ensure references include release(), scope,
dispatcher, and createChildScope().
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioHandler.kt`:
- Line 29: The TODOs indicate technical debt: make the AudioHandler class
internal and change its public APIs to return Result<> instead of throwing/using
nullable returns, so update the AudioHandler declaration to internal and
refactor its public methods (e.g., start/stop/prepare functions in AudioHandler)
to return Result<T> wrapping success/error; additionally merge the
AudioSwitchHandler functionality into MicrophoneManager by moving/combining
methods and state from AudioSwitchHandler into MicrophoneManager and removing
the separate AudioSwitchHandler type, ensuring callers now use MicrophoneManager
for device switching and preserving existing behavior and error propagation via
Result<>.
- Around line 69-82: The methods start(), stop(), and selectDevice() currently
fire-and-forget with scope.launch which prevents callers (e.g.,
MicrophoneManager.cleanup()) from deterministically waiting for
controller.start()/controller.stop()/controller.selectDevice() to finish; change
these APIs to either return the Job from scope.launch (e.g., return scope.launch
{ controller.stop() }) or provide suspending variants (suspend fun stop() {
ensureScope()?.let { it.launch... } or directly call controller.stop() if
already suspending) so callers can await completion; keep ensureScope() and
controller.* calls intact and update call sites like MicrophoneManager.cleanup()
to await the returned Job or call the suspending variant.
- Around line 58-67: ensureScope() currently returns null when
StreamVideo.instanceOrNull() is missing or not a StreamVideoClient, causing
start()/stop()/selectDevice() to silently no-op; update ensureScope (in class
AudioHandler) to emit a clear warning log (e.g., Log.w or your existing logger)
right before returning null that states StreamVideo is unavailable or not a
StreamVideoClient and that audio operations will be skipped, and also log when
creating the child scope (when ctx.createChildScope() runs) so initialization
failures are visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c246058c-767a-4343-aa77-fb5f5a4c49c8
📒 Files selected for processing (5)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/MediaManager.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioExecutionContext.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioHandler.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/audio/AudioSwitchController.kt
|
|
🚀 Available in v1.22.0 |



Goal
We were making 4 IPC calls when toggling the speaker
This was causing 50-60 framedrops
Implementation
We have changed any internal implementation
main threadto a new threadaudio-switch-threadWe replaced
HandlerThreadwithCouroutinebacked bySingleThreadExecuter. Its lifecycle is cleaned up whenStreamClientis cleaned upWe separated AudioSwitch core logic from concurrency. So we moved the core logic in
AudioSwitchControllerand concurrency logic inAudioHandlerAudioExecutionContext - A new class to provide coroutine scope for
AudioSwitch🎨 UI Changes
None
Testing
Depends on PR: #1650, #1646