diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index b7761bf244..3117134f4d 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -517,7 +517,7 @@ ndk::ScopedAStatus Module::connectExternalDevice(const AudioPort& in_templateIdA connectedPort.id = ++getConfig().nextPortId; auto [connectedPortsIt, _] = - mConnectedDevicePorts.insert(std::pair(connectedPort.id, std::vector())); + mConnectedDevicePorts.insert(std::pair(connectedPort.id, std::set())); LOG(DEBUG) << __func__ << ": template port " << templateId << " external device connected, " << "connected port ID " << connectedPort.id; ports.push_back(connectedPort); @@ -550,9 +550,21 @@ ndk::ScopedAStatus Module::connectExternalDevice(const AudioPort& in_templateIdA // of all profiles from all routable dynamic device ports would be more involved. for (const auto mixPortId : routablePortIds) { auto portsIt = findById(ports, mixPortId); - if (portsIt != ports.end() && portsIt->profiles.empty()) { - portsIt->profiles = connectedPort.profiles; - connectedPortsIt->second.push_back(portsIt->id); + if (portsIt != ports.end()) { + if (portsIt->profiles.empty()) { + portsIt->profiles = connectedPort.profiles; + connectedPortsIt->second.insert(portsIt->id); + } else { + // Check if profiles are non empty because they were populated by + // a previous connection. Otherwise, it means that they are not empty because + // the mix port has static profiles. + for (const auto cp : mConnectedDevicePorts) { + if (cp.second.count(portsIt->id) > 0) { + connectedPortsIt->second.insert(portsIt->id); + break; + } + } + } } } *_aidl_return = std::move(connectedPort); @@ -607,13 +619,20 @@ ndk::ScopedAStatus Module::disconnectExternalDevice(int32_t in_portId) { } } - for (const auto mixPortId : connectedPortsIt->second) { + // Clear profiles for mix ports that are not connected to any other ports. + std::set mixPortsToClear = std::move(connectedPortsIt->second); + mConnectedDevicePorts.erase(connectedPortsIt); + for (const auto& connectedPort : mConnectedDevicePorts) { + for (int32_t mixPortId : connectedPort.second) { + mixPortsToClear.erase(mixPortId); + } + } + for (int32_t mixPortId : mixPortsToClear) { auto mixPortIt = findById(ports, mixPortId); if (mixPortIt != ports.end()) { mixPortIt->profiles = {}; } } - mConnectedDevicePorts.erase(connectedPortsIt); return ndk::ScopedAStatus::ok(); } diff --git a/audio/aidl/default/include/core-impl/Module.h b/audio/aidl/default/include/core-impl/Module.h index fb3eef214f..bfdab51454 100644 --- a/audio/aidl/default/include/core-impl/Module.h +++ b/audio/aidl/default/include/core-impl/Module.h @@ -142,7 +142,7 @@ class Module : public BnModule { // ids of device ports created at runtime via 'connectExternalDevice'. // Also stores a list of ids of mix ports with dynamic profiles that were populated from // the connected port. This list can be empty, thus an int->int multimap can't be used. - using ConnectedDevicePorts = std::map>; + using ConnectedDevicePorts = std::map>; // Maps port ids and port config ids to patch ids. // Multimap because both ports and configs can be used by multiple patches. using Patches = std::multimap; diff --git a/audio/aidl/vts/Android.bp b/audio/aidl/vts/Android.bp index f7cf4cece3..a2f0260e30 100644 --- a/audio/aidl/vts/Android.bp +++ b/audio/aidl/vts/Android.bp @@ -26,7 +26,10 @@ cc_defaults { "libaudioaidlcommon", "libaidlcommonsupport", ], - header_libs: ["libaudioaidl_headers"], + header_libs: [ + "libaudioaidl_headers", + "libexpectedutils_headers", + ], cflags: [ "-Wall", "-Wextra", diff --git a/audio/aidl/vts/ModuleConfig.cpp b/audio/aidl/vts/ModuleConfig.cpp index 8fdb1552df..af3597d164 100644 --- a/audio/aidl/vts/ModuleConfig.cpp +++ b/audio/aidl/vts/ModuleConfig.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include "ModuleConfig.h" @@ -499,18 +500,13 @@ std::vector ModuleConfig::generateAudioDevicePortConfigs( return result; } -const ndk::ScopedAStatus& ModuleConfig::onExternalDeviceConnected(IModule* module, - const AudioPort& port) { - // Update ports and routes - mStatus = module->getAudioPorts(&mPorts); - if (!mStatus.isOk()) return mStatus; - mStatus = module->getAudioRoutes(&mRoutes); - if (!mStatus.isOk()) return mStatus; +ndk::ScopedAStatus ModuleConfig::onExternalDeviceConnected(IModule* module, const AudioPort& port) { + RETURN_STATUS_IF_ERROR(module->getAudioPorts(&mPorts)); + RETURN_STATUS_IF_ERROR(module->getAudioRoutes(&mRoutes)); // Validate port is present in module if (std::find(mPorts.begin(), mPorts.end(), port) == mPorts.end()) { - mStatus = ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_STATE); - return mStatus; + return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } if (port.flags.getTag() == aidl::android::media::audio::common::AudioIoFlags::Tag::input) { @@ -518,23 +514,20 @@ const ndk::ScopedAStatus& ModuleConfig::onExternalDeviceConnected(IModule* modul } else { mConnectedExternalSinkDevicePorts.insert(port.id); } - return mStatus; + return ndk::ScopedAStatus::ok(); } -const ndk::ScopedAStatus& ModuleConfig::onExternalDeviceDisconnected(IModule* module, - const AudioPort& port) { - // Update ports and routes - mStatus = module->getAudioPorts(&mPorts); - if (!mStatus.isOk()) return mStatus; - mStatus = module->getAudioRoutes(&mRoutes); - if (!mStatus.isOk()) return mStatus; +ndk::ScopedAStatus ModuleConfig::onExternalDeviceDisconnected(IModule* module, + const AudioPort& port) { + RETURN_STATUS_IF_ERROR(module->getAudioPorts(&mPorts)); + RETURN_STATUS_IF_ERROR(module->getAudioRoutes(&mRoutes)); if (port.flags.getTag() == aidl::android::media::audio::common::AudioIoFlags::Tag::input) { mConnectedExternalSourceDevicePorts.erase(port.id); } else { mConnectedExternalSinkDevicePorts.erase(port.id); } - return mStatus; + return ndk::ScopedAStatus::ok(); } bool ModuleConfig::isMmapSupported() const { diff --git a/audio/aidl/vts/ModuleConfig.h b/audio/aidl/vts/ModuleConfig.h index bce1de175f..0cbf24ddfc 100644 --- a/audio/aidl/vts/ModuleConfig.h +++ b/audio/aidl/vts/ModuleConfig.h @@ -157,10 +157,10 @@ class ModuleConfig { return *config.begin(); } - const ndk::ScopedAStatus& onExternalDeviceConnected( + ndk::ScopedAStatus onExternalDeviceConnected( aidl::android::hardware::audio::core::IModule* module, const aidl::android::media::audio::common::AudioPort& port); - const ndk::ScopedAStatus& onExternalDeviceDisconnected( + ndk::ScopedAStatus onExternalDeviceDisconnected( aidl::android::hardware::audio::core::IModule* module, const aidl::android::media::audio::common::AudioPort& port); diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index 025056e132..20062e94ad 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -46,6 +46,7 @@ #include #include #include +#include #include #include "AudioHalBinderServiceUtil.h" @@ -412,9 +413,21 @@ class AudioCoreModuleBase { void SetUpImpl(const std::string& moduleName) { ASSERT_NO_FATAL_FAILURE(ConnectToService(moduleName)); + ASSERT_IS_OK(module->getAudioPorts(&initialPorts)); + ASSERT_IS_OK(module->getAudioRoutes(&initialRoutes)); } - void TearDownImpl() { debug.reset(); } + void TearDownImpl() { + debug.reset(); + std::vector finalPorts; + ASSERT_IS_OK(module->getAudioPorts(&finalPorts)); + EXPECT_NO_FATAL_FAILURE(VerifyVectorsAreEqual(initialPorts, finalPorts)) + << "The list of audio ports was not restored to the initial state"; + std::vector finalRoutes; + ASSERT_IS_OK(module->getAudioRoutes(&finalRoutes)); + EXPECT_NO_FATAL_FAILURE(VerifyVectorsAreEqual(initialRoutes, finalRoutes)) + << "The list of audio routes was not restored to the initial state"; + } void ConnectToService(const std::string& moduleName) { ASSERT_EQ(module, nullptr); @@ -502,17 +515,24 @@ class AudioCoreModuleBase { } } + // Warning: modifies the vectors! + template + void VerifyVectorsAreEqual(std::vector& v1, std::vector& v2) { + ASSERT_EQ(v1.size(), v2.size()); + std::sort(v1.begin(), v1.end()); + std::sort(v2.begin(), v2.end()); + if (v1 != v2) { + FAIL() << "Vectors are not equal: v1 = " << ::android::internal::ToString(v1) + << ", v2 = " << ::android::internal::ToString(v2); + } + } + std::shared_ptr module; std::unique_ptr moduleConfig; AudioHalBinderServiceUtil binderUtil; std::unique_ptr debug; -}; - -class AudioCoreModule : public AudioCoreModuleBase, public testing::TestWithParam { - public: - void SetUp() override { ASSERT_NO_FATAL_FAILURE(SetUpImpl(GetParam())); } - - void TearDown() override { ASSERT_NO_FATAL_FAILURE(TearDownImpl()); } + std::vector initialPorts; + std::vector initialRoutes; }; class WithDevicePortConnectedState { @@ -530,16 +550,19 @@ class WithDevicePortConnectedState { << "when external device disconnected"; } } + ScopedAStatus SetUpNoChecks(IModule* module, ModuleConfig* moduleConfig) { + RETURN_STATUS_IF_ERROR(module->connectExternalDevice(mIdAndData, &mConnectedPort)); + RETURN_STATUS_IF_ERROR(moduleConfig->onExternalDeviceConnected(module, mConnectedPort)); + mModule = module; + mModuleConfig = moduleConfig; + return ScopedAStatus::ok(); + } void SetUp(IModule* module, ModuleConfig* moduleConfig) { - ASSERT_IS_OK(module->connectExternalDevice(mIdAndData, &mConnectedPort)) + ASSERT_NE(moduleConfig, nullptr); + ASSERT_IS_OK(SetUpNoChecks(module, moduleConfig)) << "when connecting device port ID & data " << mIdAndData.toString(); ASSERT_NE(mIdAndData.id, getId()) << "ID of the connected port must not be the same as the ID of the template port"; - ASSERT_NE(moduleConfig, nullptr); - ASSERT_IS_OK(moduleConfig->onExternalDeviceConnected(module, mConnectedPort)) - << "when external device connected"; - mModule = module; - mModuleConfig = moduleConfig; } int32_t getId() const { return mConnectedPort.id; } const AudioPort& get() { return mConnectedPort; } @@ -551,6 +574,13 @@ class WithDevicePortConnectedState { AudioPort mConnectedPort; }; +class AudioCoreModule : public AudioCoreModuleBase, public testing::TestWithParam { + public: + void SetUp() override { ASSERT_NO_FATAL_FAILURE(SetUpImpl(GetParam())); } + + void TearDown() override { ASSERT_NO_FATAL_FAILURE(TearDownImpl()); } +}; + class StreamContext { public: typedef AidlMessageQueue CommandMQ; @@ -1258,11 +1288,8 @@ TEST_P(AudioCoreModule, GetAudioPortsIsStable) { ASSERT_IS_OK(module->getAudioPorts(&ports1)); std::vector ports2; ASSERT_IS_OK(module->getAudioPorts(&ports2)); - ASSERT_EQ(ports1.size(), ports2.size()) - << "Sizes of audio port arrays do not match across consequent calls to getAudioPorts"; - std::sort(ports1.begin(), ports1.end()); - std::sort(ports2.begin(), ports2.end()); - EXPECT_EQ(ports1, ports2); + EXPECT_NO_FATAL_FAILURE(VerifyVectorsAreEqual(ports1, ports2)) + << "Audio port arrays do not match across consequent calls to getAudioPorts"; } TEST_P(AudioCoreModule, GetAudioRoutesIsStable) { @@ -1270,11 +1297,8 @@ TEST_P(AudioCoreModule, GetAudioRoutesIsStable) { ASSERT_IS_OK(module->getAudioRoutes(&routes1)); std::vector routes2; ASSERT_IS_OK(module->getAudioRoutes(&routes2)); - ASSERT_EQ(routes1.size(), routes2.size()) - << "Sizes of audio route arrays do not match across consequent calls to getAudioRoutes"; - std::sort(routes1.begin(), routes1.end()); - std::sort(routes2.begin(), routes2.end()); - EXPECT_EQ(routes1, routes2); + EXPECT_NO_FATAL_FAILURE(VerifyVectorsAreEqual(routes1, routes2)) + << " Audio route arrays do not match across consequent calls to getAudioRoutes"; } TEST_P(AudioCoreModule, GetAudioRoutesAreValid) { @@ -1792,39 +1816,151 @@ TEST_P(AudioCoreModule, ExternalDevicePortRoutes) { } } +class RoutedPortsProfilesSnapshot { + public: + explicit RoutedPortsProfilesSnapshot(int32_t portId) : mPortId(portId) {} + void Capture(IModule* module) { + std::vector routes; + ASSERT_IS_OK(module->getAudioRoutesForAudioPort(mPortId, &routes)); + std::vector allPorts; + ASSERT_IS_OK(module->getAudioPorts(&allPorts)); + ASSERT_NO_FATAL_FAILURE(GetAllRoutedPorts(routes, allPorts)); + ASSERT_NO_FATAL_FAILURE(GetProfileSizes()); + } + void VerifyNoProfilesChanges(const RoutedPortsProfilesSnapshot& before) { + for (const auto& p : before.mRoutedPorts) { + auto beforeIt = before.mPortProfileSizes.find(p.id); + ASSERT_NE(beforeIt, before.mPortProfileSizes.end()) + << "port ID " << p.id << " not found in the initial profile sizes"; + EXPECT_EQ(beforeIt->second, mPortProfileSizes[p.id]) + << " port " << p.toString() << " has an unexpected profile size change" + << " following an external device connection and disconnection"; + } + } + void VerifyProfilesNonEmpty() { + for (const auto& p : mRoutedPorts) { + EXPECT_NE(0UL, mPortProfileSizes[p.id]) + << " port " << p.toString() << " must have had its profiles" + << " populated while having a connected external device"; + } + } + + const std::vector& getRoutedPorts() const { return mRoutedPorts; } + + private: + void GetAllRoutedPorts(const std::vector& routes, + std::vector& allPorts) { + for (const auto& r : routes) { + if (r.sinkPortId == mPortId) { + for (const auto& srcPortId : r.sourcePortIds) { + const auto srcPortIt = findById(allPorts, srcPortId); + ASSERT_NE(allPorts.end(), srcPortIt) << "port ID " << srcPortId; + mRoutedPorts.push_back(*srcPortIt); + } + } else { + const auto sinkPortIt = findById(allPorts, r.sinkPortId); + ASSERT_NE(allPorts.end(), sinkPortIt) << "port ID " << r.sinkPortId; + mRoutedPorts.push_back(*sinkPortIt); + } + } + } + void GetProfileSizes() { + std::transform( + mRoutedPorts.begin(), mRoutedPorts.end(), + std::inserter(mPortProfileSizes, mPortProfileSizes.end()), + [](const auto& port) { return std::make_pair(port.id, port.profiles.size()); }); + } + + const int32_t mPortId; + std::vector mRoutedPorts; + std::map mPortProfileSizes; +}; + // Note: This test relies on simulation of external device connections by the HAL module. TEST_P(AudioCoreModule, ExternalDeviceMixPortConfigs) { // After an external device has been connected, all mix ports that can be routed // to the device port for the connected device must have non-empty profiles. + // Since the test connects and disconnects a single device each time, the size + // of profiles for all mix ports routed to the device port under test must get back + // to the original count once the external device is disconnected. ASSERT_NO_FATAL_FAILURE(SetUpModuleConfig()); std::vector externalDevicePorts = moduleConfig->getExternalDevicePorts(); if (externalDevicePorts.empty()) { GTEST_SKIP() << "No external devices in the module."; } for (const auto& port : externalDevicePorts) { - WithDevicePortConnectedState portConnected(GenerateUniqueDeviceAddress(port)); - ASSERT_NO_FATAL_FAILURE(portConnected.SetUp(module.get(), moduleConfig.get())); - std::vector routes; - ASSERT_IS_OK(module->getAudioRoutesForAudioPort(portConnected.getId(), &routes)); - std::vector allPorts; - ASSERT_IS_OK(module->getAudioPorts(&allPorts)); - for (const auto& r : routes) { - if (r.sinkPortId == portConnected.getId()) { - for (const auto& srcPortId : r.sourcePortIds) { - const auto srcPortIt = findById(allPorts, srcPortId); - ASSERT_NE(allPorts.end(), srcPortIt) << "port ID " << srcPortId; - EXPECT_NE(0UL, srcPortIt->profiles.size()) - << " source port " << srcPortIt->toString() << " must have its profiles" - << " populated following external device connection"; - } - } else { - const auto sinkPortIt = findById(allPorts, r.sinkPortId); - ASSERT_NE(allPorts.end(), sinkPortIt) << "port ID " << r.sinkPortId; - EXPECT_NE(0UL, sinkPortIt->profiles.size()) - << " source port " << sinkPortIt->toString() << " must have its" - << " profiles populated following external device connection"; + SCOPED_TRACE(port.toString()); + RoutedPortsProfilesSnapshot before(port.id); + ASSERT_NO_FATAL_FAILURE(before.Capture(module.get())); + if (before.getRoutedPorts().empty()) continue; + { + WithDevicePortConnectedState portConnected(GenerateUniqueDeviceAddress(port)); + ASSERT_NO_FATAL_FAILURE(portConnected.SetUp(module.get(), moduleConfig.get())); + RoutedPortsProfilesSnapshot connected(portConnected.getId()); + ASSERT_NO_FATAL_FAILURE(connected.Capture(module.get())); + EXPECT_NO_FATAL_FAILURE(connected.VerifyProfilesNonEmpty()); + } + RoutedPortsProfilesSnapshot after(port.id); + ASSERT_NO_FATAL_FAILURE(after.Capture(module.get())); + EXPECT_NO_FATAL_FAILURE(after.VerifyNoProfilesChanges(before)); + } +} + +// Note: This test relies on simulation of external device connections by the HAL module. +TEST_P(AudioCoreModule, TwoExternalDevicesMixPortConfigsNested) { + // Ensure that in the case when two external devices are connected to the same + // device port, disconnecting one of them does not erase the profiles of routed mix ports. + // In this scenario, the connections are "nested." + ASSERT_NO_FATAL_FAILURE(SetUpModuleConfig()); + std::vector externalDevicePorts = moduleConfig->getExternalDevicePorts(); + if (externalDevicePorts.empty()) { + GTEST_SKIP() << "No external devices in the module."; + } + for (const auto& port : externalDevicePorts) { + SCOPED_TRACE(port.toString()); + WithDevicePortConnectedState portConnected1(GenerateUniqueDeviceAddress(port)); + ASSERT_NO_FATAL_FAILURE(portConnected1.SetUp(module.get(), moduleConfig.get())); + { + // Connect and disconnect another device, if possible. It might not be possible + // for point-to-point connections, like analog or SPDIF. + WithDevicePortConnectedState portConnected2(GenerateUniqueDeviceAddress(port)); + if (auto status = portConnected2.SetUpNoChecks(module.get(), moduleConfig.get()); + !status.isOk()) { + continue; } } + RoutedPortsProfilesSnapshot connected(portConnected1.getId()); + ASSERT_NO_FATAL_FAILURE(connected.Capture(module.get())); + EXPECT_NO_FATAL_FAILURE(connected.VerifyProfilesNonEmpty()); + } +} + +// Note: This test relies on simulation of external device connections by the HAL module. +TEST_P(AudioCoreModule, TwoExternalDevicesMixPortConfigsInterleaved) { + // Ensure that in the case when two external devices are connected to the same + // device port, disconnecting one of them does not erase the profiles of routed mix ports. + // In this scenario, the connections are "interleaved." + ASSERT_NO_FATAL_FAILURE(SetUpModuleConfig()); + std::vector externalDevicePorts = moduleConfig->getExternalDevicePorts(); + if (externalDevicePorts.empty()) { + GTEST_SKIP() << "No external devices in the module."; + } + for (const auto& port : externalDevicePorts) { + SCOPED_TRACE(port.toString()); + auto portConnected1 = + std::make_unique(GenerateUniqueDeviceAddress(port)); + ASSERT_NO_FATAL_FAILURE(portConnected1->SetUp(module.get(), moduleConfig.get())); + WithDevicePortConnectedState portConnected2(GenerateUniqueDeviceAddress(port)); + // Connect another device, if possible. It might not be possible for point-to-point + // connections, like analog or SPDIF. + if (auto status = portConnected2.SetUpNoChecks(module.get(), moduleConfig.get()); + !status.isOk()) { + continue; + } + portConnected1.reset(); + RoutedPortsProfilesSnapshot connected(portConnected2.getId()); + ASSERT_NO_FATAL_FAILURE(connected.Capture(module.get())); + EXPECT_NO_FATAL_FAILURE(connected.VerifyProfilesNonEmpty()); } }