From d5536d9ac6c3168d5e292a43e0e3bafc17cb1876 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Fri, 24 Mar 2023 18:27:58 -0700 Subject: [PATCH 1/2] audio: Add some utility methods, improve logging Add 'isDefaultAudioFormat' to Utils.h. Print the module type in 'setModuleDebug'. Align 'suggestDeviceAddressTag' with framework code. Bug: 273252382 Test: m Change-Id: I0248d2e866522a63a745d4af6132b7d2b6a01564 Merged-In: I0248d2e866522a63a745d4af6132b7d2b6a01564 --- audio/aidl/common/include/Utils.h | 6 +++++ audio/aidl/default/Module.cpp | 24 +++++++++++++++---- .../vts/VtsHalAudioCoreModuleTargetTest.cpp | 4 +++- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/audio/aidl/common/include/Utils.h b/audio/aidl/common/include/Utils.h index 2cf862c81d..305c92471e 100644 --- a/audio/aidl/common/include/Utils.h +++ b/audio/aidl/common/include/Utils.h @@ -99,6 +99,12 @@ constexpr size_t getFrameSizeInBytes( return 0; } +constexpr bool isDefaultAudioFormat( + const ::aidl::android::media::audio::common::AudioFormatDescription& desc) { + return desc.type == ::aidl::android::media::audio::common::AudioFormatType::DEFAULT && + desc.pcm == ::aidl::android::media::audio::common::PcmType::DEFAULT; +} + constexpr bool isTelephonyDeviceType( ::aidl::android::media::audio::common::AudioDeviceType device) { return device == ::aidl::android::media::audio::common::AudioDeviceType::IN_TELEPHONY_RX || diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index 984b9a1014..71dc4595e3 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -143,6 +143,21 @@ StreamOut::CreateInstance Module::getStreamOutCreator(Type type) { } } +std::ostream& operator<<(std::ostream& os, Module::Type t) { + switch (t) { + case Module::Type::DEFAULT: + os << "default"; + break; + case Module::Type::R_SUBMIX: + os << "r_submix"; + break; + case Module::Type::USB: + os << "usb"; + break; + } + return os; +} + void Module::cleanUpPatch(int32_t patchId) { erase_all_values(mPatches, std::set{patchId}); } @@ -352,16 +367,17 @@ void Module::updateStreamsConnectedState(const AudioPatch& oldPatch, const Audio ndk::ScopedAStatus Module::setModuleDebug( const ::aidl::android::hardware::audio::core::ModuleDebug& in_debug) { - LOG(DEBUG) << __func__ << ": old flags:" << mDebug.toString() + LOG(DEBUG) << __func__ << ": " << mType << ": old flags:" << mDebug.toString() << ", new flags: " << in_debug.toString(); if (mDebug.simulateDeviceConnections != in_debug.simulateDeviceConnections && !mConnectedDevicePorts.empty()) { - LOG(ERROR) << __func__ << ": attempting to change device connections simulation " - << "while having external devices connected"; + LOG(ERROR) << __func__ << ": " << mType + << ": attempting to change device connections simulation while having external " + << "devices connected"; return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_STATE); } if (in_debug.streamTransientStateDelayMs < 0) { - LOG(ERROR) << __func__ << ": streamTransientStateDelayMs is negative: " + LOG(ERROR) << __func__ << ": " << mType << ": streamTransientStateDelayMs is negative: " << in_debug.streamTransientStateDelayMs; return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index e790d4fcf6..cab16710a5 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -129,7 +129,9 @@ AudioDeviceAddress::Tag suggestDeviceAddressTag(const AudioDeviceDescription& de using Tag = AudioDeviceAddress::Tag; if (std::string_view connection = description.connection; connection == AudioDeviceDescription::CONNECTION_BT_A2DP || - connection == AudioDeviceDescription::CONNECTION_BT_LE || + // Note: BT LE Broadcast uses a "group id". + (description.type != AudioDeviceType::OUT_BROADCAST && + connection == AudioDeviceDescription::CONNECTION_BT_LE) || connection == AudioDeviceDescription::CONNECTION_BT_SCO || connection == AudioDeviceDescription::CONNECTION_WIRELESS) { return Tag::mac; From 7b2d12b1f7a527d23f7c8bdbc80e78939a20bb47 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Fri, 24 Mar 2023 18:29:14 -0700 Subject: [PATCH 2/2] audio: Clarify profiles management for external devices Clarify what should happen to mix port profiles after connection of an external device. Add a test to verify this behavior. Also, add an XML file for the test runner for VtsHalAudioCoreTargetTest. Bug: 273252382 Test: atest VtsHalAudioCoreTargetTest Change-Id: I3381dd29c5922bf31fa3a8ae6fa273597e8333a1 Merged-In: I3381dd29c5922bf31fa3a8ae6fa273597e8333a1 --- .../android/hardware/audio/core/IModule.aidl | 13 ++++ audio/aidl/default/Module.cpp | 62 ++++++++++++++----- .../default/include/core-impl/Configuration.h | 3 +- audio/aidl/default/include/core-impl/Module.h | 6 +- .../vts/VtsHalAudioCoreModuleTargetTest.cpp | 36 +++++++++++ audio/aidl/vts/VtsHalAudioCoreTargetTest.xml | 33 ++++++++++ 6 files changed, 134 insertions(+), 19 deletions(-) create mode 100644 audio/aidl/vts/VtsHalAudioCoreTargetTest.xml diff --git a/audio/aidl/android/hardware/audio/core/IModule.aidl b/audio/aidl/android/hardware/audio/core/IModule.aidl index 78305018a3..7622d9a16c 100644 --- a/audio/aidl/android/hardware/audio/core/IModule.aidl +++ b/audio/aidl/android/hardware/audio/core/IModule.aidl @@ -192,6 +192,19 @@ interface IModule { * device address is specified for a point-to-multipoint external device * connection. * + * Since not all modules have a DSP that could perform sample rate and + * format conversions, behavior related to mix port configurations may vary. + * For modules with a DSP, mix ports can be pre-configured and have a fixed + * set of audio profiles supported by the DSP. For modules without a DSP, + * audio profiles of mix ports may change after connecting an external + * device. The typical case is that the mix port has an empty set of + * profiles when no external devices are connected, and after external + * device connection it receives the same set of profiles as the device + * ports that they can be routed to. The client will re-query current port + * configurations using 'getAudioPorts'. All mix ports that can be routed to + * the connected device port must have a non-empty set of audio profiles + * after successful connection of an external device. + * * Handling of a disconnect is done in a reverse order: * 1. Reset port configuration using the 'resetAudioPortConfig' method. * 2. Release the connected device port by calling the 'disconnectExternalDevice' diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index 71dc4595e3..6b417a4331 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -456,38 +456,45 @@ ndk::ScopedAStatus Module::connectExternalDevice(const AudioPort& in_templateIdA LOG(DEBUG) << __func__ << ": device port " << connectedPort.id << " device set to " << connectedDevicePort.device.toString(); // Check if there is already a connected port with for the same external device. - for (auto connectedPortId : mConnectedDevicePorts) { - auto connectedPortIt = findById(ports, connectedPortId); + for (auto connectedPortPair : mConnectedDevicePorts) { + auto connectedPortIt = findById(ports, connectedPortPair.first); if (connectedPortIt->ext.get().device == connectedDevicePort.device) { LOG(ERROR) << __func__ << ": device " << connectedDevicePort.device.toString() - << " is already connected at the device port id " << connectedPortId; + << " is already connected at the device port id " + << connectedPortPair.first; return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_STATE); } } } if (!mDebug.simulateDeviceConnections) { - // In a real HAL here we would attempt querying the profiles from the device. - LOG(ERROR) << __func__ << ": failed to query supported device profiles"; - // TODO: Check the return value when it is ready for actual devices. - populateConnectedDevicePort(&connectedPort); + if (ndk::ScopedAStatus status = populateConnectedDevicePort(&connectedPort); + !status.isOk()) { + return status; + } + } else { + auto& connectedProfiles = getConfig().connectedProfiles; + if (auto connectedProfilesIt = connectedProfiles.find(templateId); + connectedProfilesIt != connectedProfiles.end()) { + connectedPort.profiles = connectedProfilesIt->second; + } + } + if (connectedPort.profiles.empty()) { + LOG(ERROR) << "Profiles of a connected port still empty after connecting external device " + << connectedPort.toString(); return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_STATE); } connectedPort.id = ++getConfig().nextPortId; - mConnectedDevicePorts.insert(connectedPort.id); + auto [connectedPortsIt, _] = + mConnectedDevicePorts.insert(std::pair(connectedPort.id, std::vector())); LOG(DEBUG) << __func__ << ": template port " << templateId << " external device connected, " << "connected port ID " << connectedPort.id; - auto& connectedProfiles = getConfig().connectedProfiles; - if (auto connectedProfilesIt = connectedProfiles.find(templateId); - connectedProfilesIt != connectedProfiles.end()) { - connectedPort.profiles = connectedProfilesIt->second; - } ports.push_back(connectedPort); onExternalDeviceConnectionChanged(connectedPort, true /*connected*/); - *_aidl_return = std::move(connectedPort); + std::vector routablePortIds; std::vector newRoutes; auto& routes = getConfig().routes; for (auto& r : routes) { @@ -497,15 +504,30 @@ ndk::ScopedAStatus Module::connectExternalDevice(const AudioPort& in_templateIdA newRoute.sinkPortId = connectedPort.id; newRoute.isExclusive = r.isExclusive; newRoutes.push_back(std::move(newRoute)); + routablePortIds.insert(routablePortIds.end(), r.sourcePortIds.begin(), + r.sourcePortIds.end()); } else { auto& srcs = r.sourcePortIds; if (std::find(srcs.begin(), srcs.end(), templateId) != srcs.end()) { srcs.push_back(connectedPort.id); + routablePortIds.push_back(r.sinkPortId); } } } routes.insert(routes.end(), newRoutes.begin(), newRoutes.end()); + // Note: this is a simplistic approach assuming that a mix port can only be populated + // from a single device port. Implementing support for stuffing dynamic profiles with a superset + // 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); + } + } + *_aidl_return = std::move(connectedPort); + return ndk::ScopedAStatus::ok(); } @@ -520,7 +542,8 @@ ndk::ScopedAStatus Module::disconnectExternalDevice(int32_t in_portId) { LOG(ERROR) << __func__ << ": port id " << in_portId << " is not a device port"; return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } - if (mConnectedDevicePorts.count(in_portId) == 0) { + auto connectedPortsIt = mConnectedDevicePorts.find(in_portId); + if (connectedPortsIt == mConnectedDevicePorts.end()) { LOG(ERROR) << __func__ << ": port id " << in_portId << " is not a connected device port"; return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } @@ -541,7 +564,6 @@ ndk::ScopedAStatus Module::disconnectExternalDevice(int32_t in_portId) { } onExternalDeviceConnectionChanged(*portIt, false /*connected*/); ports.erase(portIt); - mConnectedDevicePorts.erase(in_portId); LOG(DEBUG) << __func__ << ": connected device port " << in_portId << " released"; auto& routes = getConfig().routes; @@ -556,6 +578,14 @@ ndk::ScopedAStatus Module::disconnectExternalDevice(int32_t in_portId) { } } + for (const auto mixPortId : connectedPortsIt->second) { + 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/Configuration.h b/audio/aidl/default/include/core-impl/Configuration.h index 4dd01338e2..70320e46aa 100644 --- a/audio/aidl/default/include/core-impl/Configuration.h +++ b/audio/aidl/default/include/core-impl/Configuration.h @@ -33,7 +33,8 @@ struct Configuration { std::vector<::aidl::android::media::audio::common::AudioPort> ports; std::vector<::aidl::android::media::audio::common::AudioPortConfig> portConfigs; std::vector<::aidl::android::media::audio::common::AudioPortConfig> initialConfigs; - // Port id -> List of profiles to use when the device port state is set to 'connected'. + // Port id -> List of profiles to use when the device port state is set to 'connected' + // in connection simulation mode. std::map> connectedProfiles; std::vector routes; diff --git a/audio/aidl/default/include/core-impl/Module.h b/audio/aidl/default/include/core-impl/Module.h index 2cbda7d7b1..83ecfaa8c6 100644 --- a/audio/aidl/default/include/core-impl/Module.h +++ b/audio/aidl/default/include/core-impl/Module.h @@ -177,8 +177,10 @@ class Module : public BnModule { ChildInterface mBluetooth; ChildInterface mBluetoothA2dp; ChildInterface mBluetoothLe; - // ids of ports created at runtime via 'connectExternalDevice'. - std::set mConnectedDevicePorts; + // ids of device ports created at runtime via 'connectExternalDevice'. + // Also stores ids of mix ports with dynamic profiles which got populated from the connected + // port. + std::map> mConnectedDevicePorts; Streams mStreams; // Maps port ids and port config ids to patch ids. // Multimap because both ports and configs can be used by multiple patches. diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index cab16710a5..0ae8cfcb8c 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -1780,6 +1780,42 @@ TEST_P(AudioCoreModule, ExternalDevicePortRoutes) { } } +// 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. + 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())); + 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"; + } + } + } +} + TEST_P(AudioCoreModule, MasterMute) { bool isSupported = false; EXPECT_NO_FATAL_FAILURE(TestAccessors(module.get(), &IModule::getMasterMute, diff --git a/audio/aidl/vts/VtsHalAudioCoreTargetTest.xml b/audio/aidl/vts/VtsHalAudioCoreTargetTest.xml new file mode 100644 index 0000000000..dfc10397f7 --- /dev/null +++ b/audio/aidl/vts/VtsHalAudioCoreTargetTest.xml @@ -0,0 +1,33 @@ + + + +