Skip to content
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

Avoid initializing AudioEngine instance when audio is absent in AudioEvent #431

Closed
wants to merge 1 commit into from

Conversation

andeart
Copy link
Contributor

@andeart andeart commented Nov 8, 2024

Issue:
There is a frequent (but inconsistent) crash on Android devices with Android 10 (aka API 29) and below, caused when miniaudio disposes of its AudioEngine and attempts to free its audio buffers. This specific crash is really a race condition issue with miniaudio on Android 10-, but Rive indirectly triggers this through StateMachineController.dispose(). This results in a full crash, i.e. app quits completely and unexpectedly.

More context on how this crash is triggered from Rive code...

When an AudioEvent is received in StateMachineController.applyEvents(), an instance of Rive's AudioEngine is created through the audioPlayer getter, which consequently also initializes a miniaudio player. Note that this getter is currently invoked and the AudioEngine is instantiated even if the AudioEvent lacks an audio asset.

While this is harmless on its own, in such cases where the AudioEngine is created, calling StateMachineController.dispose() later also attempts to dispose the miniaudio player, resulting in the Android crash referenced above.

The crash stack trace looks something like this...
E/libc++abi: Pure virtual function called!
F/libc: Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 24289 (AudioTrack), pid 24246 (ter_audio_crash)
*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
backtrace:
#00  pc 0x00000000000705ac  /apex/com.android.runtime/lib64/bionic/libc.so (abort+160)
#01  pc 0x00000000000500fc  /system/lib64/libc++.so (abort_message+232)
#02  pc 0x0000000000065bf4  /system/lib64/libc++.so (__cxa_pure_virtual+16)
#03  pc 0x00000000000257ec  /system/lib64/libaaudio.so (aaudio::AudioStreamLegacy::processCallbackCommon(int, void*)+1112)
#04  pc 0x000000000006c3a8  /system/lib64/libaudioclient.so (android::AudioTrack::processAudioBuffer()+2628)
#05  pc 0x000000000006b65c  /system/lib64/libaudioclient.so (android::AudioTrack::AudioTrackThread::threadLoop()+264)
#06  pc 0x00000000000136c4  /system/lib64/libutils.so (android::Thread::_threadLoop(void*)+316)
#07  pc 0x00000000000e5e4c  /system/lib64/libandroid_runtime.so (android::AndroidRuntime::javaThreadShell(void*)+140)
#08  pc 0x00000000000cf700  /apex/com.android.runtime/lib64/bionic/libc.so (__pthread_start(void*)+36)
#09  pc 0x00000000000720e8  /apex/com.android.runtime/lib64/bionic/libc.so (__start_thread+64)

On a related note, this condition also affects SoLoud, another Flutter package using miniaudio. SoLoud addresses it by switching to OpenSL for Android 10 and below.


Solution:
The perfect solution would be to fix the deref race conditions in miniaudio for Android 10.

But as a preventative measure in Rive, we can also avoid the crash circumstances for Rive users who don't use audio, by bypassing the miniaudio engine creation/disposal (through StateMachineController.audioPlayer) when audio is absent from an AudioEvent.

This change also provides a minor performance benefit by avoiding allocations through AudioEngine.init() if the file happens to send an empty AudioEvent.


Additional context:
I ran the package tests in my local clone, but they fail. As I understand, this is to be expected for now because of the custom dylib depdendencies. I tried to use the library generation script from this thread but unfortunately couldn't fix the tests. I'm happy to try any alternative suggestions to help with checks.

Let me know if I missed anything please. Thanks for creating this useful runtime!

@HayesGordon
Copy link
Contributor

Hi @andeart, Thanks for the contribution. I co-authored this PR with you because we use a private mono repository and push the updates downstream to this repo.

Closing this PR, Your commit is already included and we published 0.13.16 with the fix.

rive-engineering pushed a commit that referenced this pull request Nov 13, 2024
Community PR from AOFL: #431

I'm co-authoring to get it in

Slack discussion: https://2dimensions.slack.com/archives/C073BQNPQ5T/p1731028216451459

We will need to look into the actual fix. I'm not that familiar with our Audio system, and not sure if there are edge cases to consider with this change.

Diffs=
aea593c2df Avoid initializing an AudioEngine instance if there is no audio present (#8541)
3a638e61b1 Work around Galaxy S22 compiler bugs (#8549)
01bc166a8e invert order of advance between parent and child (#8547)
26d8b495b6 Prevent NestedArtboard advance when not playing (#8542)
ded183b39c Rhi typeless uav support (#8507)
6804948c90 new arithmetic data converter that uses a viewmodel as input (#8536)
532ca5dfb3 Unreal build use build rive.sh (#8502)
692dc6f659 default data converters runtime (#8503)
9aa4503c18 LayoutComponent updates properly when scaleType changes (#8535)
46b0899967 fix data enum importer (#8531)
4bbd7d495c Fix marking nested artboard layout dirty (#8534)
fa4f11f061 add feather property (#8533)
97050e4fa7 Pass scaleType down to sizeable children (#8524)
071b19ba62 don't expose yoga includes (#8526)
ad1ca22bd5 Nnnn render update fixes (#8527)
8826406ff1 editor and runtime: fix vector n-slicer hit area (#8512)
98e6822f02 Rename "atlas" -> "coverage" in the clockwise shader (#8518)
01066ff6a0 Fix bidi (#8517)
8296d87711 conditionnally  add dirt and advance (#8516)
8738368345 Updates to AdvanceFlags (#8514)
17cbda4b69 Add AdvanceFlags (#8492)
6cb69b688b Fix for layout shape paint bug (#8498)
33dc66a73e editor and runtime: allow vertices in a vector nslicer to be out of bounds (#8495)
cba259e6cd add data bind support for vertex properties (#8494)
f4c87ee495 Nnnn merge fills and strokes (#8465)
af9e217d23 Use renderImage size to scale images in layouts (#8487)
ce9e44c6b3 editor: NSlicer should not clamp children (#8459)
446682cda9 web: decode image on demand, not render (#8484)

Co-authored-by: Anurag Devanapally <[email protected]>
Co-authored-by: Gordon <[email protected]>
@andeart
Copy link
Contributor Author

andeart commented Nov 15, 2024

Thanks for the assist with this, @HayesGordon !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants