Repository navigation
fix: resolve Android stop() hang (self-join deadlock + leaked input buffers) - #508
iawaisrana wants to merge 7 commits into
Conversation
stopEncoder() ran on its own handlerThread and called handlerThread.join() on itself, deadlocking forever and preventing the completion callback from firing. RecorderController.stop() would then hang indefinitely on Android whenever the AAC/CommonEncoder path was used. Removes the self-join, invokes the completion callback before quitting the handler thread instead of after, posts MediaCodec callbacks and the method channel result onto explicit handlers for thread-safety, and adds @volatile to fields read across threads. Cherry-picked from upstream PR SimformSolutionsPvtLtd#486.
signalToStop() relies on a future onInputBufferAvailable callback to queue EOS when no input buffer is available at the moment stop() is called. But recording has already stopped feeding new audio by then, so that callback may never arrive, leaving stopEncoder() (and the completion callback / Dart-side stop() future) hanging indefinitely. Reproduced consistently on a second back-to-back recording. Add a 500ms grace-period fallback: if EOS hasn't been queued naturally by then, force stopEncoder() directly. It's idempotent and always invokes the completion callback, so the Dart Future can never hang.
Different devices/Codec2 implementations may need more or less time before the natural EOS path can be considered stalled. Hardcoding 500ms risked either forcing a stop too early on slower devices (truncating the recording's tail) or not being adjustable for apps that hit this more severely. Adds AndroidEncoderSettings.stopTimeoutMs (default 500, matching the previous hardcoded value), threaded through to RecorderSettings on both the Dart and native side, down to CommonEncoder.signalToStop()'s postDelayed fallback.
onInputBufferAvailable stored the available index in a single field, so whenever the codec offered more than one buffer before audio was ready (e.g. the burst right after start()), each callback overwrote the previous index. Overwritten buffers were never queued back and never returned to the codec's pool. A race between the recording and encoder threads could drop the last circulating one, after which no further onInputBufferAvailable arrived: encoding silently stalled mid-recording and EOS could never be queued, so stop() hung (or, with the grace-period fallback, completed with a truncated file). - Keep every available index in a queue and pair it with queued audio under one lock; queue EOS from the same path, exactly once, once the backlog has drained. - Join the recording thread in stop() before signalling EOS so no audio is fed after it. Drop the completion lambda that joined the still-live recording thread from the encoder thread (a hang if the codec errored mid-recording). - Invoke the stop() callback immediately if the encoder already stopped on its own, and at most once. - Create a fresh CommonEncoder per recording and drop the post-stop state reset, which could race with the next recording's initCodec(). - Always arm the stopTimeoutMs fallback; the natural path cancels it. Root cause analysis by @victorrss in SimformSolutionsPvtLtd#470.
iawaisrana
changed the title
62436c7 to
8eb87bb
Compare
|
Independent confirmation from a Flutter app in development (Flutter 3.47.2, On 2.0.2, our Patrol integration test, which records a ~2–4 s take and then calls We traced it independently to the held-input-buffer case described here. With this PR applied unmodified on top of 2.0.2 (only the |
yash-fadadu-simform
left a comment
yash-fadadu-simform
left a comment
There was a problem hiding this comment.
Thanks for this, and for tracking down the leaked input buffers. It fixes the root cause of #470: every input buffer index is now kept, EOS is queued from the same locked path as soon as the backlog drains, and the self-join is gone. It also addresses all four review points from #486 (purging handler messages before release, @Volatile fields, a single mainHandler, and EOS no longer depending only on a future callback). I've left a few small comments, none blocking.
I tested it on two Pixel 7 API 36 emulators side by side, with AAC at 44.1 kHz / 48 kbps (the issue's settings), timing each stop() with an 8s timeout and checking that the .m4a has its moov atom.
| Case | main | This PR |
|---|---|---|
| record 3s → stop (×10) | ❌ 7/10 hung (first hang at #4, every stop after it hung too) | ✅ 10/10, 49–463 ms |
| record 3s → pause → stop (×4) | ❌ 4/4 hung | ✅ 4/4, 49–142 ms |
| record 35s → stop (×2) | ❌ 2/2 hung | ✅ 2/2, 131 / 985 ms |
| Grace-period fallback fired | n/a | ✅ never (no forcing encoder stop in logcat) |
One PR stop took 5090 ms, but logcat shows that was MPEG4Writer: final fsync() takes 4949 ms inside MediaMuxer.stop() on the shared emulator disk, not the encoder.
Outside the diff:
doc/documentation.md(theAndroidEncoderSettingssection) doesn't mentionstopTimeoutMs.CommonEncoder.initCodec'sonEncodingCompletedparameter has no callers now and could be removed.- The description doesn't say
Fixes #470, so the issue won't close on merge. #486 can be closed as superseded.
This was an emulator run, not a physical device, so OEM codecs (e.g. Samsung) weren't covered. I did not test the AMR encoders, did not get the fallback timer to fire, and did not reproduce the release ordering race in my comment below.
- Release the recorder before posting the stop result, so a record() issued right after stop() returns can't have its new AudioRecord released by the previous session's completion callback. - Read androidStopTimeoutMs as Number so values above Int range don't throw ClassCastException. - Reference RecorderController.stop in the stopTimeoutMs dartdoc instead of a non-existent stopRecording(). - Assert stopTimeoutMs is positive; a zero or negative value would force the encoder to stop before EOS and truncate every recording.
|
@yash-fadadu-simform thanks for the thorough review and the emulator testing. All four inline suggestions are incorporated in ccba431:
Outside the diff:
|
What
Fixes #470.
Fixes the Android
stop()hang. Supersedes #486, whose self-join deadlock fix is included here. Three causes, all inCommonEncoder:stopEncoder()calledhandlerThread.join()from a callback running on that same thread.onInputBufferAvailablestored the available index in a single field. Whenever the codec offered more than one buffer before audio was ready (e.g. the burst right afterstart()), each callback overwrote the previous index. Overwritten buffers were never queued back, so they never returned to the codec's pool. Once a thread race dropped the last circulating buffer, no furtheronInputBufferAvailablearrived: encoding silently stalled mid-recording, EOS could never be queued, andstop()waited forever. This is why longer recordings reproduced it far more often.stop()depended entirely on the codec delivering the EOS output buffer.How
stop()joins the recording thread before signalling EOS, so no audio is fed to the codec after EOS.onErrormid-recording),stop()'s completion callback runs immediately instead of never, and it runs at most once. This also removes a completion lambda that joined the still-running recording thread from the encoder thread.CommonEncoderis created per recording, and the post-stop state reset is removed. That reset could race with the next recording'sinitCodec(), which is what made back-to-back recordings hang.AndroidEncoderSettings.stopTimeoutMs(default 500ms),stopEncoder()is forced. With the buffer accounting fixed, this should practically never fire. If it does, it logsEncoder did not finish within the grace period; forcing encoder stop.Why configurable
How long the natural path takes may vary by device/OEM Codec2 implementation, so the grace period is exposed as
AndroidEncoderSettings.stopTimeoutMs(default 500ms) rather than hardcoded.Testing
Verified on a physical Android device in a production app (AAC /
.m4a):stop()returned promptly in every case, the files played through to the end, and the grace-period fallback did not fire.I didn't add automated tests. The repo has no existing test infrastructure for the native Android side, and the bug is a device-dependent async timing race that isn't practical to reproduce deterministically in a unit test. Happy to add coverage if there's a preferred approach.