From dc41773ba323196ffba37325f50519beae9c578e Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Fri, 29 Sep 2023 15:49:35 -0700 Subject: [PATCH 1/2] audio: Fix update of an existing patch The code for updating the existing patch did not modify the patch stored in the module's list of patches. Added a test which switches the patch to another port config and validates that 'Module.getAudioPatches' returns the updated patch. Bug: 302573756 Test: atest VtsHalAudioCoreTargetTest Change-Id: I0e3412b9387cd451436a48af116dc5a940d868cf --- audio/aidl/default/Module.cpp | 3 +- .../vts/VtsHalAudioCoreModuleTargetTest.cpp | 54 ++++++++++++++++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index 3117134f4d..0d4e21f311 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -515,7 +515,7 @@ ndk::ScopedAStatus Module::connectExternalDevice(const AudioPort& in_templateIdA } } - connectedPort.id = ++getConfig().nextPortId; + connectedPort.id = getConfig().nextPortId++; auto [connectedPortsIt, _] = mConnectedDevicePorts.insert(std::pair(connectedPort.id, std::set())); LOG(DEBUG) << __func__ << ": template port " << templateId << " external device connected, " @@ -865,6 +865,7 @@ ndk::ScopedAStatus Module::setAudioPatch(const AudioPatch& in_requested, AudioPa patches.push_back(*_aidl_return); } else { oldPatch = *existing; + *existing = *_aidl_return; } patchesBackup = mPatches; registerPatch(*_aidl_return); diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index cd765d2225..d86e171465 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -1237,11 +1237,25 @@ class WithAudioPatch { const AudioPortConfig& portConfig2) : mSrcPortConfig(sinkIsCfg1 ? portConfig2 : portConfig1), mSinkPortConfig(sinkIsCfg1 ? portConfig1 : portConfig2) {} + WithAudioPatch(const WithAudioPatch& patch, const AudioPortConfig& srcPortConfig, + const AudioPortConfig& sinkPortConfig) + : mInitialPatch(patch.mPatch), + mSrcPortConfig(srcPortConfig), + mSinkPortConfig(sinkPortConfig), + mModule(patch.mModule), + mPatch(patch.mPatch) {} WithAudioPatch(const WithAudioPatch&) = delete; WithAudioPatch& operator=(const WithAudioPatch&) = delete; ~WithAudioPatch() { if (mModule != nullptr && mPatch.id != 0) { - EXPECT_IS_OK(mModule->resetAudioPatch(mPatch.id)) << "patch id " << getId(); + if (mInitialPatch.has_value()) { + AudioPatch ignored; + // This releases our port configs so that they can be reset. + EXPECT_IS_OK(mModule->setAudioPatch(*mInitialPatch, &ignored)) + << "patch id " << mInitialPatch->id; + } else { + EXPECT_IS_OK(mModule->resetAudioPatch(mPatch.id)) << "patch id " << getId(); + } } } void SetUpPortConfigs(IModule* module) { @@ -1263,6 +1277,16 @@ class WithAudioPatch { EXPECT_GT(latencyMs, 0) << "patch id " << getId(); } } + void VerifyAgainstAllPatches(IModule* module) { + std::vector allPatches; + ASSERT_IS_OK(module->getAudioPatches(&allPatches)); + const auto& patchIt = findById(allPatches, getId()); + ASSERT_NE(patchIt, allPatches.end()) << "patch id " << getId(); + if (get() != *patchIt) { + FAIL() << "Stored patch: " << get().toString() << " is not the same as returned " + << "by the HAL module: " << patchIt->toString(); + } + } int32_t getId() const { return mPatch.id; } const AudioPatch& get() const { return mPatch; } const AudioPortConfig& getSinkPortConfig() const { return mSinkPortConfig.get(); } @@ -1272,6 +1296,7 @@ class WithAudioPatch { } private: + std::optional mInitialPatch; WithAudioPortConfig mSrcPortConfig; WithAudioPortConfig mSinkPortConfig; IModule* mModule = nullptr; @@ -3630,9 +3655,12 @@ class AudioModulePatch : public AudioCoreModule { patches.push_back( std::make_unique(srcSink.first, srcSink.second)); EXPECT_NO_FATAL_FAILURE(patches[patches.size() - 1]->SetUp(module.get())); + EXPECT_NO_FATAL_FAILURE( + patches[patches.size() - 1]->VerifyAgainstAllPatches(module.get())); } else { WithAudioPatch patch(srcSink.first, srcSink.second); EXPECT_NO_FATAL_FAILURE(patch.SetUp(module.get())); + EXPECT_NO_FATAL_FAILURE(patch.VerifyAgainstAllPatches(module.get())); } } } @@ -3653,6 +3681,29 @@ class AudioModulePatch : public AudioCoreModule { } } + void UpdatePatchPorts(bool isInput) { + auto srcSinkGroups = moduleConfig->getRoutableSrcSinkGroups(isInput); + if (srcSinkGroups.empty()) { + GTEST_SKIP() << "No routes to any attached " << direction(isInput, false) << " devices"; + } + bool hasAtLeastOnePair = false; + for (const auto& srcSinkGroup : srcSinkGroups) { + const auto& srcSinks = srcSinkGroup.second; + if (srcSinks.size() < 2) continue; + hasAtLeastOnePair = true; + const auto& pair1 = srcSinks[0]; + const auto& pair2 = srcSinks[1]; + WithAudioPatch patch(pair1.first, pair1.second); + ASSERT_NO_FATAL_FAILURE(patch.SetUp(module.get())); + WithAudioPatch update(patch, pair2.first, pair2.second); + EXPECT_NO_FATAL_FAILURE(update.SetUp(module.get())); + EXPECT_NO_FATAL_FAILURE(update.VerifyAgainstAllPatches(module.get())); + } + if (!hasAtLeastOnePair) { + GTEST_SKIP() << "No routes with multiple sources"; + } + } + void UpdateInvalidPatchId(bool isInput) { auto srcSinkGroups = moduleConfig->getRoutableSrcSinkGroups(isInput); if (srcSinkGroups.empty()) { @@ -3692,6 +3743,7 @@ TEST_PATCH_BOTH_DIRECTIONS(SetNonRoutablePatch); TEST_PATCH_BOTH_DIRECTIONS(SetPatch); TEST_PATCH_BOTH_DIRECTIONS(UpdateInvalidPatchId); TEST_PATCH_BOTH_DIRECTIONS(UpdatePatch); +TEST_PATCH_BOTH_DIRECTIONS(UpdatePatchPorts); TEST_P(AudioModulePatch, ResetInvalidPatchId) { std::set patchIds; From 89a8ea964d569f410695fb7cef6f3e854aafcbb8 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Fri, 29 Sep 2023 17:02:12 -0700 Subject: [PATCH 2/2] audio: Fix notification of streams of the device change Replace the incorrect logic which was considering each port individually with the correct logic which considers a connection of a mix port to device ports. Bug: 302573756 Test: atest VtsHalAudioCoreTargetTest Change-Id: I7df6e32315945897d88923ea4d9744e590e85dfd --- audio/aidl/default/Module.cpp | 65 ++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index 0d4e21f311..663bfa4f76 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -340,21 +340,43 @@ void Module::registerPatch(const AudioPatch& patch) { ndk::ScopedAStatus Module::updateStreamsConnectedState(const AudioPatch& oldPatch, const AudioPatch& newPatch) { - // Streams from the old patch need to be disconnected, streams from the new - // patch need to be connected. If the stream belongs to both patches, no need - // to update it. + // Notify streams about the new set of devices they are connected to. auto maybeFailure = ndk::ScopedAStatus::ok(); - std::set idsToDisconnect, idsToConnect, idsToDisconnectOnFailure; - idsToDisconnect.insert(oldPatch.sourcePortConfigIds.begin(), - oldPatch.sourcePortConfigIds.end()); - idsToDisconnect.insert(oldPatch.sinkPortConfigIds.begin(), oldPatch.sinkPortConfigIds.end()); - idsToConnect.insert(newPatch.sourcePortConfigIds.begin(), newPatch.sourcePortConfigIds.end()); - idsToConnect.insert(newPatch.sinkPortConfigIds.begin(), newPatch.sinkPortConfigIds.end()); - std::for_each(idsToDisconnect.begin(), idsToDisconnect.end(), [&](const auto& portConfigId) { - if (idsToConnect.count(portConfigId) == 0 && mStreams.count(portConfigId) != 0) { - if (auto status = mStreams.setStreamConnectedDevices(portConfigId, {}); status.isOk()) { + using Connections = + std::map>; + Connections oldConnections, newConnections; + auto fillConnectionsHelper = [&](Connections& connections, + const std::vector& mixPortCfgIds, + const std::vector& devicePortCfgIds) { + for (int32_t mixPortCfgId : mixPortCfgIds) { + connections[mixPortCfgId].insert(devicePortCfgIds.begin(), devicePortCfgIds.end()); + } + }; + auto fillConnections = [&](Connections& connections, const AudioPatch& patch) { + if (std::find_if(patch.sourcePortConfigIds.begin(), patch.sourcePortConfigIds.end(), + [&](int32_t portConfigId) { return mStreams.count(portConfigId) > 0; }) != + patch.sourcePortConfigIds.end()) { + // Sources are mix ports. + fillConnectionsHelper(connections, patch.sourcePortConfigIds, patch.sinkPortConfigIds); + } else if (std::find_if(patch.sinkPortConfigIds.begin(), patch.sinkPortConfigIds.end(), + [&](int32_t portConfigId) { + return mStreams.count(portConfigId) > 0; + }) != patch.sinkPortConfigIds.end()) { + // Sources are device ports. + fillConnectionsHelper(connections, patch.sinkPortConfigIds, patch.sourcePortConfigIds); + } // Otherwise, there are no streams to notify. + }; + fillConnections(oldConnections, oldPatch); + fillConnections(newConnections, newPatch); + + std::for_each(oldConnections.begin(), oldConnections.end(), [&](const auto& connectionPair) { + const int32_t mixPortConfigId = connectionPair.first; + if (auto it = newConnections.find(mixPortConfigId); + it == newConnections.end() || it->second != connectionPair.second) { + if (auto status = mStreams.setStreamConnectedDevices(mixPortConfigId, {}); + status.isOk()) { LOG(DEBUG) << "updateStreamsConnectedState: The stream on port config id " - << portConfigId << " has been disconnected"; + << mixPortConfigId << " has been disconnected"; } else { // Disconnection is tricky to roll back, just register a failure. maybeFailure = std::move(status); @@ -362,23 +384,26 @@ ndk::ScopedAStatus Module::updateStreamsConnectedState(const AudioPatch& oldPatc } }); if (!maybeFailure.isOk()) return maybeFailure; - std::for_each(idsToConnect.begin(), idsToConnect.end(), [&](const auto& portConfigId) { - if (idsToDisconnect.count(portConfigId) == 0 && mStreams.count(portConfigId) != 0) { - const auto connectedDevices = findConnectedDevices(portConfigId); + std::set idsToDisconnectOnFailure; + std::for_each(newConnections.begin(), newConnections.end(), [&](const auto& connectionPair) { + const int32_t mixPortConfigId = connectionPair.first; + if (auto it = oldConnections.find(mixPortConfigId); + it == oldConnections.end() || it->second != connectionPair.second) { + const auto connectedDevices = findConnectedDevices(mixPortConfigId); if (connectedDevices.empty()) { // This is important as workers use the vector size to derive the connection status. LOG(FATAL) << "updateStreamsConnectedState: No connected devices found for port " "config id " - << portConfigId; + << mixPortConfigId; } - if (auto status = mStreams.setStreamConnectedDevices(portConfigId, connectedDevices); + if (auto status = mStreams.setStreamConnectedDevices(mixPortConfigId, connectedDevices); status.isOk()) { LOG(DEBUG) << "updateStreamsConnectedState: The stream on port config id " - << portConfigId << " has been connected to: " + << mixPortConfigId << " has been connected to: " << ::android::internal::ToString(connectedDevices); } else { maybeFailure = std::move(status); - idsToDisconnectOnFailure.insert(portConfigId); + idsToDisconnectOnFailure.insert(mixPortConfigId); } } });