From 24a9fb0d4a3acd5d0c2ef96147650464a91eb8d9 Mon Sep 17 00:00:00 2001 From: Francois Gaffie Date: Fri, 18 Jan 2019 17:51:34 +0100 Subject: [PATCH] libaudioclient: force onAudioDeviceUpdate on registration This CL forces the onAudioDeviceUpdate on register event. It also fixes the loss of callback on AudioTrack or AudioRecord client if received before ioHandle is assigned. Test: audio smoke tests Test: CTS for AudioTrack and AudioRouting Change-Id: I119b5c407da68a5b55162550bea5fa7e724165d1 Signed-off-by: Francois Gaffie --- media/libaudioclient/AudioRecord.cpp | 17 ++++-- media/libaudioclient/AudioSystem.cpp | 57 ++++++++++++------- media/libaudioclient/AudioTrack.cpp | 18 +++--- .../include/media/AudioRecord.h | 2 +- .../include/media/AudioSystem.h | 32 ++++++++++- .../libaudioclient/include/media/AudioTrack.h | 2 +- 6 files changed, 90 insertions(+), 38 deletions(-) diff --git a/media/libaudioclient/AudioRecord.cpp b/media/libaudioclient/AudioRecord.cpp index 72a23e3022..8afb1cc5e7 100644 --- a/media/libaudioclient/AudioRecord.cpp +++ b/media/libaudioclient/AudioRecord.cpp @@ -355,7 +355,10 @@ status_t AudioRecord::set( } // create the IAudioRecord - status = createRecord_l(0 /*epoch*/, mOpPackageName); + { + AutoMutex lock(mLock); + status = createRecord_l(0 /*epoch*/, mOpPackageName); + } ALOGV("%s(%d): status %d", __func__, mPortId, status); @@ -1358,12 +1361,14 @@ status_t AudioRecord::removeAudioDeviceCallback( ALOGW("%s(%d): removing NULL callback!", __func__, mPortId); return BAD_VALUE; } - AutoMutex lock(mLock); - if (mDeviceCallback.unsafe_get() != callback.get()) { - ALOGW("%s(%d): removing different callback!", __func__, mPortId); - return INVALID_OPERATION; + { + AutoMutex lock(mLock); + if (mDeviceCallback.unsafe_get() != callback.get()) { + ALOGW("%s(%d): removing different callback!", __func__, mPortId); + return INVALID_OPERATION; + } + mDeviceCallback.clear(); } - mDeviceCallback.clear(); if (mInput != AUDIO_IO_HANDLE_NONE) { AudioSystem::removeAudioDeviceCallback(this, mInput); } diff --git a/media/libaudioclient/AudioSystem.cpp b/media/libaudioclient/AudioSystem.cpp index b83a441e32..33c2008480 100644 --- a/media/libaudioclient/AudioSystem.cpp +++ b/media/libaudioclient/AudioSystem.cpp @@ -522,8 +522,9 @@ void AudioSystem::AudioFlingerClient::ioConfigChanged(audio_io_config_event even if (ioDesc == 0 || ioDesc->mIoHandle == AUDIO_IO_HANDLE_NONE) return; audio_port_handle_t deviceId = AUDIO_PORT_HANDLE_NONE; - Vector < wp > callbacks; - + AudioDeviceCallbacks callbacks; + bool deviceValidOrChanged = false; + Mutex::Autolock _l(mCallbacksLock); { Mutex::Autolock _l(mLock); @@ -546,6 +547,13 @@ void AudioSystem::AudioFlingerClient::ioConfigChanged(audio_io_config_event even ssize_t ioIndex = mAudioDeviceCallbacks.indexOfKey(ioDesc->mIoHandle); if (ioIndex >= 0) { callbacks = mAudioDeviceCallbacks.valueAt(ioIndex); + deviceValidOrChanged = true; + } + } + if (event == AUDIO_OUTPUT_REGISTERED || event == AUDIO_INPUT_REGISTERED) { + ssize_t ioIndex = mAudioDeviceCallbacks.indexOfKey(ioDesc->mIoHandle); + if ((ioIndex >= 0) && !mAudioDeviceCallbacks.valueAt(ioIndex).notifiedOnce()) { + callbacks = mAudioDeviceCallbacks.valueAt(ioIndex); } } } @@ -584,6 +592,7 @@ void AudioSystem::AudioFlingerClient::ioConfigChanged(audio_io_config_event even mIoDescriptors.replaceValueFor(ioDesc->mIoHandle, ioDesc); if (deviceId != ioDesc->getDeviceId()) { + deviceValidOrChanged = true; deviceId = ioDesc->getDeviceId(); ssize_t ioIndex = mAudioDeviceCallbacks.indexOfKey(ioDesc->mIoHandle); if (ioIndex >= 0) { @@ -600,22 +609,28 @@ void AudioSystem::AudioFlingerClient::ioConfigChanged(audio_io_config_event even } break; } } - bool callbackRemoved = false; // callbacks.size() != 0 => ioDesc->mIoHandle and deviceId are valid - for (size_t i = 0; i < callbacks.size(); ) { - sp callback = callbacks[i].promote(); - if (callback.get() != nullptr) { - callback->onAudioDeviceUpdate(ioDesc->mIoHandle, deviceId); - i++; - } else { - callbacks.removeAt(i); - callbackRemoved = true; + if (callbacks.size() != 0) { + for (size_t i = 0; i < callbacks.size(); ) { + sp callback = callbacks[i].promote(); + if (callback.get() != nullptr) { + // Call the callback only if the device actually changed, the input or output was + // opened or closed or the client was newly registered and the callback was never + // called + if (!callback->notifiedOnce() || deviceValidOrChanged) { + // Must be called without mLock held. May lead to dead lock if calling for + // example getRoutedDevice that updates the device and tries to acquire mLock. + callback->onAudioDeviceUpdate(ioDesc->mIoHandle, deviceId); + callback->setNotifiedOnce(); + } + i++; + } else { + callbacks.removeAt(i); + } } - } - // clean up callback list while we are here if some clients have disappeared without - // unregistering their callback - if (callbackRemoved) { - Mutex::Autolock _l(mLock); + callbacks.setNotifiedOnce(); + // clean up callback list while we are here if some clients have disappeared without + // unregistering their callback, or if cb was served for the first time since registered mAudioDeviceCallbacks.replaceValueFor(ioDesc->mIoHandle, callbacks); } } @@ -671,8 +686,8 @@ sp AudioSystem::AudioFlingerClient::getIoDescriptor(audio_io_ status_t AudioSystem::AudioFlingerClient::addAudioDeviceCallback( const wp& callback, audio_io_handle_t audioIo) { - Mutex::Autolock _l(mLock); - Vector < wp > callbacks; + Mutex::Autolock _l(mCallbacksLock); + AudioDeviceCallbacks callbacks; ssize_t ioIndex = mAudioDeviceCallbacks.indexOfKey(audioIo); if (ioIndex >= 0) { callbacks = mAudioDeviceCallbacks.valueAt(ioIndex); @@ -684,7 +699,7 @@ status_t AudioSystem::AudioFlingerClient::addAudioDeviceCallback( } } callbacks.add(callback); - + callbacks.resetNotifiedOnce(); mAudioDeviceCallbacks.replaceValueFor(audioIo, callbacks); return NO_ERROR; } @@ -692,12 +707,12 @@ status_t AudioSystem::AudioFlingerClient::addAudioDeviceCallback( status_t AudioSystem::AudioFlingerClient::removeAudioDeviceCallback( const wp& callback, audio_io_handle_t audioIo) { - Mutex::Autolock _l(mLock); + Mutex::Autolock _l(mCallbacksLock); ssize_t ioIndex = mAudioDeviceCallbacks.indexOfKey(audioIo); if (ioIndex < 0) { return INVALID_OPERATION; } - Vector < wp > callbacks = mAudioDeviceCallbacks.valueAt(ioIndex); + AudioDeviceCallbacks callbacks = mAudioDeviceCallbacks.valueAt(ioIndex); size_t cbIndex; for (cbIndex = 0; cbIndex < callbacks.size(); cbIndex++) { diff --git a/media/libaudioclient/AudioTrack.cpp b/media/libaudioclient/AudioTrack.cpp index 79abea07e3..b5a7ebea26 100644 --- a/media/libaudioclient/AudioTrack.cpp +++ b/media/libaudioclient/AudioTrack.cpp @@ -621,8 +621,10 @@ status_t AudioTrack::set( } // create the IAudioTrack - status = createTrack_l(); - + { + AutoMutex lock(mLock); + status = createTrack_l(); + } if (status != NO_ERROR) { if (mAudioTrackThread != 0) { mAudioTrackThread->requestExit(); // see comment in AudioTrack.h @@ -2957,12 +2959,14 @@ status_t AudioTrack::removeAudioDeviceCallback( ALOGW("%s(%d): removing NULL callback!", __func__, mPortId); return BAD_VALUE; } - AutoMutex lock(mLock); - if (mDeviceCallback.unsafe_get() != callback.get()) { - ALOGW("%s(%d): removing different callback!", __func__, mPortId); - return INVALID_OPERATION; + { + AutoMutex lock(mLock); + if (mDeviceCallback.unsafe_get() != callback.get()) { + ALOGW("%s removing different callback!", __FUNCTION__); + return INVALID_OPERATION; + } + mDeviceCallback.clear(); } - mDeviceCallback.clear(); if (mOutput != AUDIO_IO_HANDLE_NONE) { AudioSystem::removeAudioDeviceCallback(this, mOutput); } diff --git a/media/libaudioclient/include/media/AudioRecord.h b/media/libaudioclient/include/media/AudioRecord.h index 1f718448af..4707c4ae1d 100644 --- a/media/libaudioclient/include/media/AudioRecord.h +++ b/media/libaudioclient/include/media/AudioRecord.h @@ -677,7 +677,7 @@ private: sp mCblkMemory; audio_track_cblk_t* mCblk; // re-load after mLock.unlock() sp mBufferMemory; - audio_io_handle_t mInput; // returned by AudioSystem::getInput() + audio_io_handle_t mInput = AUDIO_IO_HANDLE_NONE; // from AudioSystem::getInputforAttr() int mPreviousPriority; // before start() SchedPolicy mPreviousSchedulingGroup; diff --git a/media/libaudioclient/include/media/AudioSystem.h b/media/libaudioclient/include/media/AudioSystem.h index 87a9919126..74b994ed01 100644 --- a/media/libaudioclient/include/media/AudioSystem.h +++ b/media/libaudioclient/include/media/AudioSystem.h @@ -398,6 +398,15 @@ public: virtual void onAudioDeviceUpdate(audio_io_handle_t audioIo, audio_port_handle_t deviceId) = 0; + bool notifiedOnce() const { return mNotifiedOnce; } + void setNotifiedOnce() { mNotifiedOnce = true; } + private: + /** + * @brief mNotifiedOnce it forces the callback to be called at least once when + * registered with a VALID AudioDevice, and allows not to flood other listeners + * on this iohandle that already know the valid device. + */ + bool mNotifiedOnce = false; }; static status_t addAudioDeviceCallback(const wp& callback, @@ -443,8 +452,27 @@ private: private: Mutex mLock; DefaultKeyedVector > mIoDescriptors; - DefaultKeyedVector > > - mAudioDeviceCallbacks; + + class AudioDeviceCallbacks : public Vector> + { + public: + /** + * @brief notifiedOnce ensures that if a client adds a callback, it must at least be + * called once with the device on which it will be routed to. + * @return true if already notified or nobody waits for a callback, false otherwise. + */ + bool notifiedOnce() const { return (size() == 0) || mNotifiedOnce; } + void setNotifiedOnce() { mNotifiedOnce = true; } + void resetNotifiedOnce() { mNotifiedOnce = false; } + private: + /** + * @brief mNotifiedOnce it forces each callback to be called at least once when + * registered with a VALID AudioDevice + */ + bool mNotifiedOnce = false; + }; + Mutex mCallbacksLock; // prevents race on Callbacks + DefaultKeyedVector mAudioDeviceCallbacks; // cached values for recording getInputBufferSize() queries size_t mInBuffSize; // zero indicates cache is invalid uint32_t mInSamplingRate; diff --git a/media/libaudioclient/include/media/AudioTrack.h b/media/libaudioclient/include/media/AudioTrack.h index cbb750f90d..12f5d71435 100644 --- a/media/libaudioclient/include/media/AudioTrack.h +++ b/media/libaudioclient/include/media/AudioTrack.h @@ -1021,7 +1021,7 @@ public: sp mAudioTrack; sp mCblkMemory; audio_track_cblk_t* mCblk; // re-load after mLock.unlock() - audio_io_handle_t mOutput; // returned by AudioSystem::getOutputForAttr() + audio_io_handle_t mOutput = AUDIO_IO_HANDLE_NONE; // from AudioSystem::getOutputForAttr() sp mAudioTrackThread; bool mThreadCanCallJava;