From 4f0f17d2f10ceb22c2e23b593bab434fc899ecb7 Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Wed, 12 May 2010 02:05:53 -0700 Subject: [PATCH] Fix issue 2678048: binder death detection in AudioFlinger is broken. There is a bug in the way notification client list is managed when the client binder interface dies that makes that the dead client is not removed from the list: the week reference passed by binderDied() cannot be promoted and compared to the strong references in the list. The fix consists in creating a new NotificationClient class that implements the binder DeathRecipient and holds a strong reference to the IAudioFlingerClient interface. A new instance of this class is created for each cient and a strong reference to this object is added to the notification client list maintained by AudioFlinger. When binderDied() is called on this object, it is removed from the list preventing AudioFlinger to notify this client for further io changes. Also added code to disable LifeVibes effects when the client that has enabled the enhancements dies. Change-Id: Icedc4af171759e9ae9a575d82d44784b4e8267e8 --- libs/audioflinger/AudioFlinger.cpp | 98 ++++++++++++++++++++---------- libs/audioflinger/AudioFlinger.h | 32 ++++++++-- 2 files changed, 92 insertions(+), 38 deletions(-) diff --git a/libs/audioflinger/AudioFlinger.cpp b/libs/audioflinger/AudioFlinger.cpp index 2414e8dc5e039..06443ef6712a1 100644 --- a/libs/audioflinger/AudioFlinger.cpp +++ b/libs/audioflinger/AudioFlinger.cpp @@ -142,6 +142,7 @@ AudioFlinger::AudioFlinger() } #ifdef LVMX LifeVibes::init(); + mLifeVibesClientPid = -1; #endif } @@ -596,8 +597,10 @@ status_t AudioFlinger::setParameters(int ioHandle, const String8& keyValuePairs) int musicEnabled = -1; if (NO_ERROR == param.get(key, value)) { if (value == LifevibesEnable) { + mLifeVibesClientPid = IPCThreadState::self()->getCallingPid(); musicEnabled = 1; } else if (value == LifevibesDisable) { + mLifeVibesClientPid = -1; musicEnabled = 0; } } @@ -609,7 +612,7 @@ status_t AudioFlinger::setParameters(int ioHandle, const String8& keyValuePairs) mHardwareStatus = AUDIO_SET_PARAMETER; result = mAudioHardware->setParameters(keyValuePairs); #ifdef LVMX - if ((NO_ERROR == result) && (musicEnabled != -1)) { + if (musicEnabled != -1) { LifeVibes::enableMusic((bool) musicEnabled); } #endif @@ -713,51 +716,57 @@ status_t AudioFlinger::getRenderPosition(uint32_t *halFrames, uint32_t *dspFrame void AudioFlinger::registerClient(const sp& client) { - LOGV("registerClient() %p, tid %d, calling tid %d", client.get(), gettid(), IPCThreadState::self()->getCallingPid()); Mutex::Autolock _l(mLock); - sp binder = client->asBinder(); - if (mNotificationClients.indexOf(binder) < 0) { - LOGV("Adding notification client %p", binder.get()); - binder->linkToDeath(this); - mNotificationClients.add(binder); - } + int pid = IPCThreadState::self()->getCallingPid(); + if (mNotificationClients.indexOfKey(pid) < 0) { + sp notificationClient = new NotificationClient(this, + client, + pid); + LOGV("registerClient() client %p, pid %d", notificationClient.get(), pid); - // the config change is always sent from playback or record threads to avoid deadlock - // with AudioSystem::gLock - for (size_t i = 0; i < mPlaybackThreads.size(); i++) { - mPlaybackThreads.valueAt(i)->sendConfigEvent(AudioSystem::OUTPUT_OPENED); - } + mNotificationClients.add(pid, notificationClient); - for (size_t i = 0; i < mRecordThreads.size(); i++) { - mRecordThreads.valueAt(i)->sendConfigEvent(AudioSystem::INPUT_OPENED); - } -} + sp binder = client->asBinder(); + binder->linkToDeath(notificationClient); -void AudioFlinger::binderDied(const wp& who) { + // the config change is always sent from playback or record threads to avoid deadlock + // with AudioSystem::gLock + for (size_t i = 0; i < mPlaybackThreads.size(); i++) { + mPlaybackThreads.valueAt(i)->sendConfigEvent(AudioSystem::OUTPUT_OPENED); + } - LOGV("binderDied() %p, tid %d, calling tid %d", who.unsafe_get(), gettid(), IPCThreadState::self()->getCallingPid()); - Mutex::Autolock _l(mLock); - - IBinder *binder = who.unsafe_get(); - - if (binder != NULL) { - int index = mNotificationClients.indexOf(binder); - if (index >= 0) { - LOGV("Removing notification client %p", binder); - mNotificationClients.removeAt(index); + for (size_t i = 0; i < mRecordThreads.size(); i++) { + mRecordThreads.valueAt(i)->sendConfigEvent(AudioSystem::INPUT_OPENED); } } } +void AudioFlinger::removeNotificationClient(pid_t pid) +{ + Mutex::Autolock _l(mLock); + + int index = mNotificationClients.indexOfKey(pid); + if (index >= 0) { + sp client = mNotificationClients.valueFor(pid); + LOGV("removeNotificationClient() %p, pid %d", client.get(), pid); +#ifdef LVMX + if (pid == mLifeVibesClientPid) { + LOGV("Disabling lifevibes"); + LifeVibes::enableMusic(false); + mLifeVibesClientPid = -1; + } +#endif + mNotificationClients.removeItem(pid); + } +} + // audioConfigChanged_l() must be called with AudioFlinger::mLock held -void AudioFlinger::audioConfigChanged_l(int event, int ioHandle, void *param2) { +void AudioFlinger::audioConfigChanged_l(int event, int ioHandle, void *param2) +{ size_t size = mNotificationClients.size(); for (size_t i = 0; i < size; i++) { - sp binder = mNotificationClients.itemAt(i); - LOGV("audioConfigChanged_l() Notifying change to client %p", binder.get()); - sp client = interface_cast (binder); - client->ioConfigChanged(event, ioHandle, param2); + mNotificationClients.valueAt(i)->client()->ioConfigChanged(event, ioHandle, param2); } } @@ -768,6 +777,7 @@ void AudioFlinger::removeClient_l(pid_t pid) mClients.removeItem(pid); } + // ---------------------------------------------------------------------------- AudioFlinger::ThreadBase::ThreadBase(const sp& audioFlinger, int id) @@ -3086,6 +3096,28 @@ const sp& AudioFlinger::Client::heap() const // ---------------------------------------------------------------------------- +AudioFlinger::NotificationClient::NotificationClient(const sp& audioFlinger, + const sp& client, + pid_t pid) + : mAudioFlinger(audioFlinger), mPid(pid), mClient(client) +{ +} + +AudioFlinger::NotificationClient::~NotificationClient() +{ + mClient.clear(); +} + +void AudioFlinger::NotificationClient::binderDied(const wp& who) +{ + sp keep(this); + { + mAudioFlinger->removeNotificationClient(mPid); + } +} + +// ---------------------------------------------------------------------------- + AudioFlinger::TrackHandle::TrackHandle(const sp& track) : BnAudioTrack(), mTrack(track) diff --git a/libs/audioflinger/AudioFlinger.h b/libs/audioflinger/AudioFlinger.h index 739ec3331165b..13aac8b96f8cf 100644 --- a/libs/audioflinger/AudioFlinger.h +++ b/libs/audioflinger/AudioFlinger.h @@ -57,7 +57,7 @@ class AudioResampler; static const nsecs_t kStandbyTimeInNsecs = seconds(3); -class AudioFlinger : public BnAudioFlinger, public IBinder::DeathRecipient +class AudioFlinger : public BnAudioFlinger { public: static void instantiate(); @@ -139,9 +139,6 @@ public: virtual status_t getRenderPosition(uint32_t *halFrames, uint32_t *dspFrames, int output); - // IBinder::DeathRecipient - virtual void binderDied(const wp& who); - enum hardware_call_state { AUDIO_HW_IDLE = 0, AUDIO_HW_INIT, @@ -205,6 +202,27 @@ private: pid_t mPid; }; + // --- Notification Client --- + class NotificationClient : public IBinder::DeathRecipient { + public: + NotificationClient(const sp& audioFlinger, + const sp& client, + pid_t pid); + virtual ~NotificationClient(); + + sp client() { return mClient; } + + // IBinder::DeathRecipient + virtual void binderDied(const wp& who); + + private: + NotificationClient(const NotificationClient&); + NotificationClient& operator = (const NotificationClient&); + + sp mAudioFlinger; + pid_t mPid; + sp mClient; + }; class TrackHandle; class RecordHandle; @@ -685,6 +703,7 @@ private: void removeClient_l(pid_t pid); + void removeNotificationClient(pid_t pid); // record thread @@ -796,8 +815,11 @@ private: DefaultKeyedVector< int, sp > mRecordThreads; - SortedVector< sp > mNotificationClients; + DefaultKeyedVector< pid_t, sp > mNotificationClients; int mNextThreadId; +#ifdef LVMX + int mLifeVibesClientPid; +#endif }; // ----------------------------------------------------------------------------