libaudiohal@aidl: Align ownership of patches with the framework
DeviceHalAidl creates patches when opening streams.
Later these patches can be "re-created" by the framework,
that means, the framework creates a patch for the same
pair of device and mix port configurations via
'createAudioPatch' call. In this case, ownership of
the patch must be transferred to the framework, it will
release it via 'releaseAudioPatch' later.
Properly separate ownership of port configs between streams
and patches to avoid attempting to release port configs
belonging to patches.
Bug: 302132812
Bug: 303742495
Test: run `atest CtsMediaAudioTestCases \
--test-filter=".*AudioPlaybackCaptureTest#testPlaybackCaptureDoS`
and check for errors from the AIDL HAL
Change-Id: I695af339e7293a12d07cf0d35d10a4c29008a3d2
diff --git a/media/libaudiohal/impl/DeviceHalAidl.cpp b/media/libaudiohal/impl/DeviceHalAidl.cpp
index bd95e9d..8a843ed 100644
--- a/media/libaudiohal/impl/DeviceHalAidl.cpp
+++ b/media/libaudiohal/impl/DeviceHalAidl.cpp
@@ -477,7 +477,7 @@
{
std::lock_guard l(mLock);
mCallbacks.emplace(cbCookie, Callbacks{});
- mMapper.addStream(*outStream, aidlPatch.id);
+ mMapper.addStream(*outStream, mixPortConfig.id, aidlPatch.id);
}
if (streamCb) streamCb->setCookie(cbCookie);
eventCb->setCookie(cbCookie);
@@ -543,7 +543,7 @@
std::move(ret.stream), mVendorExt, this /*micInfoProvider*/);
{
std::lock_guard l(mLock);
- mMapper.addStream(*inStream, aidlPatch.id);
+ mMapper.addStream(*inStream, mixPortConfig.id, aidlPatch.id);
}
cleanups.disarmAll();
return OK;
diff --git a/media/libaudiohal/impl/Hal2AidlMapper.cpp b/media/libaudiohal/impl/Hal2AidlMapper.cpp
index 28b12c2..f187057 100644
--- a/media/libaudiohal/impl/Hal2AidlMapper.cpp
+++ b/media/libaudiohal/impl/Hal2AidlMapper.cpp
@@ -88,8 +88,9 @@
: mInstance(instance), mModule(module) {
}
-void Hal2AidlMapper::addStream(const sp<StreamHalInterface>& stream, int32_t patchId) {
- mStreams.insert(std::pair(stream, patchId));
+void Hal2AidlMapper::addStream(
+ const sp<StreamHalInterface>& stream, int32_t portConfigId, int32_t patchId) {
+ mStreams.insert(std::pair(stream, std::pair(portConfigId, patchId)));
}
bool Hal2AidlMapper::audioDeviceMatches(const AudioDevice& device, const AudioPort& p) {
@@ -164,8 +165,15 @@
} else {
bool created = false;
RETURN_STATUS_IF_ERROR(findOrCreatePatch(patch, &patch, &created));
- // Since no cleanup of the patch is needed, 'created' is ignored.
+ // No cleanup of the patch is needed, it is managed by the framework.
*patchId = patch.id;
+ if (!created) {
+ // The framework might have "created" a patch which already existed due to
+ // stream creation. Need to release the ownership from the stream.
+ for (auto& s : mStreams) {
+ if (s.second.second == patch.id) s.second.second = -1;
+ }
+ }
}
return OK;
}
@@ -622,23 +630,17 @@
return OK;
}
-bool Hal2AidlMapper::isPortHeldByAStream(int32_t portId) {
+bool Hal2AidlMapper::isPortBeingHeld(int32_t portId) {
// It is assumed that mStreams has already been cleaned up.
- for (const auto& streamPair : mStreams) {
- int32_t patchId = streamPair.second;
- auto patchIt = mPatches.find(patchId);
- if (patchIt == mPatches.end()) continue;
- for (int32_t id : patchIt->second.sourcePortConfigIds) {
- auto portConfigIt = mPortConfigs.find(id);
- if (portConfigIt != mPortConfigs.end() && portConfigIt->second.portId == portId) {
- return true;
- }
+ for (const auto& s : mStreams) {
+ if (portConfigBelongsToPort(s.second.first, portId)) return true;
+ }
+ for (const auto& [_, patch] : mPatches) {
+ for (int32_t id : patch.sourcePortConfigIds) {
+ if (portConfigBelongsToPort(id, portId)) return true;
}
- for (int32_t id : patchIt->second.sinkPortConfigIds) {
- auto portConfigIt = mPortConfigs.find(id);
- if (portConfigIt != mPortConfigs.end() && portConfigIt->second.portId == portId) {
- return true;
- }
+ for (int32_t id : patch.sinkPortConfigIds) {
+ if (portConfigBelongsToPort(id, portId)) return true;
}
}
return false;
@@ -686,21 +688,26 @@
}
status_t Hal2AidlMapper::releaseAudioPatch(int32_t patchId) {
- if (auto it = mPatches.find(patchId); it != mPatches.end()) {
- mPatches.erase(it);
- if (ndk::ScopedAStatus status = mModule->resetAudioPatch(patchId); !status.isOk()) {
- ALOGE("%s: error while resetting patch %d: %s",
- __func__, patchId, status.getDescription().c_str());
- return statusTFromBinderStatus(status);
- }
- return OK;
- }
- ALOGE("%s: patch id %d not found", __func__, patchId);
- return BAD_VALUE;
+ return releaseAudioPatches({patchId});
}
-void Hal2AidlMapper::resetPatch(int32_t patchId) {
- (void)releaseAudioPatch(patchId);
+status_t Hal2AidlMapper::releaseAudioPatches(const std::set<int32_t>& patchIds) {
+ status_t result = OK;
+ for (const auto patchId : patchIds) {
+ if (auto it = mPatches.find(patchId); it != mPatches.end()) {
+ mPatches.erase(it);
+ if (ndk::ScopedAStatus status = mModule->resetAudioPatch(patchId); !status.isOk()) {
+ ALOGE("%s: error while resetting patch %d: %s",
+ __func__, patchId, status.getDescription().c_str());
+ result = statusTFromBinderStatus(status);
+ }
+ } else {
+ ALOGE("%s: patch id %d not found", __func__, patchId);
+ result = BAD_VALUE;
+ }
+ }
+ resetUnusedPortConfigs();
+ return result;
}
void Hal2AidlMapper::resetPortConfig(int32_t portConfigId) {
@@ -716,22 +723,22 @@
ALOGE("%s: port config id %d not found", __func__, portConfigId);
}
-void Hal2AidlMapper::resetUnusedPatches() {
- // Since patches can be created independently of streams via 'createAudioPatch',
+void Hal2AidlMapper::resetUnusedPatchesAndPortConfigs() {
+ // Since patches can be created independently of streams via 'createOrUpdatePatch',
// here we only clean up patches for released streams.
+ std::set<int32_t> patchesToRelease;
for (auto it = mStreams.begin(); it != mStreams.end(); ) {
if (auto streamSp = it->first.promote(); streamSp) {
++it;
} else {
- resetPatch(it->second);
+ if (const int32_t patchId = it->second.second; patchId != -1) {
+ patchesToRelease.insert(patchId);
+ }
it = mStreams.erase(it);
}
}
-}
-
-void Hal2AidlMapper::resetUnusedPatchesAndPortConfigs() {
- resetUnusedPatches();
- resetUnusedPortConfigs();
+ // 'releaseAudioPatches' also resets unused port configs.
+ releaseAudioPatches(patchesToRelease);
}
void Hal2AidlMapper::resetUnusedPortConfigs() {
@@ -749,6 +756,9 @@
for (int32_t id : mInitialPortConfigIds) {
portConfigIds.erase(id);
}
+ for (const auto& s : mStreams) {
+ portConfigIds.erase(s.second.first);
+ }
std::set<int32_t> retryDeviceDisconnection;
for (const auto& portConfigAndIdPair : portConfigIds) {
resetPortConfig(portConfigAndIdPair.first);
@@ -758,7 +768,7 @@
}
}
for (int32_t portId : retryDeviceDisconnection) {
- if (!isPortHeldByAStream(portId)) {
+ if (!isPortBeingHeld(portId)) {
if (auto status = mModule->disconnectExternalDevice(portId); status.isOk()) {
eraseConnectedPort(portId);
ALOGD("%s: executed postponed external device disconnection for port ID %d",
@@ -848,7 +858,7 @@
}
// Streams are closed by AudioFlinger independently from device disconnections.
// It is possible that the stream has not been closed yet.
- if (!isPortHeldByAStream(portId)) {
+ if (!isPortBeingHeld(portId)) {
RETURN_STATUS_IF_ERROR(statusTFromBinderStatus(
mModule->disconnectExternalDevice(portId)));
eraseConnectedPort(portId);
diff --git a/media/libaudiohal/impl/Hal2AidlMapper.h b/media/libaudiohal/impl/Hal2AidlMapper.h
index 9283fb4..70a2bd7 100644
--- a/media/libaudiohal/impl/Hal2AidlMapper.h
+++ b/media/libaudiohal/impl/Hal2AidlMapper.h
@@ -44,7 +44,7 @@
const std::string& instance,
const std::shared_ptr<::aidl::android::hardware::audio::core::IModule>& module);
- void addStream(const sp<StreamHalInterface>& stream, int32_t patchId);
+ void addStream(const sp<StreamHalInterface>& stream, int32_t portConfigId, int32_t patchId);
status_t createOrUpdatePatch(
const std::vector<::aidl::android::media::audio::common::AudioPortConfig>& sources,
const std::vector<::aidl::android::media::audio::common::AudioPortConfig>& sinks,
@@ -114,7 +114,12 @@
// Answers the question "whether portID 'first' is reachable from portID 'second'?"
// It's not a map because both portIDs are known. The matrix is symmetric.
using RoutingMatrix = std::set<std::pair<int32_t, int32_t>>;
- using Streams = std::map<wp<StreamHalInterface>, int32_t /*patch ID*/>;
+ // There is always a port config ID set. The patch ID is set after stream
+ // creation, and can be set to '-1' later if the framework happens to create
+ // a patch between the same endpoints. In that case, the ownership of the patch
+ // is on the framework.
+ using Streams = std::map<wp<StreamHalInterface>,
+ std::pair<int32_t /*port config ID*/, int32_t /*patch ID*/>>;
const std::string mInstance;
const std::shared_ptr<::aidl::android::hardware::audio::core::IModule> mModule;
@@ -147,10 +152,14 @@
const std::optional<::aidl::android::media::audio::common::AudioConfig>& config,
const std::optional<::aidl::android::media::audio::common::AudioIoFlags>& flags,
int32_t ioHandle);
- bool isPortHeldByAStream(int32_t portId);
- void resetPatch(int32_t patchId);
+ bool isPortBeingHeld(int32_t portId);
+ bool portConfigBelongsToPort(int32_t portConfigId, int32_t portId) {
+ auto it = mPortConfigs.find(portConfigId);
+ return it != mPortConfigs.end() && it->second.portId == portId;
+ }
+ status_t releaseAudioPatches(const std::set<int32_t>& patchIds);
+ void resetPatch(int32_t patchId) { (void)releaseAudioPatch(patchId); }
void resetPortConfig(int32_t portConfigId);
- void resetUnusedPatches();
void resetUnusedPortConfigs();
status_t updateAudioPort(
int32_t portId, ::aidl::android::media::audio::common::AudioPort* port);