From d2d089fc86c62843992e7d5b371ee9227189a1e6 Mon Sep 17 00:00:00 2001 From: Glenn Kasten Date: Wed, 5 Nov 2014 11:48:12 -0800 Subject: Improve AudioTrack recovery from mediaserver death 1. Fix race condition in handling of binder death notifications. AudioSystem has a mixture of APIs for both ordinary app clients, and the AudioFlinger and AudioPolicy services within mediaserver. Due to this mix of uses, it is possible for there to be "surprising" sequences of calls on the call stack. Previously, we used a single mutex for all global variables, but this caused a deadlock. To avoid the deadlock, we unlocked the mutex during the critical sequence of calls. But this was a a crucial place where it should have stayed locked; see Change-Id I315c1c5066f62b05e1c13b04fae1272b5fbce977 Now we use separate mutexes for the AudioFlinger, AudioPolicy, and audio port related global variables. This allows us to correctly hold each mutex throughout the atomic region, even when AudioFlinger calls AudioPolicy via AudioSystem, or vice-versa. 2. AudioSystem::clearAudioConfigCache now clears the IAudioFlinger reference. 3. Make AudioSystem::get_audio_policy_service more like get_audio_flinger. Bug: 18242291 Change-Id: I9761443d8337df5bf66d4ca2316a9fd0bd11be94 --- include/media/AudioSystem.h | 5 ++++- media/libmedia/AudioSystem.cpp | 48 ++++++++++++++++++++++++++---------------- media/libmedia/AudioTrack.cpp | 2 +- 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/include/media/AudioSystem.h b/include/media/AudioSystem.h index f8c0198..341f9ab 100644 --- a/include/media/AudioSystem.h +++ b/include/media/AudioSystem.h @@ -377,7 +377,10 @@ private: friend class AudioFlingerClient; friend class AudioPolicyServiceClient; - static Mutex gLock; + static Mutex gLock; // protects all members except gAudioPolicyService, + // gAudioPolicyServiceClient, and gAudioPortCallback + static Mutex gLockAPS; // protects gAudioPolicyService and gAudioPolicyServiceClient + static Mutex gLockAPC; // protects gAudioPortCallback static sp gAudioFlinger; static audio_error_callback gAudioErrorCallback; diff --git a/media/libmedia/AudioSystem.cpp b/media/libmedia/AudioSystem.cpp index dda3657..724bc68 100644 --- a/media/libmedia/AudioSystem.cpp +++ b/media/libmedia/AudioSystem.cpp @@ -32,6 +32,8 @@ namespace android { // client singleton for AudioFlinger binder interface Mutex AudioSystem::gLock; +Mutex AudioSystem::gLockAPS; +Mutex AudioSystem::gLockAPC; sp AudioSystem::gAudioFlinger; sp AudioSystem::gAudioFlingerClient; audio_error_callback AudioSystem::gAudioErrorCallback = NULL; @@ -70,9 +72,9 @@ const sp& AudioSystem::get_audio_flinger() } binder->linkToDeath(gAudioFlingerClient); gAudioFlinger = interface_cast(binder); + LOG_ALWAYS_FATAL_IF(gAudioFlinger == 0); gAudioFlinger->registerClient(gAudioFlingerClient); } - ALOGE_IF(gAudioFlinger==0, "no AudioFlinger!?"); return gAudioFlinger; } @@ -559,6 +561,7 @@ bool AudioSystem::routedToA2dpOutput(audio_stream_type_t streamType) // client singleton for AudioPolicyService binder interface +// protected by gLockAPS sp AudioSystem::gAudioPolicyService; sp AudioSystem::gAudioPolicyServiceClient; @@ -566,7 +569,7 @@ sp AudioSystem::gAudioPolicyServiceClient // establish binder interface to AudioPolicy service const sp& AudioSystem::get_audio_policy_service() { - gLock.lock(); + Mutex::Autolock _l(gLockAPS); if (gAudioPolicyService == 0) { sp sm = defaultServiceManager(); sp binder; @@ -582,15 +585,10 @@ const sp& AudioSystem::get_audio_policy_service() } binder->linkToDeath(gAudioPolicyServiceClient); gAudioPolicyService = interface_cast(binder); - gLock.unlock(); - // Registering the client takes the AudioPolicyService lock. - // Don't hold the AudioSystem lock at the same time. + LOG_ALWAYS_FATAL_IF(gAudioPolicyService == 0); gAudioPolicyService->registerClient(gAudioPolicyServiceClient); - } else { - // There exists a benign race condition where gAudioPolicyService - // is set, but gAudioPolicyServiceClient is not yet registered. - gLock.unlock(); } + return gAudioPolicyService; } @@ -856,9 +854,18 @@ status_t AudioSystem::setLowRamDevice(bool isLowRamDevice) void AudioSystem::clearAudioConfigCache() { - Mutex::Autolock _l(gLock); + // called by restoreTrack_l(), which needs new IAudioFlinger and IAudioPolicyService instances ALOGV("clearAudioConfigCache()"); - gOutputs.clear(); + { + Mutex::Autolock _l(gLock); + gOutputs.clear(); + gAudioFlinger.clear(); + } + { + Mutex::Autolock _l(gLockAPS); + gAudioPolicyService.clear(); + } + // Do not clear gAudioPortCallback } bool AudioSystem::isOffloadSupported(const audio_offload_info_t& info) @@ -920,7 +927,7 @@ status_t AudioSystem::setAudioPortConfig(const struct audio_port_config *config) void AudioSystem::setAudioPortCallback(sp callBack) { - Mutex::Autolock _l(gLock); + Mutex::Autolock _l(gLockAPC); gAudioPortCallback = callBack; } @@ -952,18 +959,23 @@ audio_mode_t AudioSystem::getPhoneState() void AudioSystem::AudioPolicyServiceClient::binderDied(const wp& who __unused) { - Mutex::Autolock _l(gLock); - if (gAudioPortCallback != 0) { - gAudioPortCallback->onServiceDied(); + { + Mutex::Autolock _l(gLockAPC); + if (gAudioPortCallback != 0) { + gAudioPortCallback->onServiceDied(); + } + } + { + Mutex::Autolock _l(gLockAPS); + AudioSystem::gAudioPolicyService.clear(); } - AudioSystem::gAudioPolicyService.clear(); ALOGW("AudioPolicyService server died!"); } void AudioSystem::AudioPolicyServiceClient::onAudioPortListUpdate() { - Mutex::Autolock _l(gLock); + Mutex::Autolock _l(gLockAPC); if (gAudioPortCallback != 0) { gAudioPortCallback->onAudioPortListUpdate(); } @@ -971,7 +983,7 @@ void AudioSystem::AudioPolicyServiceClient::onAudioPortListUpdate() void AudioSystem::AudioPolicyServiceClient::onAudioPatchListUpdate() { - Mutex::Autolock _l(gLock); + Mutex::Autolock _l(gLockAPC); if (gAudioPortCallback != 0) { gAudioPortCallback->onAudioPatchListUpdate(); } diff --git a/media/libmedia/AudioTrack.cpp b/media/libmedia/AudioTrack.cpp index 082a5e1..cd493f6 100644 --- a/media/libmedia/AudioTrack.cpp +++ b/media/libmedia/AudioTrack.cpp @@ -1828,7 +1828,7 @@ status_t AudioTrack::restoreTrack_l(const char *from) status_t result; // refresh the audio configuration cache in this process to make sure we get new - // output parameters in createTrack_l() + // output parameters and new IAudioFlinger in createTrack_l() AudioSystem::clearAudioConfigCache(); if (isOffloadedOrDirect_l()) { -- cgit v1.1