Fixed ScreenAudioCapturer crash, stale audio replay, and AudioRecord leak - #982
Conversation
…fter MediaProjection stops
🦋 Changeset detectedLatest commit: 048ae62 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
initAudioRecord runs on WebRTC's audio record thread, while releaseAudioResources is called by the app from a thread of its choosing. A release landing between AudioRecord creation and its assignment to the field saw a null audioRecord, did nothing, and left init to publish a recording AudioRecord that nothing owned. Publication and release now share a lock, and a released capturer stays released, so an init that finishes after a release discards its AudioRecord instead of stranding it. Leaked recorders hold the playback capture input open: with 100 racing release/init pairs on an Android 17 emulator, 9 recorders were stranded and the remaining 91 creation attempts failed with "could not open input for device AUDIO_DEVICE_IN_REMOTE_SUBMIX". After the fix all 100 discard cleanly and no creation fails.
adrian-niculescu
changed the title
| val audioCaptureConfig = AudioPlaybackCaptureConfiguration.Builder(mediaProjection) | ||
| .apply(captureConfigurator) | ||
| .build() | ||
| val channelMask = if (channelCount == 1) AudioFormat.CHANNEL_IN_MONO else AudioFormat.CHANNEL_IN_STEREO |
There was a problem hiding this comment.
nit, can you add an assert in this function that channelCount is either 1 or 2 ?
There was a problem hiding this comment.
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 catch (Exception) and take down the process from WebRTC's audio record thread - the same failure mode this PR removes. An invalid channel count now logs and returns false, same as the other degrade paths.
| 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) { |
There was a problem hiding this comment.
nit, will be be slightly less error prone to do
if (minBufferSize <= 0) {
LKLog.e { "AudioRecord.getMinBufferSize error: $minBufferSize" }
return false
}
There was a problem hiding this comment.
Indeed, changed to <= 0, since a successful minimum buffer size is always positive.
| } | ||
| LKLog.v { "AudioRecord.getMinBufferSize: $minBufferSize" } | ||
| val audioRecord = try { | ||
| val audioCaptureConfig = AudioPlaybackCaptureConfiguration.Builder(mediaProjection) |
There was a problem hiding this comment.
I think AudioPlaybackCaptureConfiguration requires secific Android version, any idea if that is higher than our SDK's minimal supported version ?
There was a problem hiding this comment.
Yes. Playback capture requires Android 10: both AudioPlaybackCaptureConfiguration and AudioRecord.Builder.setAudioPlaybackCaptureConfig were added in API 29, while the SDK minimum is 21. ScreenAudioCapturer is already annotated @RequiresApi(Build.VERSION_CODES.Q), so lint requires callers whose minimum is below 29 to guard its use. The SDK does not invoke this feature automatically; consumers explicitly call createFromScreenShareTrack. Since Android has no public playback-capture API before API 29, @RequiresApi is the appropriate contract. The annotation predates this PR.
| synchronized(audioRecordLock) { | ||
| isReleased = true | ||
| audioRecord?.release() | ||
| audioRecord = null |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't think resetting hasInitialized on every negative read is a safe recovery policy. It retries immediately on the next callback, including after unrecoverable projection loss, and if recreation succeeds but reads keep failing it turns into repeated create/destroy churn. releaseAudioResources() is also the app's explicit terminal teardown path, and without the terminal flag a release racing initAudioRecord can strand a recording AudioRecord again, which this PR fixes. Reliable recovery would need a separate internal teardown path plus bounded retries with backoff. I would keep the deterministic mic-only fallback here and treat audioserver-crash recovery separately if we get a reproducible case for it.
| // 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() |
There was a problem hiding this comment.
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.
Correct: any negative read makes this ScreenAudioCapturer instance 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.
davidliu
left a comment
davidliu
left a comment
There was a problem hiding this comment.
LGTM, thanks for the PR!
|
Thank you for the swift review! |
3 participants
When the MediaProjection backing a ScreenAudioCapturer stops (the user ends the share from the status bar chip, or another app takes over the projection), two things go wrong:
initAudioRecordthrows on WebRTC's audio record thread:AudioRecord.Builder.build()fails withUnsupportedOperationException, and the playback capture config builder can throwIllegalStateException. OnlystartRecording()was guarded, and the audio record thread has no handler around the buffer callback, so the process dies.AudioRecord.read()starts failing, but the return value was ignored and the buffer still holds the previous frame. That frame kept getting mixed into the mic track on every callback, so listeners heard the last captured 10 ms looping until the callback was detached.initAudioRecordnow returns false on any creation failure, matching its existing degrade paths, and releases the record if it never reaches the recording state.onBufferRequestonly mixes fully read buffers; on a read error it logs, releases the AudioRecord, and the track degrades to mic-only audio. Read errors are not alwaysERROR_DEAD_OBJECT(Android 17 returnsERROR_BAD_VALUEafter the record restore fails), so any negative result is treated as terminal.The second commit fixes a related problem in the teardown path rather than in projection loss.
releaseAudioResources()is called by the app from a thread of its choosing, whileinitAudioRecordruns on the audio record thread. A release landing betweenAudioRecord.Builder.build()and the assignment to the field saw a nullaudioRecord, did nothing, and left init to publish a recordingAudioRecordthat nothing owned. Publication and release now share a lock, and a released capturer stays released, so an init finishing after a release discards its record instead of stranding it.Verified with the screenshare-audio example on an Android 17 Pixel 6a and and an emulator: stopping the share from the status bar chip while the capturer is attached logs a single
AudioRecord.read failed: -2. Stopping screen share audio capture.and the session continues on mic audio with no crash. For the teardown race, driving 100 racing release/init pairs against a real MediaProjection stranded 9 recorders before the fix, and the remaining 91 creation attempts then failed withcould not open input for device AUDIO_DEVICE_IN_REMOTE_SUBMIX, so the leak also breaks later capture attempts. After the fix all 100 discard cleanly and none fail../gradlew test, spotless, and detekt pass.