-
Notifications
You must be signed in to change notification settings - Fork 152
Fixed ScreenAudioCapturer crash, stale audio replay, and AudioRecord leak #982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4bb1d15
80760fc
048ae62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "client-sdk-android": patch | ||
| --- | ||
|
|
||
| Fix `ScreenAudioCapturer` crashing the audio thread when its `MediaProjection` is revoked (for example via the system "stop sharing" chip) before the first microphone buffer arrives. `initAudioRecord` now returns false when the `AudioRecord` cannot be created instead of throwing inside WebRTC's audio record thread, where an unhandled exception kills the process; only `startRecording()` was guarded before. `AudioRecord.read` failures are also handled now: the return value was ignored, so once the projection died the buffer's stale contents (the last captured frame) were mixed into the microphone track on every callback, an audible loop until the callback was detached. On a read error the capturer releases its `AudioRecord` and degrades to mic-only audio. | ||
|
|
||
| `releaseAudioResources` is also safe to call while `initAudioRecord` is still running. It runs on the app's thread while init runs on the audio record thread, and it used to observe a null `audioRecord` and do nothing, so the recorder that init went on to publish stayed running until finalization. Leaked recorders hold the playback capture input open, and later capture attempts fail once enough of them accumulate. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright 2024-2025 LiveKit, Inc. | ||
| * Copyright 2024-2026 LiveKit, Inc. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
|
|
@@ -111,9 +111,25 @@ constructor( | |
| */ | ||
| private val captureConfigurator: AudioPlaybackCaptureConfigurator = DEFAULT_CONFIGURATOR, | ||
| ) : MixerAudioBufferCallback() { | ||
| /** | ||
| * Guards publication and release of [audioRecord]. [initAudioRecord] runs on WebRTC's audio | ||
| * record thread, while [releaseAudioResources] is called by the app from a thread of its | ||
| * choosing. | ||
| */ | ||
| private val audioRecordLock = Any() | ||
|
|
||
| @Volatile | ||
| private var audioRecord: AudioRecord? = null | ||
|
|
||
| /** | ||
| * Terminal once set: releasing is what an app does when it is done capturing, and a capturer | ||
| * is tied to the single [MediaProjection] it was constructed with. | ||
| */ | ||
| private var isReleased = false | ||
|
|
||
| private var hasInitialized = false | ||
|
|
||
| @Volatile | ||
| private var byteBuffer: ByteBuffer? = null | ||
|
|
||
| /** | ||
|
|
@@ -131,7 +147,15 @@ constructor( | |
|
|
||
| val audioRecord = this.audioRecord ?: return null | ||
| val recordBuffer = this.byteBuffer ?: return null | ||
| audioRecord.read(recordBuffer, recordBuffer.capacity()) | ||
| val readResult = audioRecord.read(recordBuffer, recordBuffer.capacity()) | ||
| if (readResult != recordBuffer.capacity()) { | ||
| // On a short or failed read, the buffer contents are stale; skip mixing rather than replay them. | ||
| if (readResult < 0) { | ||
| LKLog.w { "AudioRecord.read failed: $readResult. Stopping screen share audio capture." } | ||
| releaseAudioResources() | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| if (abs(gain - DEFAULT_GAIN) > MIN_GAIN_CHANGE) { | ||
| recordBuffer.position(0) | ||
|
|
@@ -158,45 +182,65 @@ constructor( | |
| return BufferResponse(recordBuffer) | ||
| } | ||
|
|
||
| /** | ||
| * Initializes the [AudioRecord] used to capture the playback audio. | ||
| * | ||
| * This is handled automatically when used as an audio buffer callback, | ||
| * and does not need to be called manually. | ||
| * | ||
| * @return true if the audio record was successfully created and started. Returns false | ||
| * if audio capture is unavailable (for example, if the media projection has been stopped, | ||
| * or the capturer has been released), in which case no audio will be mixed. | ||
| */ | ||
| @SuppressLint("MissingPermission") | ||
| fun initAudioRecord(audioFormat: Int, channelCount: Int, sampleRate: Int): Boolean { | ||
| val audioCaptureConfig = AudioPlaybackCaptureConfiguration.Builder(mediaProjection) | ||
| .apply(captureConfigurator) | ||
| .build() | ||
| val channelMask = if (channelCount == 1) AudioFormat.CHANNEL_IN_MONO else AudioFormat.CHANNEL_IN_STEREO | ||
|
|
||
| val minBufferSize = AudioRecord.getMinBufferSize(sampleRate, channelMask, audioFormat) | ||
| if (minBufferSize == AudioRecord.ERROR || minBufferSize == AudioRecord.ERROR_BAD_VALUE) { | ||
| throw IllegalStateException("minBuffer size error: $minBufferSize") | ||
| if (channelCount != 1 && channelCount != 2) { | ||
| LKLog.e { "Unsupported channel count: $channelCount" } | ||
| return false | ||
| } | ||
| LKLog.v { "AudioRecord.getMinBufferSize: $minBufferSize" } | ||
| val audioRecord = try { | ||
| val audioCaptureConfig = AudioPlaybackCaptureConfiguration.Builder(mediaProjection) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think AudioPlaybackCaptureConfiguration requires secific Android version, any idea if that is higher than our SDK's minimal supported version ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Playback capture requires Android 10: both |
||
| .apply(captureConfigurator) | ||
| .build() | ||
| val channelMask = if (channelCount == 1) AudioFormat.CHANNEL_IN_MONO else AudioFormat.CHANNEL_IN_STEREO | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, can you add an assert in this function that channelCount is either 1 or 2 ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a check, though as an explicit guard rather than an assert: assertions are normally disabled on Android, and when enabled the AssertionError would escape the existing |
||
|
|
||
| val minBufferSize = AudioRecord.getMinBufferSize(sampleRate, channelMask, audioFormat) | ||
| if (minBufferSize <= 0) { | ||
| LKLog.e { "AudioRecord.getMinBufferSize error: $minBufferSize" } | ||
| return false | ||
| } | ||
| LKLog.v { "AudioRecord.getMinBufferSize: $minBufferSize" } | ||
|
|
||
| val bytesPerFrame = channelCount * getBytesPerSample(audioFormat) | ||
| val framesPerBuffer = sampleRate / 100 | ||
| val readBufferCapacity = bytesPerFrame * framesPerBuffer | ||
| val byteBuffer = ByteBuffer.allocateDirect(readBufferCapacity) | ||
| .order(ByteOrder.nativeOrder()) | ||
| val bytesPerFrame = channelCount * getBytesPerSample(audioFormat) | ||
| val framesPerBuffer = sampleRate / 100 | ||
| val readBufferCapacity = bytesPerFrame * framesPerBuffer | ||
| val byteBuffer = ByteBuffer.allocateDirect(readBufferCapacity) | ||
| .order(ByteOrder.nativeOrder()) | ||
|
|
||
| if (!byteBuffer.hasArray()) { | ||
| LKLog.e { "ByteBuffer does not have backing array." } | ||
| if (!byteBuffer.hasArray()) { | ||
| LKLog.e { "ByteBuffer does not have backing array." } | ||
| return false | ||
| } | ||
|
|
||
| this.byteBuffer = byteBuffer | ||
| val bufferSizeInBytes: Int = max(BUFFER_SIZE_FACTOR * minBufferSize, readBufferCapacity) | ||
|
|
||
| AudioRecord.Builder() | ||
| .setAudioFormat( | ||
| AudioFormat.Builder() | ||
| .setEncoding(audioFormat) | ||
| .setSampleRate(sampleRate) | ||
| .setChannelMask(channelMask) | ||
| .build(), | ||
| ) | ||
| .setBufferSizeInBytes(bufferSizeInBytes) | ||
| .setAudioPlaybackCaptureConfig(audioCaptureConfig) | ||
| .build() | ||
| } catch (e: Exception) { | ||
| LKLog.e(e) { "Failed to create AudioRecord for screen share audio capture:" } | ||
| return false | ||
| } | ||
|
|
||
| this.byteBuffer = byteBuffer | ||
| val bufferSizeInBytes: Int = max(BUFFER_SIZE_FACTOR * minBufferSize, readBufferCapacity) | ||
|
|
||
| val audioRecord = AudioRecord.Builder() | ||
| .setAudioFormat( | ||
| AudioFormat.Builder() | ||
| .setEncoding(audioFormat) | ||
| .setSampleRate(sampleRate) | ||
| .setChannelMask(channelMask) | ||
| .build(), | ||
| ) | ||
| .setBufferSizeInBytes(bufferSizeInBytes) | ||
| .setAudioPlaybackCaptureConfig(audioCaptureConfig) | ||
| .build() | ||
|
|
||
| try { | ||
| audioRecord.startRecording() | ||
| } catch (e: Exception) { | ||
|
|
@@ -208,10 +252,18 @@ constructor( | |
| LKLog.e { | ||
| "AudioRecord.startRecording failed - incorrect state: ${audioRecord.recordingState}" | ||
| } | ||
| audioRecord.release() | ||
| return false | ||
| } | ||
|
|
||
| this.audioRecord = audioRecord | ||
| synchronized(audioRecordLock) { | ||
| if (isReleased) { | ||
| LKLog.w { "Screen share audio capturer was released while starting up. Discarding the AudioRecord." } | ||
| audioRecord.release() | ||
| return false | ||
| } | ||
| this.audioRecord = audioRecord | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
@@ -220,12 +272,15 @@ constructor( | |
| * Release any audio resources associated with this capturer. | ||
| * This is not managed by LiveKit, so you must call this function | ||
| * when finished to prevent memory leaks. | ||
| * | ||
| * Safe to call from any thread, and at any point relative to [initAudioRecord]. Once released, | ||
| * the capturer stays released and no further audio is mixed in. | ||
| */ | ||
| fun releaseAudioResources() { | ||
| val audioRecord = this.audioRecord | ||
| if (audioRecord != null) { | ||
| audioRecord.release() | ||
| this.audioRecord = null | ||
| synchronized(audioRecordLock) { | ||
| isReleased = true | ||
| audioRecord?.release() | ||
| audioRecord = null | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rather than setting isReleased to true here, should we allow the app to auto recover from some read failures? Like if (!isReleased) { hasInitialized = false } So that the onBufferRequest() will recreate the buffer again.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think resetting |
||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this code, it seems that one negative read will permanently disable screen share audio until the apps restart the screen audio capturer ?
What negative errors audioRecord.read() can give out ? should some of the errors be recoverable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct: any negative read makes this
ScreenAudioCapturerinstance terminal, so resuming screen audio requires installing a new capturer.read(ByteBuffer, int)documents ERROR_INVALID_OPERATION (-3), ERROR_BAD_VALUE (-2), ERROR_DEAD_OBJECT (-6), and ERROR (-1). ERROR_DEAD_OBJECT is the only one that explicitly calls for recreating the AudioRecord. However, the projection-stop case this PR fixes returned ERROR_BAD_VALUE on Android 17 after the platform's own record restore failed, so the error code does not reliably tell us whether the projection is still usable. For projection loss, falling back permanently to mic-only audio is intentional: a new share requires a new MediaProjection and therefore a new capturer.