Merge "Wire up the microphone toggle to the audio service"
diff --git a/media/libaudiohal/Android.bp b/media/libaudiohal/Android.bp
index 482f40e..d9a7804 100644
--- a/media/libaudiohal/Android.bp
+++ b/media/libaudiohal/Android.bp
@@ -31,6 +31,7 @@
header_libs: [
"libaudiohal_headers",
"libbase_headers",
+ "libmediautils_headers",
]
}
diff --git a/media/libaudiohal/impl/StreamHalHidl.cpp b/media/libaudiohal/impl/StreamHalHidl.cpp
index 09a7c1c..3f9bccb 100644
--- a/media/libaudiohal/impl/StreamHalHidl.cpp
+++ b/media/libaudiohal/impl/StreamHalHidl.cpp
@@ -61,10 +61,6 @@
}
}
-StreamHalHidl::~StreamHalHidl() {
- mStream = nullptr;
-}
-
status_t StreamHalHidl::getSampleRate(uint32_t *rate) {
if (!mStream) return NO_INIT;
return processReturn("getSampleRate", mStream->getSampleRate(), rate);
@@ -286,7 +282,7 @@
}
private:
- wp<StreamOutHalHidl> mStream;
+ const wp<StreamOutHalHidl> mStream;
};
} // namespace
@@ -297,21 +293,19 @@
StreamOutHalHidl::~StreamOutHalHidl() {
if (mStream != 0) {
- if (mCallback.unsafe_get()) {
+ if (mCallback.load().unsafe_get()) {
processReturn("clearCallback", mStream->clearCallback());
}
#if MAJOR_VERSION >= 6
- if (mEventCallback.unsafe_get() != nullptr) {
+ if (mEventCallback.load().unsafe_get() != nullptr) {
processReturn("setEventCallback",
mStream->setEventCallback(nullptr));
}
#endif
processReturn("close", mStream->close());
- mStream.clear();
}
- mCallback.clear();
- mEventCallback.clear();
- hardware::IPCThreadState::self()->flushCommands();
+ mCallback = nullptr;
+ mEventCallback = nullptr;
if (mEfGroup) {
EventFlag::deleteEventFlag(&mEfGroup);
}
@@ -364,7 +358,7 @@
if (bytes == 0 && !mDataMQ) {
// Can't determine the size for the MQ buffer. Wait for a non-empty write request.
- ALOGW_IF(mCallback.unsafe_get(), "First call to async write with 0 bytes");
+ ALOGW_IF(mCallback.load().unsafe_get(), "First call to async write with 0 bytes");
return OK;
}
@@ -680,28 +674,28 @@
#endif
void StreamOutHalHidl::onWriteReady() {
- sp<StreamOutHalInterfaceCallback> callback = mCallback.promote();
+ sp<StreamOutHalInterfaceCallback> callback = mCallback.load().promote();
if (callback == 0) return;
ALOGV("asyncCallback onWriteReady");
callback->onWriteReady();
}
void StreamOutHalHidl::onDrainReady() {
- sp<StreamOutHalInterfaceCallback> callback = mCallback.promote();
+ sp<StreamOutHalInterfaceCallback> callback = mCallback.load().promote();
if (callback == 0) return;
ALOGV("asyncCallback onDrainReady");
callback->onDrainReady();
}
void StreamOutHalHidl::onError() {
- sp<StreamOutHalInterfaceCallback> callback = mCallback.promote();
+ sp<StreamOutHalInterfaceCallback> callback = mCallback.load().promote();
if (callback == 0) return;
ALOGV("asyncCallback onError");
callback->onError();
}
void StreamOutHalHidl::onCodecFormatChanged(const std::basic_string<uint8_t>& metadataBs) {
- sp<StreamOutHalInterfaceEventCallback> callback = mEventCallback.promote();
+ sp<StreamOutHalInterfaceEventCallback> callback = mEventCallback.load().promote();
if (callback == nullptr) return;
ALOGV("asyncCodecFormatCallback %s", __func__);
callback->onCodecFormatChanged(metadataBs);
@@ -715,8 +709,6 @@
StreamInHalHidl::~StreamInHalHidl() {
if (mStream != 0) {
processReturn("close", mStream->close());
- mStream.clear();
- hardware::IPCThreadState::self()->flushCommands();
}
if (mEfGroup) {
EventFlag::deleteEventFlag(&mEfGroup);
diff --git a/media/libaudiohal/impl/StreamHalHidl.h b/media/libaudiohal/impl/StreamHalHidl.h
index 88f8587..8b1c3d3 100644
--- a/media/libaudiohal/impl/StreamHalHidl.h
+++ b/media/libaudiohal/impl/StreamHalHidl.h
@@ -25,6 +25,7 @@
#include <fmq/EventFlag.h>
#include <fmq/MessageQueue.h>
#include <media/audiohal/StreamHalInterface.h>
+#include <mediautils/Synchronization.h>
#include "ConversionHelperHidl.h"
#include "StreamPowerLog.h"
@@ -100,9 +101,6 @@
// Subclasses can not be constructed directly by clients.
explicit StreamHalHidl(IStream *stream);
- // The destructor automatically closes the stream.
- virtual ~StreamHalHidl();
-
status_t getCachedBufferSize(size_t *size);
bool requestHalThreadPriority(pid_t threadPid, pid_t threadId);
@@ -112,7 +110,7 @@
private:
const int HAL_THREAD_PRIORITY_DEFAULT = -1;
- IStream *mStream;
+ IStream * const mStream;
int mHalThreadPriority;
size_t mCachedBufferSize;
};
@@ -184,9 +182,16 @@
typedef MessageQueue<uint8_t, hardware::kSynchronizedReadWrite> DataMQ;
typedef MessageQueue<WriteStatus, hardware::kSynchronizedReadWrite> StatusMQ;
- wp<StreamOutHalInterfaceCallback> mCallback;
- wp<StreamOutHalInterfaceEventCallback> mEventCallback;
- sp<IStreamOut> mStream;
+ // Do not move the Defer. This should be the first member variable in the class;
+ // thus the last member destructor called upon instance destruction.
+ //
+ // The last step is to flush all binder commands so that the AudioFlinger
+ // may recognize the deletion of IStreamOut (mStream) with less delay. See b/35394629.
+ mediautils::Defer mLast{[]() { hardware::IPCThreadState::self()->flushCommands(); }};
+
+ mediautils::atomic_wp<StreamOutHalInterfaceCallback> mCallback;
+ mediautils::atomic_wp<StreamOutHalInterfaceEventCallback> mEventCallback;
+ const sp<IStreamOut> mStream;
std::unique_ptr<CommandMQ> mCommandMQ;
std::unique_ptr<DataMQ> mDataMQ;
std::unique_ptr<StatusMQ> mStatusMQ;
@@ -242,7 +247,14 @@
typedef MessageQueue<uint8_t, hardware::kSynchronizedReadWrite> DataMQ;
typedef MessageQueue<ReadStatus, hardware::kSynchronizedReadWrite> StatusMQ;
- sp<IStreamIn> mStream;
+ // Do not move the Defer. This should be the first member variable in the class;
+ // thus the last member destructor called upon instance destruction.
+ //
+ // The last step is to flush all binder commands so that the AudioFlinger
+ // may recognize the deletion of IStreamIn (mStream) with less delay. See b/35394629.
+ mediautils::Defer mLast{[]() { hardware::IPCThreadState::self()->flushCommands(); }};
+
+ const sp<IStreamIn> mStream;
std::unique_ptr<CommandMQ> mCommandMQ;
std::unique_ptr<DataMQ> mDataMQ;
std::unique_ptr<StatusMQ> mStatusMQ;
diff --git a/media/mtp/IMtpDatabase.h b/media/mtp/IMtpDatabase.h
index 81fa60c..3b9bbb0 100644
--- a/media/mtp/IMtpDatabase.h
+++ b/media/mtp/IMtpDatabase.h
@@ -39,7 +39,7 @@
// Called to report success or failure of the SendObject file transfer.
virtual void endSendObject(MtpObjectHandle handle,
bool succeeded) = 0;
-
+
// Called to rescan a file, such as after an edit.
virtual void rescanFile(const char* path,
MtpObjectHandle handle,
@@ -91,6 +91,8 @@
int64_t& outFileLength,
MtpObjectFormat& outFormat) = 0;
+ virtual int openFilePath(const char* path, bool transcode) = 0;
+
virtual MtpResponseCode beginDeleteObject(MtpObjectHandle handle) = 0;
virtual void endDeleteObject(MtpObjectHandle handle, bool succeeded) = 0;
diff --git a/media/mtp/MtpServer.cpp b/media/mtp/MtpServer.cpp
index 8677b90..d0ad22c 100644
--- a/media/mtp/MtpServer.cpp
+++ b/media/mtp/MtpServer.cpp
@@ -790,11 +790,34 @@
auto start = std::chrono::steady_clock::now();
const char* filePath = (const char *)pathBuf;
- mtp_file_range mfr;
- mfr.fd = open(filePath, O_RDONLY);
- if (mfr.fd < 0) {
- return MTP_RESPONSE_GENERAL_ERROR;
+ mtp_file_range mfr;
+ struct stat sstat;
+ uint64_t finalsize;
+ bool transcode = android::base::GetBoolProperty("sys.fuse.transcode_mtp", true);
+ if (!transcode) {
+ ALOGD("Mtp transcode disabled");
+ mfr.fd = mDatabase->openFilePath(filePath, false);
+ // Doing this here because we want to update fileLength only for this case and leave the
+ // regular path as unchanged as possible.
+ if (mfr.fd >= 0) {
+ fstat(mfr.fd, &sstat);
+ finalsize = sstat.st_size;
+ fileLength = finalsize;
+ } else {
+ ALOGW("Mtp open with no transcoding failed for %s. Falling back to the original",
+ filePath);
+ }
}
+
+ if (transcode || mfr.fd < 0) {
+ mfr.fd = open(filePath, O_RDONLY);
+ if (mfr.fd < 0) {
+ return MTP_RESPONSE_GENERAL_ERROR;
+ }
+ fstat(mfr.fd, &sstat);
+ finalsize = sstat.st_size;
+ }
+
mfr.offset = 0;
mfr.length = fileLength;
mfr.command = mRequest.getOperationCode();
@@ -815,9 +838,6 @@
auto end = std::chrono::steady_clock::now();
std::chrono::duration<double> diff = end - start;
- struct stat sstat;
- fstat(mfr.fd, &sstat);
- uint64_t finalsize = sstat.st_size;
ALOGV("Sent a file over MTP. Time: %f s, Size: %" PRIu64 ", Rate: %f bytes/s",
diff.count(), finalsize, ((double) finalsize) / diff.count());
closeObjFd(mfr.fd, filePath);
diff --git a/media/mtp/tests/MtpFuzzer/MtpMockDatabase.cpp b/media/mtp/tests/MtpFuzzer/MtpMockDatabase.cpp
index 5d95aa2..8aafe33 100644
--- a/media/mtp/tests/MtpFuzzer/MtpMockDatabase.cpp
+++ b/media/mtp/tests/MtpFuzzer/MtpMockDatabase.cpp
@@ -252,6 +252,11 @@
return MTP_RESPONSE_OK;
}
+int MtpMockDatabase::openFilePath(const char* path, bool transcode) {
+ ALOGD("MockDatabase %s: filePath=%s transcode=%d\n", __func__, path, transcode);
+ return 0;
+}
+
MtpResponseCode MtpMockDatabase::beginDeleteObject(MtpObjectHandle handle) {
ALOGD("MockDatabase %s: ohandle=%u\n", __func__, handle);
return MTP_RESPONSE_OK;
diff --git a/media/mtp/tests/MtpFuzzer/MtpMockDatabase.h b/media/mtp/tests/MtpFuzzer/MtpMockDatabase.h
index 876719e..e9e6a64 100644
--- a/media/mtp/tests/MtpFuzzer/MtpMockDatabase.h
+++ b/media/mtp/tests/MtpFuzzer/MtpMockDatabase.h
@@ -90,6 +90,8 @@
MtpResponseCode getObjectFilePath(MtpObjectHandle handle, MtpStringBuffer& outFilePath,
int64_t& outFileLength, MtpObjectFormat& outFormat);
+ int openFilePath(const char* path, bool transcode);
+
MtpResponseCode beginDeleteObject(MtpObjectHandle handle);
void endDeleteObject(MtpObjectHandle handle, bool succeeded);
diff --git a/media/utils/Android.bp b/media/utils/Android.bp
index e3f1e44..952fe37 100644
--- a/media/utils/Android.bp
+++ b/media/utils/Android.bp
@@ -60,3 +60,10 @@
local_include_dirs: ["include"],
export_include_dirs: ["include"],
}
+
+cc_library_headers {
+ name: "libmediautils_headers",
+ vendor_available: true, // required for platform/hardware/interfaces
+
+ export_include_dirs: ["include"],
+}
diff --git a/media/utils/include/mediautils/Synchronization.h b/media/utils/include/mediautils/Synchronization.h
new file mode 100644
index 0000000..aef4967
--- /dev/null
+++ b/media/utils/include/mediautils/Synchronization.h
@@ -0,0 +1,133 @@
+/*
+ * Copyright 2021, The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+#include <mutex>
+#include <utils/RefBase.h>
+
+namespace android::mediautils {
+
+/**
+ * The LockItem class introduces a simple template which mimics atomic<T>
+ * for non-trivially copyable types. For trivially copyable types,
+ * the LockItem will statically assert that an atomic<T> should be used instead.
+ *
+ * The default lock mutex is std::mutex which is suitable for all but rare cases
+ * e.g. recursive constructors that might be found in tree construction,
+ * setters that might recurse onto the same object.
+ */
+
+template <typename T, typename L = std::mutex, int FLAGS = 0>
+class LockItem {
+protected:
+ mutable L mLock;
+ mutable T mT;
+
+public:
+ enum {
+ // Best practices for smart pointers and complex containers is to move to a temp
+ // and invoke destructor outside of lock. This reduces time under lock and in
+ // some cases eliminates deadlock.
+ FLAG_DTOR_OUT_OF_LOCK = 1,
+ };
+
+ // Check type, suggest std::atomic if possible.
+ static_assert(!std::is_trivially_copyable_v<T>,
+ "type is trivially copyable, please use std::atomic instead");
+
+ // Allow implicit conversions as expected for some types, e.g. sp -> wp.
+ template <typename... Args>
+ LockItem(Args&&... args) : mT(std::forward<Args>(args)...) {
+ }
+
+ // NOT copy or move / assignable or constructible.
+
+ // Do not enable this because it may lead to confusion because it returns
+ // a copy-value not a reference.
+ // operator T() const { return load(); }
+
+ // any conversion done under lock.
+ template <typename U>
+ void operator=(U&& u) {
+ store(std::forward<U>(u));
+ }
+
+ // returns a copy-value not a reference.
+ T load() const {
+ std::lock_guard lock(mLock);
+ return mT;
+ }
+
+ // any conversion done under lock.
+ template <typename U>
+ void store(U&& u) {
+ if constexpr ((FLAGS & FLAG_DTOR_OUT_OF_LOCK) != 0) {
+ std::unique_lock lock(mLock);
+ T temp = std::move(mT);
+ mT = std::forward<U>(u);
+ lock.unlock();
+ } else {
+ std::lock_guard lock(mLock);
+ mT = std::forward<U>(u);
+ }
+ }
+};
+
+/**
+ * atomic_wp<> and atomic_sp<> are used for concurrent access to Android
+ * sp<> and wp<> smart pointers, including their modifiers. We
+ * return a copy of the smart pointer with load().
+ *
+ * Historical: The importance of an atomic<std::shared_ptr<T>> class is described
+ * by Herb Sutter in the following ISO document https://isocpp.org/files/papers/N4162.pdf
+ * and is part of C++20. Lock free versions of atomic smart pointers are available
+ * publicly but usually require specialized smart pointer structs.
+ * See also https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic
+ * and https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic2
+ *
+ * We offer lock based atomic_wp<> and atomic_sp<> objects here. This is useful to
+ * copy the Android smart pointer to a different variable for subsequent local access,
+ * where the change of the original object after copy is acceptable.
+ *
+ * Note: Instead of atomics, it is often preferrable to create an explicit visible lock to
+ * ensure complete transaction consistency. For example, one might want to ensure
+ * that the method called from the smart pointer is also done under lock.
+ * This may not be possible for callbacks due to inverted lock ordering.
+ */
+
+template <typename T>
+using atomic_wp = LockItem<::android::wp<T>>;
+
+template <typename T>
+using atomic_sp = LockItem<
+ ::android::sp<T>, std::mutex, LockItem<::android::sp<T>>::FLAG_DTOR_OUT_OF_LOCK>;
+
+/**
+ * Defers a function to run in the RAII destructor.
+ * A C++ implementation of Go _defer_ https://golangr.com/defer/.
+ */
+class Defer {
+public:
+ template <typename U>
+ explicit Defer(U &&f) : mThunk(std::forward<U>(f)) {}
+ ~Defer() { mThunk(); }
+
+private:
+ const std::function<void()> mThunk;
+};
+
+} // namespace android::mediautils
+
diff --git a/media/utils/tests/Android.bp b/media/utils/tests/Android.bp
new file mode 100644
index 0000000..bb413c3
--- /dev/null
+++ b/media/utils/tests/Android.bp
@@ -0,0 +1,19 @@
+cc_test {
+ name: "media_synchronization_tests",
+
+ cflags: [
+ "-Wall",
+ "-Werror",
+ "-Wextra",
+ ],
+
+ shared_libs: [
+ "liblog",
+ "libmediautils",
+ "libutils",
+ ],
+
+ srcs: [
+ "media_synchronization_tests.cpp",
+ ],
+}
diff --git a/media/utils/tests/media_synchronization_tests.cpp b/media/utils/tests/media_synchronization_tests.cpp
new file mode 100644
index 0000000..169768e
--- /dev/null
+++ b/media/utils/tests/media_synchronization_tests.cpp
@@ -0,0 +1,82 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#define LOG_TAG "media_synchronization_tests"
+
+#include <mediautils/Synchronization.h>
+
+#include <gtest/gtest.h>
+#include <utils/Log.h>
+
+using namespace android;
+using namespace android::mediautils;
+
+// Simple Test Class
+template <typename T>
+class MyObject : public RefBase {
+ T value_;
+ public:
+ MyObject(const T& value) : value_(value) {}
+ MyObject(const MyObject<T>& mo) : value_(mo.get()) {}
+ T get() const { return value_; }
+ void set(const T& value) { value_ = value; }
+};
+
+TEST(media_synchronization_tests, atomic_wp) {
+ sp<MyObject<int>> refobj = new MyObject<int>(20);
+ atomic_wp<MyObject<int>> wpobj = refobj;
+
+ // we can promote.
+ ASSERT_EQ(20, wpobj.load().promote()->get());
+
+ // same underlying object for sp and atomic_wp.
+ ASSERT_EQ(refobj.get(), wpobj.load().promote().get());
+
+ // behavior is consistent with same underlying object.
+ wpobj.load().promote()->set(10);
+ ASSERT_EQ(10, refobj->get());
+ refobj->set(5);
+ ASSERT_EQ(5, wpobj.load().promote()->get());
+
+ // we can clear our weak ptr.
+ wpobj = nullptr;
+ ASSERT_EQ(nullptr, wpobj.load().promote());
+
+ // didn't affect our original obj.
+ ASSERT_NE(nullptr, refobj.get());
+}
+
+TEST(media_synchronization_tests, atomic_sp) {
+ sp<MyObject<int>> refobj = new MyObject<int>(20);
+ atomic_sp<MyObject<int>> spobj = refobj;
+
+ // same underlying object for sp and atomic_sp.
+ ASSERT_EQ(refobj.get(), spobj.load().get());
+
+ // behavior is consistent with same underlying object.
+ ASSERT_EQ(20, spobj.load()->get());
+ spobj.load()->set(10);
+ ASSERT_EQ(10, refobj->get());
+ refobj->set(5);
+ ASSERT_EQ(5, spobj.load()->get());
+
+ // we can clear spobj.
+ spobj = nullptr;
+ ASSERT_EQ(nullptr, spobj.load().get());
+
+ // didn't affect our original obj.
+ ASSERT_NE(nullptr, refobj.get());
+}
diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index a6f0953..b8257d3 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -128,9 +128,6 @@
// we define a minimum time during which a global effect is considered enabled.
static const nsecs_t kMinGlobalEffectEnabletimeNs = seconds(7200);
-Mutex gLock;
-wp<AudioFlinger> gAudioFlinger;
-
// Keep a strong reference to media.log service around forever.
// The service is within our parent process so it can never die in a way that we could observe.
// These two variables are const after initialization.
@@ -268,7 +265,7 @@
mMode = AUDIO_MODE_NORMAL;
- gAudioFlinger = this;
+ gAudioFlinger = this; // we are already refcounted, store into atomic pointer.
mDevicesFactoryHalCallback = new DevicesFactoryHalCallbackImpl;
mDevicesFactoryHal->setCallbackOnce(mDevicesFactoryHalCallback);
@@ -325,11 +322,9 @@
sp<MmapStreamInterface>& interface,
audio_port_handle_t *handle)
{
- sp<AudioFlinger> af;
- {
- Mutex::Autolock _l(gLock);
- af = gAudioFlinger.promote();
- }
+ // TODO: Use ServiceManager to get IAudioFlinger instead of by atomic pointer.
+ // This allows moving oboeservice (AAudio) to a separate process in the future.
+ sp<AudioFlinger> af = AudioFlinger::gAudioFlinger.load(); // either nullptr or singleton AF.
status_t ret = NO_INIT;
if (af != 0) {
ret = af->openMmapStream(
diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h
index 1c6f57c..1d530a4 100644
--- a/services/audioflinger/AudioFlinger.h
+++ b/services/audioflinger/AudioFlinger.h
@@ -73,6 +73,7 @@
#include <media/ExtendedAudioBufferProvider.h>
#include <media/VolumeShaper.h>
#include <mediautils/ServiceUtilities.h>
+#include <mediautils/Synchronization.h>
#include <audio_utils/clock.h>
#include <audio_utils/FdToString.h>
@@ -305,6 +306,24 @@
Mutex mUnregisteredWritersLock;
public:
+ // Life cycle of gAudioFlinger and AudioFlinger:
+ //
+ // AudioFlinger is created once and survives until audioserver crashes
+ // irrespective of sp<> and wp<> as it is refcounted by ServiceManager and we
+ // don't issue a ServiceManager::tryUnregisterService().
+ //
+ // gAudioFlinger is an atomic pointer set on AudioFlinger::onFirstRef().
+ // After this is set, it is safe to obtain a wp<> or sp<> from it as the
+ // underlying object does not go away.
+ //
+ // Note: For most inner classes, it is acceptable to hold a reference to the outer
+ // AudioFlinger instance as creation requires AudioFlinger to exist in the first place.
+ //
+ // An atomic here ensures underlying writes have completed before setting
+ // the pointer. Access by memory_order_seq_cst.
+ //
+
+ static inline std::atomic<AudioFlinger *> gAudioFlinger = nullptr;
class SyncEvent;
diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp
index 3ab7737..56d32a6 100644
--- a/services/audioflinger/Effects.cpp
+++ b/services/audioflinger/Effects.cpp
@@ -2785,11 +2785,7 @@
const effect_uuid_t *pEffectUuid, int32_t sessionId, int32_t deviceId,
sp<EffectHalInterface> *effect) {
status_t status = NO_INIT;
- sp<AudioFlinger> af = mAudioFlinger.promote();
- if (af == nullptr) {
- return status;
- }
- sp<EffectsFactoryHalInterface> effectsFactory = af->getEffectsFactory();
+ sp<EffectsFactoryHalInterface> effectsFactory = mAudioFlinger.getEffectsFactory();
if (effectsFactory != 0) {
status = effectsFactory->createEffect(pEffectUuid, sessionId, io(), deviceId, effect);
}
@@ -2798,25 +2794,19 @@
bool AudioFlinger::EffectChain::EffectCallback::updateOrphanEffectChains(
const sp<AudioFlinger::EffectBase>& effect) {
- sp<AudioFlinger> af = mAudioFlinger.promote();
- if (af == nullptr) {
- return false;
- }
// in EffectChain context, an EffectBase is always from an EffectModule so static cast is safe
- return af->updateOrphanEffectChains(effect->asEffectModule());
+ return mAudioFlinger.updateOrphanEffectChains(effect->asEffectModule());
}
status_t AudioFlinger::EffectChain::EffectCallback::allocateHalBuffer(
size_t size, sp<EffectBufferHalInterface>* buffer) {
- sp<AudioFlinger> af = mAudioFlinger.promote();
- LOG_ALWAYS_FATAL_IF(af == nullptr, "allocateHalBuffer() could not retrieved audio flinger");
- return af->mEffectsFactoryHal->allocateBuffer(size, buffer);
+ return mAudioFlinger.mEffectsFactoryHal->allocateBuffer(size, buffer);
}
status_t AudioFlinger::EffectChain::EffectCallback::addEffectToHal(
sp<EffectHalInterface> effect) {
status_t result = NO_INIT;
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return result;
}
@@ -2832,7 +2822,7 @@
status_t AudioFlinger::EffectChain::EffectCallback::removeEffectFromHal(
sp<EffectHalInterface> effect) {
status_t result = NO_INIT;
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return result;
}
@@ -2846,7 +2836,7 @@
}
audio_io_handle_t AudioFlinger::EffectChain::EffectCallback::io() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return AUDIO_IO_HANDLE_NONE;
}
@@ -2854,7 +2844,7 @@
}
bool AudioFlinger::EffectChain::EffectCallback::isOutput() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return true;
}
@@ -2862,7 +2852,7 @@
}
bool AudioFlinger::EffectChain::EffectCallback::isOffload() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return false;
}
@@ -2870,7 +2860,7 @@
}
bool AudioFlinger::EffectChain::EffectCallback::isOffloadOrDirect() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return false;
}
@@ -2878,7 +2868,7 @@
}
bool AudioFlinger::EffectChain::EffectCallback::isOffloadOrMmap() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return false;
}
@@ -2886,7 +2876,7 @@
}
uint32_t AudioFlinger::EffectChain::EffectCallback::sampleRate() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return 0;
}
@@ -2894,7 +2884,7 @@
}
audio_channel_mask_t AudioFlinger::EffectChain::EffectCallback::channelMask() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return AUDIO_CHANNEL_NONE;
}
@@ -2902,7 +2892,7 @@
}
uint32_t AudioFlinger::EffectChain::EffectCallback::channelCount() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return 0;
}
@@ -2910,7 +2900,7 @@
}
audio_channel_mask_t AudioFlinger::EffectChain::EffectCallback::hapticChannelMask() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return AUDIO_CHANNEL_NONE;
}
@@ -2918,7 +2908,7 @@
}
size_t AudioFlinger::EffectChain::EffectCallback::frameCount() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return 0;
}
@@ -2926,7 +2916,7 @@
}
uint32_t AudioFlinger::EffectChain::EffectCallback::latency() const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return 0;
}
@@ -2934,7 +2924,7 @@
}
void AudioFlinger::EffectChain::EffectCallback::setVolumeForOutput(float left, float right) const {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return;
}
@@ -2943,13 +2933,13 @@
void AudioFlinger::EffectChain::EffectCallback::checkSuspendOnEffectEnabled(
const sp<EffectBase>& effect, bool enabled, bool threadLocked) {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return;
}
t->checkSuspendOnEffectEnabled(enabled, effect->sessionId(), threadLocked);
- sp<EffectChain> c = mChain.promote();
+ sp<EffectChain> c = chain().promote();
if (c == nullptr) {
return;
}
@@ -2958,7 +2948,7 @@
}
void AudioFlinger::EffectChain::EffectCallback::onEffectEnable(const sp<EffectBase>& effect) {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return;
}
@@ -2969,7 +2959,7 @@
void AudioFlinger::EffectChain::EffectCallback::onEffectDisable(const sp<EffectBase>& effect) {
checkSuspendOnEffectEnabled(effect, false, false /*threadLocked*/);
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return;
}
@@ -2978,7 +2968,7 @@
bool AudioFlinger::EffectChain::EffectCallback::disconnectEffectHandle(EffectHandle *handle,
bool unpinIfLast) {
- sp<ThreadBase> t = mThread.promote();
+ sp<ThreadBase> t = thread().promote();
if (t == nullptr) {
return false;
}
@@ -2987,7 +2977,7 @@
}
void AudioFlinger::EffectChain::EffectCallback::resetVolume() {
- sp<EffectChain> c = mChain.promote();
+ sp<EffectChain> c = chain().promote();
if (c == nullptr) {
return;
}
@@ -2996,7 +2986,7 @@
}
uint32_t AudioFlinger::EffectChain::EffectCallback::strategy() const {
- sp<EffectChain> c = mChain.promote();
+ sp<EffectChain> c = chain().promote();
if (c == nullptr) {
return PRODUCT_STRATEGY_NONE;
}
@@ -3004,7 +2994,7 @@
}
int32_t AudioFlinger::EffectChain::EffectCallback::activeTrackCnt() const {
- sp<EffectChain> c = mChain.promote();
+ sp<EffectChain> c = chain().promote();
if (c == nullptr) {
return 0;
}
diff --git a/services/audioflinger/Effects.h b/services/audioflinger/Effects.h
index 89321f9..139c049 100644
--- a/services/audioflinger/Effects.h
+++ b/services/audioflinger/Effects.h
@@ -512,14 +512,21 @@
private:
+ // For transaction consistency, please consider holding the EffectChain lock before
+ // calling the EffectChain::EffectCallback methods, excepting
+ // createEffectHal and allocateHalBuffer.
+ //
+ // This prevents migration of the EffectChain to another PlaybackThread
+ // for the purposes of the EffectCallback.
class EffectCallback : public EffectCallbackInterface {
public:
// Note: ctors taking a weak pointer to their owner must not promote it
// during construction (but may keep a reference for later promotion).
EffectCallback(const wp<EffectChain>& owner,
const wp<ThreadBase>& thread)
- : mChain(owner) {
- setThread(thread);
+ : mChain(owner)
+ , mThread(thread)
+ , mAudioFlinger(*gAudioFlinger) {
}
status_t createEffectHal(const effect_uuid_t *pEffectUuid,
@@ -556,20 +563,16 @@
wp<EffectChain> chain() const override { return mChain; }
- wp<ThreadBase> thread() { return mThread; }
+ wp<ThreadBase> thread() const { return mThread.load(); }
- // TODO(b/161341295) secure this against concurrent access to mThread
- // by other callers.
void setThread(const wp<ThreadBase>& thread) {
mThread = thread;
- sp<ThreadBase> p = thread.promote();
- mAudioFlinger = p ? p->mAudioFlinger : nullptr;
}
private:
const wp<EffectChain> mChain;
- wp<ThreadBase> mThread; // TODO(b/161341295) protect against concurrent access
- wp<AudioFlinger> mAudioFlinger; // this could be const with some rearrangement.
+ mediautils::atomic_wp<ThreadBase> mThread;
+ AudioFlinger &mAudioFlinger; // implementation detail: outer instance always exists.
};
friend class AudioFlinger; // for mThread, mEffects